| Message ID | 1407235677-26324-5-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Marek, Thank you for the patch. On Tuesday 05 August 2014 12:47:32 Marek Szyprowski wrote: > This patch adds support for getting a notify for failed device driver > bind, so all the items done in BUS_NOTIFY_BIND_DRIVER event can be > cleaned if the driver fails to bind. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/base/dd.c | 10 +++++++--- > include/linux/device.h | 4 +++- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index e4ffbcf..541a41f 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -237,10 +237,14 @@ static int driver_sysfs_add(struct device *dev) > return ret; > } > > -static void driver_sysfs_remove(struct device *dev) > +static void driver_sysfs_remove(struct device *dev, int failed) > { > struct device_driver *drv = dev->driver; > > + if (failed && dev->bus) > + blocking_notifier_call_chain(&dev->bus->p->bus_notifier, > + BUS_NOTIFY_DRVBIND_FAILED, dev); This might be a stupid question, but as you only call driver_sysfs_remove with failed set to true in a single location (in the failure path of really_probe), how about moving the blocking_notifier_call_chain to that location ? The code seems to be a bit out of place here. > + > if (drv) { > sysfs_remove_link(&drv->p->kobj, kobject_name(&dev->kobj)); > sysfs_remove_link(&dev->kobj, "driver"); > @@ -316,7 +320,7 @@ static int really_probe(struct device *dev, struct > device_driver *drv) > > probe_failed: > devres_release_all(dev); > - driver_sysfs_remove(dev); > + driver_sysfs_remove(dev, true); > dev->driver = NULL; > dev_set_drvdata(dev, NULL); > > @@ -509,7 +513,7 @@ static void __device_release_driver(struct device *dev) > if (drv) { > pm_runtime_get_sync(dev); > > - driver_sysfs_remove(dev); > + driver_sysfs_remove(dev, false); > > if (dev->bus) > blocking_notifier_call_chain(&dev->bus->p->bus_notifier, > diff --git a/include/linux/device.h b/include/linux/device.h > index b387710..92daded 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -176,7 +176,7 @@ extern int bus_register_notifier(struct bus_type *bus, > extern int bus_unregister_notifier(struct bus_type *bus, > struct notifier_block *nb); > > -/* All 4 notifers below get called with the target struct device * > +/* All 7 notifers below get called with the target struct device * > * as an argument. Note that those functions are likely to be called > * with the device lock held in the core, so be careful. > */ > @@ -189,6 +189,8 @@ extern int bus_unregister_notifier(struct bus_type *bus, > unbound */ > #define BUS_NOTIFY_UNBOUND_DRIVER 0x00000006 /* driver is unbound > from the device */ > +#define BUS_NOTIFY_DRVBIND_FAILED 0x00000007 /* driver failed to bind > + to device */ > > extern struct kset *bus_get_kset(struct bus_type *bus); > extern struct klist *bus_get_device_klist(struct bus_type *bus);
On Tue, Aug 05, 2014 at 12:47:32PM +0200, Marek Szyprowski wrote: > This patch adds support for getting a notify for failed device driver > bind, so all the items done in BUS_NOTIFY_BIND_DRIVER event can be > cleaned if the driver fails to bind. But doesn't the bus know if the driver fails to bind, so why would a notifier be needed here? Can't it unwind any problems then? > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/base/dd.c | 10 +++++++--- > include/linux/device.h | 4 +++- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index e4ffbcf..541a41f 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -237,10 +237,14 @@ static int driver_sysfs_add(struct device *dev) > return ret; > } > > -static void driver_sysfs_remove(struct device *dev) > +static void driver_sysfs_remove(struct device *dev, int failed) I _hate_ having functions with a flag that does something different depending on it. If you _really_ need this, just make the notifier call before calling this function, which should work just fine, right? thanks, greg k-h
On Tue, Aug 05, 2014 at 12:47:32PM +0200, Marek Szyprowski wrote: > + if (failed && dev->bus) > + blocking_notifier_call_chain(&dev->bus->p->bus_notifier, > + BUS_NOTIFY_DRVBIND_FAILED, dev); > + Why can't you just use the notifier for BUS_NOTIFY_UNBIND_DRIVER or BUS_NOTIFY_UNBOUND_DRIVER when something goes wrong in driver initialization? Joerg
Hello, On 2014-08-25 22:05, Greg Kroah-Hartman wrote: > On Tue, Aug 05, 2014 at 12:47:32PM +0200, Marek Szyprowski wrote: >> This patch adds support for getting a notify for failed device driver >> bind, so all the items done in BUS_NOTIFY_BIND_DRIVER event can be >> cleaned if the driver fails to bind. > But doesn't the bus know if the driver fails to bind, so why would a > notifier be needed here? Can't it unwind any problems then? Some other subsystems (like IOMMU) might register its own notifiers on the given bus. Such notifier is called before driver probe (BUS_NOTIFY_BIND_DRIVER event), so one can allocate some resources there. However there is no way to do the cleanup if the driver fails to bind, because no notifier is called in such case. >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> drivers/base/dd.c | 10 +++++++--- >> include/linux/device.h | 4 +++- >> 2 files changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c >> index e4ffbcf..541a41f 100644 >> --- a/drivers/base/dd.c >> +++ b/drivers/base/dd.c >> @@ -237,10 +237,14 @@ static int driver_sysfs_add(struct device *dev) >> return ret; >> } >> >> -static void driver_sysfs_remove(struct device *dev) >> +static void driver_sysfs_remove(struct device *dev, int failed) > I _hate_ having functions with a flag that does something different > depending on it. If you _really_ need this, just make the notifier call > before calling this function, which should work just fine, right? Ok, I will fix this. If I remember correctly I followed the driver_sysfs_add() function pattern, which (surprisingly, especially when one considers only the function name) also calls the bus notifiers. Best regards
Hello, On 2014-08-25 23:18, Joerg Roedel wrote: > On Tue, Aug 05, 2014 at 12:47:32PM +0200, Marek Szyprowski wrote: >> + if (failed && dev->bus) >> + blocking_notifier_call_chain(&dev->bus->p->bus_notifier, >> + BUS_NOTIFY_DRVBIND_FAILED, dev); >> + > Why can't you just use the notifier for BUS_NOTIFY_UNBIND_DRIVER or > BUS_NOTIFY_UNBOUND_DRIVER when something goes wrong in driver > initialization? Hmmm, you might be right. BUS_NOTIFY_UNBIND_DRIVER event happens before unbinding the driver, but BUS_NOTIFY_UNBOUND_DRIVER is called when driver remove has been finished, so it can be considered as a symmetrical pair for BUS_NOTIFY_BIND_DRIVER. Driver which registered bus notifiers is mainly interested in doing right cleanup, so the code executed for IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER and IOMMU_GROUP_NOTIFY_DRVBIND_FAILED is same. I will remove this additional event and simply add a call to BUS_NOTIFY_UNBOUND_DRIVER event when driver probe fails. Best regards
diff --git a/drivers/base/dd.c b/drivers/base/dd.c index e4ffbcf..541a41f 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -237,10 +237,14 @@ static int driver_sysfs_add(struct device *dev) return ret; } -static void driver_sysfs_remove(struct device *dev) +static void driver_sysfs_remove(struct device *dev, int failed) { struct device_driver *drv = dev->driver; + if (failed && dev->bus) + blocking_notifier_call_chain(&dev->bus->p->bus_notifier, + BUS_NOTIFY_DRVBIND_FAILED, dev); + if (drv) { sysfs_remove_link(&drv->p->kobj, kobject_name(&dev->kobj)); sysfs_remove_link(&dev->kobj, "driver"); @@ -316,7 +320,7 @@ static int really_probe(struct device *dev, struct device_driver *drv) probe_failed: devres_release_all(dev); - driver_sysfs_remove(dev); + driver_sysfs_remove(dev, true); dev->driver = NULL; dev_set_drvdata(dev, NULL); @@ -509,7 +513,7 @@ static void __device_release_driver(struct device *dev) if (drv) { pm_runtime_get_sync(dev); - driver_sysfs_remove(dev); + driver_sysfs_remove(dev, false); if (dev->bus) blocking_notifier_call_chain(&dev->bus->p->bus_notifier, diff --git a/include/linux/device.h b/include/linux/device.h index b387710..92daded 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -176,7 +176,7 @@ extern int bus_register_notifier(struct bus_type *bus, extern int bus_unregister_notifier(struct bus_type *bus, struct notifier_block *nb); -/* All 4 notifers below get called with the target struct device * +/* All 7 notifers below get called with the target struct device * * as an argument. Note that those functions are likely to be called * with the device lock held in the core, so be careful. */ @@ -189,6 +189,8 @@ extern int bus_unregister_notifier(struct bus_type *bus, unbound */ #define BUS_NOTIFY_UNBOUND_DRIVER 0x00000006 /* driver is unbound from the device */ +#define BUS_NOTIFY_DRVBIND_FAILED 0x00000007 /* driver failed to bind + to device */ extern struct kset *bus_get_kset(struct bus_type *bus); extern struct klist *bus_get_device_klist(struct bus_type *bus);
This patch adds support for getting a notify for failed device driver bind, so all the items done in BUS_NOTIFY_BIND_DRIVER event can be cleaned if the driver fails to bind. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/base/dd.c | 10 +++++++--- include/linux/device.h | 4 +++- 2 files changed, 10 insertions(+), 4 deletions(-)