Async runtime put in __device_release_driver()

DISCUSSION Tomi Valkeinen 5 participants 12 messages Oct 23, 2013 – Nov 7, 2013 Driver core

lore

Share & tools

lore

Tomi Valkeinen

author

2013-10-23 10:11 UTC #0 lore raw

Hi,

I was debugging why clocks were left enabled after removing omapdss driver, and I found this commit:

fa180eb448fa263cf18dd930143b515d27d70d7b (PM / Runtime: Idle devices asynchronously after probe|release)

I don't understand how that is supposed to work.

When a driver is removed, instead of using pm_runtime_put_sync() the commit uses pm_runtime_put(), so the runtime_suspend call is queued. But who is going to handle the queued suspend call, as the driver is already removed? At least in my case, obviously nobody, as I only get runtime_resume call in my driver, never the runtime_suspend.

Is there something I need to add to my driver to make this work, or should that part of the patch be reverted?

Tomi

Ulf Hansson

2013-11-05 21:29 UTC #1 lore raw

Quoted 10 lines · Tomi Valkeinen

Hi,
I was debugging why clocks were left enabled after removing omapdss driver, and I found this commit:
fa180eb448fa263cf18dd930143b515d27d70d7b (PM / Runtime: Idle devices asynchronously after probe|release)
I don't understand how that is supposed to work.

Quoted 8 lines

When a driver is removed, instead of using pm_runtime_put_sync() the commit uses pm_runtime_put(), so the runtime_suspend call is queued. But who is going to handle the queued suspend call, as the driver is already removed? At least in my case, obviously nobody, as I only get runtime_resume call in my driver, never the runtime_suspend.
Is there something I need to add to my driver to make this work, or should that part of the patch be reverted?

I believe it is quite common that a device driver calls pm_runtime_get_sync as a part of it's remove callback, then it explicitly returns it's resources that has been fetched during probe. Like a clk_disable_unprepare for example.

The idea behind the change in __device_release_driver, was to try to prevent devices from going active->idle->active and instead just remain active (if possible).

In your case, which seems like a more modern way of implementing "remove", you shall call "pm_runtime_suspend" to make sure the runtime_suspend callbacks gets called.

Kind regards
Ulf Hansson

Quoted 3 lines

Tomi

Tomi Valkeinen

author

2013-11-06 07:51 UTC #2 lore raw

Quoted 17 lines · Ulf Hansson

Quoted 16 lines · Tomi Valkeinen
Hi,
I was debugging why clocks were left enabled after removing omapdss driver, and I found this commit:
fa180eb448fa263cf18dd930143b515d27d70d7b (PM / Runtime: Idle devices asynchronously after probe|release)
I don't understand how that is supposed to work.
When a driver is removed, instead of using pm_runtime_put_sync() the commit uses pm_runtime_put(), so the runtime_suspend call is queued. But who is going to handle the queued suspend call, as the driver is already removed? At least in my case, obviously nobody, as I only get runtime_resume call in my driver, never the runtime_suspend.

Quoted 7 lines

Quoted 2 lines
Is there something I need to add to my driver to make this work, or should that part of the patch be reverted?
I believe it is quite common that a device driver calls pm_runtime_get_sync as a part of it's remove callback, then it explicitly returns it's resources that has been fetched during probe. Like a clk_disable_unprepare for example.

I guess you mean the driver calls pm_runtime_get_sync _and_ pm_runtime_put_sync as part of its remove callback?

Probably bus drivers need to do that, but for memory mapped devices in a SoC, I don't think there's normally any need to do pm_runtime_get/put_sync during the remove callback.

Quoted 7 lines

The idea behind the change in __device_release_driver, was to try to prevent devices from going active->idle->active and instead just remain active (if possible).
In your case, which seems like a more modern way of implementing "remove", you shall call "pm_runtime_suspend" to make sure the runtime_suspend callbacks gets called.

And as far as I understand, the change creates an explicit requirement to do either pm_runtime_get/put_sync or pm_runtime_suspend inside driver's remove callback. If so, that should be mentioned in big red letters in the pm-runtime documentation.

The runtime_pm.txt doc does mention something related to this (and btw, the doc says pm_runtime_put_sync is being called, which is no longer true), but nothing clear about how the driver remove callback must be implemented.

I tried grepping the kernel sources to find out if pm_runtime_suspend is widely used to get SoC platform devices to suspend, but it doesn't seem like it is. I didn't see pm_runtime_get/put_sync being used in remove callbacks widely either, but that was more difficult one to grep.

Tomi

Rafael J. Wysocki

2013-11-06 21:49 UTC #3 lore raw

Quoted 46 lines · Tomi Valkeinen

Quoted 17 lines · Ulf Hansson
Quoted 16 lines · Tomi Valkeinen
Hi,
I was debugging why clocks were left enabled after removing omapdss driver, and I found this commit:
fa180eb448fa263cf18dd930143b515d27d70d7b (PM / Runtime: Idle devices asynchronously after probe|release)
I don't understand how that is supposed to work.
When a driver is removed, instead of using pm_runtime_put_sync() the commit uses pm_runtime_put(), so the runtime_suspend call is queued. But who is going to handle the queued suspend call, as the driver is already removed? At least in my case, obviously nobody, as I only get runtime_resume call in my driver, never the runtime_suspend.
Quoted 7 lines
Quoted 2 lines
Is there something I need to add to my driver to make this work, or should that part of the patch be reverted?
I believe it is quite common that a device driver calls pm_runtime_get_sync as a part of it's remove callback, then it explicitly returns it's resources that has been fetched during probe. Like a clk_disable_unprepare for example.
I guess you mean the driver calls pm_runtime_get_sync _and_ pm_runtime_put_sync as part of its remove callback?
Probably bus drivers need to do that, but for memory mapped devices in a SoC, I don't think there's normally any need to do pm_runtime_get/put_sync during the remove callback.
Quoted 7 lines
The idea behind the change in __device_release_driver, was to try to prevent devices from going active->idle->active and instead just remain active (if possible).
In your case, which seems like a more modern way of implementing "remove", you shall call "pm_runtime_suspend" to make sure the runtime_suspend callbacks gets called.
And as far as I understand, the change creates an explicit requirement to do either pm_runtime_get/put_sync or pm_runtime_suspend inside driver's remove callback. If so, that should be mentioned in big red letters in the pm-runtime documentation.

