diff mbox

[PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

Message ID 201104222222.01289.rjw@sisk.pl (mailing list archive)
State New, archived
Headers show

Commit Message

Rafael Wysocki April 22, 2011, 8:22 p.m. UTC
On Friday, April 22, 2011, Alan Stern wrote:
> 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.

Not only it might, but it actually does.  There are platforms currently in
the ARM tree where the runtime PM hadling of devices is set up and cleaned up
by the bus notifier, so after the notifier has run the device will be handled

That should eliminate the race between the notifier and runtime PM and still
allow the bus/driver to use runtime PM in the ->remove() callbacks.

Thanks,
Rafael
--
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

Comments

Rafael Wysocki April 22, 2011, 8:25 p.m. UTC | #1
On Friday, April 22, 2011, Rafael J. Wysocki wrote:
> On Friday, April 22, 2011, Alan Stern wrote:
> > 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.
> 
> Not only it might, but it actually does.  There are platforms currently in
> the ARM tree where the runtime PM hadling of devices is set up and cleaned up
> by the bus notifier, so after the notifier has run the device will be handled
> differently.
> 
> > Perhaps the barrier should be moved down, after the notifier call.
> > How does this patch look?
> > 
> >  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);
> 
> 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().

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

Thanks,
Rafael
--
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
Alan Stern April 22, 2011, 9:20 p.m. UTC | #2
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.  Do you know if any other subsystems rely on
the usage_count being > 0 during unbinding?

Alan Stern

--
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
diff mbox

Patch

differently.

> Perhaps the barrier should be moved down, after the notifier call.
> How does this patch look?
> 
>  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);

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