| Message ID | 20210614150846.4111871-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] driver core: Pull required checks into driver_probe_device() | expand |
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); >
On Mon, Jun 14, 2021 at 05:08:38PM +0200, 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); > + break; > + } Like Kirti said, 0 needs to be handled here. Did this not spew a lot of warnings in the logs? And we can fix up the pr_* calls to use dev_* in the future, shows the evolution of this code... thanks, greg k-h
On Tue, Jun 15, 2021 at 07:17:43AM +0200, Greg Kroah-Hartman wrote: > Like Kirti said, 0 needs to be handled here. Did this not spew a lot of > warnings in the logs? Trying again it did. But I didn't even notice given all the crap printed during a typical boot these days..
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); + 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);
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(-)