Quoted 4 lines

The runtime_pm.txt doc does mention something related to this (and btw, the doc says pm_runtime_put_sync is being called, which is no longer true), but nothing clear about how the driver remove callback must be implemented.

That's correct.

Quoted 4 lines

I tried grepping the kernel sources to find out if pm_runtime_suspend is widely used to get SoC platform devices to suspend, but it doesn't seem like it is. I didn't see pm_runtime_get/put_sync being used in remove callbacks widely either, but that was more difficult one to grep.

I think your observations are valid, which unfortunately means that we'll need to revert the commit in question, because it has changed the behavior that drivers are perfectly fine to expect given the existing documentation etc. It looks like the change was premature at least.

Greg, I wonder if you can queue up a revert of fa180eb448fa for 3.13, or do you want me to do that?

Rafael

Alan Stern

2013-11-06 22:02 UTC #4 lore raw

Quoted 59 lines · Rafael J. Wysocki

Quoted 46 lines · Tomi Valkeinen
Quoted 17 lines · Ulf Hansson
Quoted 16 lines · Tomi Valkeinen
Hi,
I was debugging why clocks were left enabled after removing omapdss driver, and I found this commit:
fa180eb448fa263cf18dd930143b515d27d70d7b (PM / Runtime: Idle devices asynchronously after probe|release)
I don't understand how that is supposed to work.
When a driver is removed, instead of using pm_runtime_put_sync() the commit uses pm_runtime_put(), so the runtime_suspend call is queued. But who is going to handle the queued suspend call, as the driver is already removed? At least in my case, obviously nobody, as I only get runtime_resume call in my driver, never the runtime_suspend.
Quoted 7 lines
Quoted 2 lines
Is there something I need to add to my driver to make this work, or should that part of the patch be reverted?
I believe it is quite common that a device driver calls pm_runtime_get_sync as a part of it's remove callback, then it explicitly returns it's resources that has been fetched during probe. Like a clk_disable_unprepare for example.
I guess you mean the driver calls pm_runtime_get_sync _and_ pm_runtime_put_sync as part of its remove callback?
Probably bus drivers need to do that, but for memory mapped devices in a SoC, I don't think there's normally any need to do pm_runtime_get/put_sync during the remove callback.
Quoted 7 lines
The idea behind the change in __device_release_driver, was to try to prevent devices from going active->idle->active and instead just remain active (if possible).
In your case, which seems like a more modern way of implementing "remove", you shall call "pm_runtime_suspend" to make sure the runtime_suspend callbacks gets called.
And as far as I understand, the change creates an explicit requirement to do either pm_runtime_get/put_sync or pm_runtime_suspend inside driver's remove callback. If so, that should be mentioned in big red letters in the pm-runtime documentation.
Quoted 4 lines
The runtime_pm.txt doc does mention something related to this (and btw, the doc says pm_runtime_put_sync is being called, which is no longer true), but nothing clear about how the driver remove callback must be implemented.
That's correct.
Quoted 4 lines
I tried grepping the kernel sources to find out if pm_runtime_suspend is widely used to get SoC platform devices to suspend, but it doesn't seem like it is. I didn't see pm_runtime_get/put_sync being used in remove callbacks widely either, but that was more difficult one to grep.

Quoted 7 lines

I think your observations are valid, which unfortunately means that we'll need to revert the commit in question, because it has changed the behavior that drivers are perfectly fine to expect given the existing documentation etc. It looks like the change was premature at least.
Greg, I wonder if you can queue up a revert of fa180eb448fa for 3.13, or do you want me to do that?

Would it be better to leave the runtime-idle callbacks (invoked during probe) async and revert only the change to __device_release_driver()?

Having an async callback after probe shouldn't cause problems, because the driver will then be bound (assuming the probe succeeded).

Alan Stern

Rafael J. Wysocki

2013-11-06 22:07 UTC #5 lore raw

Quoted 66 lines · Alan Stern

