runtime PM usage_count during driver_probe_device()?

From 069484f8d2bb86473a271c27733e10fbfd410c2c Mon Sep 17 00:00:00 2001

Continuing on the theme of runtime PM interactions with other parts of the driver core...

In drivers/base/dd.c:driver_probe_device(), the driver core increments the usage count around ->probe():

        [...]
	pm_runtime_get_noresume(dev);
	pm_runtime_barrier(dev);
	ret = really_probe(dev, drv);
	pm_runtime_put_sync(dev);

I'm not following the reason for this. On driver's I'm familar with, it's not until ->probe where pm_runtime_enable() is called. What is being protected against here?

These seem to exist since the introduction of the runtime PM core, but I can't find any explanation.

The documentation refers to the increment by the core, but not the reasons why:

If the device bus type's or driver's ->probe() or ->remove() callback runs pm_runtime_suspend() or pm_runtime_idle() or their asynchronous counterparts, they will fail returning -EAGAIN, because the device's usage counter is incremented by the core before executing ->probe() and ->remove(). Still, it may be desirable to suspend the device as soon as ->probe() or ->remove() has finished, so the PM core uses pm_runtime_idle_sync() to invoke the subsystem-level idle callback for the device at that time.

On a side note, the bit about -EAGAIN above is not accurate with today's code. For example, __pm_runtime_suspend() returns zero when the usage count decrement is non-zero, so callers can't currently know that doing a pm_runtime_suspend() or pm_runtime_put_sync() in their ->probe() actually didn't happen.

