Linux-mmc Archive on lore.kernel.org
 help / color / Atom feed
From: Alan Stern <stern@rowland.harvard.edu>
To: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>,
	Ohad Ben-Cohen <ohad@wizery.com>,
	linux-mmc@vger.kernel.org, Magnus Damm <damm@opensource.se>,
	Simon Horman <horms@verge.net.au>,
	Linux PM mailing list <linux-pm@lists.linux-foundation.org>
Subject: Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()
Date: Fri, 22 Apr 2011 11:20:26 -0400 (EDT)
Message-ID: <Pine.LNX.4.44L0.1104221058140.1877-100000@iolanthe.rowland.org> (raw)
In-Reply-To: <201104220006.09982.rjw@sisk.pl>

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);
 	}
 }
 


  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-04-19 10:46 Guennadi Liakhovetski
2011-04-19 12:44 ` Ohad Ben-Cohen
2011-04-19 13:23   ` Guennadi Liakhovetski
2011-04-19 14:16     ` Ohad Ben-Cohen
2011-04-19 14:26     ` Alan Stern
2011-04-19 22:59       ` Guennadi Liakhovetski
2011-04-20 14:22         ` Alan Stern
2011-04-20 14:50           ` Guennadi Liakhovetski
2011-04-20 15:12             ` Alan Stern
2011-04-20 20:06               ` Rafael J. Wysocki
2011-04-20 21:16                 ` Alan Stern
2011-04-20 21:44                   ` Rafael J. Wysocki
2011-04-21 13:58                     ` Alan Stern
2011-04-21 18:00                       ` Rafael J. Wysocki
2011-04-21 18:36                         ` Alan Stern
2011-04-21 20:05                           ` Rafael J. Wysocki
2011-04-21 21:48                             ` Alan Stern
2011-04-21 22:06                               ` Rafael J. Wysocki
2011-04-22 15:20                                 ` Alan Stern [this message]
2011-04-22 20:22                                   ` Rafael J. Wysocki
2011-04-22 20:25                                     ` Rafael J. Wysocki
2011-04-22 21:20                                       ` Alan Stern
2011-04-22 22:11                                         ` Rafael J. Wysocki
2011-04-25 10:29                                           ` [linux-pm] " Rafael J. Wysocki
2011-04-26 10:44                                             ` Guennadi Liakhovetski
2011-04-26 11:51                                               ` Rafael J. Wysocki
2011-04-28 22:12                                             ` Rafael J. Wysocki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Pine.LNX.4.44L0.1104221058140.1877-100000@iolanthe.rowland.org \
    --to=stern@rowland.harvard.edu \
    --cc=damm@opensource.se \
    --cc=g.liakhovetski@gmx.de \
    --cc=horms@verge.net.au \
    --cc=linux-mmc@vger.kernel.org \
    --cc=linux-pm@lists.linux-foundation.org \
    --cc=ohad@wizery.com \
    --cc=rjw@sisk.pl \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mmc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mmc/0 linux-mmc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mmc linux-mmc/ https://lore.kernel.org/linux-mmc \
		linux-mmc@vger.kernel.org
	public-inbox-index linux-mmc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mmc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git