| Message ID | 20250213105840.2864654-1-xu.yang_2@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] PM: sleep: core: Clear is_prepared if no_pm_callbacks is true before checking power.syscore | expand |
On Thu, Feb 13, 2025 at 11:58 AM Xu Yang <xu.yang_2@nxp.com> wrote: > > Currently, if power.no_callbacks is true for a device, device_prepare() > will also set power.direct_complete to true. When device_resume() check > power.direct_complete, setting power.is_prepared will be skipped if it > can directly complete. This will cause a warning when add new devices > during resume() stage. > > Although power.is_prepared should be cleared in complete() state, commit > (f76b168b6f11 PM: Rename dev_pm_info.in_suspend to is_prepared) allow > clear it in earlier resume() stage. However, we need also set is_prepared > to false before checking syscore if the device has no pm callbacks. > > Take USB as example: > The usb_interface is such a device which setting power.no_callbacks to > true. Then if the user call usb_set_interface() during resume() stage, > the kernel will print below warning since the system will create and > add ep devices. > > [ 186.461414] usb 1-1: reset high-speed USB device number 3 using ci_hdrc > [ 187.102681] ep_81: PM: parent 1-1:1.1 should not be sleeping > [ 187.105010] PM: resume devices took 0.936 seconds > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > > --- > v2: clear is_prepared before check syscore as suggested by Rafael > --- > drivers/base/power/main.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > index 40e1d8d8a589..5e69cfaec661 100644 > --- a/drivers/base/power/main.c > +++ b/drivers/base/power/main.c > @@ -926,6 +926,9 @@ static void device_resume(struct device *dev, pm_message_t state, bool async) > TRACE_DEVICE(dev); > TRACE_RESUME(0); > > + if (dev->power.no_pm_callbacks) > + dev->power.is_prepared = false; > + > if (dev->power.syscore) > goto Complete; > > -- As I've already said in the other thread, my initial feedback was based on an oversimplified view of this code, and the problem will need to be addressed differently. Sorry about that.
On Thu, Feb 13, 2025 at 10:33 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Feb 13, 2025 at 11:58 AM Xu Yang <xu.yang_2@nxp.com> wrote: > > > > Currently, if power.no_callbacks is true for a device, device_prepare() > > will also set power.direct_complete to true. When device_resume() check > > power.direct_complete, setting power.is_prepared will be skipped if it > > can directly complete. This will cause a warning when add new devices > > during resume() stage. > > > > Although power.is_prepared should be cleared in complete() state, commit > > (f76b168b6f11 PM: Rename dev_pm_info.in_suspend to is_prepared) allow > > clear it in earlier resume() stage. However, we need also set is_prepared > > to false before checking syscore if the device has no pm callbacks. > > > > Take USB as example: > > The usb_interface is such a device which setting power.no_callbacks to > > true. Then if the user call usb_set_interface() during resume() stage, > > the kernel will print below warning since the system will create and > > add ep devices. > > > > [ 186.461414] usb 1-1: reset high-speed USB device number 3 using ci_hdrc > > [ 187.102681] ep_81: PM: parent 1-1:1.1 should not be sleeping > > [ 187.105010] PM: resume devices took 0.936 seconds > > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > > > > --- > > v2: clear is_prepared before check syscore as suggested by Rafael > > --- > > drivers/base/power/main.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > > index 40e1d8d8a589..5e69cfaec661 100644 > > --- a/drivers/base/power/main.c > > +++ b/drivers/base/power/main.c > > @@ -926,6 +926,9 @@ static void device_resume(struct device *dev, pm_message_t state, bool async) > > TRACE_DEVICE(dev); > > TRACE_RESUME(0); > > > > + if (dev->power.no_pm_callbacks) > > + dev->power.is_prepared = false; > > + > > if (dev->power.syscore) > > goto Complete; > > > > -- > > As I've already said in the other thread, my initial feedback was > based on an oversimplified view of this code, and the problem will > need to be addressed differently. > > Sorry about that. Unfortunately, I don't think that this can cover all of the corner cases. Something is going to escape, this way or another. To minimize the number of things that may escape, I would first treat direct_complete as a special case, so only clear power.is_prepared early if direct_complete is set. Then, clear it only for devices with power.no_pm_callbacks set, that is if (dev->power.direct_complete) { /* Add explanatory comment here */ if (dev->power.no_pm_callbacks) dev->power.is_prepared = false; /* Match the pm_runtime_disable() in device_suspend(). */ pm_runtime_enable(dev); goto Complete; } This is not perfect because ideally the device with no callbacks should wait for its parent and suppliers (if any) to resume before new children can be added under it safely (in case the children depend on the devices that it depends on) and is_prepared may be cleared before that happens, but those new children may be added by the parent's resume callback and they won't appear if the parent is not ready anyway.
Hi Rafael, On Fri, Feb 21, 2025 at 09:49:11PM +0100, Rafael J. Wysocki wrote: > On Thu, Feb 13, 2025 at 10:33 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Thu, Feb 13, 2025 at 11:58 AM Xu Yang <xu.yang_2@nxp.com> wrote: > > > > > > Currently, if power.no_callbacks is true for a device, device_prepare() > > > will also set power.direct_complete to true. When device_resume() check > > > power.direct_complete, setting power.is_prepared will be skipped if it > > > can directly complete. This will cause a warning when add new devices > > > during resume() stage. > > > > > > Although power.is_prepared should be cleared in complete() state, commit > > > (f76b168b6f11 PM: Rename dev_pm_info.in_suspend to is_prepared) allow > > > clear it in earlier resume() stage. However, we need also set is_prepared > > > to false before checking syscore if the device has no pm callbacks. > > > > > > Take USB as example: > > > The usb_interface is such a device which setting power.no_callbacks to > > > true. Then if the user call usb_set_interface() during resume() stage, > > > the kernel will print below warning since the system will create and > > > add ep devices. > > > > > > [ 186.461414] usb 1-1: reset high-speed USB device number 3 using ci_hdrc > > > [ 187.102681] ep_81: PM: parent 1-1:1.1 should not be sleeping > > > [ 187.105010] PM: resume devices took 0.936 seconds > > > > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > > > > > > --- > > > v2: clear is_prepared before check syscore as suggested by Rafael > > > --- > > > drivers/base/power/main.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > > > index 40e1d8d8a589..5e69cfaec661 100644 > > > --- a/drivers/base/power/main.c > > > +++ b/drivers/base/power/main.c > > > @@ -926,6 +926,9 @@ static void device_resume(struct device *dev, pm_message_t state, bool async) > > > TRACE_DEVICE(dev); > > > TRACE_RESUME(0); > > > > > > + if (dev->power.no_pm_callbacks) > > > + dev->power.is_prepared = false; > > > + > > > if (dev->power.syscore) > > > goto Complete; > > > > > > -- > > > > As I've already said in the other thread, my initial feedback was > > based on an oversimplified view of this code, and the problem will > > need to be addressed differently. > > > > Sorry about that. > > Unfortunately, I don't think that this can cover all of the corner > cases. Something is going to escape, this way or another. Agree. Although I don't know how the corner case looks, it seems like a risk in the future. > > To minimize the number of things that may escape, I would first treat > direct_complete as a special case, so only clear power.is_prepared > early if direct_complete is set. > > Then, clear it only for devices with power.no_pm_callbacks set, that is > > if (dev->power.direct_complete) { > /* Add explanatory comment here */ > if (dev->power.no_pm_callbacks) > dev->power.is_prepared = false; > > /* Match the pm_runtime_disable() in device_suspend(). */ > pm_runtime_enable(dev); > goto Complete; > } Thank you for the analysis and suggestions! Combined checking direct_complete and no_pm_callbacks seems like a better solution and this will precisely cover the issue I reported. I will adjust the patch and send v3 later. > > This is not perfect because ideally the device with no callbacks > should wait for its parent and suppliers (if any) to resume before new > children can be added under it safely (in case the children depend on > the devices that it depends on) and is_prepared may be cleared before > that happens, but those new children may be added by the parent's > resume callback and they won't appear if the parent is not ready > anyway. Understand. Thanks, Xu Yang
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 40e1d8d8a589..5e69cfaec661 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -926,6 +926,9 @@ static void device_resume(struct device *dev, pm_message_t state, bool async) TRACE_DEVICE(dev); TRACE_RESUME(0); + if (dev->power.no_pm_callbacks) + dev->power.is_prepared = false; + if (dev->power.syscore) goto Complete;
Currently, if power.no_callbacks is true for a device, device_prepare() will also set power.direct_complete to true. When device_resume() check power.direct_complete, setting power.is_prepared will be skipped if it can directly complete. This will cause a warning when add new devices during resume() stage. Although power.is_prepared should be cleared in complete() state, commit (f76b168b6f11 PM: Rename dev_pm_info.in_suspend to is_prepared) allow clear it in earlier resume() stage. However, we need also set is_prepared to false before checking syscore if the device has no pm callbacks. Take USB as example: The usb_interface is such a device which setting power.no_callbacks to true. Then if the user call usb_set_interface() during resume() stage, the kernel will print below warning since the system will create and add ep devices. [ 186.461414] usb 1-1: reset high-speed USB device number 3 using ci_hdrc [ 187.102681] ep_81: PM: parent 1-1:1.1 should not be sleeping [ 187.105010] PM: resume devices took 0.936 seconds Signed-off-by: Xu Yang <xu.yang_2@nxp.com> --- v2: clear is_prepared before check syscore as suggested by Rafael --- drivers/base/power/main.c | 3 +++ 1 file changed, 3 insertions(+)