Another curiosity is that, contrary to the above documentation, there is no usage_count increment before the bus/driver ->remove() (although there is a _get_sync/_put_sync around the sysfs_remove and notifier just before the bus/driver->remove().

Also, below is a patch for a typo in the above Documentation exerpt.

Kevin

From 069484f8d2bb86473a271c27733e10fbfd410c2c Mon Sep 17 00:00:00 2001
From: Kevin Hilman <[email protected]>
Date: Thu, 30 Jun 2011 15:07:31 -0700
Subject: [PATCH] PM: Documentation: fix typo: pm_runtime_idle_sync() doesn't exist.

Replace reference to pm_runtime_idle_sync() in the driver core with pm_runtime_put_sync() which is used in the code.

Signed-off-by Kevin Hilman <[email protected]>

---
 Documentation/power/runtime_pm.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Documentation/power/runtime_pm.txt +1 -1

index 22accb3..518d9be 100644
@@ -506,7 +506,7 @@ pm_runtime_suspend() or pm_runtime_idle() or their asynchronous counterparts,
 they will fail returning -EAGAIN, because the device's usage counter is
 incremented by the core before executing ->probe() and ->remove().  Still, it
 may be desirable to suspend the device as soon as ->probe() or ->remove() has
-finished, so the PM core uses pm_runtime_idle_sync() to invoke the
+finished, so the PM core uses pm_runtime_put_sync() to invoke the
 subsystem-level idle callback for the device at that time.
 
 The user space can effectively disallow the driver of the device to power manage

Continuing on the theme of runtime PM interactions with other parts of the driver core...

In drivers/base/dd.c:driver_probe_device(), the driver core increments the usage count around ->probe():

        [...]
	pm_runtime_get_noresume(dev);
	pm_runtime_barrier(dev);
	ret = really_probe(dev, drv);
	pm_runtime_put_sync(dev);

I'm not following the reason for this. On driver's I'm familar with, it's not until ->probe where pm_runtime_enable() is called. What is being protected against here?

These seem to exist since the introduction of the runtime PM core, but I can't find any explanation.

The documentation refers to the increment by the core, but not the reasons why:

If the device bus type's or driver's ->probe() or ->remove() callback runs pm_runtime_suspend() or pm_runtime_idle() or their asynchronous counterparts, they will fail returning -EAGAIN, because the device's usage counter is incremented by the core before executing ->probe() and ->remove(). Still, it may be desirable to suspend the device as soon as ->probe() or ->remove() has finished, so the PM core uses pm_runtime_idle_sync() to invoke the subsystem-level idle callback for the device at that time.

On a side note, the bit about -EAGAIN above is not accurate with today's code. For example, __pm_runtime_suspend() returns zero when the usage count decrement is non-zero, so callers can't currently know that doing a pm_runtime_suspend() or pm_runtime_put_sync() in their ->probe() actually didn't happen.

Another curiosity is that, contrary to the above documentation, there is no usage_count increment before the bus/driver ->remove() (although there is a _get_sync/_put_sync around the sysfs_remove and notifier just before the bus/driver->remove().

Also, below is a patch for a typo in the above Documentation exerpt.

Kevin

From 069484f8d2bb86473a271c27733e10fbfd410c2c Mon Sep 17 00:00:00 2001
From: Kevin Hilman <[email protected]>
Date: Thu, 30 Jun 2011 15:07:31 -0700
Subject: [PATCH] PM: Documentation: fix typo: pm_runtime_idle_sync() doesn't exist.

Replace reference to pm_runtime_idle_sync() in the driver core with pm_runtime_put_sync() which is used in the code.

Signed-off-by Kevin Hilman <[email protected]>

---
 Documentation/power/runtime_pm.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Documentation/power/runtime_pm.txt +1 -1

index 22accb3..518d9be 100644
@@ -506,7 +506,7 @@ pm_runtime_suspend() or pm_runtime_idle() or their asynchronous counterparts,
 they will fail returning -EAGAIN, because the device's usage counter is
 incremented by the core before executing ->probe() and ->remove().  Still, it
 may be desirable to suspend the device as soon as ->probe() or ->remove() has
-finished, so the PM core uses pm_runtime_idle_sync() to invoke the
+finished, so the PM core uses pm_runtime_put_sync() to invoke the
 subsystem-level idle callback for the device at that time.
 
 The user space can effectively disallow the driver of the device to power manage

Quoted 31 lines · Kevin Hilman

Continuing on the theme of runtime PM interactions with other parts of the driver core...

In drivers/base/dd.c:driver_probe_device(), the driver core increments the usage count around ->probe():

        [...]
	pm_runtime_get_noresume(dev);
	pm_runtime_barrier(dev);
	ret = really_probe(dev, drv);
	pm_runtime_put_sync(dev);

I'm not following the reason for this. On driver's I'm familar with, it's not until ->probe where pm_runtime_enable() is called. What is being protected against here?

These seem to exist since the introduction of the runtime PM core, but I can't find any explanation.

The documentation refers to the increment by the core, but not the reasons why:

If the device bus type's or driver's ->probe() or ->remove() callback runs pm_runtime_suspend() or pm_runtime_idle() or their asynchronous counterparts, they will fail returning -EAGAIN, because the device's usage counter is incremented by the core before executing ->probe() and ->remove(). Still, it may be desirable to suspend the device as soon as ->probe() or ->remove() has finished, so the PM core uses pm_runtime_idle_sync() to invoke the subsystem-level idle callback for the device at that time.

Quoted 5 lines

On a side note, the bit about -EAGAIN above is not accurate with today's code. For example, __pm_runtime_suspend() returns zero when the usage count decrement is non-zero, so callers can't currently know that doing a pm_runtime_suspend() or pm_runtime_put_sync() in their ->probe() actually didn't happen.

Oops, I'm not quite right here.

The doc is actually right here if the driver uses pm_runtime_suspend() or pm_runtime_idle() in ->probe(). It's only when drivers use pm_runtime_put_sync() where there wouldn't be an -EAGAIN, but that seems correct. Sorry for the noise.

I'm still confused about the usage_count increment around ->probe though.

In theory it is possible that a subsystem (e.g. bus type) will enable runtime PM for devices without drivers and will (for example) put them into low power states until the drivers are loaded.  Then, it makes sense for the core to prevent such transitions from racing with .probe().

I'm not sure if this happens in practice, though,

In theory it is possible that a subsystem (e.g. bus type) will enable runtime PM for devices without drivers and will (for example) put them into low power states until the drivers are loaded.  Then, it makes sense for the core to prevent such transitions from racing with .probe().

I'm not sure if this happens in practice, though,

Quoted 6 lines · Rafael J. Wysocki

In theory it is possible that a subsystem (e.g. bus type) will enable runtime PM for devices without drivers and will (for example) put them into low power states until the drivers are loaded.  Then, it makes sense for the core to prevent such transitions from racing with .probe().

I'm not sure if this happens in practice, though,

Yeah, this is exactly what the SDIO subsystem is doing.

Quoted 6 lines · Rafael J. Wysocki

In theory it is possible that a subsystem (e.g. bus type) will enable runtime PM for devices without drivers and will (for example) put them into low power states until the drivers are loaded.  Then, it makes sense for the core to prevent such transitions from racing with .probe().

I'm not sure if this happens in practice, though,

Yeah, this is exactly what the SDIO subsystem is doing.

In theory it is possible that a subsystem (e.g. bus type) will enable runtime PM for devices without drivers and will (for example) put them into low power states until the drivers are loaded.  Then, it makes sense for the core to prevent such transitions from racing with .probe().

I'm not sure if this happens in practice, though, but also I'm not sure whether or not that's used by USB.  Moreover, even if that doesn't happen right now, it may start to happen at one point.

In theory it is possible that a subsystem (e.g. bus type) will enable runtime PM for devices without drivers and will (for example) put them into low power states until the drivers are loaded.  Then, it makes sense for the core to prevent such transitions from racing with .probe().

I'm not sure if this happens in practice, though, but also I'm not sure whether or not that's used by USB.  Moreover, even if that doesn't happen right now, it may start to happen at one point.

Quoted 31 lines · Kevin Hilman

Continuing on the theme of runtime PM interactions with other parts of the driver core...

In drivers/base/dd.c:driver_probe_device(), the driver core increments the usage count around ->probe():

        [...]
	pm_runtime_get_noresume(dev);
	pm_runtime_barrier(dev);
	ret = really_probe(dev, drv);
	pm_runtime_put_sync(dev);

I'm not following the reason for this. On driver's I'm familar with, it's not until ->probe where pm_runtime_enable() is called. What is being protected against here?

These seem to exist since the introduction of the runtime PM core, but I can't find any explanation.

The documentation refers to the increment by the core, but not the reasons why:

If the device bus type's or driver's ->probe() or ->remove() callback runs pm_runtime_suspend() or pm_runtime_idle() or their asynchronous counterparts, they will fail returning -EAGAIN, because the device's usage counter is incremented by the core before executing ->probe() and ->remove(). Still, it may be desirable to suspend the device as soon as ->probe() or ->remove() has finished, so the PM core uses pm_runtime_idle_sync() to invoke the subsystem-level idle callback for the device at that time.

Quoted 5 lines

On a side note, the bit about -EAGAIN above is not accurate with today's code. For example, __pm_runtime_suspend() returns zero when the usage count decrement is non-zero, so callers can't currently know that doing a pm_runtime_suspend() or pm_runtime_put_sync() in their ->probe() actually didn't happen.

Oops, I'm not quite right here.

The doc is actually right here if the driver uses pm_runtime_suspend() or pm_runtime_idle() in ->probe(). It's only when drivers use pm_runtime_put_sync() where there wouldn't be an -EAGAIN, but that seems correct. Sorry for the noise.

I'm still confused about the usage_count increment around ->probe though.

I'm still confused about the usage_count increment around ->probe though.

I'm still confused about the usage_count increment around ->probe though.

This patch (as1475) adds device_lock() and device_unlock() calls to the store methods for the power/control and power/autosuspend_delay_ms sysfs attribute files. We don't want badly timed writes to these files to cause runtime_resume callbacks to occur while a driver is being probed for a device.

Signed-off-by Alan Stern <[email protected]>

---

 drivers/base/power/sysfs.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: usb-3.0/drivers/base/power/sysfs.c
===================================================================

drivers/base/power/sysfs.c +5 -1

@@ -116,12 +116,14 @@ static ssize_t control_store(struct devi
 	cp = memchr(buf, '\n', n);
 	if (cp)
 		len = cp - buf;
+	device_lock(dev);
 	if (len == sizeof ctrl_auto - 1 && strncmp(buf, ctrl_auto, len) == 0)
 		pm_runtime_allow(dev);
 	else if (len == sizeof ctrl_on - 1 && strncmp(buf, ctrl_on, len) == 0)
 		pm_runtime_forbid(dev);
 	else
-		return -EINVAL;
+		n = -EINVAL;
+	device_unlock(dev);
 	return n;
 }
 

@@ -205,7 +207,9 @@ static ssize_t autosuspend_delay_ms_stor
 	if (strict_strtol(buf, 10, &delay) != 0 || delay != (int) delay)
 		return -EINVAL;
 
+	device_lock(dev);
 	pm_runtime_set_autosuspend_delay(dev, delay);
+	device_unlock(dev);
 	return n;
 }
 

Another curiosity is that, contrary to the above documentation, there is no usage_count increment before the bus/driver ->remove() (although there is a _get_sync/_put_sync around the sysfs_remove and notifier just before the bus/driver->remove().

Another curiosity is that, contrary to the above documentation, there is no usage_count increment before the bus/driver ->remove() (although there is a _get_sync/_put_sync around the sysfs_remove and notifier just before the bus/driver->remove().

If the device bus type's or driver's ->probe() or ->remove() callback runs pm_runtime_suspend() or pm_runtime_idle() or their asynchronous counterparts, they will fail returning -EAGAIN, because the device's usage counter is incremented by the core before executing ->probe() and ->remove(). Still, it may be desirable to suspend the device as soon as ->probe() or ->remove() has finished, so the PM core uses pm_runtime_idle_sync() to invoke the subsystem-level idle callback for the device at that time.

Another curiosity is that, contrary to the above documentation, there is no usage_count increment before the bus/driver ->remove() (although there is a _get_sync/_put_sync around the sysfs_remove and notifier just before the bus/driver->remove().

From eef73ab2feb203bacb57dc35862f2a9969b61593 Mon Sep 17 00:00:00 2001

If the device bus type's or driver's ->probe() or ->remove() callback runs pm_runtime_suspend() or pm_runtime_idle() or their asynchronous counterparts, they will fail returning -EAGAIN, because the device's usage counter is incremented by the core before executing ->probe() and ->remove(). Still, it may be desirable to suspend the device as soon as ->probe() or ->remove() has finished, so the PM core uses pm_runtime_idle_sync() to invoke the subsystem-level idle callback for the device at that time.

Another curiosity is that, contrary to the above documentation, there is no usage_count increment before the bus/driver ->remove() (although there is a _get_sync/_put_sync around the sysfs_remove and notifier just before the bus/driver->remove().

From eef73ab2feb203bacb57dc35862f2a9969b61593 Mon Sep 17 00:00:00 2001

OK, so the ->probe() part has been explained and makes sense, but I would expect ->remove() to be similarily protected (as the documentation states.) But that is not the case. Is that a bug? If so, patch below makes the code match the documentation.

Kevin

From eef73ab2feb203bacb57dc35862f2a9969b61593 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <[email protected]>
Date: Fri, 1 Jul 2011 07:37:47 -0700
Subject: [PATCH] driver core: prevent runtime PM races with ->remove()

Runtime PM Documentation states that the runtime PM usage count is incremented during driver ->probe() and ->remove(). This is designed to prevent driver runtime PM races with subsystems which may initiate runtime PM transitions before during and after drivers are loaded.

Current code increments the usage_count during ->probe() but not during ->remove(). This patch fixes the ->remove() part and makes the code match the documentation.

Signed-off-by Kevin Hilman <[email protected]>

---
 drivers/base/dd.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

drivers/base/dd.c +0 -3

index 6658da7..47e079d 100644
@@ -329,13 +329,13 @@ static void __device_release_driver(struct device *dev)
 			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 						     BUS_NOTIFY_UNBIND_DRIVER,
 						     dev);
-
-		pm_runtime_put_sync(dev);
-

OK, so the ->probe() part has been explained and makes sense, but I would expect ->remove() to be similarily protected (as the documentation states.) But that is not the case. Is that a bug? If so, patch below makes the code match the documentation.

Kevin

From eef73ab2feb203bacb57dc35862f2a9969b61593 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <[email protected]>
Date: Fri, 1 Jul 2011 07:37:47 -0700
Subject: [PATCH] driver core: prevent runtime PM races with ->remove()

Runtime PM Documentation states that the runtime PM usage count is incremented during driver ->probe() and ->remove(). This is designed to prevent driver runtime PM races with subsystems which may initiate runtime PM transitions before during and after drivers are loaded.

Current code increments the usage_count during ->probe() but not during ->remove(). This patch fixes the ->remove() part and makes the code match the documentation.

Signed-off-by Kevin Hilman <[email protected]>

---
 drivers/base/dd.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

drivers/base/dd.c +0 -3

index 6658da7..47e079d 100644
@@ -329,13 +329,13 @@ static void __device_release_driver(struct device *dev)
 			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 						     BUS_NOTIFY_UNBIND_DRIVER,
 						     dev);
-
-		pm_runtime_put_sync(dev);
-

Quoted 4 lines · Kevin Hilman

OK, so the ->probe() part has been explained and makes sense, but I would expect ->remove() to be similarily protected (as the documentation states.) But that is not the case. Is that a bug? If so, patch below makes the code match the documentation.

I suspect it is a bug, but it's hard to be sure. It's so _blatantly_ wrong that it looks like it was done deliberately.

Quoted 37 lines

Kevin

From eef73ab2feb203bacb57dc35862f2a9969b61593 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <[email protected]>
Date: Fri, 1 Jul 2011 07:37:47 -0700
Subject: [PATCH] driver core: prevent runtime PM races with ->remove()

Runtime PM Documentation states that the runtime PM usage count is incremented during driver ->probe() and ->remove(). This is designed to prevent driver runtime PM races with subsystems which may initiate runtime PM transitions before during and after drivers are loaded.

Current code increments the usage_count during ->probe() but not during ->remove(). This patch fixes the ->remove() part and makes the code match the documentation.

Signed-off-by Kevin Hilman <[email protected]>

---
 drivers/base/dd.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

drivers/base/dd.c +1 -3

index 6658da7..47e079d 100644
@@ -329,13 +329,13 @@ static void __device_release_driver(struct device *dev)
 			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 						     BUS_NOTIFY_UNBIND_DRIVER,
 						     dev);
-
-		pm_runtime_put_sync(dev);
-
 		if (dev->bus && dev->bus->remove)
 			dev->bus->remove(dev);
 		else if (drv->remove)
 			drv->remove(dev);
+

drivers/base/dd.c static void __device_release_driver(struct device *dev) · line 341

Quoted 5 lines

drivers/base/dd.c +2 -0

+		pm_runtime_put_sync(dev);
+
 		devres_release_all(dev);
 		dev->driver = NULL;
 		klist_remove(&dev->p->knode_driver);

To be safer, the put_sync() call should be moved down here. Or maybe even after the blocking_notifier_call_chain() that occurs here.

Quoted 4 lines · Kevin Hilman

OK, so the ->probe() part has been explained and makes sense, but I would expect ->remove() to be similarily protected (as the documentation states.) But that is not the case. Is that a bug? If so, patch below makes the code match the documentation.

I suspect it is a bug, but it's hard to be sure. It's so _blatantly_ wrong that it looks like it was done deliberately.

Quoted 37 lines

Kevin

From eef73ab2feb203bacb57dc35862f2a9969b61593 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <[email protected]>
Date: Fri, 1 Jul 2011 07:37:47 -0700
Subject: [PATCH] driver core: prevent runtime PM races with ->remove()

Runtime PM Documentation states that the runtime PM usage count is incremented during driver ->probe() and ->remove(). This is designed to prevent driver runtime PM races with subsystems which may initiate runtime PM transitions before during and after drivers are loaded.

Current code increments the usage_count during ->probe() but not during ->remove(). This patch fixes the ->remove() part and makes the code match the documentation.

Signed-off-by Kevin Hilman <[email protected]>

---
 drivers/base/dd.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

drivers/base/dd.c +1 -3

index 6658da7..47e079d 100644
@@ -329,13 +329,13 @@ static void __device_release_driver(struct device *dev)
 			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 						     BUS_NOTIFY_UNBIND_DRIVER,
 						     dev);
-
-		pm_runtime_put_sync(dev);
-
 		if (dev->bus && dev->bus->remove)
 			dev->bus->remove(dev);
 		else if (drv->remove)
 			drv->remove(dev);
+

drivers/base/dd.c static void __device_release_driver(struct device *dev) · line 341

Quoted 5 lines

drivers/base/dd.c +2 -0

+		pm_runtime_put_sync(dev);
+
 		devres_release_all(dev);
 		dev->driver = NULL;
 		klist_remove(&dev->p->knode_driver);

To be safer, the put_sync() call should be moved down here. Or maybe even after the blocking_notifier_call_chain() that occurs here.

Quoted 14 lines

Quoted 14 lines

drivers/base/dd.c +1 -3

@@ -329,13 +329,13 @@ static void __device_release_driver(struct device *dev)
 			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 						     BUS_NOTIFY_UNBIND_DRIVER,
 						     dev);
