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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
