* use of pm_runtime_disable() from driver probe? @ 2012-07-06 22:29 Kevin Hilman 2012-07-07 19:25 ` Rafael J. Wysocki 0 siblings, 1 reply; 16+ messages in thread From: Kevin Hilman @ 2012-07-06 22:29 UTC (permalink / raw) To: linux-pm Hello, I've got a question around the proper use of pm_runtime_disable() in a driver's ->probe method. Basically, using pm_runtime_disable() in ->probe() (e.g because of a error/failure) can result in the device remaning runtime enabled forever. This is because the driver core wraps the probe in _get_noresume()/_put_sync() so any usage of _put_sync() in ->probe itself does not trigger an actual device runtime suspend. If the driver also calls pm_runtime_disable() in probe, that results in the device remaining runtime enabled. Consider this example (based on drivers/mmc/host/omap_hsmmc.c). In the normal/success path, everything works fine. But in the failure case, the after the failed probe, the driver calls pm_runtime_disable() and the device is left runtime enabled: driver_probe() { /* usage count == 1; due to _get_noresume() in driver core */ pm_runtime_enable(); pm_runtime_get_sync() /* usage_count == 2 */ /* probe HW */ if (failure) goto error; /* otherwise, finish HW init */ pm_runtime_put_sync(); /* usage_count == 1 */ /* _put_sync() in driver core will make count = 0 * and device will be runtime suspended */ return 0; error: pm_runtime_put_sync(); /* usage_count = 1 */ pm_runtime_disable(); /* Now, the _put_sync() in driver core will not actually suspend since runtime PM has been disabled. */ } So, the question is: is it right to use pm_runtime_disable() this way? Removing it means the device is left in runtime suspended state even in the error case. However, without the pm_runtime_disable(), if this driver was a module, repeated attempts to load would result in unbalalced calls to pm_runtime_enable. What is the right way to handle the runtime PM enable/disable in a failed probe attempt? Thanks, Kevin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: use of pm_runtime_disable() from driver probe? 2012-07-06 22:29 use of pm_runtime_disable() from driver probe? Kevin Hilman @ 2012-07-07 19:25 ` Rafael J. Wysocki 2012-07-08 2:01 ` Alan Stern 2012-07-10 18:11 ` Kevin Hilman 0 siblings, 2 replies; 16+ messages in thread From: Rafael J. Wysocki @ 2012-07-07 19:25 UTC (permalink / raw) To: Kevin Hilman; +Cc: linux-pm Hi, On Saturday, July 07, 2012, Kevin Hilman wrote: > Hello, > > I've got a question around the proper use of pm_runtime_disable() in a > driver's ->probe method. Basically, using pm_runtime_disable() in > ->probe() (e.g because of a error/failure) can result in the device > remaning runtime enabled forever. Er, no. > This is because the driver core wraps > the probe in _get_noresume()/_put_sync() so any usage of _put_sync() in > ->probe itself does not trigger an actual device runtime suspend. If > the driver also calls pm_runtime_disable() in probe, that results in the > device remaining runtime enabled. I'm not quite sure what way. pm_runtime_disable() increments dev->power.disable_count and if that is not decremented later, the device remains runtime disabled. You probably wanted to say that the device will end up in the full-power state in that case, which isn't the same and is intentional. > Consider this example (based on drivers/mmc/host/omap_hsmmc.c). In the > normal/success path, everything works fine. But in the failure case, > the after the failed probe, the driver calls pm_runtime_disable() and > the device is left runtime enabled: > > driver_probe() > { > /* usage count == 1; due to _get_noresume() in driver core */ > pm_runtime_enable(); > pm_runtime_get_sync() > /* usage_count == 2 */ > > /* probe HW */ > if (failure) > goto error; > > /* otherwise, finish HW init */ > > pm_runtime_put_sync(); > /* usage_count == 1 */ > /* _put_sync() in driver core will make count = 0 > * and device will be runtime suspended */ > return 0; > > error: > pm_runtime_put_sync(); > /* usage_count = 1 */ > pm_runtime_disable(); > > /* Now, the _put_sync() in driver core will not actually suspend > since runtime PM has been disabled. */ > } > > > So, the question is: is it right to use pm_runtime_disable() this way? If your intention is to put the device into a low-power state in case of a failing .probe(), this isn't the way to do it. pm_runtime_disable() means "don't do any runtime power management on this device any more" regardless of the current state of the device (it may resume the device in some situations, but it won't suspend it, although it may wait for a suspend in progress to complete). That said, I'm not aware of any other generally reliable way to do that either. The problem is, if .probe() is failing we don't have callbacks allowing us to power manage the device (there may be bus type callbacks knowing how to do that, but not for the platform bus type). > Removing it means the device is left in runtime suspended state even in > the error case. However, without the pm_runtime_disable(), if this > driver was a module, repeated attempts to load would result in > unbalalced calls to pm_runtime_enable. > > What is the right way to handle the runtime PM enable/disable in a > failed probe attempt? I would recommend using pm_runtime_disable() and then either pm_runtime_set_active() or pm_runtime_set_suspended() depending on what the real power state of the device is at this point. Anyway, you can't force the device into a low-power state using runtime PM after a failing probe, at least in general. Thanks, Rafael ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: use of pm_runtime_disable() from driver probe? 2012-07-07 19:25 ` Rafael J. Wysocki @ 2012-07-08 2:01 ` Alan Stern 2012-07-08 14:59 ` Rafael J. Wysocki 2012-07-10 18:11 ` Kevin Hilman 1 sibling, 1 reply; 16+ messages in thread From: Alan Stern @ 2012-07-08 2:01 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Kevin Hilman, linux-pm On Sat, 7 Jul 2012, Rafael J. Wysocki wrote: > > What is the right way to handle the runtime PM enable/disable in a > > failed probe attempt? > > I would recommend using pm_runtime_disable() and then either > pm_runtime_set_active() or pm_runtime_set_suspended() depending on what the > real power state of the device is at this point. > > Anyway, you can't force the device into a low-power state using runtime PM > after a failing probe, at least in general. Right. About the best you could do would be to call the appropriate runtime_suspend method directly, instead of going through the runtime PM core. However, if the probe is failing then there's no reason to think the runtime_suspend method will work. Basically, on the platform bus, if you don't have a driver for a device (which you don't, if the probe routine fails) then you can't do runtime PM on it. But what if the device belongs to a PM domain? Putting the entire domain back into a low-power state might have the desired effect -- but the probe routine wouldn't be able to do this. The platform subsystem would have to take care of it somehow. Alan Stern ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: use of pm_runtime_disable() from driver probe? 2012-07-08 2:01 ` Alan Stern @ 2012-07-08 14:59 ` Rafael J. Wysocki 0 siblings, 0 replies; 16+ messages in thread From: Rafael J. Wysocki @ 2012-07-08 14:59 UTC (permalink / raw) To: Alan Stern; +Cc: Kevin Hilman, linux-pm On Sunday, July 08, 2012, Alan Stern wrote: > On Sat, 7 Jul 2012, Rafael J. Wysocki wrote: > > > > What is the right way to handle the runtime PM enable/disable in a > > > failed probe attempt? > > > > I would recommend using pm_runtime_disable() and then either > > pm_runtime_set_active() or pm_runtime_set_suspended() depending on what the > > real power state of the device is at this point. > > > > Anyway, you can't force the device into a low-power state using runtime PM > > after a failing probe, at least in general. > > Right. About the best you could do would be to call the appropriate > runtime_suspend method directly, instead of going through the runtime > PM core. However, if the probe is failing then there's no reason to > think the runtime_suspend method will work. > > Basically, on the platform bus, if you don't have a driver for a device > (which you don't, if the probe routine fails) then you can't do runtime > PM on it. > > But what if the device belongs to a PM domain? Putting the entire > domain back into a low-power state might have the desired effect -- but > the probe routine wouldn't be able to do this. The platform subsystem > would have to take care of it somehow. Yes. For example, on sh platforms we call pm_gnepd_poweroff_unused() from a late initcall (drivers/sh/pm_runtime.c), which causes all PM domains that only contain suspended devices and devices without drivers to be turned off. Thanks, Rafael ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: use of pm_runtime_disable() from driver probe? 2012-07-07 19:25 ` Rafael J. Wysocki 2012-07-08 2:01 ` Alan Stern @ 2012-07-10 18:11 ` Kevin Hilman 2012-07-10 18:31 ` Rafael J. Wysocki 1 sibling, 1 reply; 16+ messages in thread From: Kevin Hilman @ 2012-07-10 18:11 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: linux-pm "Rafael J. Wysocki" <rjw@sisk.pl> writes: > Hi, > > On Saturday, July 07, 2012, Kevin Hilman wrote: >> Hello, >> >> I've got a question around the proper use of pm_runtime_disable() in a >> driver's ->probe method. Basically, using pm_runtime_disable() in >> ->probe() (e.g because of a error/failure) can result in the device >> remaning runtime enabled forever. > > Er, no. Right, poor choice of words. What I meant was "...can result in the device remaining powered on forever." >> This is because the driver core wraps >> the probe in _get_noresume()/_put_sync() so any usage of _put_sync() in >> ->probe itself does not trigger an actual device runtime suspend. If >> the driver also calls pm_runtime_disable() in probe, that results in the >> device remaining runtime enabled. > > I'm not quite sure what way. pm_runtime_disable() increments > dev->power.disable_count and if that is not decremented later, the > device remains runtime disabled. > > You probably wanted to say that the device will end up in the full-power > state in that case, which isn't the same and is intentional. Correct. I'm glad you understood what I meant instead of what I said. I wish my kids could do that. ;) [...] >> So, the question is: is it right to use pm_runtime_disable() this way? > > If your intention is to put the device into a low-power state in case of > a failing .probe(), this isn't the way to do it. pm_runtime_disable() > means "don't do any runtime power management on this device any more" > regardless of the current state of the device (it may resume the device > in some situations, but it won't suspend it, although it may wait for > a suspend in progress to complete). That's correct, but I also think that is what driver writers probably believe they are doing. IOW, after a probe failure, the driver writer does indeed mean "don't do runtime PM on this device any more." But they really mean "don't do runtime PM on this device *after* it's been powered down." However, because of the get/put in the driver core, it's not entirely obvious to driver writers that calling pm_runtime_disable() actually keeps the device powered up, even though they just called _put_sync(). Another way of stating the problem is that the _put_sync() in the driver core's driver_probe_device() assumes that runtime PM is enabled for that device. > That said, I'm not aware of any other generally reliable way to do that > either. That's what I was afraid of. :( > The problem is, if .probe() is failing we don't have callbacks > allowing us to power manage the device (there may be bus type callbacks > knowing how to do that, but not for the platform bus type). There are callbacks for the PM domains, but they are not called either if the driver calls pm_runtime_disable(). >> Removing it means the device is left in runtime suspended state even in >> the error case. However, without the pm_runtime_disable(), if this >> driver was a module, repeated attempts to load would result in >> unbalalced calls to pm_runtime_enable. >> >> What is the right way to handle the runtime PM enable/disable in a >> failed probe attempt? > > I would recommend using pm_runtime_disable() and then either > pm_runtime_set_active() or pm_runtime_set_suspended() depending on what the > real power state of the device is at this point. That doesn't help because it only changes the runtime PM core's notion of the power state, not the physical power state because it doesn't call the subsystem callbacks. Or, are you suggesting to set the state to suspended, and then use a late_initcall to compare the runtime status with the physical device status and power down devices accordingly? > Anyway, you can't force the device into a low-power state using > runtime PM after a failing probe, at least in general. Well, using PM domains, that's exactly what can happen if the driver doesn't call pm_runtime_disable() because the _put_sync() in the driver core will trigger the PM domain callbacks. Kevin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: use of pm_runtime_disable() from driver probe? 2012-07-10 18:11 ` Kevin Hilman @ 2012-07-10 18:31 ` Rafael J. Wysocki 2012-07-10 18:47 ` Alan Stern 0 siblings, 1 reply; 16+ messages in thread From: Rafael J. Wysocki @ 2012-07-10 18:31 UTC (permalink / raw) To: Kevin Hilman; +Cc: linux-pm On Tuesday, July 10, 2012, Kevin Hilman wrote: > "Rafael J. Wysocki" <rjw@sisk.pl> writes: > > > Hi, > > > > On Saturday, July 07, 2012, Kevin Hilman wrote: > >> Hello, > >> > >> I've got a question around the proper use of pm_runtime_disable() in a > >> driver's ->probe method. Basically, using pm_runtime_disable() in > >> ->probe() (e.g because of a error/failure) can result in the device > >> remaning runtime enabled forever. > > > > Er, no. > > Right, poor choice of words. What I meant was "...can result in the > device remaining powered on forever." > > >> This is because the driver core wraps > >> the probe in _get_noresume()/_put_sync() so any usage of _put_sync() in > >> ->probe itself does not trigger an actual device runtime suspend. If > >> the driver also calls pm_runtime_disable() in probe, that results in the > >> device remaining runtime enabled. > > > > I'm not quite sure what way. pm_runtime_disable() increments > > dev->power.disable_count and if that is not decremented later, the > > device remains runtime disabled. > > > > You probably wanted to say that the device will end up in the full-power > > state in that case, which isn't the same and is intentional. > > Correct. I'm glad you understood what I meant instead of what I said. > I wish my kids could do that. ;) > > [...] > > >> So, the question is: is it right to use pm_runtime_disable() this way? > > > > If your intention is to put the device into a low-power state in case of > > a failing .probe(), this isn't the way to do it. pm_runtime_disable() > > means "don't do any runtime power management on this device any more" > > regardless of the current state of the device (it may resume the device > > in some situations, but it won't suspend it, although it may wait for > > a suspend in progress to complete). > > That's correct, but I also think that is what driver writers probably > believe they are doing. IOW, after a probe failure, the driver writer > does indeed mean "don't do runtime PM on this device any more." But > they really mean "don't do runtime PM on this device *after* it's been > powered down." > > However, because of the get/put in the driver core, it's not entirely > obvious to driver writers that calling pm_runtime_disable() actually > keeps the device powered up, even though they just called _put_sync(). > > Another way of stating the problem is that the _put_sync() in the driver > core's driver_probe_device() assumes that runtime PM is enabled for that > device. > > > That said, I'm not aware of any other generally reliable way to do that > > either. > > That's what I was afraid of. :( > > > The problem is, if .probe() is failing we don't have callbacks > > allowing us to power manage the device (there may be bus type callbacks > > knowing how to do that, but not for the platform bus type). > > There are callbacks for the PM domains, but they are not called either > if the driver calls pm_runtime_disable(). Oh, I think I see what the problem is. > >> Removing it means the device is left in runtime suspended state even in > >> the error case. However, without the pm_runtime_disable(), if this > >> driver was a module, repeated attempts to load would result in > >> unbalalced calls to pm_runtime_enable. > >> > >> What is the right way to handle the runtime PM enable/disable in a > >> failed probe attempt? > > > > I would recommend using pm_runtime_disable() and then either > > pm_runtime_set_active() or pm_runtime_set_suspended() depending on what the > > real power state of the device is at this point. > > That doesn't help because it only changes the runtime PM core's notion > of the power state, not the physical power state because it doesn't call > the subsystem callbacks. > > Or, are you suggesting to set the state to suspended, and then use a > late_initcall to compare the runtime status with the physical device > status and power down devices accordingly? > > > Anyway, you can't force the device into a low-power state using > > runtime PM after a failing probe, at least in general. > > Well, using PM domains, that's exactly what can happen if the driver > doesn't call pm_runtime_disable() because the _put_sync() in the driver > core will trigger the PM domain callbacks. OK, so if you have PM domains, then the case is equivalent to having a bus type with its own runtime PM callbacks. In that case, if .probe() fails, it obviously doesn't mean that the device shouldn't be power managed, so the driver shouldn't call pm_runtime_disable(). Generally, if runtime PM was enabled for a device before .probe() has been called, the driver shouldn't disable it in .probe() whatever the reason, because it may not have enough information for deciding whether or not runtime PM should be disabled. Thanks, Rafael ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: use of pm_runtime_disable() from driver probe? 2012-07-10 18:31 ` Rafael J. Wysocki @ 2012-07-10 18:47 ` Alan Stern 2012-07-10 19:14 ` Rafael J. Wysocki 0 siblings, 1 reply; 16+ messages in thread From: Alan Stern @ 2012-07-10 18:47 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Kevin Hilman, linux-pm On Tue, 10 Jul 2012, Rafael J. Wysocki wrote: > > > Anyway, you can't force the device into a low-power state using > > > runtime PM after a failing probe, at least in general. > > > > Well, using PM domains, that's exactly what can happen if the driver > > doesn't call pm_runtime_disable() because the _put_sync() in the driver > > core will trigger the PM domain callbacks. > > OK, so if you have PM domains, then the case is equivalent to having a bus > type with its own runtime PM callbacks. In that case, if .probe() fails, > it obviously doesn't mean that the device shouldn't be power managed, > so the driver shouldn't call pm_runtime_disable(). > > Generally, if runtime PM was enabled for a device before .probe() has been > called, the driver shouldn't disable it in .probe() whatever the reason, > because it may not have enough information for deciding whether or not > runtime PM should be disabled. So if the PM domain code called pm_runtime_enable() then the domain code should be responsible for calling pm_runtime_disable() too, presumably after putting the device back into a low-power state. I'm not sure when that would occur, however. Immediately after registering the device, if no driver is bound? In the case where the probe routine called pm_runtime_enable(), you're stuck. The probe routine _has_ to call pm_runtime_disable() when a failure occurs, to keep the disable count balanced. Alan Stern ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: use of pm_runtime_disable() from driver probe? 2012-07-10 18:47 ` Alan Stern @ 2012-07-10 19:14 ` Rafael J. Wysocki 2012-07-10 19:41 ` Rafael J. Wysocki 0 siblings, 1 reply; 16+ messages in thread From: Rafael J. Wysocki @ 2012-07-10 19:14 UTC (permalink / raw) To: Alan Stern; +Cc: Kevin Hilman, linux-pm On Tuesday, July 10, 2012, Alan Stern wrote: > On Tue, 10 Jul 2012, Rafael J. Wysocki wrote: > > > > > Anyway, you can't force the device into a low-power state using > > > > runtime PM after a failing probe, at least in general. > > > > > > Well, using PM domains, that's exactly what can happen if the driver > > > doesn't call pm_runtime_disable() because the _put_sync() in the driver > > > core will trigger the PM domain callbacks. > > > > OK, so if you have PM domains, then the case is equivalent to having a bus > > type with its own runtime PM callbacks. In that case, if .probe() fails, > > it obviously doesn't mean that the device shouldn't be power managed, > > so the driver shouldn't call pm_runtime_disable(). > > > > Generally, if runtime PM was enabled for a device before .probe() has been > > called, the driver shouldn't disable it in .probe() whatever the reason, > > because it may not have enough information for deciding whether or not > > runtime PM should be disabled. > > So if the PM domain code called pm_runtime_enable() then the domain > code should be responsible for calling pm_runtime_disable() too, > presumably after putting the device back into a low-power state. I'm > not sure when that would occur, however. Immediately after registering > the device, if no driver is bound? > > In the case where the probe routine called pm_runtime_enable(), you're > stuck. The probe routine _has_ to call pm_runtime_disable() when a > failure occurs, to keep the disable count balanced. Yes, I has just been thinking about that. If .probe() enabled runtime PM and called pm_runtime_get_sync() (or _resume), it can't clean up properly in case of an error, because its pm_runtime_put_sync() (or _suspend) won't be effective and you're right that it has to call pm_runtime_disable(). So, we don't handle this particular case correctly. I'm not sure what the solution should be, though. We could remove the runtime PM operations around really_probe(), but then there may be drivers assuming that the core will call pm_runtime_put_sync() after .probe() has returned. Thanks, Rafael ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: use of pm_runtime_disable() from driver probe? 2012-07-10 19:14 ` Rafael J. Wysocki @ 2012-07-10 19:41 ` Rafael J. Wysocki 2012-07-10 20:17 ` Alan Stern 0 siblings, 1 reply; 16+ messages in thread From: Rafael J. Wysocki @ 2012-07-10 19:41 UTC (permalink / raw) To: linux-pm; +Cc: Kevin Hilman On Tuesday, July 10, 2012, Rafael J. Wysocki wrote: > On Tuesday, July 10, 2012, Alan Stern wrote: > > On Tue, 10 Jul 2012, Rafael J. Wysocki wrote: > > > > > > > Anyway, you can't force the device into a low-power state using > > > > > runtime PM after a failing probe, at least in general. > > > > > > > > Well, using PM domains, that's exactly what can happen if the driver > > > > doesn't call pm_runtime_disable() because the _put_sync() in the driver > > > > core will trigger the PM domain callbacks. > > > > > > OK, so if you have PM domains, then the case is equivalent to having a bus > > > type with its own runtime PM callbacks. In that case, if .probe() fails, > > > it obviously doesn't mean that the device shouldn't be power managed, > > > so the driver shouldn't call pm_runtime_disable(). > > > > > > Generally, if runtime PM was enabled for a device before .probe() has been > > > called, the driver shouldn't disable it in .probe() whatever the reason, > > > because it may not have enough information for deciding whether or not > > > runtime PM should be disabled. > > > > So if the PM domain code called pm_runtime_enable() then the domain > > code should be responsible for calling pm_runtime_disable() too, > > presumably after putting the device back into a low-power state. I'm > > not sure when that would occur, however. Immediately after registering > > the device, if no driver is bound? > > > > In the case where the probe routine called pm_runtime_enable(), you're > > stuck. The probe routine _has_ to call pm_runtime_disable() when a > > failure occurs, to keep the disable count balanced. > > Yes, I has just been thinking about that. > > If .probe() enabled runtime PM and called pm_runtime_get_sync() (or _resume), > it can't clean up properly in case of an error, because its > pm_runtime_put_sync() (or _suspend) won't be effective and you're right that > it has to call pm_runtime_disable(). > > So, we don't handle this particular case correctly. > > I'm not sure what the solution should be, though. We could remove the > runtime PM operations around really_probe(), but then there may be drivers > assuming that the core will call pm_runtime_put_sync() after .probe() > has returned. I have an idea. What about the following patch? It shouldn't matter except for the cases when .probe() attempts to suspend the device by itself. --- drivers/base/dd.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) Index: linux/drivers/base/dd.c =================================================================== --- linux.orig/drivers/base/dd.c +++ linux/drivers/base/dd.c @@ -356,10 +356,8 @@ int driver_probe_device(struct device_dr pr_debug("bus: '%s': %s: matched device %s with driver %s\n", drv->bus->name, __func__, dev_name(dev), drv->name); - pm_runtime_get_noresume(dev); - pm_runtime_barrier(dev); ret = really_probe(dev, drv); - pm_runtime_put_sync(dev); + pm_runtime_idle(dev); return ret; } @@ -406,9 +404,8 @@ int device_attach(struct device *dev) ret = 0; } } else { - pm_runtime_get_noresume(dev); ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach); - pm_runtime_put_sync(dev); + pm_runtime_idle(dev); } out_unlock: device_unlock(dev); ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: use of pm_runtime_disable() from driver probe? 2012-07-10 19:41 ` Rafael J. Wysocki @ 2012-07-10 20:17 ` Alan Stern 2012-07-10 21:04 ` Kevin Hilman 2012-07-10 22:31 ` Rafael J. Wysocki 0 siblings, 2 replies; 16+ messages in thread From: Alan Stern @ 2012-07-10 20:17 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Kevin Hilman, linux-pm On Tue, 10 Jul 2012, Rafael J. Wysocki wrote: > > If .probe() enabled runtime PM and called pm_runtime_get_sync() (or _resume), > > it can't clean up properly in case of an error, because its > > pm_runtime_put_sync() (or _suspend) won't be effective and you're right that > > it has to call pm_runtime_disable(). > > > > So, we don't handle this particular case correctly. > > > > I'm not sure what the solution should be, though. We could remove the > > runtime PM operations around really_probe(), but then there may be drivers > > assuming that the core will call pm_runtime_put_sync() after .probe() > > has returned. > > I have an idea. > > What about the following patch? It shouldn't matter except for the cases when > .probe() attempts to suspend the device by itself. > > --- > drivers/base/dd.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > Index: linux/drivers/base/dd.c > =================================================================== > --- linux.orig/drivers/base/dd.c > +++ linux/drivers/base/dd.c > @@ -356,10 +356,8 @@ int driver_probe_device(struct device_dr > pr_debug("bus: '%s': %s: matched device %s with driver %s\n", > drv->bus->name, __func__, dev_name(dev), drv->name); > > - pm_runtime_get_noresume(dev); > - pm_runtime_barrier(dev); > ret = really_probe(dev, drv); > - pm_runtime_put_sync(dev); > + pm_runtime_idle(dev); > > return ret; > } > @@ -406,9 +404,8 @@ int device_attach(struct device *dev) > ret = 0; > } > } else { > - pm_runtime_get_noresume(dev); > ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach); > - pm_runtime_put_sync(dev); > + pm_runtime_idle(dev); > } > out_unlock: > device_unlock(dev); This opens up the possibility of calling probe while a runtime resume or suspend is in progress. (On the other hand, the existing code doesn't prevent a concurrent runtime resume.) Maybe it would be best to leave the pm_runtime_barrier(). Apart from that, I think it would solve the problem. Alan Stern ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: use of pm_runtime_disable() from driver probe? 2012-07-10 20:17 ` Alan Stern @ 2012-07-10 21:04 ` Kevin Hilman 2012-07-10 22:31 ` Rafael J. Wysocki 1 sibling, 0 replies; 16+ messages in thread From: Kevin Hilman @ 2012-07-10 21:04 UTC (permalink / raw) To: Alan Stern; +Cc: linux-pm Alan Stern <stern@rowland.harvard.edu> writes: > On Tue, 10 Jul 2012, Rafael J. Wysocki wrote: > >> > If .probe() enabled runtime PM and called pm_runtime_get_sync() (or _resume), >> > it can't clean up properly in case of an error, because its >> > pm_runtime_put_sync() (or _suspend) won't be effective and you're right that >> > it has to call pm_runtime_disable(). >> > >> > So, we don't handle this particular case correctly. Right, this is exactly the case I'm working on now. The driver is calling pm_runtime_enable() and _disable() in .probe(). >> > I'm not sure what the solution should be, though. We could remove the >> > runtime PM operations around really_probe(), but then there may be drivers >> > assuming that the core will call pm_runtime_put_sync() after .probe() >> > has returned. >> >> I have an idea. >> >> What about the following patch? It shouldn't matter except for the cases when >> .probe() attempts to suspend the device by itself. >> >> --- >> drivers/base/dd.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> Index: linux/drivers/base/dd.c >> =================================================================== >> --- linux.orig/drivers/base/dd.c >> +++ linux/drivers/base/dd.c >> @@ -356,10 +356,8 @@ int driver_probe_device(struct device_dr >> pr_debug("bus: '%s': %s: matched device %s with driver %s\n", >> drv->bus->name, __func__, dev_name(dev), drv->name); >> >> - pm_runtime_get_noresume(dev); >> - pm_runtime_barrier(dev); >> ret = really_probe(dev, drv); >> - pm_runtime_put_sync(dev); >> + pm_runtime_idle(dev); >> >> return ret; >> } >> @@ -406,9 +404,8 @@ int device_attach(struct device *dev) >> ret = 0; >> } >> } else { >> - pm_runtime_get_noresume(dev); >> ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach); >> - pm_runtime_put_sync(dev); >> + pm_runtime_idle(dev); >> } >> out_unlock: >> device_unlock(dev); > > > This opens up the possibility of calling probe while a runtime resume > or suspend is in progress. (On the other hand, the existing code > doesn't prevent a concurrent runtime resume.) Maybe it would be best > to leave the pm_runtime_barrier(). > > Apart from that, I think it would solve the problem. I just tested it and it definitely solves the problem. I forgot exactly why the get/put was added around probe in the first place, so am not sure if this opens up other potential problems like Alan mentioned. In any case, if you go forward with this patch, feel free to add: Reviewed-by: Kevin Hilman <khilman@ti.com> Tested-by: Kevin Hilman <khilman@ti.com> Kevin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: use of pm_runtime_disable() from driver probe? 2012-07-10 20:17 ` Alan Stern 2012-07-10 21:04 ` Kevin Hilman @ 2012-07-10 22:31 ` Rafael J. Wysocki 2012-07-11 14:20 ` Alan Stern 1 sibling, 1 reply; 16+ messages in thread From: Rafael J. Wysocki @ 2012-07-10 22:31 UTC (permalink / raw) To: Alan Stern; +Cc: Kevin Hilman, linux-pm On Tuesday, July 10, 2012, Alan Stern wrote: > On Tue, 10 Jul 2012, Rafael J. Wysocki wrote: > > > > If .probe() enabled runtime PM and called pm_runtime_get_sync() (or _resume), > > > it can't clean up properly in case of an error, because its > > > pm_runtime_put_sync() (or _suspend) won't be effective and you're right that > > > it has to call pm_runtime_disable(). > > > > > > So, we don't handle this particular case correctly. > > > > > > I'm not sure what the solution should be, though. We could remove the > > > runtime PM operations around really_probe(), but then there may be drivers > > > assuming that the core will call pm_runtime_put_sync() after .probe() > > > has returned. > > > > I have an idea. > > > > What about the following patch? It shouldn't matter except for the cases when > > .probe() attempts to suspend the device by itself. > > > > --- > > drivers/base/dd.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > Index: linux/drivers/base/dd.c > > =================================================================== > > --- linux.orig/drivers/base/dd.c > > +++ linux/drivers/base/dd.c > > @@ -356,10 +356,8 @@ int driver_probe_device(struct device_dr > > pr_debug("bus: '%s': %s: matched device %s with driver %s\n", > > drv->bus->name, __func__, dev_name(dev), drv->name); > > > > - pm_runtime_get_noresume(dev); > > - pm_runtime_barrier(dev); > > ret = really_probe(dev, drv); > > - pm_runtime_put_sync(dev); > > + pm_runtime_idle(dev); > > > > return ret; > > } > > @@ -406,9 +404,8 @@ int device_attach(struct device *dev) > > ret = 0; > > } > > } else { > > - pm_runtime_get_noresume(dev); > > ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach); > > - pm_runtime_put_sync(dev); > > + pm_runtime_idle(dev); > > } > > out_unlock: > > device_unlock(dev); > > > This opens up the possibility of calling probe while a runtime resume > or suspend is in progress. (On the other hand, the existing code > doesn't prevent a concurrent runtime resume.) Maybe it would be best > to leave the pm_runtime_barrier(). That wouldn't close the race, though, because the suspend/resume may still be started right after _barrier has returned. The race is only possible if runtime PM is enabled by a subsystem or PM domain code before the first eligible driver is registered and if that code is not careful enough to get ready for driver registration. I'm not sure how likely it is to happen in practice. Thanks, Rafael ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: use of pm_runtime_disable() from driver probe? 2012-07-10 22:31 ` Rafael J. Wysocki @ 2012-07-11 14:20 ` Alan Stern 2012-07-11 17:36 ` Rafael J. Wysocki 0 siblings, 1 reply; 16+ messages in thread From: Alan Stern @ 2012-07-11 14:20 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Kevin Hilman, linux-pm On Wed, 11 Jul 2012, Rafael J. Wysocki wrote: > > This opens up the possibility of calling probe while a runtime resume > > or suspend is in progress. (On the other hand, the existing code > > doesn't prevent a concurrent runtime resume.) Maybe it would be best > > to leave the pm_runtime_barrier(). > > That wouldn't close the race, though, because the suspend/resume may still > be started right after _barrier has returned. True, but see below. > The race is only possible if runtime PM is enabled by a subsystem or PM domain > code before the first eligible driver is registered and if that code is not > careful enough to get ready for driver registration. I'm not sure how likely > it is to happen in practice. It's not just the first eligible driver. Drivers can be bound and unbound dynamically, and suspends/resume operations can sit on the wait queue or wait until a timer expires. We don't want an old request suddenly to take effect in the middle of a probe. The barrier will get rid of any old requests. New ones would have to be added after the probe starts, which as you say, is unlikely. Alan Stern ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: use of pm_runtime_disable() from driver probe? 2012-07-11 14:20 ` Alan Stern @ 2012-07-11 17:36 ` Rafael J. Wysocki 2012-07-11 23:07 ` Kevin Hilman 0 siblings, 1 reply; 16+ messages in thread From: Rafael J. Wysocki @ 2012-07-11 17:36 UTC (permalink / raw) To: Alan Stern, Kevin Hilman; +Cc: linux-pm On Wednesday, July 11, 2012, Alan Stern wrote: > On Wed, 11 Jul 2012, Rafael J. Wysocki wrote: > > > > This opens up the possibility of calling probe while a runtime resume > > > or suspend is in progress. (On the other hand, the existing code > > > doesn't prevent a concurrent runtime resume.) Maybe it would be best > > > to leave the pm_runtime_barrier(). > > > > That wouldn't close the race, though, because the suspend/resume may still > > be started right after _barrier has returned. > > True, but see below. > > > The race is only possible if runtime PM is enabled by a subsystem or PM domain > > code before the first eligible driver is registered and if that code is not > > careful enough to get ready for driver registration. I'm not sure how likely > > it is to happen in practice. > > It's not just the first eligible driver. Drivers can be bound and > unbound dynamically, and suspends/resume operations can sit on the wait > queue or wait until a timer expires. We don't want an old request > suddenly to take effect in the middle of a probe. > > The barrier will get rid of any old requests. New ones would have to > be added after the probe starts, which as you say, is unlikely. OK, I'll keep the barrier, then. Kevin, can you please double check the patch below? Rafael --- drivers/base/dd.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) Index: linux/drivers/base/dd.c =================================================================== --- linux.orig/drivers/base/dd.c +++ linux/drivers/base/dd.c @@ -356,10 +356,9 @@ int driver_probe_device(struct device_dr pr_debug("bus: '%s': %s: matched device %s with driver %s\n", drv->bus->name, __func__, dev_name(dev), drv->name); - pm_runtime_get_noresume(dev); pm_runtime_barrier(dev); ret = really_probe(dev, drv); - pm_runtime_put_sync(dev); + pm_runtime_idle(dev); return ret; } @@ -406,9 +405,8 @@ int device_attach(struct device *dev) ret = 0; } } else { - pm_runtime_get_noresume(dev); ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach); - pm_runtime_put_sync(dev); + pm_runtime_idle(dev); } out_unlock: device_unlock(dev); ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: use of pm_runtime_disable() from driver probe? 2012-07-11 17:36 ` Rafael J. Wysocki @ 2012-07-11 23:07 ` Kevin Hilman 2012-07-11 23:16 ` Rafael J. Wysocki 0 siblings, 1 reply; 16+ messages in thread From: Kevin Hilman @ 2012-07-11 23:07 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: linux-pm "Rafael J. Wysocki" <rjw@sisk.pl> writes: > On Wednesday, July 11, 2012, Alan Stern wrote: >> On Wed, 11 Jul 2012, Rafael J. Wysocki wrote: >> >> > > This opens up the possibility of calling probe while a runtime resume >> > > or suspend is in progress. (On the other hand, the existing code >> > > doesn't prevent a concurrent runtime resume.) Maybe it would be best >> > > to leave the pm_runtime_barrier(). >> > >> > That wouldn't close the race, though, because the suspend/resume may still >> > be started right after _barrier has returned. >> >> True, but see below. >> >> > The race is only possible if runtime PM is enabled by a subsystem or PM domain >> > code before the first eligible driver is registered and if that code is not >> > careful enough to get ready for driver registration. I'm not sure how likely >> > it is to happen in practice. >> >> It's not just the first eligible driver. Drivers can be bound and >> unbound dynamically, and suspends/resume operations can sit on the wait >> queue or wait until a timer expires. We don't want an old request >> suddenly to take effect in the middle of a probe. >> >> The barrier will get rid of any old requests. New ones would have to >> be added after the probe starts, which as you say, is unlikely. > > OK, I'll keep the barrier, then. > > Kevin, can you please double check the patch below? Yup, this verion looks right and I tested it and verified that it still fixes the problem I'm seeing. Reviewed-by: Kevin Hilman <khilman@ti.com> Tested-by: Kevin Hilman <khilman@ti.com> Thanks! Kevin > Rafael > > --- > drivers/base/dd.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > Index: linux/drivers/base/dd.c > =================================================================== > --- linux.orig/drivers/base/dd.c > +++ linux/drivers/base/dd.c > @@ -356,10 +356,9 @@ int driver_probe_device(struct device_dr > pr_debug("bus: '%s': %s: matched device %s with driver %s\n", > drv->bus->name, __func__, dev_name(dev), drv->name); > > - pm_runtime_get_noresume(dev); > pm_runtime_barrier(dev); > ret = really_probe(dev, drv); > - pm_runtime_put_sync(dev); > + pm_runtime_idle(dev); > > return ret; > } > @@ -406,9 +405,8 @@ int device_attach(struct device *dev) > ret = 0; > } > } else { > - pm_runtime_get_noresume(dev); > ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach); > - pm_runtime_put_sync(dev); > + pm_runtime_idle(dev); > } > out_unlock: > device_unlock(dev); ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: use of pm_runtime_disable() from driver probe? 2012-07-11 23:07 ` Kevin Hilman @ 2012-07-11 23:16 ` Rafael J. Wysocki 0 siblings, 0 replies; 16+ messages in thread From: Rafael J. Wysocki @ 2012-07-11 23:16 UTC (permalink / raw) To: Kevin Hilman; +Cc: linux-pm On Thursday, July 12, 2012, Kevin Hilman wrote: > "Rafael J. Wysocki" <rjw@sisk.pl> writes: > > > On Wednesday, July 11, 2012, Alan Stern wrote: > >> On Wed, 11 Jul 2012, Rafael J. Wysocki wrote: > >> > >> > > This opens up the possibility of calling probe while a runtime resume > >> > > or suspend is in progress. (On the other hand, the existing code > >> > > doesn't prevent a concurrent runtime resume.) Maybe it would be best > >> > > to leave the pm_runtime_barrier(). > >> > > >> > That wouldn't close the race, though, because the suspend/resume may still > >> > be started right after _barrier has returned. > >> > >> True, but see below. > >> > >> > The race is only possible if runtime PM is enabled by a subsystem or PM domain > >> > code before the first eligible driver is registered and if that code is not > >> > careful enough to get ready for driver registration. I'm not sure how likely > >> > it is to happen in practice. > >> > >> It's not just the first eligible driver. Drivers can be bound and > >> unbound dynamically, and suspends/resume operations can sit on the wait > >> queue or wait until a timer expires. We don't want an old request > >> suddenly to take effect in the middle of a probe. > >> > >> The barrier will get rid of any old requests. New ones would have to > >> be added after the probe starts, which as you say, is unlikely. > > > > OK, I'll keep the barrier, then. > > > > Kevin, can you please double check the patch below? > > Yup, this verion looks right and I tested it and verified that it still > fixes the problem I'm seeing. > > Reviewed-by: Kevin Hilman <khilman@ti.com> > Tested-by: Kevin Hilman <khilman@ti.com> Cool, thanks! I'll submit it officially (with a changelog and tags) tomorrow. Rafael > > --- > > drivers/base/dd.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > Index: linux/drivers/base/dd.c > > =================================================================== > > --- linux.orig/drivers/base/dd.c > > +++ linux/drivers/base/dd.c > > @@ -356,10 +356,9 @@ int driver_probe_device(struct device_dr > > pr_debug("bus: '%s': %s: matched device %s with driver %s\n", > > drv->bus->name, __func__, dev_name(dev), drv->name); > > > > - pm_runtime_get_noresume(dev); > > pm_runtime_barrier(dev); > > ret = really_probe(dev, drv); > > - pm_runtime_put_sync(dev); > > + pm_runtime_idle(dev); > > > > return ret; > > } > > @@ -406,9 +405,8 @@ int device_attach(struct device *dev) > > ret = 0; > > } > > } else { > > - pm_runtime_get_noresume(dev); > > ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach); > > - pm_runtime_put_sync(dev); > > + pm_runtime_idle(dev); > > } > > out_unlock: > > device_unlock(dev); > > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2012-07-11 23:16 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-06 22:29 use of pm_runtime_disable() from driver probe? Kevin Hilman 2012-07-07 19:25 ` Rafael J. Wysocki 2012-07-08 2:01 ` Alan Stern 2012-07-08 14:59 ` Rafael J. Wysocki 2012-07-10 18:11 ` Kevin Hilman 2012-07-10 18:31 ` Rafael J. Wysocki 2012-07-10 18:47 ` Alan Stern 2012-07-10 19:14 ` Rafael J. Wysocki 2012-07-10 19:41 ` Rafael J. Wysocki 2012-07-10 20:17 ` Alan Stern 2012-07-10 21:04 ` Kevin Hilman 2012-07-10 22:31 ` Rafael J. Wysocki 2012-07-11 14:20 ` Alan Stern 2012-07-11 17:36 ` Rafael J. Wysocki 2012-07-11 23:07 ` Kevin Hilman 2012-07-11 23:16 ` Rafael J. Wysocki
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.