-
-		pm_runtime_put_sync(dev);
-
 		if (dev->bus && dev->bus->remove)
 			dev->bus->remove(dev);
 		else if (drv->remove)
 			drv->remove(dev);
+

Quoted 8 lines

drivers/base/dd.c static void __device_release_driver(struct device *dev) · line 341

Quoted 5 lines

drivers/base/dd.c +2 -0

+		pm_runtime_put_sync(dev);
+
 		devres_release_all(dev);
 		dev->driver = NULL;
 		klist_remove(&dev->p->knode_driver);

To be safer, the put_sync() call should be moved down here. Or maybe even after the blocking_notifier_call_chain() that occurs here.

I was actually thinking about the other direction: moving the get_sync after the first notifier chain. IOW, the get_sync/put_sync only protects the ->remove() calls, not the notifiers.

The protection around the notifiers doesn't make sense to me, at least in the context of driver runtime PM racing with the subsystem. Especially since these notifiers are likely how the subsystem/bus/pm_domain code getting notified that there may be a device to manage in the first place.

Quoted 14 lines

Quoted 14 lines

drivers/base/dd.c +1 -3

@@ -329,13 +329,13 @@ static void __device_release_driver(struct device *dev)
 			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 						     BUS_NOTIFY_UNBIND_DRIVER,
 						     dev);
