Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755325AbYCYPGz (ORCPT ); Tue, 25 Mar 2008 11:06:55 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754549AbYCYPGq (ORCPT ); Tue, 25 Mar 2008 11:06:46 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:54403 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1753051AbYCYPGp (ORCPT ); Tue, 25 Mar 2008 11:06:45 -0400 Date: Tue, 25 Mar 2008 11:06:44 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: "Rafael J. Wysocki" cc: pm list , ACPI Devel Maling List , Greg KH , Len Brown , LKML , Alexey Starikovskiy , David Brownell , Pavel Machek , Benjamin Herrenschmidt Subject: Re: [RFC][PATCH] PM: Introduce new top level suspend and hibernation callbacks (rev. 3) In-Reply-To: <200803242218.47898.rjw@sisk.pl> Message-ID: 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: 3131 Lines: 92 On Mon, 24 Mar 2008, Rafael J. Wysocki wrote: > > Can we also have a DPM_PREPARING state, set when ->prepare() is about > > to be called? The PM core wouldn't make use of it but some drivers > > would. (I can't think of any use at all for the analogous > > DPM_COMPLETING state, however.) > > Hmm. dev->power.status is protected by dpm_list_mtx. Do you think it would be > useful to have an accessor function for reading it under the lock? I don't think so. What I have in mind is situations where there accessed has already been synchronized by other means, while the prepare() method is running. For example: Task 0 Task 1 ------ ------ ->prepare() is called Waits for currently-running registration in task 1 to finish Does other stuff Receives a request to register a new child under dev Sees that dev->power.state is still DPM_ON, so goes ahead with the child's registration ->prepare() returns dev->power.state is set to DPM_SUSPENDING device_pm_add() checks dev->power.state and fails the registration If dev->power.state had been set to DPM_PREPARING before ->prepare() was called, then task 1 would have avoided trying to register the child. > > > + dev->power.status = DPM_RESUMING; > > > + get_device(dev); > > > + mutex_unlock(&dpm_list_mtx); > > > + > > > + resume_device(dev, state); > > > + > > > + mutex_lock(&dpm_list_mtx); > > > + put_device(dev); > > > + } > > > + if (!list_empty(&dev->power.entry)) > > > + list_move_tail(&dev->power.entry, &list); > > > > A little problem here: You refer to dev after calling put_device(). > > The device can't be removed at this point, because we hold dpm_list_mtx, which > is needed by device_del(). True, it can't be removed at this point. But it _can_ be removed between the calls to resume_device() and mutex_lock(). > > > } > > > - if (!error) > > > - all_sleeping = true; > > > + list_splice(&list, &dpm_list); > > > > Instead you could eliminate the list_splice_init() above and put here: > > > > list_splice(&list, dpm_list->prev); > > > > This will move the entries from list to the end of dpm_list. > > dpm_list may be empty at this point. Wouldn't that cause any trouble? It will still work correctly. If dpm_list is empty then dpm_list->prev is equal to &dpm_list, so it will do the same thing as your current code does. I just thought of another problem. At the point where local_irq_disable() is called, in between device_suspend() and device_power_down(), it is possible in a preemptible kernel that another task is holding dpm_list_mtx and is in the middle of updating the list pointers. This would mess up the traversal in device_power_down(). I'm not sure about the best way to prevent this. Is it legal to call unlock_mutex() while interrupts or preemption are disabled? Alan Stern -- 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/