Re: [PATCH 02/10] driver core: Better distinguish probe errors in really_probe

[Kirti Wankhede <kwankhede@nvidia.com>]

On 6/14/2021 8:38 PM, Christoph Hellwig wrote:
> really_probe tries to special case errors from ->probe, but due to all
> other initialization added to the function over time now a lot of
> internal errors hit that code path as well.  Untangle that by adding
> a new probe_err local variable and apply the special casing only to
> that.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/base/dd.c | 72 +++++++++++++++++++++++++++--------------------
>   1 file changed, 41 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 7477d3322b3a..999bc737a8f0 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -513,12 +513,42 @@ static ssize_t state_synced_show(struct device *dev,
>   }
>   static DEVICE_ATTR_RO(state_synced);
>   
> +
> +static int call_driver_probe(struct device *dev, struct device_driver *drv)
> +{
> +	int ret = 0;
> +
> +	if (dev->bus->probe)
> +		ret = dev->bus->probe(dev);
> +	else if (drv->probe)
> +		ret = drv->probe(dev);
> +
> +	switch (ret) {
> +	case -EPROBE_DEFER:
> +		/* Driver requested deferred probing */
> +		dev_dbg(dev, "Driver %s requests probe deferral\n", drv->name);
> +		break;
> +	case -ENODEV:
> +	case -ENXIO:
> +		pr_debug("%s: probe of %s rejects match %d\n",
> +			 drv->name, dev_name(dev), ret);
> +		break;
> +	default:
> +		/* driver matched but the probe failed */
> +		pr_warn("%s: probe of %s failed with error %d\n",
> +			drv->name, dev_name(dev), ret);

There should be case 0, that is, success case before default case as below:
+	case 0:
+		/* Driver returned success */
+		break;

Otherwise even in case of success, above warning would mislead that 
probe has failed.

Thanks,
Kirti

> +		break;
> +	}
> +
> +	return ret;
> +}
> +
>   static int really_probe(struct device *dev, struct device_driver *drv)
>   {
> -	int ret = -EPROBE_DEFER;
>   	int local_trigger_count = atomic_read(&deferred_trigger_count);
>   	bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
>   			   !drv->suppress_bind_attrs;
> +	int ret = -EPROBE_DEFER, probe_ret = 0;
>   
>   	if (defer_all_probes) {
>   		/*
> @@ -572,15 +602,15 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>   			goto probe_failed;
>   	}
>   
> -	if (dev->bus->probe) {
> -		ret = dev->bus->probe(dev);
> -		if (ret)
> -			goto probe_failed;
> -	} else if (drv->probe) {
> -		ret = drv->probe(dev);
> -		if (ret)
> -			goto probe_failed;
> -	}
> +	probe_ret = call_driver_probe(dev, drv);
> +	if (probe_ret) {
> +		/*
> +		 * Ignore errors returned by ->probe so that the next driver can
> +		 * try its luck.
> +		   */
> +		ret = 0;
> +		goto probe_failed;
> +	}
>   
>   	if (device_add_groups(dev, drv->dev_groups)) {
>   		dev_err(dev, "device_add_groups() failed\n");
> @@ -650,28 +680,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>   		dev->pm_domain->dismiss(dev);
>   	pm_runtime_reinit(dev);
>   	dev_pm_set_driver_flags(dev, 0);
> -
> -	switch (ret) {
> -	case -EPROBE_DEFER:
> -		/* Driver requested deferred probing */
> -		dev_dbg(dev, "Driver %s requests probe deferral\n", drv->name);
> +	if (probe_ret == -EPROBE_DEFER)
>   		driver_deferred_probe_add_trigger(dev, local_trigger_count);
> -		break;
> -	case -ENODEV:
> -	case -ENXIO:
> -		pr_debug("%s: probe of %s rejects match %d\n",
> -			 drv->name, dev_name(dev), ret);
> -		break;
> -	default:
> -		/* driver matched but the probe failed */
> -		pr_warn("%s: probe of %s failed with error %d\n",
> -			drv->name, dev_name(dev), ret);
> -	}
> -	/*
> -	 * Ignore errors returned by ->probe so that the next driver can try
> -	 * its luck.
> -	 */
> -	ret = 0;
>   done:
>   	atomic_dec(&probe_count);
>   	wake_up_all(&probe_waitqueue);
>