From patchwork Fri Apr 22 15:20:26 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alan Stern X-Patchwork-Id: 727541 Received: from smtp1.linux-foundation.org (smtp1.linux-foundation.org [140.211.169.13]) by demeter1.kernel.org (8.14.4/8.14.3) with ESMTP id p3MFMvpm032346 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=FAIL) for ; Fri, 22 Apr 2011 15:23:17 GMT Received: from daredevil.linux-foundation.org (localhost [127.0.0.1]) by smtp1.linux-foundation.org (8.14.2/8.13.5/Debian-3ubuntu1.1) with ESMTP id p3MFKVTf028102; Fri, 22 Apr 2011 08:20:33 -0700 Received: from iolanthe.rowland.org (iolanthe.rowland.org [192.131.102.54]) by smtp1.linux-foundation.org (8.14.2/8.13.5/Debian-3ubuntu1.1) with SMTP id p3MFKR2H028091 for ; Fri, 22 Apr 2011 08:20:28 -0700 Received: (qmail 18294 invoked by uid 2102); 22 Apr 2011 11:20:26 -0400 Received: from localhost (sendmail-bs@127.0.0.1) by localhost with SMTP; 22 Apr 2011 11:20:26 -0400 Date: Fri, 22 Apr 2011 11:20:26 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: "Rafael J. Wysocki" In-Reply-To: <201104220006.09982.rjw@sisk.pl> Message-ID: MIME-Version: 1.0 Received-SPF: pass (localhost is always allowed.) X-Spam-Status: No, hits=-3.997 required=5 tests=AWL, BAYES_00, OSDL_HEADER_SUBJECT_BRACKETED X-Spam-Checker-Version: SpamAssassin 3.2.4-osdl_revision__1.47__ X-MIMEDefang-Filter: lf$Revision: 1.188 $ X-Scanned-By: MIMEDefang 2.63 on 140.211.169.21 Cc: linux-mmc@vger.kernel.org, Magnus Damm , Linux PM mailing list , Guennadi Liakhovetski Subject: Re: [linux-pm] [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() X-BeenThere: linux-pm@lists.linux-foundation.org X-Mailman-Version: 2.1.9 Precedence: list List-Id: Linux power management 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 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 15:23:17 +0000 (UTC) On Fri, 22 Apr 2011, Rafael J. Wysocki wrote: > > The subsystem should be smart enough to handle runtime PM requests while > > the driver's remove callback is running. > > If we make such a rule, we may as well remove all of the runtime PM > calls from __device_release_driver(). > > > > I think the current code is better than any of the alternatives considered > > > so far. > > > > Then you think Guennadi should leave his patch as it is, including the > > "reversed" put/get? > > This, or we need to remove the runtime PM calls from __device_release_driver(). Let's go back to first principles. The underlying problem we want to solve is that runtime PM callbacks race with driver unbinding. In a worst-case scenario, a driver module might be unbound and unloaded from memory and then a runtime PM callback could occur, causing an invalid memory access. Related to this is the fact that some drivers want to use runtime PM from within their remove routines. This implies that the PM core shouldn't simply disallow all runtime PM callbacks during unbinding. As it happens, the PM core doesn't call drivers' runtime PM routines directly. Instead it calls bus, type, class, and power-domain routines -- which may in turn invoke the driver routines. Put together, this all suggests that the PM core can't solve the underlying problem and shouldn't try. Instead, it should be up to the subsystems to insure they don't make invalid callbacks. For example, the USB subsystem does this by explicitly doing pm_runtime_get_sync() before unbinding a driver. Other subsystems would want to use a different approach. If we add this requirement then yes, it would make sense to remove the get_noresume and put_sync calls from __device_release_driver(). We probably want to keep the barrier, though. > I'm a bit worried about the driver_sysfs_remove() and the bus notifier that > in theory may affect the runtime PM callbacks potentially executed before > ->remove() is called. The driver_sysfs_remove() call merely gets rid of a couple of symlinks in sysfs. I don't think that will impact runtime PM. The bus notifier might, however. Perhaps the barrier should be moved down, after the notifier call. How does this patch look? Alan Stern drivers/base/dd.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) Index: usb-2.6/drivers/base/dd.c =================================================================== --- usb-2.6.orig/drivers/base/dd.c +++ usb-2.6/drivers/base/dd.c @@ -316,15 +316,13 @@ static void __device_release_driver(stru drv = dev->driver; if (drv) { - pm_runtime_get_noresume(dev); - pm_runtime_barrier(dev); - driver_sysfs_remove(dev); if (dev->bus) blocking_notifier_call_chain(&dev->bus->p->bus_notifier, BUS_NOTIFY_UNBIND_DRIVER, dev); + pm_runtime_barrier(dev); if (dev->bus && dev->bus->remove) dev->bus->remove(dev); @@ -337,8 +335,6 @@ static void __device_release_driver(stru blocking_notifier_call_chain(&dev->bus->p->bus_notifier, BUS_NOTIFY_UNBOUND_DRIVER, dev); - - pm_runtime_put_sync(dev); } }