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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AANLkTim+UoR+GeEK8T1_jgie3gwq6Pdt_=sRiSf8qx6K@mail.gmail.com>
Date:	Tue, 14 Dec 2010 18:37:16 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	"Rafael J. Wysocki" <rjw@...k.pl>,
	Alan Stern <stern@...land.harvard.edu>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux-pm mailing list <linux-pm@...ts.linux-foundation.org>
Subject: Re: [RFC][PATCH 2/4] PM: Remove redundant checks from core device
 resume routines

2010/12/13 Linus Torvalds <torvalds@...ux-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@...k.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/