-
-		pm_runtime_put_sync(dev);
-
 		if (dev->bus && dev->bus->remove)
 			dev->bus->remove(dev);
 		else if (drv->remove)
 			drv->remove(dev);
+

Quoted 8 lines

drivers/base/dd.c static void __device_release_driver(struct device *dev) · line 341

Quoted 5 lines

drivers/base/dd.c +2 -0

+		pm_runtime_put_sync(dev);
+
 		devres_release_all(dev);
 		dev->driver = NULL;
 		klist_remove(&dev->p->knode_driver);

To be safer, the put_sync() call should be moved down here. Or maybe even after the blocking_notifier_call_chain() that occurs here.

I was actually thinking about the other direction: moving the get_sync after the first notifier chain. IOW, the get_sync/put_sync only protects the ->remove() calls, not the notifiers.

The protection around the notifiers doesn't make sense to me, at least in the context of driver runtime PM racing with the subsystem. Especially since these notifiers are likely how the subsystem/bus/pm_domain code getting notified that there may be a device to manage in the first place.

Quoted 27 lines · Kevin Hilman

Quoted 14 lines

Quoted 14 lines

drivers/base/dd.c +1 -3

@@ -329,13 +329,13 @@ static void __device_release_driver(struct device *dev)
 			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 						     BUS_NOTIFY_UNBIND_DRIVER,
 						     dev);
