Re: [PATCH 04/29] drivers: base: add notifier for failed driver bind

2014-08-26 Thread Marek Szyprowski 

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.szyprow...@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, PhD
Samsung RD Institute Poland

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 04/29] drivers: base: add notifier for failed driver bind

2014-08-26 Thread Marek Szyprowski 

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
--
Marek Szyprowski, PhD
Samsung RD Institute Poland

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 04/29] drivers: base: add notifier for failed driver bind

2014-08-25 Thread Greg Kroah-Hartman 
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.szyprow...@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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 04/29] drivers: base: add notifier for failed driver bind

2014-08-25 Thread Joerg Roedel 
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

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 04/29] drivers: base: add notifier for failed driver bind

2014-08-21 Thread Laurent Pinchart 
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.szyprow...@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_DRIVER0x0006 /* driver is unbound
 from the device */
 +#define BUS_NOTIFY_DRVBIND_FAILED0x0007 /* 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);

-- 
Regards,

Laurent Pinchart

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 04/29] drivers: base: add notifier for failed driver bind

2014-08-05 Thread Marek Szyprowski 
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.szyprow...@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);
+
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  0x0006 /* driver is unbound
  from the device */
+#define BUS_NOTIFY_DRVBIND_FAILED  0x0007 /* 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);
-- 
1.9.2

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu