| Message ID | 1448551153-84719-2-git-send-email-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Thursday, November 26, 2015 05:19:07 PM Andy Shevchenko wrote: > In case ->probe() fails the notifier does not inform a subscriber about this. > In the result it might happend that some resources that had been allocated will > stay allocated and therefore lead to resource leak. > > Introduce a new notification to inform the subscriber that ->probe() failed. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> I'd rather say the problem is that the users of BUS_NOTIFY_BIND_DRIVER have no chance to do any cleanup in case of a probe failure (there may be problems even if resources aren't leaked). > --- > drivers/base/dd.c | 8 ++++++-- > include/linux/device.h | 1 + > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index a641cf3..ac071a5 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -290,7 +290,7 @@ static int really_probe(struct device *dev, struct device_driver *drv) > /* If using pinctrl, bind pins now before probing */ > ret = pinctrl_bind_pins(dev); > if (ret) > - goto probe_failed; > + goto pinctrl_bind_failed; > > if (driver_sysfs_add(dev)) { > printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n", > @@ -334,6 +334,11 @@ static int really_probe(struct device *dev, struct device_driver *drv) > goto done; > > probe_failed: > + if (dev->bus) > + blocking_notifier_call_chain(&dev->bus->p->bus_notifier, > + BUS_NOTIFY_BIND_DRIVER_ERROR, > + dev); Well, if we do that, device_bind_driver() needs to send that notification too in case it doesn't call driver_bound(). > +pinctrl_bind_failed: > devres_release_all(dev); > driver_sysfs_remove(dev); > dev->driver = NULL; > @@ -701,7 +706,6 @@ static void __device_release_driver(struct device *dev) > blocking_notifier_call_chain(&dev->bus->p->bus_notifier, > BUS_NOTIFY_UNBOUND_DRIVER, > dev); > - > } > } > > diff --git a/include/linux/device.h b/include/linux/device.h > index b8f411b..87cf423 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -191,6 +191,7 @@ extern int bus_unregister_notifier(struct bus_type *bus, > unbound */ > #define BUS_NOTIFY_UNBOUND_DRIVER 0x00000007 /* driver is unbound > from the device */ > +#define BUS_NOTIFY_BIND_DRIVER_ERROR 0x80000004 /* driver fails to be bound */ I'd call it BUS_NOTIFY_DRIVER_NOT_BOUND. > > extern struct kset *bus_get_kset(struct bus_type *bus); > extern struct klist *bus_get_device_klist(struct bus_type *bus); > Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2015-11-27 at 00:09 +0100, Rafael J. Wysocki wrote: > On Thursday, November 26, 2015 05:19:07 PM Andy Shevchenko wrote: > > In case ->probe() fails the notifier does not inform a subscriber > > about this. > > In the result it might happend that some resources that had been > > allocated will > > stay allocated and therefore lead to resource leak. > > > > Introduce a new notification to inform the subscriber that > > ->probe() failed. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > I'd rather say the problem is that the users of > BUS_NOTIFY_BIND_DRIVER have no > chance to do any cleanup in case of a probe failure (there may be > problems even > if resources aren't leaked). Thanks, Rafael, all of your comments sound reasonable for me. Will be taken into consideration in next version. > > > --- > > drivers/base/dd.c | 8 ++++++-- > > include/linux/device.h | 1 + > > 2 files changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > > index a641cf3..ac071a5 100644 > > --- a/drivers/base/dd.c > > +++ b/drivers/base/dd.c > > @@ -290,7 +290,7 @@ static int really_probe(struct device *dev, > > struct device_driver *drv) > > /* If using pinctrl, bind pins now before probing */ > > ret = pinctrl_bind_pins(dev); > > if (ret) > > - goto probe_failed; > > + goto pinctrl_bind_failed; > > > > if (driver_sysfs_add(dev)) { > > printk(KERN_ERR "%s: driver_sysfs_add(%s) > > failed\n", > > @@ -334,6 +334,11 @@ static int really_probe(struct device *dev, > > struct device_driver *drv) > > goto done; > > > > probe_failed: > > + if (dev->bus) > > + blocking_notifier_call_chain(&dev->bus->p- > > >bus_notifier, > > + BUS_NOTIFY_BIND_DRIVE > > R_ERROR, > > + dev); > > Well, if we do that, device_bind_driver() needs to send that > notification too > in case it doesn't call driver_bound(). > > > +pinctrl_bind_failed: > > devres_release_all(dev); > > driver_sysfs_remove(dev); > > dev->driver = NULL; > > @@ -701,7 +706,6 @@ static void __device_release_driver(struct > > device *dev) > > blocking_notifier_call_chain(&dev->bus->p- > > >bus_notifier, > > BUS_NOTIFY_UN > > BOUND_DRIVER, > > dev); > > - > > } > > } > > > > diff --git a/include/linux/device.h b/include/linux/device.h > > index b8f411b..87cf423 100644 > > --- a/include/linux/device.h > > +++ b/include/linux/device.h > > @@ -191,6 +191,7 @@ extern int bus_unregister_notifier(struct > > bus_type *bus, > > unbound */ > > #define BUS_NOTIFY_UNBOUND_DRIVER 0x00000007 /* driver is > > unbound > > from the > > device */ > > +#define BUS_NOTIFY_BIND_DRIVER_ERROR 0x80000004 /* driver > > fails to be bound */ > > I'd call it BUS_NOTIFY_DRIVER_NOT_BOUND. > > > > > extern struct kset *bus_get_kset(struct bus_type *bus); > > extern struct klist *bus_get_device_klist(struct bus_type *bus); > > > > Thanks, > Rafael >
diff --git a/drivers/base/dd.c b/drivers/base/dd.c index a641cf3..ac071a5 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -290,7 +290,7 @@ static int really_probe(struct device *dev, struct device_driver *drv) /* If using pinctrl, bind pins now before probing */ ret = pinctrl_bind_pins(dev); if (ret) - goto probe_failed; + goto pinctrl_bind_failed; if (driver_sysfs_add(dev)) { printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n", @@ -334,6 +334,11 @@ static int really_probe(struct device *dev, struct device_driver *drv) goto done; probe_failed: + if (dev->bus) + blocking_notifier_call_chain(&dev->bus->p->bus_notifier, + BUS_NOTIFY_BIND_DRIVER_ERROR, + dev); +pinctrl_bind_failed: devres_release_all(dev); driver_sysfs_remove(dev); dev->driver = NULL; @@ -701,7 +706,6 @@ static void __device_release_driver(struct device *dev) blocking_notifier_call_chain(&dev->bus->p->bus_notifier, BUS_NOTIFY_UNBOUND_DRIVER, dev); - } } diff --git a/include/linux/device.h b/include/linux/device.h index b8f411b..87cf423 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -191,6 +191,7 @@ extern int bus_unregister_notifier(struct bus_type *bus, unbound */ #define BUS_NOTIFY_UNBOUND_DRIVER 0x00000007 /* driver is unbound from the device */ +#define BUS_NOTIFY_BIND_DRIVER_ERROR 0x80000004 /* driver fails to be bound */ extern struct kset *bus_get_kset(struct bus_type *bus); extern struct klist *bus_get_device_klist(struct bus_type *bus);
In case ->probe() fails the notifier does not inform a subscriber about this. In the result it might happend that some resources that had been allocated will stay allocated and therefore lead to resource leak. Introduce a new notification to inform the subscriber that ->probe() failed. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/base/dd.c | 8 ++++++-- include/linux/device.h | 1 + 2 files changed, 7 insertions(+), 2 deletions(-)