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

[Marco Felsch <m.felsch@pengutronix.de>]

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
> 
> 
>