Quoted 64 lines · Rafael J. Wysocki
Quoted 46 lines · Tomi Valkeinen
Quoted 17 lines · Ulf Hansson
Quoted 16 lines · Tomi Valkeinen
Hi,
I was debugging why clocks were left enabled after removing omapdss driver, and I found this commit:
fa180eb448fa263cf18dd930143b515d27d70d7b (PM / Runtime: Idle devices asynchronously after probe|release)
I don't understand how that is supposed to work.
When a driver is removed, instead of using pm_runtime_put_sync() the commit uses pm_runtime_put(), so the runtime_suspend call is queued. But who is going to handle the queued suspend call, as the driver is already removed? At least in my case, obviously nobody, as I only get runtime_resume call in my driver, never the runtime_suspend.
Quoted 7 lines
Quoted 2 lines
Is there something I need to add to my driver to make this work, or should that part of the patch be reverted?
I believe it is quite common that a device driver calls pm_runtime_get_sync as a part of it's remove callback, then it explicitly returns it's resources that has been fetched during probe. Like a clk_disable_unprepare for example.
I guess you mean the driver calls pm_runtime_get_sync _and_ pm_runtime_put_sync as part of its remove callback?
Probably bus drivers need to do that, but for memory mapped devices in a SoC, I don't think there's normally any need to do pm_runtime_get/put_sync during the remove callback.
Quoted 7 lines
The idea behind the change in __device_release_driver, was to try to prevent devices from going active->idle->active and instead just remain active (if possible).
In your case, which seems like a more modern way of implementing "remove", you shall call "pm_runtime_suspend" to make sure the runtime_suspend callbacks gets called.
And as far as I understand, the change creates an explicit requirement to do either pm_runtime_get/put_sync or pm_runtime_suspend inside driver's remove callback. If so, that should be mentioned in big red letters in the pm-runtime documentation.
Quoted 4 lines
The runtime_pm.txt doc does mention something related to this (and btw, the doc says pm_runtime_put_sync is being called, which is no longer true), but nothing clear about how the driver remove callback must be implemented.
That's correct.
Quoted 4 lines
I tried grepping the kernel sources to find out if pm_runtime_suspend is widely used to get SoC platform devices to suspend, but it doesn't seem like it is. I didn't see pm_runtime_get/put_sync being used in remove callbacks widely either, but that was more difficult one to grep.
I think your observations are valid, which unfortunately means that we'll need to revert the commit in question, because it has changed the behavior that drivers are perfectly fine to expect given the existing documentation etc. It looks like the change was premature at least.

Quoted 8 lines

Quoted 2 lines
Greg, I wonder if you can queue up a revert of fa180eb448fa for 3.13, or do you want me to do that?
Would it be better to leave the runtime-idle callbacks (invoked during probe) async and revert only the change to __device_release_driver()?
Having an async callback after probe shouldn't cause problems, because the driver will then be bound (assuming the probe succeeded).

Right. OK, I'll prepare a patch.

Thanks,
Rafael

Ulf Hansson

2013-11-06 22:48 UTC #6 lore raw

"Rafael J. Wysocki" <[email protected]> skrev:

Quoted 95 lines · Alan Stern

Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Tomi Valkeinen
Quoted 2 lines · Ulf Hansson
Quoted 1 line
On 23 October 2013 12:11, Tomi Valkeinen
Quoted 3 lines · <[email protected]> wrote:
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Hi,
I was debugging why clocks were left enabled after removing
omapdss
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
driver, and I found this commit:
fa180eb448fa263cf18dd930143b515d27d70d7b (PM / Runtime: Idle
devices
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
asynchronously after probe|release)
I don't understand how that is supposed to work.
When a driver is removed, instead of using
pm_runtime_put_sync() the
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
commit uses pm_runtime_put(), so the runtime_suspend call is
queued. But
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
who is going to handle the queued suspend call, as the driver
is already
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
removed? At least in my case, obviously nobody, as I only get runtime_resume call in my driver, never the runtime_suspend.
Is there something I need to add to my driver to make this
work, or
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
should that part of the patch be reverted?
I believe it is quite common that a device driver calls pm_runtime_get_sync as a part of it's remove callback, then it explicitly returns it's resources that has been fetched during
probe.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Like a clk_disable_unprepare for example.
I guess you mean the driver calls pm_runtime_get_sync _and_ pm_runtime_put_sync as part of its remove callback?
Probably bus drivers need to do that, but for memory mapped
devices in a
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
SoC, I don't think there's normally any need to do pm_runtime_get/put_sync during the remove callback.
Quoted 1 line
The idea behind the change in __device_release_driver, was to
try to
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
prevent devices from going active->idle->active and instead
just
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
remain active (if possible).
In your case, which seems like a more modern way of
implementing
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
"remove", you shall call "pm_runtime_suspend" to make sure the runtime_suspend callbacks gets called.
And as far as I understand, the change creates an explicit
requirement
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
to do either pm_runtime_get/put_sync or pm_runtime_suspend inside driver's remove callback. If so, that should be mentioned in big
red
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
letters in the pm-runtime documentation.
The runtime_pm.txt doc does mention something related to this
(and btw,
Quoted 1 line
Quoted 1 line
Quoted 1 line
the doc says pm_runtime_put_sync is being called, which is no
longer
Quoted 1 line
Quoted 1 line
Quoted 1 line
true), but nothing clear about how the driver remove callback
must be
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
implemented.
That's correct.
Quoted 1 line
I tried grepping the kernel sources to find out if
pm_runtime_suspend is
Quoted 1 line
Quoted 1 line
Quoted 1 line
widely used to get SoC platform devices to suspend, but it
doesn't seem
Quoted 1 line
Quoted 1 line
Quoted 1 line
like it is. I didn't see pm_runtime_get/put_sync being used in
remove
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
callbacks widely either, but that was more difficult one to grep.
I think your observations are valid, which unfortunately means that
we'll
Quoted 1 line
Quoted 1 line
need to revert the commit in question, because it has changed the
behavior
Quoted 1 line
Quoted 1 line
that drivers are perfectly fine to expect given the existing
documentation
Quoted 3 lines
Quoted 3 lines
etc. It looks like the change was premature at least.
Greg, I wonder if you can queue up a revert of fa180eb448fa for
3.13, or
Quoted 3 lines
Quoted 1 line
do you want me to do that?
Would it be better to leave the runtime-idle callbacks (invoked

Quoted 8 lines

during
Quoted 3 lines
probe) async and revert only the change to __device_release_driver()?
Having an async callback after probe shouldn't cause problems,
because
Quoted 1 line
the driver will then be bound (assuming the probe succeeded).
Right. OK, I'll prepare a patch.

