From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Hilman Subject: Re: use of pm_runtime_disable() from driver probe? Date: Tue, 10 Jul 2012 14:04:12 -0700 Message-ID: <871ukjnx9v.fsf@ti.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: In-Reply-To: (Alan Stern's message of "Tue, 10 Jul 2012 16:17:06 -0400 (EDT)") 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-pm@lists.linux-foundation.org List-Id: linux-pm@vger.kernel.org Alan Stern writes: > On Tue, 10 Jul 2012, Rafael J. Wysocki wrote: > >> > If .probe() enabled runtime PM and called pm_runtime_get_sync() (or _resume), >> > it can't clean up properly in case of an error, because its >> > pm_runtime_put_sync() (or _suspend) won't be effective and you're right that >> > it has to call pm_runtime_disable(). >> > >> > So, we don't handle this particular case correctly. Right, this is exactly the case I'm working on now. The driver is calling pm_runtime_enable() and _disable() in .probe(). >> > I'm not sure what the solution should be, though. We could remove the >> > runtime PM operations around really_probe(), but then there may be drivers >> > assuming that the core will call pm_runtime_put_sync() after .probe() >> > has returned. >> >> I have an idea. >> >> What about the following patch? It shouldn't matter except for the cases when >> .probe() attempts to suspend the device by itself. >> >> --- >> drivers/base/dd.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> Index: linux/drivers/base/dd.c >> =================================================================== >> --- linux.orig/drivers/base/dd.c >> +++ linux/drivers/base/dd.c >> @@ -356,10 +356,8 @@ int driver_probe_device(struct device_dr >> pr_debug("bus: '%s': %s: matched device %s with driver %s\n", >> drv->bus->name, __func__, dev_name(dev), drv->name); >> >> - pm_runtime_get_noresume(dev); >> - pm_runtime_barrier(dev); >> ret = really_probe(dev, drv); >> - pm_runtime_put_sync(dev); >> + pm_runtime_idle(dev); >> >> return ret; >> } >> @@ -406,9 +404,8 @@ int device_attach(struct device *dev) >> ret = 0; >> } >> } else { >> - pm_runtime_get_noresume(dev); >> ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach); >> - pm_runtime_put_sync(dev); >> + pm_runtime_idle(dev); >> } >> out_unlock: >> device_unlock(dev); > > > This opens up the possibility of calling probe while a runtime resume > or suspend is in progress. (On the other hand, the existing code > doesn't prevent a concurrent runtime resume.) Maybe it would be best > to leave the pm_runtime_barrier(). > > Apart from that, I think it would solve the problem. I just tested it and it definitely solves the problem. I forgot exactly why the get/put was added around probe in the first place, so am not sure if this opens up other potential problems like Alan mentioned. In any case, if you go forward with this patch, feel free to add: Reviewed-by: Kevin Hilman Tested-by: Kevin Hilman Kevin