[Linaro-mm-sig,04/29] drivers: base: add notifier for failed driver bind

Message ID 1407235677-26324-5-git-send-email-m.szyprowski@samsung.com
State New
Headers show

Commit Message

Marek Szyprowski Aug. 5, 2014, 10:47 a.m.
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(-)

Comments

Laurent Pinchart Aug. 21, 2014, 7:46 p.m. | #1
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);
Greg KH Aug. 25, 2014, 8:05 p.m. | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joerg Roedel Aug. 25, 2014, 9:18 p.m. | #3
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

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Szyprowski Aug. 26, 2014, 6:23 a.m. | #4
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
Marek Szyprowski Aug. 26, 2014, 6:30 a.m. | #5
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

Patch

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);