Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757669AbZLJXvf (ORCPT ); Thu, 10 Dec 2009 18:51:35 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1755750AbZLJXvc (ORCPT ); Thu, 10 Dec 2009 18:51:32 -0500 Received: from smtp1.linux-foundation.org ([140.211.169.13]:60786 "EHLO smtp1.linux-foundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753803AbZLJXvb (ORCPT ); Thu, 10 Dec 2009 18:51:31 -0500 Date: Thu, 10 Dec 2009 15:51:15 -0800 (PST) From: Linus Torvalds X-X-Sender: torvalds@localhost.localdomain To: Alan Stern cc: "Rafael J. Wysocki" , Zhang Rui , LKML , ACPI Devel Maling List , pm list Subject: Re: Async suspend-resume patch w/ completions (was: Re: Async suspend-resume patch w/ rwsems) In-Reply-To: Message-ID: References: User-Agent: Alpine 2.00 (LFD 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2843 Lines: 72 On Thu, 10 Dec 2009, Alan Stern wrote: > > You probably didn't look closely at the original code in dpm_suspend() > and dpm_resume(). It's very awkward; each device is removed from > dpm_list, operated on, and then added on to a new local list. At the > end the new list is spliced back into dpm_list. > > This approach is better because it doesn't involve changing any list > pointers while the sleep transition is in progress. At any rate, I > don't recommend doing it in the same patch as the async stuff; it > should be done separately. Either before or after -- the two are > independent. I do agree with the "independent" part. But I don't agree about the awkwardness per se. Sure, it moves things back and forth and has private lists, but that's actually a fairly standard thing to do in those kinds of situations where you're taking something off a list, operating on it, and may need to put it back on the same list eventually. The VM layer does similar things. So that's why I think your version was actually odder - the existing list manipulation isn't all that odd. It has that strange "did we get removed while we dropped the lock and tried to suspend the device" thing, of course, but that's not entirely unheard of either. Could it be done more cleanly? I think so, but I agree with you that it's likely a separate issue. I _suspect_, for example, that we could just do something like, the appended to avoid _some_ of the subtlety. IOW, just move the device to the local list early - and if it gets removed while being suspended, it will automatically get removed from the local list (the remover doesn't care _what_ list it is on whe it does a 'list_del(power.entr)'). UNTESTED PATCH! This may be total crap, of course. But it _looks_ like an "ObviousCleanup(tm)" - famous last words. Linus --- drivers/base/power/main.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 8aa2443..f2bb493 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -687,6 +687,7 @@ static int dpm_suspend(pm_message_t state) struct device *dev = to_device(dpm_list.prev); get_device(dev); + list_move(&dev->power.entry, &list); mutex_unlock(&dpm_list_mtx); error = device_suspend(dev, state); @@ -698,8 +699,6 @@ static int dpm_suspend(pm_message_t state) break; } dev->power.status = DPM_OFF; - if (!list_empty(&dev->power.entry)) - list_move(&dev->power.entry, &list); put_device(dev); } list_splice(&list, dpm_list.prev); -- 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/