[PATCH] driver core: remove unneeded klist methods

[PATCH] driver core: remove unneeded klist methods

[Alan Stern]

Greg:

This patch (as641) removes unneeded klist methods from the driver core and
changes a klist_del call to klist_remove in device_del.

The _get and _put methods have no effect, because the klist nodes are
deleted by calling klist_remove, which waits until they are unreferenced
by any klist iterators.  Furthermore, the _puts cause problems because
they occur while the iterator is holding a spinlock.

Alan Stern



Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

James should check this, since he added these routines in the first place.  
The change to device_for_each_child is unfortunate, but also unavoidable
due to the change in device_del.  There's still a potentially dangerous
call to klist_del in attribute_container.c, but I'm not familiar enough
with that code to change it.

The SCSI core needs work.  The basic problem is that it has to keep a
reference to a SCSI device until the last command for that device
completes, but command completion occurs in an interrupt handler.  That
makes it hard to release the last reference.  I don't know how this should
be fixed; James will have to work on it.

Index: usb-2.6/drivers/base/core.c
===================================================================
--- usb-2.6.orig/drivers/base/core.c
+++ usb-2.6/drivers/base/core.c
@@ -198,20 +198,6 @@ void device_remove_file(struct device * 
 	}
 }
 
-static void klist_children_get(struct klist_node *n)
-{
-	struct device *dev = container_of(n, struct device, knode_parent);
-
-	get_device(dev);
-}
-
-static void klist_children_put(struct klist_node *n)
-{
-	struct device *dev = container_of(n, struct device, knode_parent);
-
-	put_device(dev);
-}
-
 
 /**
  *	device_initialize - init device structure.
@@ -228,8 +214,7 @@ void device_initialize(struct device *de
 {
 	kobj_set_kset_s(dev, devices_subsys);
 	kobject_init(&dev->kobj);
-	klist_init(&dev->klist_children, klist_children_get,
-		   klist_children_put);
+	klist_init(&dev->klist_children, NULL, NULL);
 	INIT_LIST_HEAD(&dev->dma_pools);
 	init_MUTEX(&dev->sem);
 	device_init_wakeup(dev, 0);
@@ -365,7 +350,7 @@ void device_del(struct device * dev)
 	struct device * parent = dev->parent;
 
 	if (parent)
-		klist_del(&dev->knode_parent);
+		klist_remove(&dev->knode_parent);
 	device_remove_file(dev, &dev->uevent_attr);
 
 	/* Notify the platform of the removal, in case they
@@ -417,17 +402,25 @@ static struct device * next_device(struc
  *
  *	We check the return of @fn each time. If it returns anything
  *	other than 0, we break out and return that value.
+ *
+ *	The code is complicated because @fn may want to call device_del().
+ *	Since the iterator holds a reference to the klist_node it is
+ *	using, the call would hang.  We get around the problem by
+ *	keeping the iterator one cycle ahead of @fn.
  */
 int device_for_each_child(struct device * parent, void * data,
 		     int (*fn)(struct device *, void *))
 {
 	struct klist_iter i;
-	struct device * child;
+	struct device * child, * next_child;
 	int error = 0;
 
 	klist_iter_init(&parent->klist_children, &i);
-	while ((child = next_device(&i)) && !error)
+	next_child = next_device(&i);
+	while ((child = next_child) && !error) {
+		next_child = next_device(&i);
 		error = fn(child, data);
+	}
 	klist_iter_exit(&i);
 	return error;
 }
Index: usb-2.6/drivers/base/bus.c
===================================================================
--- usb-2.6.orig/drivers/base/bus.c
+++ usb-2.6/drivers/base/bus.c
@@ -599,36 +599,6 @@ static void bus_remove_attrs(struct bus_
 	}
 }
 