That seems like a good way forward. Also I appoligize for not updating the doc as part of the original patch.

Kind regards
Ulf Hansson

Quoted 3 lines

Thanks,
Rafael

Rafael J. Wysocki

2013-11-07 00:04 UTC #7 lore raw

Quoted 100 lines · Ulf Hansson

"Rafael J. Wysocki" <[email protected]> skrev:
Quoted 98 lines · Alan Stern
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Tomi Valkeinen
Quoted 2 lines · Ulf Hansson
Quoted 1 line
On 23 October 2013 12:11, Tomi Valkeinen
Quoted 3 lines · <[email protected]> wrote:
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Hi,
I was debugging why clocks were left enabled after removing
omapdss
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
driver, and I found this commit:
fa180eb448fa263cf18dd930143b515d27d70d7b (PM / Runtime: Idle
devices
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
asynchronously after probe|release)
I don't understand how that is supposed to work.
When a driver is removed, instead of using
pm_runtime_put_sync() the
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
commit uses pm_runtime_put(), so the runtime_suspend call is
queued. But
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
who is going to handle the queued suspend call, as the driver
is already
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
removed? At least in my case, obviously nobody, as I only get runtime_resume call in my driver, never the runtime_suspend.
Is there something I need to add to my driver to make this
work, or
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
should that part of the patch be reverted?
I believe it is quite common that a device driver calls pm_runtime_get_sync as a part of it's remove callback, then it explicitly returns it's resources that has been fetched during
probe.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Like a clk_disable_unprepare for example.
I guess you mean the driver calls pm_runtime_get_sync _and_ pm_runtime_put_sync as part of its remove callback?
Probably bus drivers need to do that, but for memory mapped
devices in a
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
SoC, I don't think there's normally any need to do pm_runtime_get/put_sync during the remove callback.
Quoted 1 line
The idea behind the change in __device_release_driver, was to
try to
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
prevent devices from going active->idle->active and instead
just
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
remain active (if possible).
In your case, which seems like a more modern way of
implementing
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
"remove", you shall call "pm_runtime_suspend" to make sure the runtime_suspend callbacks gets called.
And as far as I understand, the change creates an explicit
requirement
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
to do either pm_runtime_get/put_sync or pm_runtime_suspend inside driver's remove callback. If so, that should be mentioned in big
red
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
letters in the pm-runtime documentation.
The runtime_pm.txt doc does mention something related to this
(and btw,
Quoted 1 line
Quoted 1 line
Quoted 1 line
the doc says pm_runtime_put_sync is being called, which is no
longer
Quoted 1 line
Quoted 1 line
Quoted 1 line
true), but nothing clear about how the driver remove callback
must be
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
implemented.
That's correct.
Quoted 1 line
I tried grepping the kernel sources to find out if
pm_runtime_suspend is
Quoted 1 line
Quoted 1 line
Quoted 1 line
widely used to get SoC platform devices to suspend, but it
doesn't seem
Quoted 1 line
Quoted 1 line
Quoted 1 line
like it is. I didn't see pm_runtime_get/put_sync being used in
remove
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
callbacks widely either, but that was more difficult one to grep.
I think your observations are valid, which unfortunately means that
we'll
Quoted 1 line
Quoted 1 line
need to revert the commit in question, because it has changed the
behavior
Quoted 1 line
Quoted 1 line
that drivers are perfectly fine to expect given the existing
documentation
Quoted 3 lines
Quoted 3 lines
etc. It looks like the change was premature at least.
Greg, I wonder if you can queue up a revert of fa180eb448fa for
3.13, or
Quoted 3 lines
Quoted 1 line
do you want me to do that?
Would it be better to leave the runtime-idle callbacks (invoked
during
Quoted 2 lines
probe) async and revert only the change to __device_release_driver()?

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Having an async callback after probe shouldn't cause problems,
because
Quoted 1 line
the driver will then be bound (assuming the probe succeeded).
Right. OK, I'll prepare a patch.
That seems like a good way forward.

There you go.

---
From: Rafael J. Wysocki <[email protected]>
Subject: PM / runtime: Use pm_runtime_put_sync() in __device_release_driver()

Commit fa180eb448fa (PM / Runtime: Idle devices asynchronously after probe|release) modified __device_release_driver() to call pm_runtime_put(dev) instead of pm_runtime_put_sync(dev) before detaching the driver from the device. However, that was a mistake, because pm_runtime_put(dev) causes rpm_idle() to be queued up and the driver may be gone already when that function is executed. That breaks the assumptions the drivers have the right to make about the core's behavior on the basis of the existing documentation and actually causes problems to happen, so revert that part of commit fa180eb448fa and restore the previous behavior of __device_release_driver().

Reported-by Tomi Valkeinen <[email protected]>

Fixes fa180eb448fa (PM / Runtime: Idle devices asynchronously after probe|release)

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

Cc 3.10+ <[email protected]> # 3.10+

---
 drivers/base/dd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/base/dd.c +1 -1

Index: linux-pm/drivers/base/dd.c
===================================================================
@@ -499,7 +499,7 @@ static void __device_release_driver(stru
 						     BUS_NOTIFY_UNBIND_DRIVER,
 						     dev);
 
-		pm_runtime_put(dev);
+		pm_runtime_put_sync(dev);
 
 		if (dev->bus && dev->bus->remove)
 			dev->bus->remove(dev);

Kevin Hilman

Acked-by

2013-11-07 00:21 UTC #8 lore raw

Quoted 127 lines · Rafael J. Wysocki

