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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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 |