PM: runtime: Fix rpm_idle() and relocate rpm_callback()
Subsystems (4)
cover PM: runtime: Fix rpm_idle() and relocate rpm_callback()
Hi All,
The first patch in this series replaces the __rpm_callback() invocation in rpm_idle() with something simpler (and more correct) and the second one moves rpm_callback() next to __rpm_callback().
Thanks!
PATCH 1/2 PM: runtime: Do not call __rpm_callback() from rpm_idle()
From: Rafael J. Wysocki <[email protected]>
Calling __rpm_callback() from rpm_idle() after adding device links support to the former is a clear mistake.
Not only it causes rpm_idle() to carry out unnecessary actions, but it is also against the assumption regarding the stability of PM-runtime status accross __rpm_callback() invocations, because rpm_suspend() and rpm_resume() may run in parallel with __rpm_callback() when it is called by rpm_idle() and the device's PM-runtime status can be updated by any of them.
--- drivers/base/power/runtime.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
drivers/base/power/runtime.c +11 -1
@@ -484,7 +484,17 @@ static int rpm_idle(struct device *dev, dev->power.idle_notification = true; - retval =__rpm_callback (callback, dev); +if (dev->power.irq_safe) +spin_unlock (&dev->power.lock); +else +spin_unlock_irq (&dev->power.lock); + + retval =callback (dev); + +if (dev->power.irq_safe) +spin_lock (&dev->power.lock); +else +spin_lock_irq (&dev->power.lock); dev->power.idle_notification = false;wake_up_all (&dev->power.wait_queue);
Re: [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle()
Quoted 8 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Calling
__rpm_callback()fromrpm_idle()after adding device links support to the former is a clear mistake.Not only it causes
rpm_idle()to carry out unnecessary actions, but it is also against the assumption regarding the stability of PM-runtime status accross__rpm_callback()invocations, becauserpm_suspend()and
accross -> across
Quoted 5 lines
rpm_resume()may run in parallel with__rpm_callback()when it is called byrpm_idle()and the device's PM-runtime status can be updated by any of them.Fixes 21d5c57b3726 ("PM / runtime: Use device links")
Quoted 31 lines
Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/base/power/runtime.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)Index:
linux-pm/drivers/base/power/runtime.c
===================================================================drivers/base/power/runtime.c +11
-1@@ -484,7 +484,17 @@ static int rpm_idle(struct device *dev, dev->power.idle_notification = true; - retval =__rpm_callback (callback, dev); +if (dev->power.irq_safe) +spin_unlock (&dev->power.lock); +else +spin_unlock_irq (&dev->power.lock); + + retval =callback (dev); + +if (dev->power.irq_safe) +spin_lock (&dev->power.lock); +else +spin_lock_irq (&dev->power.lock); dev->power.idle_notification = false;wake_up_all (&dev->power.wait_queue);
Rafael J. Wysocki
maintainerRe: [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle()
Quoted 4 lines · Adrian Hunter
Quoted 2 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Quoted 8 lines
Quoted 6 lines
Calling
__rpm_callback()fromrpm_idle()after adding device links support to the former is a clear mistake.Not only it causes
rpm_idle()to carry out unnecessary actions, but it is also against the assumption regarding the stability of PM-runtime status accross__rpm_callback()invocations, becauserpm_suspend()andaccross -> across
Fixed whey applying the patch.
Quoted 7 lines
Quoted 5 lines
rpm_resume()may run in parallel with__rpm_callback()when it is called byrpm_idle()and the device's PM-runtime status can be updated by any of them.Fixes 21d5c57b3726 ("PM / runtime: Use device links")Reviewed-by Adrian Hunter <[email protected]>
Thank you!
Quoted 32 lines
Quoted 31 lines
Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/base/power/runtime.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)Index:
linux-pm/drivers/base/power/runtime.c
===================================================================drivers/base/power/runtime.c +11
-1@@ -484,7 +484,17 @@ static int rpm_idle(struct device *dev, dev->power.idle_notification = true; - retval =__rpm_callback (callback, dev); +if (dev->power.irq_safe) +spin_unlock (&dev->power.lock); +else +spin_unlock_irq (&dev->power.lock); + + retval =callback (dev); + +if (dev->power.irq_safe) +spin_lock (&dev->power.lock); +else +spin_lock_irq (&dev->power.lock); dev->power.idle_notification = false;wake_up_all (&dev->power.wait_queue);
Re: [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle()
In reply to Rafael J. WysockiQuoted 6 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Calling
__rpm_callback()fromrpm_idle()after adding device links support to the former is a clear mistake.
Quoted 6 lines
Not only it causes
rpm_idle()to carry out unnecessary actions, but it is also against the assumption regarding the stability of PM-runtime status accross__rpm_callback()invocations, becauserpm_suspend()andrpm_resume()may run in parallel with__rpm_callback()when it is called byrpm_idle()and the device's PM-runtime status can be updated by any of them.
Urgh, that's a nasty bug you are fixing here. Is there perhaps some links to some error reports that can make sense to include here?
Quoted 12 lines
Fixes 21d5c57b3726 ("PM / runtime: Use device links")Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/base/power/runtime.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)Index:
linux-pm/drivers/base/power/runtime.c
===================================================================
drivers/base/power/runtime.c static int rpm_idle(struct device *dev, · line 487
Quoted 4 lines
drivers/base/power/runtime.c +0
-1@@ -484,7 +484,17 @@ static int rpm_idle(struct device *dev, dev->power.idle_notification = true; - retval =__rpm_callback (callback, dev);
Couldn't we just extend __rpm_callback() to take another in-parameter, rather than open-coding the below?
Note that, __rpm_callback() already uses a "bool use_links" internal variable, that indicates whether the device links should be used or not.
Quoted 7 lines
drivers/base/power/runtime.c +4
-0+if (dev->power.irq_safe) +spin_unlock (&dev->power.lock); +else +spin_unlock_irq (&dev->power.lock);
drivers/base/power/runtime.c static int rpm_idle(struct device *dev, · line 500
Quoted 10 lines
drivers/base/power/runtime.c +7
-0+ + retval =callback (dev); + +if (dev->power.irq_safe) +spin_lock (&dev->power.lock); +else +spin_lock_irq (&dev->power.lock); dev->power.idle_notification = false;wake_up_all (&dev->power.wait_queue);
Kind regards
Uffe
Rafael J. Wysocki
maintainerRe: [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle()
Quoted 15 lines · Ulf Hansson
Quoted 6 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Calling
__rpm_callback()fromrpm_idle()after adding device links support to the former is a clear mistake.Quoted 6 lines
Not only it causes
rpm_idle()to carry out unnecessary actions, but it is also against the assumption regarding the stability of PM-runtime status accross__rpm_callback()invocations, becauserpm_suspend()andrpm_resume()may run in parallel with__rpm_callback()when it is called byrpm_idle()and the device's PM-runtime status can be updated by any of them.
Quoted 2 lines
Urgh, that's a nasty bug you are fixing here. Is there perhaps some links to some error reports that can make sense to include here?
There is a bug report, but I have no confirmation that this fix is sufficient to address it (even though I'm quite confident that it will be).
Quoted 13 lines
Quoted 13 lines
Fixes 21d5c57b3726 ("PM / runtime: Use device links")Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/base/power/runtime.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)Index:
linux-pm/drivers/base/power/runtime.c
===================================================================drivers/base/power/runtime.c +0
-0@@ -484,7 +484,17 @@ static int rpm_idle(struct device *dev,
Quoted 6 lines
drivers/base/power/runtime.c static int rpm_idle(struct device *dev, · line 487
Quoted 3 lines
drivers/base/power/runtime.c +0
-1dev->power.idle_notification = true; - retval =__rpm_callback (callback, dev);Couldn't we just extend
__rpm_callback()to take another in-parameter, rather than open-coding the below?
I'd rather not do that.
I'd prefer rpm_callback() to be used only in rpm_suspend() and rpm_resume() where all of the assumptions hold and rpm_idle() really is a special case.
And there is not much open-coding here, just the locking part.
Quoted 3 lines
Note that,
__rpm_callback()already uses a "booluse_links" internal variable, that indicates whether the device links should be used or not.
Yes, it does, but why does that matter?
Quoted 17 lines
Quoted 17 lines
drivers/base/power/runtime.c +11
-0+if (dev->power.irq_safe) +spin_unlock (&dev->power.lock); +else +spin_unlock_irq (&dev->power.lock); + + retval =callback (dev); + +if (dev->power.irq_safe) +spin_lock (&dev->power.lock); +else +spin_lock_irq (&dev->power.lock); dev->power.idle_notification = false;wake_up_all (&dev->power.wait_queue);
Re: [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle()
Quoted 44 lines · Rafael J. Wysocki
Quoted 15 lines · Ulf Hansson
Quoted 6 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Calling
__rpm_callback()fromrpm_idle()after adding device links support to the former is a clear mistake.Quoted 6 lines
Not only it causes
rpm_idle()to carry out unnecessary actions, but it is also against the assumption regarding the stability of PM-runtime status accross__rpm_callback()invocations, becauserpm_suspend()andrpm_resume()may run in parallel with__rpm_callback()when it is called byrpm_idle()and the device's PM-runtime status can be updated by any of them.Quoted 2 lines
Urgh, that's a nasty bug you are fixing here. Is there perhaps some links to some error reports that can make sense to include here?
There is a bug report, but I have no confirmation that this fix is sufficient to address it (even though I'm quite confident that it will be).
Quoted 13 lines
Quoted 13 lines
Fixes 21d5c57b3726 ("PM / runtime: Use device links")Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/base/power/runtime.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)Index:
linux-pm/drivers/base/power/runtime.c
===================================================================drivers/base/power/runtime.c +0
-0@@ -484,7 +484,17 @@ static int rpm_idle(struct device *dev,Quoted 6 lines
drivers/base/power/runtime.c static int rpm_idle(struct device *dev, · line 487
Quoted 3 lines
drivers/base/power/runtime.c +0
-1dev->power.idle_notification = true; - retval =__rpm_callback (callback, dev);Couldn't we just extend
__rpm_callback()to take another in-parameter, rather than open-coding the below?
Quoted 7 lines
I'd rather not do that.
I'd prefer
rpm_callback()to be used only inrpm_suspend()andrpm_resume()where all of the assumptions hold andrpm_idle()really is a special case.And there is not much open-coding here, just the locking part.
That and the actual call to the callback. Not much, but still.
Quoted 6 lines
Quoted 4 lines
Note that,
__rpm_callback()already uses a "booluse_links" internal variable, that indicates whether the device links should be used or not.Yes, it does, but why does that matter?
It means that __rpm_callback() is already prepared to (almost) cover this case.
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
drivers/base/power/runtime.c +11
-0+if (dev->power.irq_safe) +spin_unlock (&dev->power.lock); +else +spin_unlock_irq (&dev->power.lock); + + retval =callback (dev); + +if (dev->power.irq_safe) +spin_lock (&dev->power.lock); +else +spin_lock_irq (&dev->power.lock);
Quoted 5 lines
Quoted 5 lines
drivers/base/power/runtime.c static int rpm_idle(struct device *dev, · line 500
Quoted 5 lines
drivers/base/power/runtime.c +0
-0dev->power.idle_notification = false;wake_up_all (&dev->power.wait_queue);
Note, it's not a big deal to me, if you feel strongly that your current approach is better, I am fine with that too.
Kind regards
Uffe
Rafael J. Wysocki
maintainerRe: [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle()
Quoted 48 lines · Ulf Hansson
Quoted 46 lines · Rafael J. Wysocki
Quoted 15 lines · Ulf Hansson
Quoted 6 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Calling
__rpm_callback()fromrpm_idle()after adding device links support to the former is a clear mistake.Quoted 6 lines
Not only it causes
rpm_idle()to carry out unnecessary actions, but it is also against the assumption regarding the stability of PM-runtime status accross__rpm_callback()invocations, becauserpm_suspend()andrpm_resume()may run in parallel with__rpm_callback()when it is called byrpm_idle()and the device's PM-runtime status can be updated by any of them.Quoted 2 lines
Urgh, that's a nasty bug you are fixing here. Is there perhaps some links to some error reports that can make sense to include here?
There is a bug report, but I have no confirmation that this fix is sufficient to address it (even though I'm quite confident that it will be).
Quoted 13 lines
Quoted 13 lines
Fixes 21d5c57b3726 ("PM / runtime: Use device links")Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/base/power/runtime.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)Index:
linux-pm/drivers/base/power/runtime.c
===================================================================drivers/base/power/runtime.c +0
-0@@ -484,7 +484,17 @@ static int rpm_idle(struct device *dev,Quoted 6 lines
drivers/base/power/runtime.c static int rpm_idle(struct device *dev, · line 487
Quoted 3 lines
drivers/base/power/runtime.c +0
-1dev->power.idle_notification = true; - retval =__rpm_callback (callback, dev);Couldn't we just extend
__rpm_callback()to take another in-parameter, rather than open-coding the below?I'd rather not do that.
Quoted 7 lines
Quoted 5 lines
I'd prefer
rpm_callback()to be used only inrpm_suspend()andrpm_resume()where all of the assumptions hold andrpm_idle()really is a special case.And there is not much open-coding here, just the locking part.
That and the actual call to the callback. Not much, but still.
Note that it doesn't need to check the callback pointer, though.
Moreover, IMO this code is easier to read without having to look at __rpm_callback() and reverse engineer all of the different use cases covered by it.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Note that,
__rpm_callback()already uses a "booluse_links" internal variable, that indicates whether the device links should be used or not.Yes, it does, but why does that matter?
It means that
__rpm_callback()is already prepared to (almost) cover this case.
Well, why does it have to cover all of the cases that are even somewhat related?
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
drivers/base/power/runtime.c +11
-0+if (dev->power.irq_safe) +spin_unlock (&dev->power.lock); +else +spin_unlock_irq (&dev->power.lock); + + retval =callback (dev); + +if (dev->power.irq_safe) +spin_lock (&dev->power.lock); +else +spin_lock_irq (&dev->power.lock);
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
drivers/base/power/runtime.c static int rpm_idle(struct device *dev, · line 500
Quoted 5 lines
drivers/base/power/runtime.c +0
-0dev->power.idle_notification = false;wake_up_all (&dev->power.wait_queue);Note, it's not a big deal to me, if you feel strongly that your current approach is better, I am fine with that too.
OK, thanks!
PATCH 2/2 PM: runtime: Relocate rpm_callback() right after __rpm_callback()
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
Because rpm_callback() is a wrapper around __rpm_callback(), and the only caller of it after the change eliminating an invocation of it from rpm_idle(), move the former next to the latter to make the code a bit easier to follow.
--- drivers/base/power/runtime.c | 64 +++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 32 deletions(-)
drivers/base/power/runtime.c +32 -32
@@ -422,6 +422,38 @@ fail: }/** +* rpm_callback - Run a given runtime PM callback for a given device. +* @cb: Runtime PM callback to run. +* @dev: Device to run the callback for. +*/ +static int rpm_callback (int (*cb)(struct device *),struct device *dev) +{ +int retval; + +if (dev->power.memalloc_noio) { +unsigned int noio_flag; + +/* +* Deadlock might be caused if memory allocation with +* GFP_KERNEL happens inside runtime_suspend and +* runtime_resume callbacks of one block device's +* ancestor or the block device itself. Network +* device might be thought as part of iSCSI block +* device, so network device and its ancestor should +* be marked as memalloc_noio too. +*/ + noio_flag =memalloc_noio_save (); + retval =__rpm_callback (cb, dev); +memalloc_noio_restore (noio_flag); + }else { + retval =__rpm_callback (cb, dev); + } + + dev->power.runtime_error = retval; +return retval != -EACCES ? retval : -EIO ; +} + +/** * rpm_idle - Notify device bus type if the device can be suspended. * @dev: Device to notify the bus type about. * @rpmflags: Flag bits.
@@ -505,38 +537,6 @@ static int rpm_idle(struct device *dev, }/** -* rpm_callback - Run a given runtime PM callback for a given device. -* @cb: Runtime PM callback to run. -* @dev: Device to run the callback for. -*/ -static int rpm_callback (int (*cb)(struct device *),struct device *dev) -{ -int retval; - -if (dev->power.memalloc_noio) { -unsigned int noio_flag; - -/* -* Deadlock might be caused if memory allocation with -* GFP_KERNEL happens inside runtime_suspend and -* runtime_resume callbacks of one block device's -* ancestor or the block device itself. Network -* device might be thought as part of iSCSI block -* device, so network device and its ancestor should -* be marked as memalloc_noio too. -*/ - noio_flag =memalloc_noio_save (); - retval =__rpm_callback (cb, dev); -memalloc_noio_restore (noio_flag); - }else { - retval =__rpm_callback (cb, dev); - } - - dev->power.runtime_error = retval; -return retval != -EACCES ? retval : -EIO ; -} - -/** * rpm_suspend - Carry out runtime suspend of given device. * @dev: Device to suspend. * @rpmflags: Flag bits.
Re: [PATCH v1 2/2] PM: runtime: Relocate rpm_callback() right after __rpm_callback()
Quoted 8 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Because
rpm_callback()is a wrapper around__rpm_callback(), and the only caller of it after the change eliminating an invocation of it fromrpm_idle(), move the former next to the latter to make the code a bit easier to follow.Signed-off-by Rafael J. Wysocki <[email protected]>
Quoted 89 lines
--- drivers/base/power/runtime.c | 64 +++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 32 deletions(-)Index:
linux-pm/drivers/base/power/runtime.c
===================================================================drivers/base/power/runtime.c +32
-32@@ -422,6 +422,38 @@ fail: }/** +* rpm_callback - Run a given runtime PM callback for a given device. +* @cb: Runtime PM callback to run. +* @dev: Device to run the callback for. +*/ +static int rpm_callback (int (*cb)(struct device *),struct device *dev) +{ +int retval; + +if (dev->power.memalloc_noio) { +unsigned int noio_flag; + +/* +* Deadlock might be caused if memory allocation with +* GFP_KERNEL happens inside runtime_suspend and +* runtime_resume callbacks of one block device's +* ancestor or the block device itself. Network +* device might be thought as part of iSCSI block +* device, so network device and its ancestor should +* be marked as memalloc_noio too. +*/ + noio_flag =memalloc_noio_save (); + retval =__rpm_callback (cb, dev); +memalloc_noio_restore (noio_flag); + }else { + retval =__rpm_callback (cb, dev); + } + + dev->power.runtime_error = retval; +return retval != -EACCES ? retval : -EIO ; +} + +/** * rpm_idle - Notify device bus type if the device can be suspended. * @dev: Device to notify the bus type about. * @rpmflags: Flag bits. @@ -505,38 +537,6 @@ static int rpm_idle(struct device *dev, }/** -* rpm_callback - Run a given runtime PM callback for a given device. -* @cb: Runtime PM callback to run. -* @dev: Device to run the callback for. -*/ -static int rpm_callback (int (*cb)(struct device *),struct device *dev) -{ -int retval; - -if (dev->power.memalloc_noio) { -unsigned int noio_flag; - -/* -* Deadlock might be caused if memory allocation with -* GFP_KERNEL happens inside runtime_suspend and -* runtime_resume callbacks of one block device's -* ancestor or the block device itself. Network -* device might be thought as part of iSCSI block -* device, so network device and its ancestor should -* be marked as memalloc_noio too. -*/ - noio_flag =memalloc_noio_save (); - retval =__rpm_callback (cb, dev); -memalloc_noio_restore (noio_flag); - }else { - retval =__rpm_callback (cb, dev); - } - - dev->power.runtime_error = retval; -return retval != -EACCES ? retval : -EIO ; -} - -/** * rpm_suspend - Carry out runtime suspend of given device. * @dev: Device to suspend. * @rpmflags: Flag bits.