[RFC][Patch 0/3] Fix device_move() vs. dpm_list issues.

[RFC][Patch 0/3] Fix device_move() vs. dpm_list issues.

[Cornelia Huck]

Hi,

in thread [1], we discussed the issue of device_move() causing a
reordering of devices without adapting the ancestral order in dpm_list.
If a device is moved to a new parent that was registered after the
device itself, it would still be after its new parent in dpm_list, thus
causing the parent to be suspended before its child.

This patchset attempts to remedy this situation by introducing an
interface for a driver to manipulate dpm_list with the dpm_list_mtx
held. (device_move() does not have enough information to do this
manipulation itself.) The calling sequence for a driver would be:

- lock the dpm_list
- call device_move()
- if device_move() succeeded, fix up dpm_list
- unlock the dpm_list

Following are three patches:

Patch 1 - Introduce the dpm_list manipulation interfaces. I'm not sure
whether the BUG_ON()s should be WARN_ON()s instead.

Patch 2 - Make bluetooth use the new interfaces. I didn't introduce
proper handling for device_move() failures. Very lightly tested (did
not break my setup :)), needs proper review.

Patch 3 - Make s390 use the new interfaces. I'm more confident this
patch is correct, but it is hard to test power management related stuff
without power management support (no regressions were introduced for my
attach/detach testcases though).


[1] https://lists.linux-foundation.org/pipermail/linux-pm/2007-July/014428.html

Re: [RFC][Patch 0/3] Fix device_move() vs. dpm_list issues.

[Ming Lei]

2009/3/3 Cornelia Huck <cornelia.huck@de.ibm.com>:
> Hi,
>
> in thread [1], we discussed the issue of device_move() causing a
> reordering of devices without adapting the ancestral order in dpm_list.
> If a device is moved to a new parent that was registered after the
> device itself, it would still be after its new parent in dpm_list, thus
> causing the parent to be suspended before its child.
>
> This patchset attempts to remedy this situation by introducing an
> interface for a driver to manipulate dpm_list with the dpm_list_mtx
> held. (device_move() does not have enough information to do this
> manipulation itself.) The calling sequence for a driver would be:
>
> - lock the dpm_list
> - call device_move()
> - if device_move() succeeded, fix up dpm_list
> - unlock the dpm_list

IMHO, It is better to fix up dpm_list inside device_move() , like device_add(),
which may let s390, bluetooth or other possible users more happy with
device_remove().

Thanks

>
> Following are three patches:
>
> Patch 1 - Introduce the dpm_list manipulation interfaces. I'm not sure
> whether the BUG_ON()s should be WARN_ON()s instead.
>
> Patch 2 - Make bluetooth use the new interfaces. I didn't introduce
> proper handling for device_move() failures. Very lightly tested (did
> not break my setup :)), needs proper review.
>
> Patch 3 - Make s390 use the new interfaces. I'm more confident this
> patch is correct, but it is hard to test power management related stuff
> without power management support (no regressions were introduced for my
> attach/detach testcases though).
>
>
> [1] https://lists.linux-foundation.org/pipermail/linux-pm/2007-July/014428.html
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm
>



-- 
Lei Ming

Re: [RFC][Patch 0/3] Fix device_move() vs. dpm_list issues.

[Alan Stern]

On Tue, 3 Mar 2009, Ming Lei wrote:

> 2009/3/3 Cornelia Huck <cornelia.huck@de.ibm.com>:
> > Hi,
> >
> > in thread [1], we discussed the issue of device_move() causing a
> > reordering of devices without adapting the ancestral order in dpm_list.
> > If a device is moved to a new parent that was registered after the
> > device itself, it would still be after its new parent in dpm_list, thus
> > causing the parent to be suspended before its child.
> >
> > This patchset attempts to remedy this situation by introducing an
> > interface for a driver to manipulate dpm_list with the dpm_list_mtx
> > held. (device_move() does not have enough information to do this
> > manipulation itself.) The calling sequence for a driver would be:
> >
> > - lock the dpm_list
> > - call device_move()
> > - if device_move() succeeded, fix up dpm_list
> > - unlock the dpm_list
> 
> IMHO, It is better to fix up dpm_list inside device_move() , like device_add(),
> which may let s390, bluetooth or other possible users more happy with
> device_remove().

