Re: [PATCH] PM: Acquire device locks on suspend

On Thu, 10 Jan 2008, Rafael J. Wysocki wrote:

On Wednesday, 9 of January 2008, Alan Stern wrote:

quoted

On Wed, 9 Jan 2008, Rafael J. Wysocki wrote:

quoted

quoted

In dpm_resume() you shouldn't need to use dpm_list_mtx at all, because
the list_move_tail() comes before the resume_device().  It's the same
as in dpm_power_up().

Still, device_pm_schedule_removal() can (in theory) be called concurrently
with dpm_resume() by another thread and this might corrupt the list without
the locking.

Any thread doing that would be in violation of the restrictions you're 
going to add to the kerneldoc for destroy_suspended_device().

However the overhead for the locking isn't critical.  There won't be
any contention (if everything is working right) and it isn't a hot path
anyway.  So you can leave the extra locking in if you want.  But then
you should put it in all the routines where the lists get manipulated,
not just some of them.  That is: device_power_down(), dpm_power_up(),
and even unregister_dropped_devices().

Except for those run on one CPU with interrupts disabled, I think.

Not unregister_dropped_devices()!

quoted

quoted

quoted

Also, the kerneldoc for destroy_suspended_device() should contain an 
extra paragraph warning that the routine should never be called except 
within the scope of a system sleep transition.  In practice this means 
it has to be directly or indirectly invoked by a suspend or resume 
method.

Or by a CPU hotplug notifier (that will be the majority of cases, IMO).

In your patch the call is made in response to a CPU_UP_CANCELED_FROZEN
notification.  Isn't it true that this notification is issued only as
part of a system sleep transition?

Yes, it is.

So it counts as being indirectly invoked by a resume method.

Alan Stern