-static void klist_devices_get(struct klist_node *n)
-{
-	struct device *dev = container_of(n, struct device, knode_bus);
-
-	get_device(dev);
-}
-
-static void klist_devices_put(struct klist_node *n)
-{
-	struct device *dev = container_of(n, struct device, knode_bus);
-
-	put_device(dev);
-}
-
-static void klist_drivers_get(struct klist_node *n)
-{
-	struct device_driver *drv = container_of(n, struct device_driver,
-						 knode_bus);
-
-	get_driver(drv);
-}
-
-static void klist_drivers_put(struct klist_node *n)
-{
-	struct device_driver *drv = container_of(n, struct device_driver,
-						 knode_bus);
-
-	put_driver(drv);
-}
-
 /**
  *	bus_register - register a bus with the system.
  *	@bus:	bus.
@@ -663,8 +633,8 @@ int bus_register(struct bus_type * bus)
 	if (retval)
 		goto bus_drivers_fail;
 
-	klist_init(&bus->klist_devices, klist_devices_get, klist_devices_put);
-	klist_init(&bus->klist_drivers, klist_drivers_get, klist_drivers_put);
+	klist_init(&bus->klist_devices, NULL, NULL);
+	klist_init(&bus->klist_drivers, NULL, NULL);
 	bus_add_attrs(bus);
 
 	pr_debug("bus type '%s' registered\n", bus->name);
Index: usb-2.6/drivers/base/driver.c
===================================================================
--- usb-2.6.orig/drivers/base/driver.c
+++ usb-2.6/drivers/base/driver.c
@@ -143,20 +143,6 @@ void put_driver(struct device_driver * d
 	kobject_put(&drv->kobj);
 }
 
-static void klist_devices_get(struct klist_node *n)
-{
-	struct device *dev = container_of(n, struct device, knode_driver);
-
-	get_device(dev);
-}
-
-static void klist_devices_put(struct klist_node *n)
-{
-	struct device *dev = container_of(n, struct device, knode_driver);
-
-	put_device(dev);
-}
-
 /**
  *	driver_register - register driver with bus
  *	@drv:	driver to register
@@ -176,7 +162,7 @@ int driver_register(struct device_driver
 	    (drv->bus->shutdown && drv->shutdown)) {
 		printk(KERN_WARNING "Driver '%s' needs updating - please use bus_type methods\n", drv->name);
 	}
-	klist_init(&drv->klist_devices, klist_devices_get, klist_devices_put);
+	klist_init(&drv->klist_devices, NULL, NULL);
 	init_completion(&drv->unloaded);
 	return bus_add_driver(drv);
 }

Re: [PATCH] driver core: remove unneeded klist methods

[James Bottomley]

On Fri, 2006-01-20 at 11:39 -0500, Alan Stern wrote:
> This patch (as641) removes unneeded klist methods from the driver core and
> changes a klist_del call to klist_remove in device_del.
> 
> The _get and _put methods have no effect, because the klist nodes are
> deleted by calling klist_remove, which waits until they are unreferenced
> by any klist iterators.  Furthermore, the _puts cause problems because
> they occur while the iterator is holding a spinlock.

Could you just elaborate on the actual problem that you're trying to
solve here?

Iterators of volatile lists are ipso facto just "best guess", so moving
the location of the iterator piece is OK.  However, your assumption that
only the routine called by the iterator is always going to do the final
put on the object is fundamentally flawed.  The list is volatile because
references to the object are being acquired and released all the time.
Additionally, the list nodes are embedded in the object, so by removing
the object get and put calls, you remove the ability for the object
refcounting to see the fact that the list is using the object.  This
will lead to the situation where the object could be freed while the
iterator is acting on it.  You cannot remove the object get/put calls
from the klist references otherwise the list refcounting will be totally
divorced from the object refcounting.

James

Re: [PATCH] driver core: remove unneeded klist methods

[Alan Stern]

On Fri, 20 Jan 2006, James Bottomley wrote:

> On Fri, 2006-01-20 at 11:39 -0500, Alan Stern wrote:
> > This patch (as641) removes unneeded klist methods from the driver core and
> > changes a klist_del call to klist_remove in device_del.
> > 
> > The _get and _put methods have no effect, because the klist nodes are
> > deleted by calling klist_remove, which waits until they are unreferenced
> > by any klist iterators.  Furthermore, the _puts cause problems because
> > they occur while the iterator is holding a spinlock.
> 
> Could you just elaborate on the actual problem that you're trying to
> solve here?

The problem is that put_device must not be called while holding a
spinlock.  This has always been true, but we only started noticing it
recently when Greg added a might_sleep.  Your klist method would call
put_device while holding the klist's spinlock.

> Iterators of volatile lists are ipso facto just "best guess", so moving
> the location of the iterator piece is OK.

Good, I thought so.

>  However, your assumption that
> only the routine called by the iterator is always going to do the final
> put on the object is fundamentally flawed.

That was not my assumption.  My assumption was that the routine called by
the iterator will _sometimes_ do the final put on the object.  I don't
know if that actually ever happens, but the possibility certainly exists.  
For example, there are places where a device_for_each_child iteration is
used for calling device_unregister on all the children.

>  The list is volatile because
> references to the object are being acquired and released all the time.
> Additionally, the list nodes are embedded in the object, so by removing
> the object get and put calls, you remove the ability for the object
> refcounting to see the fact that the list is using the object.

Agreed.  Note, however, that the patch doesn't do this for all objects -- 
only for struct device.

>  This
> will lead to the situation where the object could be freed while the
> iterator is acting on it.  You cannot remove the object get/put calls
> from the klist references otherwise the list refcounting will be totally
> divorced from the object refcounting.

Not so.  A device structure can't be freed before device_del returns, and
the patch makes device_del call klist_remove instead of klist_del.  The
difference between the two is that klist_remove blocks until all iterators
have finished using the klist node.  New iterators can't start using it
because the routine removes the node from the klist.

So by the time device_del returns, we are guaranteed that the embedded 
klist node is not in use and will never be used.  Thus releasing the 
device structure is safe.

Alan Stern

Re: [PATCH] driver core: remove unneeded klist methods

[James Bottomley]

On Fri, 2006-01-20 at 23:37 -0500, Alan Stern wrote:
> The problem is that put_device must not be called while holding a
> spinlock.  This has always been true, but we only started noticing it
> recently when Greg added a might_sleep.  Your klist method would call
> put_device while holding the klist's spinlock.

Right, but we currently have this problem everywhere throughout the
code.  Avoiding it by not taking references doesn't look to be the right
way to go because then we have to dismantle the whole refcounting
infrastructure.

> Not so.  A device structure can't be freed before device_del returns, and
> the patch makes device_del call klist_remove instead of klist_del.  The
> difference between the two is that klist_remove blocks until all iterators
> have finished using the klist node.  New iterators can't start using it
> because the routine removes the node from the klist.

Sorry ... forgot to mention that part ... the change from _del to
_remove ties us up with a wait for the list to actually remove.  This is
potentially dangerous because you're waiting on events you don't
control.  Additionally, next_child isn't refcounted, so it could
potentially disappear out from under you.

James

Re: [PATCH] driver core: remove unneeded klist methods

[Alan Stern]

On Sun, 22 Jan 2006, James Bottomley wrote:

> On Fri, 2006-01-20 at 23:37 -0500, Alan Stern wrote:
> > The problem is that put_device must not be called while holding a
> > spinlock.  This has always been true, but we only started noticing it
> > recently when Greg added a might_sleep.  Your klist method would call
> > put_device while holding the klist's spinlock.
> 
> Right, but we currently have this problem everywhere throughout the
> code.  Avoiding it by not taking references doesn't look to be the right
> way to go because then we have to dismantle the whole refcounting
> infrastructure.
> 
> > Not so.  A device structure can't be freed before device_del returns, and
> > the patch makes device_del call klist_remove instead of klist_del.  The
> > difference between the two is that klist_remove blocks until all iterators
> > have finished using the klist node.  New iterators can't start using it
> > because the routine removes the node from the klist.
> 
> Sorry ... forgot to mention that part ... the change from _del to
> _remove ties us up with a wait for the list to actually remove.  This is
> potentially dangerous because you're waiting on events you don't
> control.  Additionally, next_child isn't refcounted, so it could
> potentially disappear out from under you.

In this case we're only waiting for other iterators to move past our 
struct device.  The fact that next_child isn't refcounted doesn't matter, 
because (thanks to the new klist_remove) it won't go away until we're done 
with it.

I suppose a different approach to fixing this would be to make klist_del
and klist_remove check the value returned by klist_dec_and_del, and have
_them_ call the put method (after the spinlock has been released) instead
of having it called from klist_release.  Would you prefer me to change it
that way instead?

Alan Stern

Re: [PATCH] driver core: remove unneeded klist methods

[Alan Stern]

On Sun, 22 Jan 2006, James Bottomley wrote:

> Sorry ... forgot to mention that part ... the change from _del to
> _remove ties us up with a wait for the list to actually remove.  This is
> potentially dangerous because you're waiting on events you don't
> control.

I forgot to mention in my earlier reply...

If you don't do something like klist_remove, then you have a dangerous
race: removing a device while a driver is being added.  It's possible that
the driver_attach routine could iterate up to the device and then bind it
to the driver after the device had been unregistered.  The device 
structure would be deallocated when the iterator moved on, without ever 
getting unbound from the driver.

This problem could be avoided if driver_probe_device had some way to
recognize that the driver under consideration had been unregistered, but
right now there is no way for it to tell.  The driver_is_registered inline
routine merely tests the embedded klist_node to see if it is still on the
bus's klist -- which it would be, because removal from that klist is
precisely what you are trying to avoid waiting for.

Alan Stern