I agree; it would be cleaner if device_move() could fix up dpm_list 
directly.  If it doesn't have enough information to do so then change 
the interface so that it does.  That should be pretty easy since there 
are only a handful of callers.

Alan Stern

Re: [RFC][Patch 0/3] Fix device_move() vs. dpm_list issues.

[Cornelia Huck]

On Tue, 3 Mar 2009 09:55:27 -0500 (EST),
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Tue, 3 Mar 2009, Ming Lei wrote:
> 
> > 2009/3/3 Cornelia Huck <cornelia.huck@de.ibm.com>:
> > > Hi,
> > >
> > > in thread [1], we discussed the issue of device_move() causing a
> > > reordering of devices without adapting the ancestral order in dpm_list.
> > > If a device is moved to a new parent that was registered after the
> > > device itself, it would still be after its new parent in dpm_list, thus
> > > causing the parent to be suspended before its child.
> > >
> > > This patchset attempts to remedy this situation by introducing an
> > > interface for a driver to manipulate dpm_list with the dpm_list_mtx
> > > held. (device_move() does not have enough information to do this
> > > manipulation itself.) The calling sequence for a driver would be:
> > >
> > > - lock the dpm_list
> > > - call device_move()
> > > - if device_move() succeeded, fix up dpm_list
> > > - unlock the dpm_list
> > 
> > IMHO, It is better to fix up dpm_list inside device_move() , like device_add(),
> > which may let s390, bluetooth or other possible users more happy with
> > device_remove().

Maybe I'm a bit dense today, but I don't understand the problem with
device_remove()?

> 
> I agree; it would be cleaner if device_move() could fix up dpm_list 
> directly.  If it doesn't have enough information to do so then change 
> the interface so that it does.  That should be pretty easy since there 
> are only a handful of callers.

The only really obvious one is 'move to NULL' -> 'move to end of
dpm_list'.
AFAICS, for that device_move() would need the following
parameters:
- device to be moved
- new parent
- which device to move in dpm_list (device, parent, or none)
and then still the moves I do in the s390 code don't seem obvious for
the driver core to get.

Given that the callers still need to specify what to do, I find it much
easier (and the resulting code much more understandable) if the callers
fix up dpm_list...

Re: [RFC][Patch 0/3] Fix device_move() vs. dpm_list issues.

[Alan Stern]

On Tue, 3 Mar 2009, Cornelia Huck wrote:

> On Tue, 3 Mar 2009 09:55:27 -0500 (EST),
> Alan Stern <stern@rowland.harvard.edu> wrote:
> > I agree; it would be cleaner if device_move() could fix up dpm_list 
> > directly.  If it doesn't have enough information to do so then change 
> > the interface so that it does.  That should be pretty easy since there 
> > are only a handful of callers.
> 
> The only really obvious one is 'move to NULL' -> 'move to end of
> dpm_list'.
> AFAICS, for that device_move() would need the following
> parameters:
> - device to be moved
> - new parent

Those two are, of course, the parameters it already has.

> - which device to move in dpm_list (device, parent, or none)
> and then still the moves I do in the s390 code don't seem obvious for
> the driver core to get.

After looking through your s390 patch more carefully, I get a mixed-up 
feeling -- as though you think dpm_list goes in reverse order.  It 
doesn't; devices are added in order of registration, so parents come 
_before_ children.  Thus, this makes no sense:

 	ret = device_move(&cdev->dev, &sch->dev);
...

+	/*
+	 * We already reorder dpm_list here since we may not hold
+	 * the dpm_list mutex when deregistering other_sch.
+	 * Order of devices will be correct after moving sch since
+	 * sch's parent (the css) is guaranteed to be after cdev
+	 * already.
+	 */
+	device_pm_move_after(&sch->dev, &cdev->dev);

The existing code makes cdev a child of sch, so when the dust clears we
want sch to come _before_ cdev in dpm_list.  But the new code does the
opposite; it puts sch after cdev.


Anyway, the moves you add in the s390 and Bluetooth patches all fall
into one of these three patterns:

	Move the device to just after its new parent;

	Move the parent to just before the device;

	Move the device to the end.

