From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Hilman Subject: Re: subtle pm_runtime_put_sync race and sdio functions Date: Thu, 27 Jan 2011 11:22:58 -0800 Message-ID: <87hbcum9rh.fsf__44193.486593622$1296156281$gmane$org@ti.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: (Alan Stern's message of "Thu, 27 Jan 2011 13:13:33 -0500 (EST)") List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-pm-bounces@lists.linux-foundation.org Errors-To: linux-pm-bounces@lists.linux-foundation.org To: Alan Stern Cc: linux-wireless@vger.kernel.org, linux-mmc@vger.kernel.org, Ido Yariv , linux-pm@lists.linux-foundation.org, Johannes Berg List-Id: linux-pm@vger.kernel.org Alan Stern writes: > On Wed, 26 Jan 2011, Kevin Hilman wrote: > >> >> Consider this instead: Since A is required to be functional before B >> >> can be used, A must be registered before B and hence B gets suspended >> >> before A. Therefore during the prepare phase we can runtime-resume A >> >> and leave it powered up; when B needs to suspend, it won't matter that >> >> the runtime-PM calls are ineffective. >> > >> > We don't really need to do that, because the runtime resume _is_ functional >> > during system suspend. > > Not asynchronous runtime resume, because the workqueue is frozen. But > that's not the issue here. > >> > The only thing missing is a ->suspend() callback for A >> > (and a corresponding ->resume() callback to make sure A will be available to >> > B during system resume). >> > >> >> OK, I'm finally back to debugging this problem and looking for a final >> solution. >> >> I agree that what is needed is ->suspend() and ->resume() callbacks for >> A, but the question remains how to implement them. >> >> In my case, A doesn't need runtime callbacks, but *does* require that >> the subsystem callbacks are called because the subsystem actually does >> all the real PM work. On OMAP, the device PM code (clock mgmt, device >> low-power states, etc.) is common for all on-chip devices, so is handled >> in common code at the subsystem level (in this case, platform_bus.) >> >> Therefore, what is ideally needed is the ability for A's suspend to >> simply call pm_runtime_suspend() so the subsystem can do the work. >> However, since runtime transitions are locked out by this time, that >> doesn't work. IOW, what is needed is a way for a system suspend to say >> "please finish the runtime suspend that was already requested." > > Calling the runtime_suspend method directly is the way to do it. > Do you mean the driver's runtime_suspend method, or the subsystem's runtime_suspend method? In my case, the driver has no runtime_suspend method since the subystem methods are doing the heavy lifting. >> What I've done to work around this in driver A is to manually check >> pm_runtime_suspended() and directly call the subsystem's runtime >> suspend/resume (patch below[1]. NOTE, I've used the _noirq methods to >> ensure device A is available when device B needs it.) > > Hmm. The pm_runtime_suspended() check may not be needed (if A were > suspended already then B would have encountered problems). But > including it doesn't hurt. > > Using the _noirq method isn't a good idea, unless you know for certain > that the runtime_suspend handler doesn't need to sleep. > Using the normal suspend method should work okay, because B always has > to suspend before A. I do know that it doesn't need to sleep, but I think I can use the normal methods anyways in case that changes in the future. >> While this works, I'm not crazy about it since it requires the driver >> know about the subsystem (in this case the bus) where the real PM work >> is done. IMO, it would be much more intuitive (and readable) if the >> driver's suspend hooks could simply trigger a runtime suspend (either a >> new one, or one already requested.) > > This isn't clear to me. Isn't the driver registered on the bus in > question? Can't the driver therefore call the bus's runtime_suspend > routine directly, instead of dereferencing the bus->pm->runtime_suspend > pointer? Not sure what you mean by directly. The platform_bus doesn't expose its runtime PM methods since they can be customized at runtime, so they have to be called via bus->pm. Or do you mean using dev->driver instead of dev->bus? Kevin