Quoted 100 lines · Ulf Hansson
"Rafael J. Wysocki" <[email protected]> skrev:
Quoted 98 lines · Alan Stern
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Tomi Valkeinen
Quoted 2 lines · Ulf Hansson
Quoted 1 line
On 23 October 2013 12:11, Tomi Valkeinen
Quoted 3 lines · <[email protected]> wrote:
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Hi,
I was debugging why clocks were left enabled after removing
omapdss
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
driver, and I found this commit:
fa180eb448fa263cf18dd930143b515d27d70d7b (PM / Runtime: Idle
devices
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
asynchronously after probe|release)
I don't understand how that is supposed to work.
When a driver is removed, instead of using
pm_runtime_put_sync() the
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
commit uses pm_runtime_put(), so the runtime_suspend call is
queued. But
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
who is going to handle the queued suspend call, as the driver
is already
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
removed? At least in my case, obviously nobody, as I only get runtime_resume call in my driver, never the runtime_suspend.
Is there something I need to add to my driver to make this
work, or
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
should that part of the patch be reverted?
I believe it is quite common that a device driver calls pm_runtime_get_sync as a part of it's remove callback, then it explicitly returns it's resources that has been fetched during
probe.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Like a clk_disable_unprepare for example.
I guess you mean the driver calls pm_runtime_get_sync _and_ pm_runtime_put_sync as part of its remove callback?
Probably bus drivers need to do that, but for memory mapped
devices in a
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
SoC, I don't think there's normally any need to do pm_runtime_get/put_sync during the remove callback.
Quoted 1 line
The idea behind the change in __device_release_driver, was to
try to
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
prevent devices from going active->idle->active and instead
just
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
remain active (if possible).
In your case, which seems like a more modern way of
implementing
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
"remove", you shall call "pm_runtime_suspend" to make sure the runtime_suspend callbacks gets called.
And as far as I understand, the change creates an explicit
requirement
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
to do either pm_runtime_get/put_sync or pm_runtime_suspend inside driver's remove callback. If so, that should be mentioned in big
red
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
letters in the pm-runtime documentation.
The runtime_pm.txt doc does mention something related to this
(and btw,
Quoted 1 line
Quoted 1 line
Quoted 1 line
the doc says pm_runtime_put_sync is being called, which is no
longer
Quoted 1 line
Quoted 1 line
Quoted 1 line
true), but nothing clear about how the driver remove callback
must be
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
implemented.
That's correct.
Quoted 1 line
I tried grepping the kernel sources to find out if
pm_runtime_suspend is
Quoted 1 line
Quoted 1 line
Quoted 1 line
widely used to get SoC platform devices to suspend, but it
doesn't seem
Quoted 1 line
Quoted 1 line
Quoted 1 line
like it is. I didn't see pm_runtime_get/put_sync being used in
remove
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
callbacks widely either, but that was more difficult one to grep.
I think your observations are valid, which unfortunately means that
we'll
Quoted 1 line
Quoted 1 line
need to revert the commit in question, because it has changed the
behavior
Quoted 1 line
Quoted 1 line
that drivers are perfectly fine to expect given the existing
documentation
Quoted 3 lines
Quoted 3 lines
etc. It looks like the change was premature at least.
Greg, I wonder if you can queue up a revert of fa180eb448fa for
3.13, or
Quoted 3 lines
Quoted 1 line
do you want me to do that?
Would it be better to leave the runtime-idle callbacks (invoked
during
Quoted 2 lines
probe) async and revert only the change to __device_release_driver()?
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Having an async callback after probe shouldn't cause problems,
because
Quoted 1 line
the driver will then be bound (assuming the probe succeeded).
Right. OK, I'll prepare a patch.
That seems like a good way forward.
There you go.
---
From: Rafael J. Wysocki <[email protected]>
Subject: PM / runtime: Use pm_runtime_put_sync() in __device_release_driver()
Commit fa180eb448fa (PM / Runtime: Idle devices asynchronously after probe|release) modified __device_release_driver() to call pm_runtime_put(dev) instead of pm_runtime_put_sync(dev) before detaching the driver from the device. However, that was a mistake, because pm_runtime_put(dev) causes rpm_idle() to be queued up and the driver may be gone already when that function is executed. That breaks the assumptions the drivers have the right to make about the core's behavior on the basis of the existing documentation and actually causes problems to happen, so revert that part of commit fa180eb448fa and restore the previous behavior of __device_release_driver().

Quoted 4 lines

Reported-by Tomi Valkeinen <[email protected]>
Fixes fa180eb448fa (PM / Runtime: Idle devices asynchronously after probe|release)
Signed-off-by Rafael J. Wysocki <[email protected]>
Cc 3.10+ <[email protected]> # 3.10+

Acked-by Kevin Hilman <[email protected]>

I like this fix since I don't want to add any more requirements to drivers.

Kevin

Quoted 18 lines

---
 drivers/base/dd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-pm/drivers/base/dd.c
===================================================================

drivers/base/dd.c +1 -1

@@ -499,7 +499,7 @@ static void __device_release_driver(stru
                                                     BUS_NOTIFY_UNBIND_DRIVER,
                                                     dev);
 
-               pm_runtime_put(dev);
+               pm_runtime_put_sync(dev);
 
                if (dev->bus && dev->bus->remove)
                        dev->bus->remove(dev);

Rafael J. Wysocki

2013-11-07 00:53 UTC #9 lore raw

Quoted 128 lines · Kevin Hilman

