Re: [PATCH] pm: sleep: do not set is_prepared when no_pm_callbacks is set

Rafael J. Wysocki
2024-09-03T12:31:16Z

[-- Attachment #1: Type: text/plain, Size: 3037 bytes --]

On Mon, Sep 2, 2024 at 2:59 PM David Wang <00107082@163.com> wrote:
>
> When resume, a parent device with no pm callbacks
> would have "is_prepared" and "direct_complete" bit
> set, and skip the "fib" chance to unset "is_prepared"
> in device_resume because of the direct_complete bit.

Sure, but is_prepared will be cleared in device_complete() AFAICS.

> This will trigger a kernel warning when resume its child
> For example, when suspend system with an USB webcam
> opened, following warning would show up during resume:
>
> >usb 3-1.1: reset high-speed USB device number 4 using xhci_hcd
> >..
> >ep_81: PM: parent 3-1.1:1.1 should not be sleeping

This is printed in device_pm_add(), so apparently something new has
appeared under the parent while it's between "resume" and "prepare".

The parent is actually still regarded as "suspended" because any
resume callbacks have not been called for it, but new children can be
added under it at this point because doing so does not break the
dpm_list ordering and all of its ancestors have been already resumed.

> The device parenting relationships are:
> [usb 3-1.1] << [uvcvideo 3-1.1:1.1] << [ep_81].
> When resume, since the virtual [uvcvideo 3-1.1:1.1] device
> has no pm callbacks, it would not clear "is_prepared"
> once set. Then, when resume [ep_81], pm module would
> yield a warn seeing [ep_81]'s parent [uvcvideo 3-1.1:1.1]
> having "is_prepared".
>
> Do not set "is_prepared" for virtual devices having
> no pm callbacks can clear those kernel warnings.
>
> Signed-off-by: David Wang <00107082@163.com>
> ---
> drivers/base/power/main.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 934e5bb61f13..e2149ccf2c3e 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1880,7 +1880,8 @@ int dpm_prepare(pm_message_t state)
> mutex_lock(&dpm_list_mtx);
>
> if (!error) {
> - dev->power.is_prepared = true;
> + if (!dev->power.no_pm_callbacks)
> + dev->power.is_prepared = true;

This is not the way to address the issue IMV.

power.is_prepared set means that the device is in dpm_prepared_list
and I wouldn't depart from that even for devices without PM callbacks.

> if (!list_empty(&dev->power.entry))
> list_move_tail(&dev->power.entry, &dpm_prepared_list);
> } else if (error == -EAGAIN) {
> --

It would be better to add a power.no_pm_callbacks check for the parent
to device_pm_add(), but this would still suppress the warning is some
cases in which it should be printed (for example, the new device's
parent is a "virtual" device without PM callbacks, but its grandparent
is a regular device that has PM callbacks and is suspended).

Something like the attached patch (untested) might work, though.

[-- Attachment #2: pm-sleep-pm-add.patch --]
[-- Type: text/x-patch, Size: 1232 bytes --]

---
drivers/base/power/main.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -127,6 +127,8 @@ void device_pm_unlock(void)
*/
void device_pm_add(struct device *dev)
{
+ struct device *ancestor;
+
/* Skip PM setup/initialization. */
if (device_pm_not_required(dev))
return;
@@ -134,12 +136,20 @@ void device_pm_add(struct device *dev)
pr_debug("Adding info for %s:%s\n",
dev->bus ? dev->bus->name : "No Bus", dev_name(dev));
device_pm_check_callbacks(dev);
+
mutex_lock(&dpm_list_mtx);
- if (dev->parent && dev->parent->power.is_prepared)
- dev_warn(dev, "parent %s should not be sleeping\n",
- dev_name(dev->parent));
+
+ ancestor = dev->parent;
+ while (ancestor && ancestor->power.no_pm_callbacks)
+ ancestor = ancestor->parent;
+
+ if (ancestor && ancestor->power.is_prepared)
+ dev_warn(dev, "ancestor %s should not be sleeping\n",
+ dev_name(ancestor));
+
list_add_tail(&dev->power.entry, &dpm_list);
dev->power.in_dpm_list = true;
+
mutex_unlock(&dpm_list_mtx);
}