From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tony Lindgren Subject: Re: PM regression with commit 5de85b9d57ab PM runtime re-init in v4.5-rc1 Date: Wed, 3 Feb 2016 08:37:49 -0800 Message-ID: <20160203163749.GI19432@atomide.com> References: <20160202234626.GD19432@atomide.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from muru.com ([72.249.23.125]:59903 "EHLO muru.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757858AbcBCQhw (ORCPT ); Wed, 3 Feb 2016 11:37:52 -0500 Content-Disposition: inline In-Reply-To: Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Alan Stern Cc: Ulf Hansson , "Rafael J. Wysocki" , "Rafael J. Wysocki" , Kevin Hilman , "linux-pm@vger.kernel.org" , Linux OMAP Mailing List , "linux-arm-kernel@lists.infradead.org" * Alan Stern [160203 07:49]: > On Tue, 2 Feb 2016, Tony Lindgren wrote: > > > * Alan Stern [160202 13:46]: > > > On Tue, 2 Feb 2016, Tony Lindgren wrote: > > > > > > > > Also, what is autosuspend_delay set to for your device? And is > > > > > runtime_auto set? > > > > > > > > It's 100 at that point, see the commented snippet below from > > > > omap_hsmmc_probe(): > > > > > > > > pm_runtime_enable(host->dev); > > > > pm_runtime_get_sync(host->dev); > > > > pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY); > > > > /* NOTE: pm_runtime_dont_use_autosuspend(host->dev) needed here? */ > > > > pm_runtime_use_autosuspend(host->dev); > > > > ... > > > > /* gets -EPROBE_DEFER */ > > > > err_irq: > > > > ... > > > > pm_runtime_put_sync(host->dev); > > > > > > You could try changing this to pm_runtime_put_sync_suspend(). But > > > putting pm_runtime_dont_use_autosuspend() before the put_sync seems > > > like a perfectly reasonable thing to do, especially if you feel you > > > should reverse all the changes you made at the start. > > > > They both seem to fix the problem. > > So you could use either one. In my opinion, the > pm_runtime_dont_use_autosuspend() solution is a little cleaner. > > > > The reinit function gets called too late to do what you want -- namely, > > > put the hardware in a low-power state. > > > > Right, the problem is the lack of suspend on the first probe. But > > for having autosuspend timeout long enough for the next probe > > would mean that we can't reset the PM runtime state in between. > > That's one way to look at it. But in principle you don't need to > suspend the device after an unsuccessful probe. You can just leave it > at high power. If this causes problems for a second probe, it's the > second probe's own fault for assuming the actual state matches the PM > status. Yes. And I can improve the warning for omap_device so the authors for new drivers can easily fix the issue. > > > pm_runtime_put_sync() is supposed to follow the driver's wishes. It > > > invokes the driver's runtime_idle callback if there is one, and the > > > callback routine can start a suspend or an autosuspend. If there is no > > > callback, it will use whatever autosuspend setting the driver has set > > > up. If you want to override the autosuspend setting, use > > > pm_runtime_put_sync_suspend() instead. > > > > Yes.. That works too. I guess the thing to consider is if we should > > make pm_runtime_put_sync() always sync along the lines of a patch > > I posted earlier today. That could avoid quite a bit of confusion > > as already seen in this thread :) > > As Rafael pointed out, pm_runtime_put_sync() has well-documented > behavior. It shouldn't be changed. I don't see how changing the > behavior would reduce anybody's confusion. At least, anybody who reads > the documentation carefully. Right :) Tony