-
-		pm_runtime_put_sync(dev);
-
 		if (dev->bus && dev->bus->remove)
 			dev->bus->remove(dev);
 		else if (drv->remove)
 			drv->remove(dev);
+

Quoted 8 lines

drivers/base/dd.c static void __device_release_driver(struct device *dev) · line 341

Quoted 5 lines

drivers/base/dd.c +2 -0

+		pm_runtime_put_sync(dev);
+
 		devres_release_all(dev);
 		dev->driver = NULL;
 		klist_remove(&dev->p->knode_driver);

To be safer, the put_sync() call should be moved down here. Or maybe even after the blocking_notifier_call_chain() that occurs here.

I was actually thinking about the other direction: moving the get_sync after the first notifier chain. IOW, the get_sync/put_sync only protects the ->remove() calls, not the notifiers.

Quoted 5 lines

The protection around the notifiers doesn't make sense to me, at least in the context of driver runtime PM racing with the subsystem. Especially since these notifiers are likely how the subsystem/bus/pm_domain code getting notified that there may be a device to manage in the first place.

The get_sync part doesn't matter so much. Moving it past the notifier call would probably be okay -- unless one of the listeners on the notifier chain expects the device to be active. Changing the get_sync to get_noresume would probably also be okay -- subject to a similar reservation.