Quoted 127 lines · Rafael J. Wysocki
Quoted 100 lines · Ulf Hansson
"Rafael J. Wysocki" <[email protected]> skrev:
Quoted 98 lines · Alan Stern
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Tomi Valkeinen
Quoted 2 lines · Ulf Hansson
Quoted 1 line
On 23 October 2013 12:11, Tomi Valkeinen
Quoted 3 lines · <[email protected]> wrote:
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Hi,
I was debugging why clocks were left enabled after removing
omapdss
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
driver, and I found this commit:
fa180eb448fa263cf18dd930143b515d27d70d7b (PM / Runtime: Idle
devices
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
asynchronously after probe|release)
I don't understand how that is supposed to work.
When a driver is removed, instead of using
pm_runtime_put_sync() the
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
commit uses pm_runtime_put(), so the runtime_suspend call is
queued. But
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
who is going to handle the queued suspend call, as the driver
is already
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
removed? At least in my case, obviously nobody, as I only get runtime_resume call in my driver, never the runtime_suspend.
Is there something I need to add to my driver to make this
work, or
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
should that part of the patch be reverted?
I believe it is quite common that a device driver calls pm_runtime_get_sync as a part of it's remove callback, then it explicitly returns it's resources that has been fetched during
probe.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Like a clk_disable_unprepare for example.
I guess you mean the driver calls pm_runtime_get_sync _and_ pm_runtime_put_sync as part of its remove callback?
Probably bus drivers need to do that, but for memory mapped
devices in a
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
SoC, I don't think there's normally any need to do pm_runtime_get/put_sync during the remove callback.
Quoted 1 line
The idea behind the change in __device_release_driver, was to
try to
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
prevent devices from going active->idle->active and instead
just
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
remain active (if possible).
In your case, which seems like a more modern way of
implementing
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
"remove", you shall call "pm_runtime_suspend" to make sure the runtime_suspend callbacks gets called.
And as far as I understand, the change creates an explicit
requirement
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
to do either pm_runtime_get/put_sync or pm_runtime_suspend inside driver's remove callback. If so, that should be mentioned in big
red
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
letters in the pm-runtime documentation.
The runtime_pm.txt doc does mention something related to this
(and btw,
Quoted 1 line
Quoted 1 line
Quoted 1 line
the doc says pm_runtime_put_sync is being called, which is no
longer
Quoted 1 line
Quoted 1 line
Quoted 1 line
true), but nothing clear about how the driver remove callback
must be
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
implemented.
That's correct.
Quoted 1 line
I tried grepping the kernel sources to find out if
pm_runtime_suspend is
Quoted 1 line
Quoted 1 line
Quoted 1 line
widely used to get SoC platform devices to suspend, but it
doesn't seem
Quoted 1 line
Quoted 1 line
Quoted 1 line
like it is. I didn't see pm_runtime_get/put_sync being used in
remove
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
callbacks widely either, but that was more difficult one to grep.
I think your observations are valid, which unfortunately means that
we'll
Quoted 1 line
Quoted 1 line
need to revert the commit in question, because it has changed the
behavior
Quoted 1 line
Quoted 1 line
that drivers are perfectly fine to expect given the existing
documentation
Quoted 3 lines
Quoted 3 lines
etc. It looks like the change was premature at least.
Greg, I wonder if you can queue up a revert of fa180eb448fa for
3.13, or
Quoted 3 lines
Quoted 1 line
do you want me to do that?
Would it be better to leave the runtime-idle callbacks (invoked
during
Quoted 2 lines
probe) async and revert only the change to __device_release_driver()?
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Having an async callback after probe shouldn't cause problems,
because
Quoted 1 line
the driver will then be bound (assuming the probe succeeded).
Right. OK, I'll prepare a patch.
That seems like a good way forward.
There you go.
---
From: Rafael J. Wysocki <[email protected]>
Subject: PM / runtime: Use pm_runtime_put_sync() in __device_release_driver()
Commit fa180eb448fa (PM / Runtime: Idle devices asynchronously after probe|release) modified __device_release_driver() to call pm_runtime_put(dev) instead of pm_runtime_put_sync(dev) before detaching the driver from the device. However, that was a mistake, because pm_runtime_put(dev) causes rpm_idle() to be queued up and the driver may be gone already when that function is executed. That breaks the assumptions the drivers have the right to make about the core's behavior on the basis of the existing documentation and actually causes problems to happen, so revert that part of commit fa180eb448fa and restore the previous behavior of __device_release_driver().

Quoted 8 lines

Quoted 4 lines
Reported-by Tomi Valkeinen <[email protected]>
Fixes fa180eb448fa (PM / Runtime: Idle devices asynchronously after probe|release)
Signed-off-by Rafael J. Wysocki <[email protected]>
Cc 3.10+ <[email protected]> # 3.10+
Acked-by Kevin Hilman <[email protected]>
I like this fix since I don't want to add any more requirements to drivers.

OK, I've queued this up for 3.13.

Thanks!

Quoted 22 lines

Quoted 10 lines
---
 drivers/base/dd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-pm/drivers/base/dd.c
