[PATCH 1/2] of: platform: return -EPROBE_DEFER when ensuring probe with driver missing

[PATCH 1/2] of: platform: return -EPROBE_DEFER when ensuring probe with driver missing

[Ahmad Fatoum]

-ENODEV is a bad choice for an error code for of_device_ensure_probed.

The function is either called from board code or from driver frameworks,
which usually just propagate the error code with unintended
consequences:

  - A board driver probe function returning -ENODEV is silently skipped

  - A driver framework function returning -ENODEV is often interpreted
    to mean that an optional resource is not specified (e.g. in DT).

In both cases, the user isn't provided an error message and wrong
behavior can crop up later. In my case, the XHCI driver would time out,
because phy_get propagated of_device_ensure_probed's -ENODEV, which was
understood to mean that no PHY is needed and not that the PHY is
specified in the DT, but no driver was bound to it.

Instead of -ENODEV, let's thus return -EPROBE_DEFER. This can be
propagated up to the driver core, which on a deep probe system (the only
ones where of_device_ensure_probed is not a no-op) will print a fat red
error message. We could achieve the same with some other error code, but
-EPROBE_DEFER is what a non-deep-probe system would return when probes
are deferred indefinitely, so symmetry in the deep probe case fits well.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/of/platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 060fa3458bd2..664af49d268f 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -504,7 +504,7 @@ int of_device_ensure_probed(struct device_node *np)
 	 * requirements are fulfilled if 'dev->driver' is not NULL.
 	 */
 	if (!dev->driver)
-		return -ENODEV;
+		return -EPROBE_DEFER;
 
 	return 0;
 }
-- 
2.39.2

[PATCH 2/2] phy: freescale: imx8mq-usb: make i.MX8MP support more explicit

[Ahmad Fatoum]

The driver was originally written for use with the i.MX8MQ's DWC3, but
has been extended to also cover the i.MX8MP's DWC3.

While we won't change the config symbol name, because that could break
existing users, we can and should change the symbol prompt and help
text as well as the dependency to reflect that the driver also targets
the i.MX8MP.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/phy/freescale/Kconfig | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
index 4eb1f9a55c3b..04e8bcf18833 100644
--- a/drivers/phy/freescale/Kconfig
+++ b/drivers/phy/freescale/Kconfig
@@ -1,5 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0-only
-config PHY_FSL_IMX8MQ_USB
-	bool "Freescale i.MX8M USB3 PHY"
-	default ARCH_IMX8MQ
+
+config PHY_FSL_IMX8MQ_USB
+	bool "Freescale i.MX8MQ/P USB3 PHY"
+	default ARCH_IMX8MQ || ARCH_IMX8MP
+	help
+	  Enable this to add support for the USB PHY found on
+	  the i.MX8M Quad and Plus.
 
-- 
2.39.2

Re: [PATCH 1/2] of: platform: return -EPROBE_DEFER when ensuring probe with driver missing

[Marco Felsch]

Hi Ahmad,

On 24-02-16, Ahmad Fatoum wrote:
> -ENODEV is a bad choice for an error code for of_device_ensure_probed.
> 
> The function is either called from board code or from driver frameworks,
> which usually just propagate the error code with unintended
> consequences:
> 
>   - A board driver probe function returning -ENODEV is silently skipped
> 
>   - A driver framework function returning -ENODEV is often interpreted
>     to mean that an optional resource is not specified (e.g. in DT).
> 
> In both cases, the user isn't provided an error message and wrong
> behavior can crop up later. In my case, the XHCI driver would time out,
> because phy_get propagated of_device_ensure_probed's -ENODEV, which was
> understood to mean that no PHY is needed and not that the PHY is
> specified in the DT, but no driver was bound to it.
> 
> Instead of -ENODEV, let's thus return -EPROBE_DEFER. This can be
> propagated up to the driver core, which on a deep probe system (the only
> ones where of_device_ensure_probed is not a no-op) will print a fat red
> error message. We could achieve the same with some other error code, but

This big fat error should indicate that something went really wrong.

> -EPROBE_DEFER is what a non-deep-probe system would return when probes
> are deferred indefinitely, so symmetry in the deep probe case fits well.