Therefore all you need to do is add a third argument to device_move(); 
it can be a enumeration taking on one of the values

	DPM_ORDER_DEV_AFTER_PARENT,
	DPM_ORDER_PARENT_BEFORE_DEV,
	DPM_ORDER_DEV_LAST.

(Come to think of it, I don't understand the reason for moving the
device to the end of dpm_list.  What point is there in doing this?)


> Given that the callers still need to specify what to do, I find it much
> easier (and the resulting code much more understandable) if the callers
> fix up dpm_list...

I disagree.  Doing it the way described above would add less than 10
lines of code to device_move() and one argument to each caller, whereas
your changes are a lot more extensive.

Alan Stern

Re: [RFC][Patch 0/3] Fix device_move() vs. dpm_list issues.

[Ming Lei]

2009/3/3 Cornelia Huck <cornelia.huck@de.ibm.com>:
> On Tue, 3 Mar 2009 09:55:27 -0500 (EST),
> Alan Stern <stern@rowland.harvard.edu> wrote:
>
>> On Tue, 3 Mar 2009, Ming Lei wrote:
>>
>> > 2009/3/3 Cornelia Huck <cornelia.huck@de.ibm.com>:
>> > > Hi,
>> > >
>> > > in thread [1], we discussed the issue of device_move() causing a
>> > > reordering of devices without adapting the ancestral order in dpm_list.
>> > > If a device is moved to a new parent that was registered after the
>> > > device itself, it would still be after its new parent in dpm_list, thus
>> > > causing the parent to be suspended before its child.
>> > >
>> > > This patchset attempts to remedy this situation by introducing an
>> > > interface for a driver to manipulate dpm_list with the dpm_list_mtx
>> > > held. (device_move() does not have enough information to do this
>> > > manipulation itself.) The calling sequence for a driver would be:
>> > >
>> > > - lock the dpm_list
>> > > - call device_move()
>> > > - if device_move() succeeded, fix up dpm_list
>> > > - unlock the dpm_list
>> >
>> > IMHO, It is better to fix up dpm_list inside device_move() , like device_add(),
>> > which may let s390, bluetooth or other possible users more happy with
>> > device_remove().
>
> Maybe I'm a bit dense today, but I don't understand the problem with
> device_remove()?

My fault, It should be device_move(), :-)
Thanks!

>
>>
>> I agree; it would be cleaner if device_move() could fix up dpm_list
>> directly.  If it doesn't have enough information to do so then change
>> the interface so that it does.  That should be pretty easy since there
>> are only a handful of callers.
>
> The only really obvious one is 'move to NULL' -> 'move to end of
> dpm_list'.
> AFAICS, for that device_move() would need the following
> parameters:
> - device to be moved
> - new parent
> - which device to move in dpm_list (device, parent, or none)
> and then still the moves I do in the s390 code don't seem obvious for
> the driver core to get.
>
> Given that the callers still need to specify what to do, I find it much
> easier (and the resulting code much more understandable) if the callers
> fix up dpm_list...
>



-- 
Lei Ming

Re: [RFC][Patch 0/3] Fix device_move() vs. dpm_list issues.

[Cornelia Huck]

On Tue, 3 Mar 2009 14:53:18 -0500 (EST),
Alan Stern <stern@rowland.harvard.edu> wrote:

> After looking through your s390 patch more carefully, I get a mixed-up 
> feeling -- as though you think dpm_list goes in reverse order. 

list_add_tail confused me...

> Therefore all you need to do is add a third argument to device_move(); 
> it can be a enumeration taking on one of the values
> 
> 	DPM_ORDER_DEV_AFTER_PARENT,
> 	DPM_ORDER_PARENT_BEFORE_DEV,
> 	DPM_ORDER_DEV_LAST.

and DPM_ORDER_DO_NOTHING.

> 
> (Come to think of it, I don't understand the reason for moving the
> device to the end of dpm_list.  What point is there in doing this?)

Completeness. It is not strictly needed.

> 
> 
> > Given that the callers still need to specify what to do, I find it much
> > easier (and the resulting code much more understandable) if the callers
> > fix up dpm_list...
> 
> I disagree.  Doing it the way described above would add less than 10
> lines of code to device_move() and one argument to each caller, whereas
> your changes are a lot more extensive.

I'm still not quite convinced, but I'll give it a try.