The problem with the put_sync isn't the notifier. If you leave it where you've got it now, you'll end up invoking a callback at a time when the driver thinks it no longer controls the device but the driver-model core still thinks it does. You certainly want to do the

	dev->driver = NULL;

first.

Alan Stern

Quoted 27 lines · Kevin Hilman

Quoted 14 lines

Quoted 14 lines

drivers/base/dd.c +1 -3

@@ -329,13 +329,13 @@ static void __device_release_driver(struct device *dev)
 			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 						     BUS_NOTIFY_UNBIND_DRIVER,
 						     dev);
-
-		pm_runtime_put_sync(dev);
-
 		if (dev->bus && dev->bus->remove)
 			dev->bus->remove(dev);
 		else if (drv->remove)
 			drv->remove(dev);
+

Quoted 8 lines

drivers/base/dd.c static void __device_release_driver(struct device *dev) · line 341

Quoted 5 lines

drivers/base/dd.c +2 -0

+		pm_runtime_put_sync(dev);
+
 		devres_release_all(dev);
 		dev->driver = NULL;
 		klist_remove(&dev->p->knode_driver);

To be safer, the put_sync() call should be moved down here. Or maybe even after the blocking_notifier_call_chain() that occurs here.

I was actually thinking about the other direction: moving the get_sync after the first notifier chain. IOW, the get_sync/put_sync only protects the ->remove() calls, not the notifiers.

Quoted 5 lines

The protection around the notifiers doesn't make sense to me, at least in the context of driver runtime PM racing with the subsystem. Especially since these notifiers are likely how the subsystem/bus/pm_domain code getting notified that there may be a device to manage in the first place.

The get_sync part doesn't matter so much. Moving it past the notifier call would probably be okay -- unless one of the listeners on the notifier chain expects the device to be active. Changing the get_sync to get_noresume would probably also be okay -- subject to a similar reservation.

The problem with the put_sync isn't the notifier. If you leave it where you've got it now, you'll end up invoking a callback at a time when the driver thinks it no longer controls the device but the driver-model core still thinks it does. You certainly want to do the

	dev->driver = NULL;

first.

Alan Stern

Quoted 7 lines · Alan Stern

Quoted 4 lines · Kevin Hilman

OK, so the ->probe() part has been explained and makes sense, but I would expect ->remove() to be similarily protected (as the documentation states.) But that is not the case. Is that a bug? If so, patch below makes the code match the documentation.

Quoted 2 lines

I suspect it is a bug, but it's hard to be sure. It's so _blatantly_ wrong that it looks like it was done deliberately.

heh

Hi,

Quoted 9 lines · Kevin Hilman

Quoted 7 lines · Alan Stern

Quoted 4 lines · Kevin Hilman

OK, so the ->probe() part has been explained and makes sense, but I would expect ->remove() to be similarily protected (as the documentation states.) But that is not the case. Is that a bug? If so, patch below makes the code match the documentation.

Quoted 4 lines

Quoted 2 lines

I suspect it is a bug, but it's hard to be sure. It's so _blatantly_ wrong that it looks like it was done deliberately.

heh

I seem to remeber having a problem with the pm_runtime_put_sync() after drv->remove(dev) ...

So the code in question was introduced by

commit e1866b33b1e89f077b7132daae3dfd9a594e9a1a

Author: Rafael J. Wysocki <[email protected]>
Date:   Fri Apr 29 00:33:45 2011 +0200

PM / Runtime: Rework runtime PM handling during driver removal

with a long changelog explaining the reason why. Which seems to make sense. ;-)

Hi,

Quoted 9 lines · Kevin Hilman

Quoted 7 lines · Alan Stern

Quoted 4 lines · Kevin Hilman

OK, so the ->probe() part has been explained and makes sense, but I would expect ->remove() to be similarily protected (as the documentation states.) But that is not the case. Is that a bug? If so, patch below makes the code match the documentation.

Quoted 4 lines

Quoted 2 lines

I suspect it is a bug, but it's hard to be sure. It's so _blatantly_ wrong that it looks like it was done deliberately.

heh