===================================================================
drivers/base/dd.c static void __device_release_driver(stru · line 505
Quoted 8 lines
drivers/base/dd.c +1 -1
@@ -499,7 +499,7 @@ static void __device_release_driver(stru
                                                     BUS_NOTIFY_UNBIND_DRIVER,
                                                     dev);
 
-               pm_runtime_put(dev);
+               pm_runtime_put_sync(dev);
 
                if (dev->bus && dev->bus->remove)
                        dev->bus->remove(dev);
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Ulf Hansson

Acked-by

2013-11-07 08:18 UTC #10 lore raw

Quoted 129 lines · Rafael J. Wysocki

Quoted 128 lines · Kevin Hilman
Quoted 127 lines · Rafael J. Wysocki
Quoted 100 lines · Ulf Hansson
"Rafael J. Wysocki" <[email protected]> skrev:
Quoted 98 lines · Alan Stern
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Tomi Valkeinen
Quoted 2 lines · Ulf Hansson
Quoted 1 line
On 23 October 2013 12:11, Tomi Valkeinen
Quoted 3 lines · <[email protected]> wrote:
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Hi,
I was debugging why clocks were left enabled after removing
omapdss
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
driver, and I found this commit:
fa180eb448fa263cf18dd930143b515d27d70d7b (PM / Runtime: Idle
devices
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
asynchronously after probe|release)
I don't understand how that is supposed to work.
When a driver is removed, instead of using
pm_runtime_put_sync() the
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
commit uses pm_runtime_put(), so the runtime_suspend call is
queued. But
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
who is going to handle the queued suspend call, as the driver
is already
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
removed? At least in my case, obviously nobody, as I only get runtime_resume call in my driver, never the runtime_suspend.
Is there something I need to add to my driver to make this
work, or
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
should that part of the patch be reverted?
I believe it is quite common that a device driver calls pm_runtime_get_sync as a part of it's remove callback, then it explicitly returns it's resources that has been fetched during
probe.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Like a clk_disable_unprepare for example.
I guess you mean the driver calls pm_runtime_get_sync _and_ pm_runtime_put_sync as part of its remove callback?
Probably bus drivers need to do that, but for memory mapped
devices in a
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
SoC, I don't think there's normally any need to do pm_runtime_get/put_sync during the remove callback.
Quoted 1 line
The idea behind the change in __device_release_driver, was to
try to
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
prevent devices from going active->idle->active and instead
just
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
remain active (if possible).
In your case, which seems like a more modern way of
implementing
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
"remove", you shall call "pm_runtime_suspend" to make sure the runtime_suspend callbacks gets called.
And as far as I understand, the change creates an explicit
requirement
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
to do either pm_runtime_get/put_sync or pm_runtime_suspend inside driver's remove callback. If so, that should be mentioned in big
red
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
letters in the pm-runtime documentation.
The runtime_pm.txt doc does mention something related to this
(and btw,
Quoted 1 line
Quoted 1 line
Quoted 1 line
the doc says pm_runtime_put_sync is being called, which is no
longer
Quoted 1 line
Quoted 1 line
Quoted 1 line
true), but nothing clear about how the driver remove callback
must be
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
implemented.
That's correct.
Quoted 1 line
I tried grepping the kernel sources to find out if
pm_runtime_suspend is
Quoted 1 line
Quoted 1 line
Quoted 1 line
widely used to get SoC platform devices to suspend, but it
doesn't seem
Quoted 1 line
Quoted 1 line
Quoted 1 line
like it is. I didn't see pm_runtime_get/put_sync being used in
remove
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
callbacks widely either, but that was more difficult one to grep.
I think your observations are valid, which unfortunately means that
we'll
Quoted 1 line
Quoted 1 line
need to revert the commit in question, because it has changed the
behavior
Quoted 1 line
Quoted 1 line
that drivers are perfectly fine to expect given the existing
documentation
Quoted 3 lines
Quoted 3 lines
etc. It looks like the change was premature at least.
Greg, I wonder if you can queue up a revert of fa180eb448fa for
3.13, or
Quoted 3 lines
Quoted 1 line
do you want me to do that?
Would it be better to leave the runtime-idle callbacks (invoked
during
Quoted 2 lines
probe) async and revert only the change to __device_release_driver()?
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Having an async callback after probe shouldn't cause problems,
because
Quoted 1 line
the driver will then be bound (assuming the probe succeeded).
Right. OK, I'll prepare a patch.
That seems like a good way forward.
There you go.
---
From: Rafael J. Wysocki <[email protected]>
Subject: PM / runtime: Use pm_runtime_put_sync() in __device_release_driver()
Commit fa180eb448fa (PM / Runtime: Idle devices asynchronously after probe|release) modified __device_release_driver() to call pm_runtime_put(dev) instead of pm_runtime_put_sync(dev) before detaching the driver from the device. However, that was a mistake, because pm_runtime_put(dev) causes rpm_idle() to be queued up and the driver may be gone already when that function is executed. That breaks the assumptions the drivers have the right to make about the core's behavior on the basis of the existing documentation and actually causes problems to happen, so revert that part of commit fa180eb448fa and restore the previous behavior of __device_release_driver().

Quoted 8 lines

Quoted 8 lines
Quoted 4 lines
Reported-by Tomi Valkeinen <[email protected]>
Fixes fa180eb448fa (PM / Runtime: Idle devices asynchronously after probe|release)
Signed-off-by Rafael J. Wysocki <[email protected]>
Cc 3.10+ <[email protected]> # 3.10+
Acked-by Kevin Hilman <[email protected]>
I like this fix since I don't want to add any more requirements to drivers.

Agree!

Quoted 2 lines

OK, I've queued this up for 3.13.

If not to late:

Acked-by Ulf Hansson <[email protected]>

BTW, I start creating a patch on the doc to align it to the changes that the "async" patches made.

Kind regards
Ulf Hansson

Quoted 29 lines

Thanks!
Quoted 15 lines
Quoted 15 lines
---
 drivers/base/dd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-pm/drivers/base/dd.c
===================================================================
drivers/base/dd.c +1 -1
@@ -499,7 +499,7 @@ static void __device_release_driver(stru
                                                     BUS_NOTIFY_UNBIND_DRIVER,
                                                     dev);
 
-               pm_runtime_put(dev);
+               pm_runtime_put_sync(dev);
 
Quoted 7 lines
drivers/base/dd.c static void __device_release_driver(stru · line 505
Quoted 3 lines
drivers/base/dd.c +0 -0
                if (dev->bus && dev->bus->remove)
                        dev->bus->remove(dev);
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Rafael J. Wysocki

2013-11-07 18:43 UTC #11 lore raw

Quoted 139 lines · Ulf Hansson

Quoted 129 lines · Rafael J. Wysocki
Quoted 128 lines · Kevin Hilman
Quoted 127 lines · Rafael J. Wysocki
Quoted 100 lines · Ulf Hansson
"Rafael J. Wysocki" <[email protected]> skrev:
Quoted 98 lines · Alan Stern
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Tomi Valkeinen
Quoted 2 lines · Ulf Hansson
Quoted 1 line
On 23 October 2013 12:11, Tomi Valkeinen
Quoted 3 lines · <[email protected]> wrote:
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Hi,
I was debugging why clocks were left enabled after removing
omapdss
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
driver, and I found this commit:
fa180eb448fa263cf18dd930143b515d27d70d7b (PM / Runtime: Idle
devices
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
asynchronously after probe|release)
I don't understand how that is supposed to work.
When a driver is removed, instead of using
pm_runtime_put_sync() the
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
commit uses pm_runtime_put(), so the runtime_suspend call is
queued. But
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
who is going to handle the queued suspend call, as the driver
is already
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
removed? At least in my case, obviously nobody, as I only get runtime_resume call in my driver, never the runtime_suspend.
Is there something I need to add to my driver to make this
work, or
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
should that part of the patch be reverted?
I believe it is quite common that a device driver calls pm_runtime_get_sync as a part of it's remove callback, then it explicitly returns it's resources that has been fetched during
probe.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Like a clk_disable_unprepare for example.
I guess you mean the driver calls pm_runtime_get_sync _and_ pm_runtime_put_sync as part of its remove callback?
Probably bus drivers need to do that, but for memory mapped
devices in a
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
SoC, I don't think there's normally any need to do pm_runtime_get/put_sync during the remove callback.
Quoted 1 line
The idea behind the change in __device_release_driver, was to
try to
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
prevent devices from going active->idle->active and instead
just
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
remain active (if possible).
In your case, which seems like a more modern way of
implementing
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
"remove", you shall call "pm_runtime_suspend" to make sure the runtime_suspend callbacks gets called.
And as far as I understand, the change creates an explicit
requirement
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
to do either pm_runtime_get/put_sync or pm_runtime_suspend inside driver's remove callback. If so, that should be mentioned in big
red
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
letters in the pm-runtime documentation.
The runtime_pm.txt doc does mention something related to this
(and btw,
Quoted 1 line
Quoted 1 line
Quoted 1 line
the doc says pm_runtime_put_sync is being called, which is no
longer
Quoted 1 line
Quoted 1 line
Quoted 1 line
true), but nothing clear about how the driver remove callback
must be
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
implemented.
That's correct.
Quoted 1 line
I tried grepping the kernel sources to find out if
pm_runtime_suspend is
Quoted 1 line
Quoted 1 line
Quoted 1 line
widely used to get SoC platform devices to suspend, but it
doesn't seem
Quoted 1 line
Quoted 1 line
Quoted 1 line
like it is. I didn't see pm_runtime_get/put_sync being used in
remove
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
callbacks widely either, but that was more difficult one to grep.
I think your observations are valid, which unfortunately means that
we'll
Quoted 1 line
Quoted 1 line
need to revert the commit in question, because it has changed the
behavior
Quoted 1 line
Quoted 1 line
that drivers are perfectly fine to expect given the existing
documentation
Quoted 3 lines
Quoted 3 lines
etc. It looks like the change was premature at least.
Greg, I wonder if you can queue up a revert of fa180eb448fa for
3.13, or
Quoted 3 lines
Quoted 1 line
do you want me to do that?
Would it be better to leave the runtime-idle callbacks (invoked
during
Quoted 2 lines
probe) async and revert only the change to __device_release_driver()?
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Having an async callback after probe shouldn't cause problems,
because
Quoted 1 line
the driver will then be bound (assuming the probe succeeded).
Right. OK, I'll prepare a patch.
That seems like a good way forward.
There you go.
---
From: Rafael J. Wysocki <[email protected]>
Subject: PM / runtime: Use pm_runtime_put_sync() in __device_release_driver()
Commit fa180eb448fa (PM / Runtime: Idle devices asynchronously after probe|release) modified __device_release_driver() to call pm_runtime_put(dev) instead of pm_runtime_put_sync(dev) before detaching the driver from the device. However, that was a mistake, because pm_runtime_put(dev) causes rpm_idle() to be queued up and the driver may be gone already when that function is executed. That breaks the assumptions the drivers have the right to make about the core's behavior on the basis of the existing documentation and actually causes problems to happen, so revert that part of commit fa180eb448fa and restore the previous behavior of __device_release_driver().
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
Reported-by Tomi Valkeinen <[email protected]>
Fixes fa180eb448fa (PM / Runtime: Idle devices asynchronously after probe|release)
Signed-off-by Rafael J. Wysocki <[email protected]>
Cc 3.10+ <[email protected]> # 3.10+
Acked-by Kevin Hilman <[email protected]>
I like this fix since I don't want to add any more requirements to drivers.

Quoted 8 lines

Agree!
Quoted 2 lines
OK, I've queued this up for 3.13.
If not to late:
Acked-by Ulf Hansson <[email protected]>

No, it isn't, thanks!

Quoted 2 lines

BTW, I start creating a patch on the doc to align it to the changes that the "async" patches made.

I've seen it, please address the Alan's comments in that patch.

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.