lkml.org 
[lkml]   [2010]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [RFC][PATCH 2/4] PM: Remove redundant checks from core device resume routines
    From
    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2010-12-14 11:39    [W:0.029 / U:0.292 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site