From patchwork Fri Apr 22 22:11:47 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rafael Wysocki X-Patchwork-Id: 728511 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter1.kernel.org (8.14.4/8.14.3) with ESMTP id p3MMBrYg031290 for ; Fri, 22 Apr 2011 22:11:55 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756767Ab1DVWLv (ORCPT ); Fri, 22 Apr 2011 18:11:51 -0400 Received: from ogre.sisk.pl ([217.79.144.158]:45236 "EHLO ogre.sisk.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756740Ab1DVWLv (ORCPT ); Fri, 22 Apr 2011 18:11:51 -0400 Received: from localhost (localhost.localdomain [127.0.0.1]) by ogre.sisk.pl (Postfix) with ESMTP id 522E51AC27A; Sat, 23 Apr 2011 00:10:35 +0200 (CEST) Received: from ogre.sisk.pl ([127.0.0.1]) by localhost (ogre.sisk.pl [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 07358-07; Sat, 23 Apr 2011 00:10:14 +0200 (CEST) Received: from ferrari.rjw.lan (220-bem-13.acn.waw.pl [82.210.184.220]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by ogre.sisk.pl (Postfix) with ESMTP id 711B71AC027; Sat, 23 Apr 2011 00:10:14 +0200 (CEST) From: "Rafael J. Wysocki" To: Alan Stern Subject: Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() Date: Sat, 23 Apr 2011 00:11:47 +0200 User-Agent: KMail/1.13.6 (Linux/2.6.39-rc4+; KDE/4.6.0; x86_64; ; ) Cc: Guennadi Liakhovetski , "Ohad Ben-Cohen" , linux-mmc@vger.kernel.org, Magnus Damm , Simon Horman , Linux PM mailing list References: In-Reply-To: MIME-Version: 1.0 Message-Id: <201104230011.48073.rjw@sisk.pl> X-Virus-Scanned: amavisd-new at ogre.sisk.pl using MkS_Vir for Linux Sender: linux-mmc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-mmc@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Fri, 22 Apr 2011 22:12:03 +0000 (UTC) On Friday, April 22, 2011, Alan Stern wrote: > On Fri, 22 Apr 2011, Rafael J. Wysocki wrote: > > > > The barrier would not prevent the race between the notifier and runtie PM > > > from taking place. Why don't we do something like this instead: > > > > > > --- > > > drivers/base/dd.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > Index: linux-2.6/drivers/base/dd.c > > > =================================================================== > > > --- linux-2.6.orig/drivers/base/dd.c > > > +++ linux-2.6/drivers/base/dd.c > > > @@ -326,6 +326,8 @@ static void __device_release_driver(stru > > > BUS_NOTIFY_UNBIND_DRIVER, > > > dev); > > > > > > + pm_runtime_put_sync(dev); > > > + > > > > In fact, I think this one may be _noidle. If we allow the bus/driver > > to do what they wont, we might as well let them handle the "device idle" > > case from ->remove(). > > Maybe... But keeping it put_sync doesn't do any harm. In Guennadi's > case, it might allow him to get rid of the pm_runtime_suspend() call in > the remove routine. > > > > if (dev->bus && dev->bus->remove) > > > dev->bus->remove(dev); > > > else if (drv->remove) > > > @@ -338,7 +340,6 @@ static void __device_release_driver(stru > > > BUS_NOTIFY_UNBOUND_DRIVER, > > > dev); > > > > > > - pm_runtime_put_sync(dev); > > > } > > > } > > Basically this is okay with me, and it should allow Guennadi to avoid > the extra put/get pair. OK, so I'm going to put the appended patch into my linux-next branch (hopefully, the problem is explained sufficiently in the changelog). > Do you know if any other subsystems rely on the usage_count > being > 0 during unbinding? Do you mean if I know of subsystems that the patch below may cause to fail? No, I don't. :-) Rafael --- From: Rafael J. Wysocki Subject: PM / Runtime: Allow runtime suspend to be done from remove callbacks The driver core prevents race conditions between device runtime PM and driver removal from happening by incrementing the runtime PM usage counter of the device and executing pm_runtime_barrier() before running the bus notifier and the ->remove() callbacks provided by the device's subsystem or driver. This guarantees that, if a future runtime suspend of the device has been scheduled or a runtime resume or idle request has been queued up right before the driver removal, it will be canceled or waited for to complete and no other asynchronous runtime suspend or idle requests for the device will be put into the PM workqueue until the ->remove() callback returns. However, this also means that the device's subsystem or driver will not be able put it into the suspended state by calling pm_runtime_suspend() from its ->remove() routine. This turns out to be a major inconvenience for some subsystems and drivers that want to leave the devices they handle in the suspended state. To allow subsystems and drivers to put devices into the suspended state by calling pm_runtime_suspend() from their ->remove() routines execute pm_runtime_put_sync() after running the bus notifier in __device_release_driver(). [The bus notifier still needs to be protected from racing with runtime PM routines, because it is used by some subsystems to carry out operations affecting the runtime PM functionality.] This will require subsystems and drivers to make their ->remove() callbacks avoid races with runtime PM directly, but it will allow of more flexibility in the handling of devices during the removal of their drivers. Reported-by: Guennadi Liakhovetski Signed-off-by: Rafael J. Wysocki --- drivers/base/dd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Index: linux-2.6/drivers/base/dd.c =================================================================== --- linux-2.6.orig/drivers/base/dd.c +++ linux-2.6/drivers/base/dd.c @@ -326,6 +326,8 @@ static void __device_release_driver(stru BUS_NOTIFY_UNBIND_DRIVER, dev); + pm_runtime_put_sync(dev); + if (dev->bus && dev->bus->remove) dev->bus->remove(dev); else if (drv->remove) @@ -338,7 +340,6 @@ static void __device_release_driver(stru BUS_NOTIFY_UNBOUND_DRIVER, dev); - pm_runtime_put_sync(dev); } }