| 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
| ||
|
Message-Id: <201012132358.44354.rjw@sisk.pl>
Date: Mon, 13 Dec 2010 23:58:44 +0100
From: "Rafael J. Wysocki" <rjw@...k.pl>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: 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
On Monday, December 13, 2010, Linus Torvalds wrote:
> 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?
No, you're right. Somehow I didn't notice this possible simplification.
> (And same comment for that other identical case in dpm_complete())
Yeah.
In addition to that error messages need not be printed under the mutex.
Updated patch is appended.
Thanks,
Rafael
---
From: Rafael J. Wysocki <rjw@...k.pl>
Subject: PM: Remove redundant checks from core device resume routines
Since a separate list of devices is used to link devices that have
completed each stage of suspend (or resume), it is not necessary to
check dev->power.status in the core device resume routines any more.
Signed-off-by: Rafael J. Wysocki <rjw@...k.pl>
---
drivers/base/power/main.c | 44 +++++++++++++++++---------------------------
1 file changed, 17 insertions(+), 27 deletions(-)
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -485,22 +485,18 @@ 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;
+ list_move_tail(&dev->power.entry, &dpm_suspended_list);
+ mutex_unlock(&dpm_list_mtx);
- error = device_resume_noirq(dev, state);
+ error = device_resume_noirq(dev, state);
+ if (error)
+ pm_dev_err(dev, state, " early", error);
- mutex_lock(&dpm_list_mtx);
- if (error)
- pm_dev_err(dev, state, " early", error);
- }
- if (!list_empty(&dev->power.entry))
- list_move_tail(&dev->power.entry, &dpm_suspended_list);
+ mutex_lock(&dpm_list_mtx);
put_device(dev);
}
mutex_unlock(&dpm_list_mtx);
@@ -619,9 +615,6 @@ static void dpm_resume(pm_message_t stat
async_error = 0;
list_for_each_entry(dev, &dpm_suspended_list, power.entry) {
- if (dev->power.status < DPM_OFF)
- continue;
-
INIT_COMPLETION(dev->power.completion);
if (is_async(dev)) {
get_device(dev);
@@ -632,16 +625,16 @@ static void dpm_resume(pm_message_t stat
while (!list_empty(&dpm_suspended_list)) {
dev = to_device(dpm_suspended_list.next);
get_device(dev);
- if (dev->power.status >= DPM_OFF && !is_async(dev)) {
+ if (!is_async(dev)) {
int error;
mutex_unlock(&dpm_list_mtx);
error = device_resume(dev, state, false);
-
- mutex_lock(&dpm_list_mtx);
if (error)
pm_dev_err(dev, state, "", error);
+
+ mutex_lock(&dpm_list_mtx);
}
if (!list_empty(&dev->power.entry))
list_move_tail(&dev->power.entry, &dpm_prepared_list);
@@ -697,17 +690,14 @@ static void dpm_complete(pm_message_t st
struct device *dev = to_device(dpm_prepared_list.prev);
get_device(dev);
- if (dev->power.status > DPM_ON) {
- dev->power.status = DPM_ON;
- mutex_unlock(&dpm_list_mtx);
+ dev->power.status = DPM_ON;
+ list_move(&dev->power.entry, &list);
+ mutex_unlock(&dpm_list_mtx);
- device_complete(dev, state);
- pm_runtime_put_sync(dev);
+ device_complete(dev, state);
+ pm_runtime_put_sync(dev);
- mutex_lock(&dpm_list_mtx);
- }
- if (!list_empty(&dev->power.entry))
- list_move(&dev->power.entry, &list);
+ mutex_lock(&dpm_list_mtx);
put_device(dev);
}
list_splice(&list, &dpm_list);
--
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/
Powered by blists - more mailing lists