I seem to remeber having a problem with the pm_runtime_put_sync() after drv->remove(dev) ...

So the code in question was introduced by

commit e1866b33b1e89f077b7132daae3dfd9a594e9a1a

Author: Rafael J. Wysocki <[email protected]>
Date:   Fri Apr 29 00:33:45 2011 +0200

PM / Runtime: Rework runtime PM handling during driver removal

with a long changelog explaining the reason why. Which seems to make sense. ;-)

Quoted 26 lines · Rafael J. Wysocki

Hi,

Quoted 9 lines · Kevin Hilman

Quoted 7 lines · Alan Stern

Quoted 4 lines · Kevin Hilman

OK, so the ->probe() part has been explained and makes sense, but I would expect ->remove() to be similarily protected (as the documentation states.) But that is not the case. Is that a bug? If so, patch below makes the code match the documentation.

Quoted 4 lines

Quoted 2 lines

I suspect it is a bug, but it's hard to be sure. It's so _blatantly_ wrong that it looks like it was done deliberately.

heh

I seem to remeber having a problem with the pm_runtime_put_sync() after drv->remove(dev) ...

So the code in question was introduced by

commit e1866b33b1e89f077b7132daae3dfd9a594e9a1a

Author: Rafael J. Wysocki <[email protected]>
Date:   Fri Apr 29 00:33:45 2011 +0200

Quoted 3 lines

PM / Runtime: Rework runtime PM handling during driver removal

with a long changelog explaining the reason why. Which seems to make sense. ;-)

Okay, that seems fair enough. Looks like the documentation needs to be updated to match, though.

And we probably still want to make sure that access to the power/control and related attribute files is mutually exclusive with probe and remove.

Quoted 26 lines · Rafael J. Wysocki

Hi,

Quoted 9 lines · Kevin Hilman

Quoted 7 lines · Alan Stern

Quoted 4 lines · Kevin Hilman

OK, so the ->probe() part has been explained and makes sense, but I would expect ->remove() to be similarily protected (as the documentation states.) But that is not the case. Is that a bug? If so, patch below makes the code match the documentation.

Quoted 4 lines

Quoted 2 lines

I suspect it is a bug, but it's hard to be sure. It's so _blatantly_ wrong that it looks like it was done deliberately.

heh

I seem to remeber having a problem with the pm_runtime_put_sync() after drv->remove(dev) ...

So the code in question was introduced by

commit e1866b33b1e89f077b7132daae3dfd9a594e9a1a

Author: Rafael J. Wysocki <[email protected]>
Date:   Fri Apr 29 00:33:45 2011 +0200

Quoted 3 lines

PM / Runtime: Rework runtime PM handling during driver removal

with a long changelog explaining the reason why. Which seems to make sense. ;-)

Okay, that seems fair enough. Looks like the documentation needs to be updated to match, though.

And we probably still want to make sure that access to the power/control and related attribute files is mutually exclusive with probe and remove.

From: Rafael J. Wysocki <[email protected]>

Commit e1866b33b1e89f077b7132daae3dfd9a594e9a1a (PM / Runtime: Rework runtime PM handling during driver removal) forgot to update the documentation in Documentation/power/runtime_pm.txt to match the new code in drivers/base/dd.c. Update that documentation to match the code it describes.

Signed-off-by Rafael J. Wysocki <[email protected]>

From: Rafael J. Wysocki <[email protected]>

Commit e1866b33b1e89f077b7132daae3dfd9a594e9a1a (PM / Runtime: Rework runtime PM handling during driver removal) forgot to update the documentation in Documentation/power/runtime_pm.txt to match the new code in drivers/base/dd.c. Update that documentation to match the code it describes.

Signed-off-by Rafael J. Wysocki <[email protected]>

Quoted 7 lines · Alan Stern

Quoted 4 lines · Kevin Hilman

OK, so the ->probe() part has been explained and makes sense, but I would expect ->remove() to be similarily protected (as the documentation states.) But that is not the case. Is that a bug? If so, patch below makes the code match the documentation.

Quoted 2 lines

I suspect it is a bug, but it's hard to be sure. It's so _blatantly_ wrong that it looks like it was done deliberately.

heh

Continuing on the theme of runtime PM interactions with other parts of the driver core...

In drivers/base/dd.c:driver_probe_device(), the driver core increments the usage count around ->probe():

        [...]
	pm_runtime_get_noresume(dev);
	pm_runtime_barrier(dev);
	ret = really_probe(dev, drv);
	pm_runtime_put_sync(dev);

I'm not following the reason for this. On driver's I'm familar with, it's not until ->probe where pm_runtime_enable() is called. What is being protected against here?

These seem to exist since the introduction of the runtime PM core, but I can't find any explanation.

The documentation refers to the increment by the core, but not the reasons why:

If the device bus type's or driver's ->probe() or ->remove() callback runs pm_runtime_suspend() or pm_runtime_idle() or their asynchronous counterparts, they will fail returning -EAGAIN, because the device's usage counter is incremented by the core before executing ->probe() and ->remove(). Still, it may be desirable to suspend the device as soon as ->probe() or ->remove() has finished, so the PM core uses pm_runtime_idle_sync() to invoke the subsystem-level idle callback for the device at that time.

On a side note, the bit about -EAGAIN above is not accurate with today's code. For example, __pm_runtime_suspend() returns zero when the usage count decrement is non-zero, so callers can't currently know that doing a pm_runtime_suspend() or pm_runtime_put_sync() in their ->probe() actually didn't happen.

Another curiosity is that, contrary to the above documentation, there is no usage_count increment before the bus/driver ->remove() (although there is a _get_sync/_put_sync around the sysfs_remove and notifier just before the bus/driver->remove().

Also, below is a patch for a typo in the above Documentation exerpt.

Kevin

From 069484f8d2bb86473a271c27733e10fbfd410c2c Mon Sep 17 00:00:00 2001
From: Kevin Hilman <[email protected]>
Date: Thu, 30 Jun 2011 15:07:31 -0700
Subject: [PATCH] PM: Documentation: fix typo: pm_runtime_idle_sync() doesn't exist.

Replace reference to pm_runtime_idle_sync() in the driver core with pm_runtime_put_sync() which is used in the code.

Signed-off-by Kevin Hilman <[email protected]>

---
 Documentation/power/runtime_pm.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Documentation/power/runtime_pm.txt +1 -1

index 22accb3..518d9be 100644
@@ -506,7 +506,7 @@ pm_runtime_suspend() or pm_runtime_idle() or their asynchronous counterparts,
 they will fail returning -EAGAIN, because the device's usage counter is
 incremented by the core before executing ->probe() and ->remove().  Still, it
 may be desirable to suspend the device as soon as ->probe() or ->remove() has
-finished, so the PM core uses pm_runtime_idle_sync() to invoke the
+finished, so the PM core uses pm_runtime_put_sync() to invoke the
 subsystem-level idle callback for the device at that time.
 
 The user space can effectively disallow the driver of the device to power manage

Continuing on the theme of runtime PM interactions with other parts of the driver core...

In drivers/base/dd.c:driver_probe_device(), the driver core increments the usage count around ->probe():

        [...]
	pm_runtime_get_noresume(dev);
	pm_runtime_barrier(dev);
	ret = really_probe(dev, drv);
	pm_runtime_put_sync(dev);

I'm not following the reason for this. On driver's I'm familar with, it's not until ->probe where pm_runtime_enable() is called. What is being protected against here?

These seem to exist since the introduction of the runtime PM core, but I can't find any explanation.

The documentation refers to the increment by the core, but not the reasons why:

If the device bus type's or driver's ->probe() or ->remove() callback runs pm_runtime_suspend() or pm_runtime_idle() or their asynchronous counterparts, they will fail returning -EAGAIN, because the device's usage counter is incremented by the core before executing ->probe() and ->remove(). Still, it may be desirable to suspend the device as soon as ->probe() or ->remove() has finished, so the PM core uses pm_runtime_idle_sync() to invoke the subsystem-level idle callback for the device at that time.

On a side note, the bit about -EAGAIN above is not accurate with today's code. For example, __pm_runtime_suspend() returns zero when the usage count decrement is non-zero, so callers can't currently know that doing a pm_runtime_suspend() or pm_runtime_put_sync() in their ->probe() actually didn't happen.

Another curiosity is that, contrary to the above documentation, there is no usage_count increment before the bus/driver ->remove() (although there is a _get_sync/_put_sync around the sysfs_remove and notifier just before the bus/driver->remove().

Also, below is a patch for a typo in the above Documentation exerpt.

Kevin

From 069484f8d2bb86473a271c27733e10fbfd410c2c Mon Sep 17 00:00:00 2001
From: Kevin Hilman <[email protected]>
Date: Thu, 30 Jun 2011 15:07:31 -0700
Subject: [PATCH] PM: Documentation: fix typo: pm_runtime_idle_sync() doesn't exist.

Replace reference to pm_runtime_idle_sync() in the driver core with pm_runtime_put_sync() which is used in the code.

Signed-off-by Kevin Hilman <[email protected]>

---
 Documentation/power/runtime_pm.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Documentation/power/runtime_pm.txt +1 -1

index 22accb3..518d9be 100644
@@ -506,7 +506,7 @@ pm_runtime_suspend() or pm_runtime_idle() or their asynchronous counterparts,
 they will fail returning -EAGAIN, because the device's usage counter is
 incremented by the core before executing ->probe() and ->remove().  Still, it
 may be desirable to suspend the device as soon as ->probe() or ->remove() has
-finished, so the PM core uses pm_runtime_idle_sync() to invoke the
+finished, so the PM core uses pm_runtime_put_sync() to invoke the
 subsystem-level idle callback for the device at that time.
 
 The user space can effectively disallow the driver of the device to power manage