| Message ID | 201104222222.01289.rjw@sisk.pl (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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
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
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);
}
}