Re: [Patch -mm 2/2] driver core: Introduce device_move(): move a device

Re: [Patch -mm 2/2] driver core: Introduce device_move(): move a device

[Alan Stern]

Cornelia Huck wrote:

> +	if (old_parent)
> +		klist_del(&dev->knode_parent);
> +	klist_add_tail(&dev->knode_parent, &new_parent->klist_children);

> +			klist_del(&dev->knode_parent);
> +			if (old_parent)
> +				klist_add_tail(&dev->knode_parent,
> 						&old_parent->klist_children);

This is wrong.  klist_del() does not wait for the knode to be removed from 
its klist.  You need to use klist_remove().

I don't see any protection against new_parent being removed while dev is
being transferred under it.  Are you relying on the caller to make sure
this never happens?

Alan Stern

Re: [Patch -mm 2/2] driver core: Introduce device_move(): move a device

[Cornelia Huck]

On Wed, 22 Nov 2006 10:32:47 -0500 (EST),
Alan Stern <stern@rowland.harvard.edu> wrote:

> Cornelia Huck wrote:
> 
> > +	if (old_parent)
> > +		klist_del(&dev->knode_parent);
> > +	klist_add_tail(&dev->knode_parent, &new_parent->klist_children);
> 
> > +			klist_del(&dev->knode_parent);
> > +			if (old_parent)
> > +				klist_add_tail(&dev->knode_parent,
> > 						&old_parent->klist_children);
> 
> This is wrong.  klist_del() does not wait for the knode to be removed from
> its klist.  You need to use klist_remove().

Hmpf, you're right.

> I don't see any protection against new_parent being removed while dev is
> being transferred under it.  Are you relying on the caller to make sure
> this never happens?

Is there any mechanism in the driver core to avoid such races? The only
locking I can see are klists and dev->sem (which only protects
probing). AFAICS, the caller needs to ensure consistency anyway (like
with the subchannel mutex we introduced in s390 to ensure device
register and unregister cannot be called concurrently).

-- 
Cornelia Huck
Linux for zSeries Developer
Tel.: +49-7031-16-4837, Mail: cornelia.huck@de.ibm.com

[Patch -mm] driver core: Use klist_remove() in device_move().

[Cornelia Huck]

From: Cornelia Huck <cornelia.huck@de.ibm.com>

As pointed out by Alan Stern, device_move needs to use klist_remove which waits
until removal is complete.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

---
 drivers/base/core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

--- linux-2.6-CH.orig/drivers/base/core.c
+++ linux-2.6-CH/drivers/base/core.c
@@ -1022,7 +1022,7 @@ int device_move(struct device *dev, stru
 	old_parent = dev->parent;
 	dev->parent = new_parent;
 	if (old_parent)
-		klist_del(&dev->knode_parent);
+		klist_remove(&dev->knode_parent);
 	klist_add_tail(&dev->knode_parent, &new_parent->klist_children);
 	if (!dev->class)
 		goto out_put;
@@ -1031,7 +1031,7 @@ int device_move(struct device *dev, stru
 		/* We ignore errors on cleanup since we're hosed anyway... */
 		device_move_class_links(dev, new_parent, old_parent);
 		if (!kobject_move(&dev->kobj, &old_parent->kobj)) {
-			klist_del(&dev->knode_parent);
+			klist_remove(&dev->knode_parent);
 			if (old_parent)
 				klist_add_tail(&dev->knode_parent,
 					       &old_parent->klist_children);

Re: [Patch -mm 2/2] driver core: Introduce device_move(): move a device

[Alan Stern]

On Wed, 22 Nov 2006, Cornelia Huck wrote:

> On Wed, 22 Nov 2006 10:32:47 -0500 (EST),
> Alan Stern <stern@rowland.harvard.edu> wrote:

> > I don't see any protection against new_parent being removed while dev is
> > being transferred under it.  Are you relying on the caller to make sure
> > this never happens?
> 
> Is there any mechanism in the driver core to avoid such races? The only
> locking I can see are klists and dev->sem (which only protects
> probing). AFAICS, the caller needs to ensure consistency anyway (like
> with the subchannel mutex we introduced in s390 to ensure device
> register and unregister cannot be called concurrently).

Generally the driver core does rely on callers to handle these things.
I just wanted to make sure you were aware of the issue.

Alan Stern