Albeit I get your point, I'm not very happy to return this error code
here since the whole idea of deep-probe was to get rid of EPROBE_DEFER.

> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/of/platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 060fa3458bd2..664af49d268f 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -504,7 +504,7 @@ int of_device_ensure_probed(struct device_node *np)
>  	 * requirements are fulfilled if 'dev->driver' is not NULL.
>  	 */
>  	if (!dev->driver)
> -		return -ENODEV;
> +		return -EPROBE_DEFER;

What about a new error code, e.g. ENODRV which is only used once in this
function? Additional we can add an error message like:
pr_err("No driver found for %pO\n, np); since we know that the driver is
missing.

If you want to stick with -EPROBE_DEFER you need at least adapt the
above comment and the function doc.

Regards,
  Marco

>  
>  	return 0;
>  }
> -- 
> 2.39.2
> 
> 
> 

Re: [PATCH 1/2] of: platform: return -EPROBE_DEFER when ensuring probe with driver missing

[Ahmad Fatoum]

Hello Marco,

On 17.02.24 00:02, Marco Felsch wrote:
> Hi Ahmad,
> 
> On 24-02-16, Ahmad Fatoum wrote:
>> -ENODEV is a bad choice for an error code for of_device_ensure_probed.
>>
>> The function is either called from board code or from driver frameworks,
>> which usually just propagate the error code with unintended
>> consequences:
>>
>>   - A board driver probe function returning -ENODEV is silently skipped
>>
>>   - A driver framework function returning -ENODEV is often interpreted
>>     to mean that an optional resource is not specified (e.g. in DT).
>>
>> In both cases, the user isn't provided an error message and wrong
>> behavior can crop up later. In my case, the XHCI driver would time out,
>> because phy_get propagated of_device_ensure_probed's -ENODEV, which was
>> understood to mean that no PHY is needed and not that the PHY is
>> specified in the DT, but no driver was bound to it.
>>
>> Instead of -ENODEV, let's thus return -EPROBE_DEFER. This can be
>> propagated up to the driver core, which on a deep probe system (the only
>> ones where of_device_ensure_probed is not a no-op) will print a fat red
>> error message. We could achieve the same with some other error code, but
> 
> This big fat error should indicate that something went really wrong.

Having a dependency on a device without a driver is something that shouldn't
happen, so a big fat error is appropriate.

> 
>> -EPROBE_DEFER is what a non-deep-probe system would return when probes
>> are deferred indefinitely, so symmetry in the deep probe case fits well.
> 
> Albeit I get your point, I'm not very happy to return this error code
> here since the whole idea of deep-probe was to get rid of EPROBE_DEFER.

I thought that deep probe disables the probe deferral mechanism, but apparently
returning EPROBE_DEFER still ends up with the device making it into the deferral
list. Shouldn't we disable the deferral list when deep probe is enabled for a board?


>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  drivers/of/platform.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 060fa3458bd2..664af49d268f 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -504,7 +504,7 @@ int of_device_ensure_probed(struct device_node *np)
>>  	 * requirements are fulfilled if 'dev->driver' is not NULL.
>>  	 */
>>  	if (!dev->driver)
>> -		return -ENODEV;
>> +		return -EPROBE_DEFER;
> 
> What about a new error code, e.g. ENODRV which is only used once in this
> function? Additional we can add an error message like:
> pr_err("No driver found for %pO\n, np); since we know that the driver is
> missing.

I think EPROBE_DEFER is an appropriate error code. Yes, deep probe is meant
to replace probe deferral, but if there's no driver, we can't do anything,
except for indefinite deferral of the probe, so the error code sounds
appropriate to me. An extra benefit is that kernel drivers being ported
will often just forward EPROBE_DEFER, while an unknown error code may
trigger an extra warning or different unwanted behavior.

> If you want to stick with -EPROBE_DEFER you need at least adapt the
> above comment and the function doc.

Sure, I can do that for v2.

Cheers,
Ahmad

> 
> Regards,
>   Marco
> 
>>  
>>  	return 0;
>>  }
>> -- 
>> 2.39.2
>>
>>
>>
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |