Re: [RFC][PATCH 2/4] PM: Remove redundant checks from core device resume routines

Ming Lei
2010-12-14T10:37:36Z

2010/12/13 Linus Torvalds <torvalds@linux-foundation.org>:
> So I really like this series not only because it implements what I
> suggested, but also because each patch seems to remove more lines than
> it adds. That's always nice, and much too unusual.
>
> But in this one, I really think you should simplify/clarify things further:
>
> On Sun, Dec 12, 2010 at 4:31 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>
>> +++ linux-2.6/drivers/base/power/main.c
>> @@ -485,20 +485,17 @@ void dpm_resume_noirq(pm_message_t state
>>        transition_started = false;
>>        while (!list_empty(&dpm_noirq_list)) {
>>                struct device *dev = to_device(dpm_noirq_list.next);
>> +               int error;
>>
>>                get_device(dev);
>> -               if (dev->power.status > DPM_OFF) {
>> -                       int error;
>> -
>> -                       dev->power.status = DPM_OFF;
>> -                       mutex_unlock(&dpm_list_mtx);
>> +               dev->power.status = DPM_OFF;
>> +               mutex_unlock(&dpm_list_mtx);
>
> I think you should move the device to the dpm_suspended list _here_,
> before dropping the mutex. That way the power.status thing matches the
> list.
>
> So then you'd just remove the crazy conditional "if it's still on a
> list, move it to the right list" thing, and these two lines:
>
>>                if (!list_empty(&dev->power.entry))
>>                        list_move_tail(&dev->power.entry, &dpm_suspended_list);
>
> Would just be that plain
>
>        list_move_tail(&dev->power.entry, &dpm_suspended_list);
>
> before you even drop the lock. That look much simpler, and the list
> movement seems a lot more obvious, no?
>
> If an unregister event (or whatever) happens while you had the mutex
> unlocked, it will just remove it from the new list (the one that
> matches the power state). So no need for that whole complexity with
> "what happens with the list if somebody removed the device while we
> were busy suspending/resuming it".
>
> Or am I missing something?
>
> (And same comment for that other identical case in dpm_complete())

Seems it may apply in other cases(dpm_prepare/dpm_suspend
/dpm_suspend_noirq) too?

thanks,
--
Lei Ming