| Message ID | 12619233.O9o76ZdvQC@rjwysocki.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3775fc538f535a7c5adaf11990c7932a0bd1f9eb |
| Headers | show |
| Series | [v1] PM: sleep: core: Synchronize runtime PM status of parents and children | expand |
On Tue, Jan 28, 2025 at 08:24:41PM +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Commit 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the > resume phase") overlooked the case in which the parent of a device with > DPM_FLAG_SMART_SUSPEND set did not use that flag and could be runtime- > suspended before a transition into a system-wide sleep state. In that > case, if the child is resumed during the subsequent transition from > that state into the working state, its runtime PM status will be set to > RPM_ACTIVE, but the runtime PM status of the parent will not be updated > accordingly, even though the parent will be resumed too, because of the > dev_pm_skip_suspend() check in device_resume_noirq(). > > Address this problem by tracking the need to set the runtime PM status > to RPM_ACTIVE during system-wide resume transitions for devices with > DPM_FLAG_SMART_SUSPEND set and all of the devices depended on by them. > > Fixes: 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the resume phase") > Closes: https://lore.kernel.org/linux-pm/Z30p2Etwf3F2AUvD@hovoldconsulting.com/ > Reported-by: Johan Hovold <johan@kernel.org> > Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Thanks for tracking this down Mani, and thanks Rafael for the quick fix. As expected this makes the warning go away also in my setup, and the patch itself looks correct to me: Reviewed-by: Johan Hovold <johan+linaro@kernel.org> Tested-by: Johan Hovold <johan+linaro@kernel.org> Johan
On Tue, 28 Jan 2025 at 20:24, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Commit 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the > resume phase") overlooked the case in which the parent of a device with > DPM_FLAG_SMART_SUSPEND set did not use that flag and could be runtime- > suspended before a transition into a system-wide sleep state. In that > case, if the child is resumed during the subsequent transition from > that state into the working state, its runtime PM status will be set to > RPM_ACTIVE, but the runtime PM status of the parent will not be updated > accordingly, even though the parent will be resumed too, because of the > dev_pm_skip_suspend() check in device_resume_noirq(). > > Address this problem by tracking the need to set the runtime PM status > to RPM_ACTIVE during system-wide resume transitions for devices with > DPM_FLAG_SMART_SUSPEND set and all of the devices depended on by them. > > Fixes: 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the resume phase") > Closes: https://lore.kernel.org/linux-pm/Z30p2Etwf3F2AUvD@hovoldconsulting.com/ > Reported-by: Johan Hovold <johan@kernel.org> > Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/base/power/main.c | 29 ++++++++++++++++++++--------- > include/linux/pm.h | 1 + > 2 files changed, 21 insertions(+), 9 deletions(-) > > --- a/drivers/base/power/main.c > +++ b/drivers/base/power/main.c > @@ -656,13 +656,15 @@ > * so change its status accordingly. > * > * Otherwise, the device is going to be resumed, so set its PM-runtime > - * status to "active", but do that only if DPM_FLAG_SMART_SUSPEND is set > - * to avoid confusing drivers that don't use it. > + * status to "active" unless its power.set_active flag is clear, in > + * which case it is not necessary to update its PM-runtime status. > */ > - if (skip_resume) > + if (skip_resume) { > pm_runtime_set_suspended(dev); > - else if (dev_pm_skip_suspend(dev)) > + } else if (dev->power.set_active) { > pm_runtime_set_active(dev); > + dev->power.set_active = false; > + } > > if (dev->pm_domain) { > info = "noirq power domain "; > @@ -1189,18 +1191,24 @@ > return PMSG_ON; > } > > -static void dpm_superior_set_must_resume(struct device *dev) > +static void dpm_superior_set_must_resume(struct device *dev, bool set_active) > { > struct device_link *link; > int idx; > > - if (dev->parent) > + if (dev->parent) { > dev->parent->power.must_resume = true; > + if (set_active) > + dev->parent->power.set_active = true; > + } > > idx = device_links_read_lock(); > > - list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) > + list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) { > link->supplier->power.must_resume = true; > + if (set_active) > + link->supplier->power.set_active = true; If I understand correctly, the suppliers are already handled when the pm_runtime_set_active() is called for consumers, so the above should not be needed. That said, maybe we instead allow parent/child to work in the similar way as for consumer/suppliers, when pm_runtime_set_active() is called for the child. In other words, when pm_runtime_set_active() is called for a child and the parent is runtime PM enabled, let's runtime resume it too, as we do for suppliers. Would that work, you think? > + } > > device_links_read_unlock(idx); > } > @@ -1278,8 +1286,11 @@ > dev->power.may_skip_resume)) > dev->power.must_resume = true; > > - if (dev->power.must_resume) > - dpm_superior_set_must_resume(dev); > + if (dev->power.must_resume) { > + dev->power.set_active = dev->power.set_active || > + dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND); > + dpm_superior_set_must_resume(dev, dev->power.set_active); > + } > > Complete: > complete_all(&dev->power.completion); > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -683,6 +683,7 @@ > bool no_pm_callbacks:1; /* Owned by the PM core */ > bool async_in_progress:1; /* Owned by the PM core */ > bool must_resume:1; /* Owned by the PM core */ > + bool set_active:1; /* Owned by the PM core */ > bool may_skip_resume:1; /* Set by subsystems */ > #else > bool should_wakeup:1; > > > Kind regards Uffe
On Wed, Jan 29, 2025 at 12:53 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Tue, 28 Jan 2025 at 20:24, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Commit 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the > > resume phase") overlooked the case in which the parent of a device with > > DPM_FLAG_SMART_SUSPEND set did not use that flag and could be runtime- > > suspended before a transition into a system-wide sleep state. In that > > case, if the child is resumed during the subsequent transition from > > that state into the working state, its runtime PM status will be set to > > RPM_ACTIVE, but the runtime PM status of the parent will not be updated > > accordingly, even though the parent will be resumed too, because of the > > dev_pm_skip_suspend() check in device_resume_noirq(). > > > > Address this problem by tracking the need to set the runtime PM status > > to RPM_ACTIVE during system-wide resume transitions for devices with > > DPM_FLAG_SMART_SUSPEND set and all of the devices depended on by them. > > > > Fixes: 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the resume phase") > > Closes: https://lore.kernel.org/linux-pm/Z30p2Etwf3F2AUvD@hovoldconsulting.com/ > > Reported-by: Johan Hovold <johan@kernel.org> > > Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/base/power/main.c | 29 ++++++++++++++++++++--------- > > include/linux/pm.h | 1 + > > 2 files changed, 21 insertions(+), 9 deletions(-) > > > > --- a/drivers/base/power/main.c > > +++ b/drivers/base/power/main.c > > @@ -656,13 +656,15 @@ > > * so change its status accordingly. > > * > > * Otherwise, the device is going to be resumed, so set its PM-runtime > > - * status to "active", but do that only if DPM_FLAG_SMART_SUSPEND is set > > - * to avoid confusing drivers that don't use it. > > + * status to "active" unless its power.set_active flag is clear, in > > + * which case it is not necessary to update its PM-runtime status. > > */ > > - if (skip_resume) > > + if (skip_resume) { > > pm_runtime_set_suspended(dev); > > - else if (dev_pm_skip_suspend(dev)) > > + } else if (dev->power.set_active) { > > pm_runtime_set_active(dev); > > + dev->power.set_active = false; > > + } > > > > if (dev->pm_domain) { > > info = "noirq power domain "; > > @@ -1189,18 +1191,24 @@ > > return PMSG_ON; > > } > > > > -static void dpm_superior_set_must_resume(struct device *dev) > > +static void dpm_superior_set_must_resume(struct device *dev, bool set_active) > > { > > struct device_link *link; > > int idx; > > > > - if (dev->parent) > > + if (dev->parent) { > > dev->parent->power.must_resume = true; > > + if (set_active) > > + dev->parent->power.set_active = true; > > + } > > > > idx = device_links_read_lock(); > > > > - list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) > > + list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) { > > link->supplier->power.must_resume = true; > > + if (set_active) > > + link->supplier->power.set_active = true; > > If I understand correctly, the suppliers are already handled when the > pm_runtime_set_active() is called for consumers, so the above should > not be needed. It is needed because pm_runtime_set_active() doesn't cause the setting to propagate to the parent's/suppliers of the suppliers AFAICS. > That said, maybe we instead allow parent/child to work in the similar > way as for consumer/suppliers, when pm_runtime_set_active() is called > for the child. In other words, when pm_runtime_set_active() is called > for a child and the parent is runtime PM enabled, let's runtime resume > it too, as we do for suppliers. Would that work, you think? The parent is not runtime-PM enabled when this happens.
On Wed, 29 Jan 2025 at 16:55, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Jan 29, 2025 at 12:53 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Tue, 28 Jan 2025 at 20:24, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Commit 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the > > > resume phase") overlooked the case in which the parent of a device with > > > DPM_FLAG_SMART_SUSPEND set did not use that flag and could be runtime- > > > suspended before a transition into a system-wide sleep state. In that > > > case, if the child is resumed during the subsequent transition from > > > that state into the working state, its runtime PM status will be set to > > > RPM_ACTIVE, but the runtime PM status of the parent will not be updated > > > accordingly, even though the parent will be resumed too, because of the > > > dev_pm_skip_suspend() check in device_resume_noirq(). > > > > > > Address this problem by tracking the need to set the runtime PM status > > > to RPM_ACTIVE during system-wide resume transitions for devices with > > > DPM_FLAG_SMART_SUSPEND set and all of the devices depended on by them. > > > > > > Fixes: 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the resume phase") > > > Closes: https://lore.kernel.org/linux-pm/Z30p2Etwf3F2AUvD@hovoldconsulting.com/ > > > Reported-by: Johan Hovold <johan@kernel.org> > > > Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > --- > > > drivers/base/power/main.c | 29 ++++++++++++++++++++--------- > > > include/linux/pm.h | 1 + > > > 2 files changed, 21 insertions(+), 9 deletions(-) > > > > > > --- a/drivers/base/power/main.c > > > +++ b/drivers/base/power/main.c > > > @@ -656,13 +656,15 @@ > > > * so change its status accordingly. > > > * > > > * Otherwise, the device is going to be resumed, so set its PM-runtime > > > - * status to "active", but do that only if DPM_FLAG_SMART_SUSPEND is set > > > - * to avoid confusing drivers that don't use it. > > > + * status to "active" unless its power.set_active flag is clear, in > > > + * which case it is not necessary to update its PM-runtime status. > > > */ > > > - if (skip_resume) > > > + if (skip_resume) { > > > pm_runtime_set_suspended(dev); > > > - else if (dev_pm_skip_suspend(dev)) > > > + } else if (dev->power.set_active) { > > > pm_runtime_set_active(dev); > > > + dev->power.set_active = false; > > > + } > > > > > > if (dev->pm_domain) { > > > info = "noirq power domain "; > > > @@ -1189,18 +1191,24 @@ > > > return PMSG_ON; > > > } > > > > > > -static void dpm_superior_set_must_resume(struct device *dev) > > > +static void dpm_superior_set_must_resume(struct device *dev, bool set_active) > > > { > > > struct device_link *link; > > > int idx; > > > > > > - if (dev->parent) > > > + if (dev->parent) { > > > dev->parent->power.must_resume = true; > > > + if (set_active) > > > + dev->parent->power.set_active = true; > > > + } > > > > > > idx = device_links_read_lock(); > > > > > > - list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) > > > + list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) { > > > link->supplier->power.must_resume = true; > > > + if (set_active) > > > + link->supplier->power.set_active = true; > > > > If I understand correctly, the suppliers are already handled when the > > pm_runtime_set_active() is called for consumers, so the above should > > not be needed. > > It is needed because pm_runtime_set_active() doesn't cause the setting > to propagate to the parent's/suppliers of the suppliers AFAICS. Hmm, even if that sounds reasonable, I don't think it's a good idea as it may introduce interesting propagation problems between drivers. For example, consider that Saravana is trying to enable runtime PM for fw_devlinks. It would mean synchronization issues for the runtime PM status, all over the place. That said, is even consumer/suppliers part of the problem we are trying to solve? > > > That said, maybe we instead allow parent/child to work in the similar > > way as for consumer/suppliers, when pm_runtime_set_active() is called > > for the child. In other words, when pm_runtime_set_active() is called > > for a child and the parent is runtime PM enabled, let's runtime resume > > it too, as we do for suppliers. Would that work, you think? > > The parent is not runtime-PM enabled when this happens. That sounds really weird to me. Does that mean that the parent has not been system resumed either? If so, isn't that really the root cause for this problem, or what am I missing? Kind regards Uffe
On Wed, Jan 29, 2025 at 5:42 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Wed, 29 Jan 2025 at 16:55, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Wed, Jan 29, 2025 at 12:53 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > On Tue, 28 Jan 2025 at 20:24, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > Commit 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the > > > > resume phase") overlooked the case in which the parent of a device with > > > > DPM_FLAG_SMART_SUSPEND set did not use that flag and could be runtime- > > > > suspended before a transition into a system-wide sleep state. In that > > > > case, if the child is resumed during the subsequent transition from > > > > that state into the working state, its runtime PM status will be set to > > > > RPM_ACTIVE, but the runtime PM status of the parent will not be updated > > > > accordingly, even though the parent will be resumed too, because of the > > > > dev_pm_skip_suspend() check in device_resume_noirq(). > > > > > > > > Address this problem by tracking the need to set the runtime PM status > > > > to RPM_ACTIVE during system-wide resume transitions for devices with > > > > DPM_FLAG_SMART_SUSPEND set and all of the devices depended on by them. > > > > > > > > Fixes: 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the resume phase") > > > > Closes: https://lore.kernel.org/linux-pm/Z30p2Etwf3F2AUvD@hovoldconsulting.com/ > > > > Reported-by: Johan Hovold <johan@kernel.org> > > > > Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > --- > > > > drivers/base/power/main.c | 29 ++++++++++++++++++++--------- > > > > include/linux/pm.h | 1 + > > > > 2 files changed, 21 insertions(+), 9 deletions(-) > > > > > > > > --- a/drivers/base/power/main.c > > > > +++ b/drivers/base/power/main.c > > > > @@ -656,13 +656,15 @@ > > > > * so change its status accordingly. > > > > * > > > > * Otherwise, the device is going to be resumed, so set its PM-runtime > > > > - * status to "active", but do that only if DPM_FLAG_SMART_SUSPEND is set > > > > - * to avoid confusing drivers that don't use it. > > > > + * status to "active" unless its power.set_active flag is clear, in > > > > + * which case it is not necessary to update its PM-runtime status. > > > > */ > > > > - if (skip_resume) > > > > + if (skip_resume) { > > > > pm_runtime_set_suspended(dev); > > > > - else if (dev_pm_skip_suspend(dev)) > > > > + } else if (dev->power.set_active) { > > > > pm_runtime_set_active(dev); > > > > + dev->power.set_active = false; > > > > + } > > > > > > > > if (dev->pm_domain) { > > > > info = "noirq power domain "; > > > > @@ -1189,18 +1191,24 @@ > > > > return PMSG_ON; > > > > } > > > > > > > > -static void dpm_superior_set_must_resume(struct device *dev) > > > > +static void dpm_superior_set_must_resume(struct device *dev, bool set_active) > > > > { > > > > struct device_link *link; > > > > int idx; > > > > > > > > - if (dev->parent) > > > > + if (dev->parent) { > > > > dev->parent->power.must_resume = true; > > > > + if (set_active) > > > > + dev->parent->power.set_active = true; > > > > + } > > > > > > > > idx = device_links_read_lock(); > > > > > > > > - list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) > > > > + list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) { > > > > link->supplier->power.must_resume = true; > > > > + if (set_active) > > > > + link->supplier->power.set_active = true; > > > > > > If I understand correctly, the suppliers are already handled when the > > > pm_runtime_set_active() is called for consumers, so the above should > > > not be needed. > > > > It is needed because pm_runtime_set_active() doesn't cause the setting > > to propagate to the parent's/suppliers of the suppliers AFAICS. > > Hmm, even if that sounds reasonable, I don't think it's a good idea as > it may introduce interesting propagation problems between drivers. > > For example, consider that Saravana is trying to enable runtime PM for > fw_devlinks. It would mean synchronization issues for the runtime PM > status, all over the place. What synchronization issues? > That said, is even consumer/suppliers part of the problem we are > trying to solve? They are in general. It's just that stuff that was runtime-suspended prior to a system-wide suspend may need to be resumed and marked as RPM_ACTIVE during system-wide resume because one of the devices wants/needs to be resumed then. If this turns out to be problematic, the problem will need to be addressed, but for now I'm not seeing why there would be a problem. > > > > > That said, maybe we instead allow parent/child to work in the similar > > > way as for consumer/suppliers, when pm_runtime_set_active() is called > > > for the child. In other words, when pm_runtime_set_active() is called > > > for a child and the parent is runtime PM enabled, let's runtime resume > > > it too, as we do for suppliers. Would that work, you think? > > > > The parent is not runtime-PM enabled when this happens. > > That sounds really weird to me. > > Does that mean that the parent has not been system resumed either? Yes. It hasn't been resumed yet, but it is known that it will be resumed. > If so, isn't that really the root cause for this problem, No, it is not. > or what am I missing? Essentially, what I said above. If a device that was suspended prior to a system-wide suspend wants/needs to be resumed during the subsequent system-wide resume, and it was runtime-PM-enabled before the suspend transition, it needs to (a) be runtime-PM-enabled during the subsequent system-wide resume transition and (b) it also needs to be marked as RPM_ACTIVE because in fact it is not suspended any more. The existing code before the patch takes care of this for the device itself, but not for the devices it depends on which also need to be resumed (which happens) and marked as RPM_ACTIVE (which doesn't happen) and the patch just makes sure that the latter will happen. Actually, what happens now is that the actual state of the parent during the system-wide resume, right before re-enabling runtime PM for it, does not match its runtime PM status which is still RPM_SUSPENDED. That's what is fixed here and it applies to the parent as well as to all of the other devices depended on by the child and the parent.
On Wed, 29 Jan 2025 at 17:58, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Jan 29, 2025 at 5:42 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Wed, 29 Jan 2025 at 16:55, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Wed, Jan 29, 2025 at 12:53 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > On Tue, 28 Jan 2025 at 20:24, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > Commit 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the > > > > > resume phase") overlooked the case in which the parent of a device with > > > > > DPM_FLAG_SMART_SUSPEND set did not use that flag and could be runtime- > > > > > suspended before a transition into a system-wide sleep state. In that > > > > > case, if the child is resumed during the subsequent transition from > > > > > that state into the working state, its runtime PM status will be set to > > > > > RPM_ACTIVE, but the runtime PM status of the parent will not be updated > > > > > accordingly, even though the parent will be resumed too, because of the > > > > > dev_pm_skip_suspend() check in device_resume_noirq(). > > > > > > > > > > Address this problem by tracking the need to set the runtime PM status > > > > > to RPM_ACTIVE during system-wide resume transitions for devices with > > > > > DPM_FLAG_SMART_SUSPEND set and all of the devices depended on by them. > > > > > > > > > > Fixes: 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the resume phase") > > > > > Closes: https://lore.kernel.org/linux-pm/Z30p2Etwf3F2AUvD@hovoldconsulting.com/ > > > > > Reported-by: Johan Hovold <johan@kernel.org> > > > > > Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > --- > > > > > drivers/base/power/main.c | 29 ++++++++++++++++++++--------- > > > > > include/linux/pm.h | 1 + > > > > > 2 files changed, 21 insertions(+), 9 deletions(-) > > > > > > > > > > --- a/drivers/base/power/main.c > > > > > +++ b/drivers/base/power/main.c > > > > > @@ -656,13 +656,15 @@ > > > > > * so change its status accordingly. > > > > > * > > > > > * Otherwise, the device is going to be resumed, so set its PM-runtime > > > > > - * status to "active", but do that only if DPM_FLAG_SMART_SUSPEND is set > > > > > - * to avoid confusing drivers that don't use it. > > > > > + * status to "active" unless its power.set_active flag is clear, in > > > > > + * which case it is not necessary to update its PM-runtime status. > > > > > */ > > > > > - if (skip_resume) > > > > > + if (skip_resume) { > > > > > pm_runtime_set_suspended(dev); > > > > > - else if (dev_pm_skip_suspend(dev)) > > > > > + } else if (dev->power.set_active) { > > > > > pm_runtime_set_active(dev); > > > > > + dev->power.set_active = false; > > > > > + } > > > > > > > > > > if (dev->pm_domain) { > > > > > info = "noirq power domain "; > > > > > @@ -1189,18 +1191,24 @@ > > > > > return PMSG_ON; > > > > > } > > > > > > > > > > -static void dpm_superior_set_must_resume(struct device *dev) > > > > > +static void dpm_superior_set_must_resume(struct device *dev, bool set_active) > > > > > { > > > > > struct device_link *link; > > > > > int idx; > > > > > > > > > > - if (dev->parent) > > > > > + if (dev->parent) { > > > > > dev->parent->power.must_resume = true; > > > > > + if (set_active) > > > > > + dev->parent->power.set_active = true; > > > > > + } > > > > > > > > > > idx = device_links_read_lock(); > > > > > > > > > > - list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) > > > > > + list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) { > > > > > link->supplier->power.must_resume = true; > > > > > + if (set_active) > > > > > + link->supplier->power.set_active = true; > > > > > > > > If I understand correctly, the suppliers are already handled when the > > > > pm_runtime_set_active() is called for consumers, so the above should > > > > not be needed. > > > > > > It is needed because pm_runtime_set_active() doesn't cause the setting > > > to propagate to the parent's/suppliers of the suppliers AFAICS. > > > > Hmm, even if that sounds reasonable, I don't think it's a good idea as > > it may introduce interesting propagation problems between drivers. > > > > For example, consider that Saravana is trying to enable runtime PM for > > fw_devlinks. It would mean synchronization issues for the runtime PM > > status, all over the place. > > What synchronization issues? Changing the runtime PM status for a parent/supplier that doesn't have DPM_FLAG_SMART_SUSPEND, is likely to confuse their drivers. You also removed that part of the comment a few lines above, in device_resume_noirq(). I am not sure I understand why? > > > That said, is even consumer/suppliers part of the problem we are > > trying to solve? > > They are in general. > > It's just that stuff that was runtime-suspended prior to a system-wide > suspend may need to be resumed and marked as RPM_ACTIVE during > system-wide resume because one of the devices wants/needs to be > resumed then. > > If this turns out to be problematic, the problem will need to be > addressed, but for now I'm not seeing why there would be a problem. > > > > > > > > That said, maybe we instead allow parent/child to work in the similar > > > > way as for consumer/suppliers, when pm_runtime_set_active() is called > > > > for the child. In other words, when pm_runtime_set_active() is called > > > > for a child and the parent is runtime PM enabled, let's runtime resume > > > > it too, as we do for suppliers. Would that work, you think? > > > > > > The parent is not runtime-PM enabled when this happens. > > > > That sounds really weird to me. > > > > Does that mean that the parent has not been system resumed either? > > Yes. > > It hasn't been resumed yet, but it is known that it will be resumed. > > > If so, isn't that really the root cause for this problem, > > No, it is not. > > > or what am I missing? > > Essentially, what I said above. > > If a device that was suspended prior to a system-wide suspend > wants/needs to be resumed during the subsequent system-wide resume, > and it was runtime-PM-enabled before the suspend transition, it needs > to (a) be runtime-PM-enabled during the subsequent system-wide resume > transition and (b) it also needs to be marked as RPM_ACTIVE because in > fact it is not suspended any more. The existing code before the patch > takes care of this for the device itself, but not for the devices it > depends on which also need to be resumed (which happens) and marked as > RPM_ACTIVE (which doesn't happen) and the patch just makes sure that > the latter will happen. Thanks for clarifying! > > Actually, what happens now is that the actual state of the parent > during the system-wide resume, right before re-enabling runtime PM for > it, does not match its runtime PM status which is still RPM_SUSPENDED. > That's what is fixed here and it applies to the parent as well as to > all of the other devices depended on by the child and the parent. Well, unfortunately I don't think it will work to call pm_runtime_set_active() for parents/suppliers like this. I think we need the drivers for the parents/suppliers to be in agreement with the behaviour of DPM_FLAG_SMART_SUSPEND to allow the propagation. Not sure how to best achieve this though. Kind regards Uffe
On Thu, Jan 30, 2025 at 12:11 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Wed, 29 Jan 2025 at 17:58, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Wed, Jan 29, 2025 at 5:42 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > On Wed, 29 Jan 2025 at 16:55, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > On Wed, Jan 29, 2025 at 12:53 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > On Tue, 28 Jan 2025 at 20:24, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > > > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > > > Commit 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the > > > > > > resume phase") overlooked the case in which the parent of a device with > > > > > > DPM_FLAG_SMART_SUSPEND set did not use that flag and could be runtime- > > > > > > suspended before a transition into a system-wide sleep state. In that > > > > > > case, if the child is resumed during the subsequent transition from > > > > > > that state into the working state, its runtime PM status will be set to > > > > > > RPM_ACTIVE, but the runtime PM status of the parent will not be updated > > > > > > accordingly, even though the parent will be resumed too, because of the > > > > > > dev_pm_skip_suspend() check in device_resume_noirq(). > > > > > > > > > > > > Address this problem by tracking the need to set the runtime PM status > > > > > > to RPM_ACTIVE during system-wide resume transitions for devices with > > > > > > DPM_FLAG_SMART_SUSPEND set and all of the devices depended on by them. > > > > > > > > > > > > Fixes: 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the resume phase") > > > > > > Closes: https://lore.kernel.org/linux-pm/Z30p2Etwf3F2AUvD@hovoldconsulting.com/ > > > > > > Reported-by: Johan Hovold <johan@kernel.org> > > > > > > Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > --- > > > > > > drivers/base/power/main.c | 29 ++++++++++++++++++++--------- > > > > > > include/linux/pm.h | 1 + > > > > > > 2 files changed, 21 insertions(+), 9 deletions(-) > > > > > > > > > > > > --- a/drivers/base/power/main.c > > > > > > +++ b/drivers/base/power/main.c > > > > > > @@ -656,13 +656,15 @@ > > > > > > * so change its status accordingly. > > > > > > * > > > > > > * Otherwise, the device is going to be resumed, so set its PM-runtime > > > > > > - * status to "active", but do that only if DPM_FLAG_SMART_SUSPEND is set > > > > > > - * to avoid confusing drivers that don't use it. > > > > > > + * status to "active" unless its power.set_active flag is clear, in > > > > > > + * which case it is not necessary to update its PM-runtime status. > > > > > > */ > > > > > > - if (skip_resume) > > > > > > + if (skip_resume) { > > > > > > pm_runtime_set_suspended(dev); > > > > > > - else if (dev_pm_skip_suspend(dev)) > > > > > > + } else if (dev->power.set_active) { > > > > > > pm_runtime_set_active(dev); > > > > > > + dev->power.set_active = false; > > > > > > + } > > > > > > > > > > > > if (dev->pm_domain) { > > > > > > info = "noirq power domain "; > > > > > > @@ -1189,18 +1191,24 @@ > > > > > > return PMSG_ON; > > > > > > } > > > > > > > > > > > > -static void dpm_superior_set_must_resume(struct device *dev) > > > > > > +static void dpm_superior_set_must_resume(struct device *dev, bool set_active) > > > > > > { > > > > > > struct device_link *link; > > > > > > int idx; > > > > > > > > > > > > - if (dev->parent) > > > > > > + if (dev->parent) { > > > > > > dev->parent->power.must_resume = true; > > > > > > + if (set_active) > > > > > > + dev->parent->power.set_active = true; > > > > > > + } > > > > > > > > > > > > idx = device_links_read_lock(); > > > > > > > > > > > > - list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) > > > > > > + list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) { > > > > > > link->supplier->power.must_resume = true; > > > > > > + if (set_active) > > > > > > + link->supplier->power.set_active = true; > > > > > > > > > > If I understand correctly, the suppliers are already handled when the > > > > > pm_runtime_set_active() is called for consumers, so the above should > > > > > not be needed. > > > > > > > > It is needed because pm_runtime_set_active() doesn't cause the setting > > > > to propagate to the parent's/suppliers of the suppliers AFAICS. > > > > > > Hmm, even if that sounds reasonable, I don't think it's a good idea as > > > it may introduce interesting propagation problems between drivers. > > > > > > For example, consider that Saravana is trying to enable runtime PM for > > > fw_devlinks. It would mean synchronization issues for the runtime PM > > > status, all over the place. > > > > What synchronization issues? > > Changing the runtime PM status for a parent/supplier that doesn't have > DPM_FLAG_SMART_SUSPEND, is likely to confuse their drivers. I'm not sure why though. > You also removed that part of the comment a few lines above, in > device_resume_noirq(). I am not sure I understand why? Not removed, but replaced. The set_active flag is only set for devices with DPM_FLAG_SMART_SUSPEND set and devices depended on by them. Also, it is only set for devices whose must_resume is set, which for devices with DPM_FLAG_SMART_SUSPEND means that they literally must be resumed. Consequently, the devices depended on by them also must be resumed. > > > > > That said, is even consumer/suppliers part of the problem we are > > > trying to solve? > > > > They are in general. > > > > It's just that stuff that was runtime-suspended prior to a system-wide > > suspend may need to be resumed and marked as RPM_ACTIVE during > > system-wide resume because one of the devices wants/needs to be > > resumed then. > > > > If this turns out to be problematic, the problem will need to be > > addressed, but for now I'm not seeing why there would be a problem. > > > > > > > > > > > That said, maybe we instead allow parent/child to work in the similar > > > > > way as for consumer/suppliers, when pm_runtime_set_active() is called > > > > > for the child. In other words, when pm_runtime_set_active() is called > > > > > for a child and the parent is runtime PM enabled, let's runtime resume > > > > > it too, as we do for suppliers. Would that work, you think? > > > > > > > > The parent is not runtime-PM enabled when this happens. > > > > > > That sounds really weird to me. > > > > > > Does that mean that the parent has not been system resumed either? > > > > Yes. > > > > It hasn't been resumed yet, but it is known that it will be resumed. > > > > > If so, isn't that really the root cause for this problem, > > > > No, it is not. > > > > > or what am I missing? > > > > Essentially, what I said above. > > > > If a device that was suspended prior to a system-wide suspend > > wants/needs to be resumed during the subsequent system-wide resume, > > and it was runtime-PM-enabled before the suspend transition, it needs > > to (a) be runtime-PM-enabled during the subsequent system-wide resume > > transition and (b) it also needs to be marked as RPM_ACTIVE because in > > fact it is not suspended any more. The existing code before the patch > > takes care of this for the device itself, but not for the devices it > > depends on which also need to be resumed (which happens) and marked as > > RPM_ACTIVE (which doesn't happen) and the patch just makes sure that > > the latter will happen. > > Thanks for clarifying! > > > > > Actually, what happens now is that the actual state of the parent > > during the system-wide resume, right before re-enabling runtime PM for > > it, does not match its runtime PM status which is still RPM_SUSPENDED. > > That's what is fixed here and it applies to the parent as well as to > > all of the other devices depended on by the child and the parent. > > Well, unfortunately I don't think it will work to call > pm_runtime_set_active() for parents/suppliers like this. As stated above, if a device with DPM_FLAG_SMART_SUSPEND has power.must_resume set, it must be resumed. Therefore, all of the devices depended on by it must be resumed (literally, they need to be powered up and configured to work). This is already a rule without the patch. Accordingly, they all effectively will be "active" and so their runtime PM status must reflect this. I don't see anything that cannot work, but I see why it is broken without this patch. > I think we need the drivers for the parents/suppliers to be in > agreement with the behaviour of DPM_FLAG_SMART_SUSPEND to allow the > propagation. Not sure how to best achieve this though. It would be good to actually identify the cases in which it may not work and they can be fixed on top of this patch.
On Thu, 30 Jan 2025 at 14:19, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Jan 30, 2025 at 12:11 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Wed, 29 Jan 2025 at 17:58, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Wed, Jan 29, 2025 at 5:42 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > On Wed, 29 Jan 2025 at 16:55, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > On Wed, Jan 29, 2025 at 12:53 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > > > On Tue, 28 Jan 2025 at 20:24, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > > > > > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > > > > > Commit 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the > > > > > > > resume phase") overlooked the case in which the parent of a device with > > > > > > > DPM_FLAG_SMART_SUSPEND set did not use that flag and could be runtime- > > > > > > > suspended before a transition into a system-wide sleep state. In that > > > > > > > case, if the child is resumed during the subsequent transition from > > > > > > > that state into the working state, its runtime PM status will be set to > > > > > > > RPM_ACTIVE, but the runtime PM status of the parent will not be updated > > > > > > > accordingly, even though the parent will be resumed too, because of the > > > > > > > dev_pm_skip_suspend() check in device_resume_noirq(). > > > > > > > > > > > > > > Address this problem by tracking the need to set the runtime PM status > > > > > > > to RPM_ACTIVE during system-wide resume transitions for devices with > > > > > > > DPM_FLAG_SMART_SUSPEND set and all of the devices depended on by them. > > > > > > > > > > > > > > Fixes: 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the resume phase") > > > > > > > Closes: https://lore.kernel.org/linux-pm/Z30p2Etwf3F2AUvD@hovoldconsulting.com/ > > > > > > > Reported-by: Johan Hovold <johan@kernel.org> > > > > > > > Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > --- > > > > > > > drivers/base/power/main.c | 29 ++++++++++++++++++++--------- > > > > > > > include/linux/pm.h | 1 + > > > > > > > 2 files changed, 21 insertions(+), 9 deletions(-) > > > > > > > > > > > > > > --- a/drivers/base/power/main.c > > > > > > > +++ b/drivers/base/power/main.c > > > > > > > @@ -656,13 +656,15 @@ > > > > > > > * so change its status accordingly. > > > > > > > * > > > > > > > * Otherwise, the device is going to be resumed, so set its PM-runtime > > > > > > > - * status to "active", but do that only if DPM_FLAG_SMART_SUSPEND is set > > > > > > > - * to avoid confusing drivers that don't use it. > > > > > > > + * status to "active" unless its power.set_active flag is clear, in > > > > > > > + * which case it is not necessary to update its PM-runtime status. > > > > > > > */ > > > > > > > - if (skip_resume) > > > > > > > + if (skip_resume) { > > > > > > > pm_runtime_set_suspended(dev); > > > > > > > - else if (dev_pm_skip_suspend(dev)) > > > > > > > + } else if (dev->power.set_active) { > > > > > > > pm_runtime_set_active(dev); > > > > > > > + dev->power.set_active = false; > > > > > > > + } > > > > > > > > > > > > > > if (dev->pm_domain) { > > > > > > > info = "noirq power domain "; > > > > > > > @@ -1189,18 +1191,24 @@ > > > > > > > return PMSG_ON; > > > > > > > } > > > > > > > > > > > > > > -static void dpm_superior_set_must_resume(struct device *dev) > > > > > > > +static void dpm_superior_set_must_resume(struct device *dev, bool set_active) > > > > > > > { > > > > > > > struct device_link *link; > > > > > > > int idx; > > > > > > > > > > > > > > - if (dev->parent) > > > > > > > + if (dev->parent) { > > > > > > > dev->parent->power.must_resume = true; > > > > > > > + if (set_active) > > > > > > > + dev->parent->power.set_active = true; > > > > > > > + } > > > > > > > > > > > > > > idx = device_links_read_lock(); > > > > > > > > > > > > > > - list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) > > > > > > > + list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) { > > > > > > > link->supplier->power.must_resume = true; > > > > > > > + if (set_active) > > > > > > > + link->supplier->power.set_active = true; > > > > > > > > > > > > If I understand correctly, the suppliers are already handled when the > > > > > > pm_runtime_set_active() is called for consumers, so the above should > > > > > > not be needed. > > > > > > > > > > It is needed because pm_runtime_set_active() doesn't cause the setting > > > > > to propagate to the parent's/suppliers of the suppliers AFAICS. > > > > > > > > Hmm, even if that sounds reasonable, I don't think it's a good idea as > > > > it may introduce interesting propagation problems between drivers. > > > > > > > > For example, consider that Saravana is trying to enable runtime PM for > > > > fw_devlinks. It would mean synchronization issues for the runtime PM > > > > status, all over the place. > > > > > > What synchronization issues? > > > > Changing the runtime PM status for a parent/supplier that doesn't have > > DPM_FLAG_SMART_SUSPEND, is likely to confuse their drivers. > > I'm not sure why though. > > > You also removed that part of the comment a few lines above, in > > device_resume_noirq(). I am not sure I understand why? > > Not removed, but replaced. > > The set_active flag is only set for devices with > DPM_FLAG_SMART_SUSPEND set and devices depended on by them. Also, it > is only set for devices whose must_resume is set, which for devices > with DPM_FLAG_SMART_SUSPEND means that they literally must be resumed. > Consequently, the devices depended on by them also must be resumed. > > > > > > > > That said, is even consumer/suppliers part of the problem we are > > > > trying to solve? > > > > > > They are in general. > > > > > > It's just that stuff that was runtime-suspended prior to a system-wide > > > suspend may need to be resumed and marked as RPM_ACTIVE during > > > system-wide resume because one of the devices wants/needs to be > > > resumed then. > > > > > > If this turns out to be problematic, the problem will need to be > > > addressed, but for now I'm not seeing why there would be a problem. > > > > > > > > > > > > > > That said, maybe we instead allow parent/child to work in the similar > > > > > > way as for consumer/suppliers, when pm_runtime_set_active() is called > > > > > > for the child. In other words, when pm_runtime_set_active() is called > > > > > > for a child and the parent is runtime PM enabled, let's runtime resume > > > > > > it too, as we do for suppliers. Would that work, you think? > > > > > > > > > > The parent is not runtime-PM enabled when this happens. > > > > > > > > That sounds really weird to me. > > > > > > > > Does that mean that the parent has not been system resumed either? > > > > > > Yes. > > > > > > It hasn't been resumed yet, but it is known that it will be resumed. > > > > > > > If so, isn't that really the root cause for this problem, > > > > > > No, it is not. > > > > > > > or what am I missing? > > > > > > Essentially, what I said above. > > > > > > If a device that was suspended prior to a system-wide suspend > > > wants/needs to be resumed during the subsequent system-wide resume, > > > and it was runtime-PM-enabled before the suspend transition, it needs > > > to (a) be runtime-PM-enabled during the subsequent system-wide resume > > > transition and (b) it also needs to be marked as RPM_ACTIVE because in > > > fact it is not suspended any more. The existing code before the patch > > > takes care of this for the device itself, but not for the devices it > > > depends on which also need to be resumed (which happens) and marked as > > > RPM_ACTIVE (which doesn't happen) and the patch just makes sure that > > > the latter will happen. > > > > Thanks for clarifying! > > > > > > > > Actually, what happens now is that the actual state of the parent > > > during the system-wide resume, right before re-enabling runtime PM for > > > it, does not match its runtime PM status which is still RPM_SUSPENDED. > > > That's what is fixed here and it applies to the parent as well as to > > > all of the other devices depended on by the child and the parent. > > > > Well, unfortunately I don't think it will work to call > > pm_runtime_set_active() for parents/suppliers like this. > > As stated above, if a device with DPM_FLAG_SMART_SUSPEND has > power.must_resume set, it must be resumed. Therefore, all of the > devices depended on by it must be resumed (literally, they need to be > powered up and configured to work). This is already a rule without > the patch. > > Accordingly, they all effectively will be "active" and so their > runtime PM status must reflect this. From a theoretical point of view, yes I agree. > > I don't see anything that cannot work, but I see why it is broken > without this patch. You are right that the behaviour is broken and needs to be fixed, but I am not convinced that $subject patch is the way forward. See more below. > > > I think we need the drivers for the parents/suppliers to be in > > agreement with the behaviour of DPM_FLAG_SMART_SUSPEND to allow the > > propagation. Not sure how to best achieve this though. > > It would be good to actually identify the cases in which it may not > work and they can be fixed on top of this patch. The problem with $subject patch is that drivers/buses are required to check the runtime PM status, with pm_runtime_suspended(), pm_runtime_status_suspended() or pm_runtime_active() in one of its system suspend/resume callbacks , to synchronize it with the HW state for its device (turn on/off clocks for example). Certainly, we can not rely on drivers to conform to this behaviour and there are plenty of cases where they really don't. For example, we have drivers that only implements runtime PM support or simply don't care about the runtime PM status during system resume, but just leaves the device in the state it is already in. Moreover, we have the users of pm_runtime_force_suspend|resume(), which we also know *not* to work with DPM_FLAG_SMART_SUSPEND and thus $subject patch too. I am less worried about these cases though, as I believe we should be able to fix them, by taking into account the suggested "->power.set_active flag", or something along those lines. That said, it seems like we need another way forward. Kind regards Uffe
On Fri, Jan 31, 2025 at 11:02 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 30 Jan 2025 at 14:19, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Thu, Jan 30, 2025 at 12:11 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > On Wed, 29 Jan 2025 at 17:58, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > On Wed, Jan 29, 2025 at 5:42 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > On Wed, 29 Jan 2025 at 16:55, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > > > On Wed, Jan 29, 2025 at 12:53 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > > > > > On Tue, 28 Jan 2025 at 20:24, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > > > > > > > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > > > > > > > Commit 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the > > > > > > > > resume phase") overlooked the case in which the parent of a device with > > > > > > > > DPM_FLAG_SMART_SUSPEND set did not use that flag and could be runtime- > > > > > > > > suspended before a transition into a system-wide sleep state. In that > > > > > > > > case, if the child is resumed during the subsequent transition from > > > > > > > > that state into the working state, its runtime PM status will be set to > > > > > > > > RPM_ACTIVE, but the runtime PM status of the parent will not be updated > > > > > > > > accordingly, even though the parent will be resumed too, because of the > > > > > > > > dev_pm_skip_suspend() check in device_resume_noirq(). > > > > > > > > > > > > > > > > Address this problem by tracking the need to set the runtime PM status > > > > > > > > to RPM_ACTIVE during system-wide resume transitions for devices with > > > > > > > > DPM_FLAG_SMART_SUSPEND set and all of the devices depended on by them. > > > > > > > > > > > > > > > > Fixes: 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the resume phase") > > > > > > > > Closes: https://lore.kernel.org/linux-pm/Z30p2Etwf3F2AUvD@hovoldconsulting.com/ > > > > > > > > Reported-by: Johan Hovold <johan@kernel.org> > > > > > > > > Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > --- > > > > > > > > drivers/base/power/main.c | 29 ++++++++++++++++++++--------- > > > > > > > > include/linux/pm.h | 1 + > > > > > > > > 2 files changed, 21 insertions(+), 9 deletions(-) > > > > > > > > > > > > > > > > --- a/drivers/base/power/main.c > > > > > > > > +++ b/drivers/base/power/main.c > > > > > > > > @@ -656,13 +656,15 @@ > > > > > > > > * so change its status accordingly. > > > > > > > > * > > > > > > > > * Otherwise, the device is going to be resumed, so set its PM-runtime > > > > > > > > - * status to "active", but do that only if DPM_FLAG_SMART_SUSPEND is set > > > > > > > > - * to avoid confusing drivers that don't use it. > > > > > > > > + * status to "active" unless its power.set_active flag is clear, in > > > > > > > > + * which case it is not necessary to update its PM-runtime status. > > > > > > > > */ > > > > > > > > - if (skip_resume) > > > > > > > > + if (skip_resume) { > > > > > > > > pm_runtime_set_suspended(dev); > > > > > > > > - else if (dev_pm_skip_suspend(dev)) > > > > > > > > + } else if (dev->power.set_active) { > > > > > > > > pm_runtime_set_active(dev); > > > > > > > > + dev->power.set_active = false; > > > > > > > > + } > > > > > > > > > > > > > > > > if (dev->pm_domain) { > > > > > > > > info = "noirq power domain "; > > > > > > > > @@ -1189,18 +1191,24 @@ > > > > > > > > return PMSG_ON; > > > > > > > > } > > > > > > > > > > > > > > > > -static void dpm_superior_set_must_resume(struct device *dev) > > > > > > > > +static void dpm_superior_set_must_resume(struct device *dev, bool set_active) > > > > > > > > { > > > > > > > > struct device_link *link; > > > > > > > > int idx; > > > > > > > > > > > > > > > > - if (dev->parent) > > > > > > > > + if (dev->parent) { > > > > > > > > dev->parent->power.must_resume = true; > > > > > > > > + if (set_active) > > > > > > > > + dev->parent->power.set_active = true; > > > > > > > > + } > > > > > > > > > > > > > > > > idx = device_links_read_lock(); > > > > > > > > > > > > > > > > - list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) > > > > > > > > + list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) { > > > > > > > > link->supplier->power.must_resume = true; > > > > > > > > + if (set_active) > > > > > > > > + link->supplier->power.set_active = true; > > > > > > > > > > > > > > If I understand correctly, the suppliers are already handled when the > > > > > > > pm_runtime_set_active() is called for consumers, so the above should > > > > > > > not be needed. > > > > > > > > > > > > It is needed because pm_runtime_set_active() doesn't cause the setting > > > > > > to propagate to the parent's/suppliers of the suppliers AFAICS. > > > > > > > > > > Hmm, even if that sounds reasonable, I don't think it's a good idea as > > > > > it may introduce interesting propagation problems between drivers. > > > > > > > > > > For example, consider that Saravana is trying to enable runtime PM for > > > > > fw_devlinks. It would mean synchronization issues for the runtime PM > > > > > status, all over the place. > > > > > > > > What synchronization issues? > > > > > > Changing the runtime PM status for a parent/supplier that doesn't have > > > DPM_FLAG_SMART_SUSPEND, is likely to confuse their drivers. > > > > I'm not sure why though. > > > > > You also removed that part of the comment a few lines above, in > > > device_resume_noirq(). I am not sure I understand why? > > > > Not removed, but replaced. > > > > The set_active flag is only set for devices with > > DPM_FLAG_SMART_SUSPEND set and devices depended on by them. Also, it > > is only set for devices whose must_resume is set, which for devices > > with DPM_FLAG_SMART_SUSPEND means that they literally must be resumed. > > Consequently, the devices depended on by them also must be resumed. > > > > > > > > > > > That said, is even consumer/suppliers part of the problem we are > > > > > trying to solve? > > > > > > > > They are in general. > > > > > > > > It's just that stuff that was runtime-suspended prior to a system-wide > > > > suspend may need to be resumed and marked as RPM_ACTIVE during > > > > system-wide resume because one of the devices wants/needs to be > > > > resumed then. > > > > > > > > If this turns out to be problematic, the problem will need to be > > > > addressed, but for now I'm not seeing why there would be a problem. > > > > > > > > > > > > > > > > > That said, maybe we instead allow parent/child to work in the similar > > > > > > > way as for consumer/suppliers, when pm_runtime_set_active() is called > > > > > > > for the child. In other words, when pm_runtime_set_active() is called > > > > > > > for a child and the parent is runtime PM enabled, let's runtime resume > > > > > > > it too, as we do for suppliers. Would that work, you think? > > > > > > > > > > > > The parent is not runtime-PM enabled when this happens. > > > > > > > > > > That sounds really weird to me. > > > > > > > > > > Does that mean that the parent has not been system resumed either? > > > > > > > > Yes. > > > > > > > > It hasn't been resumed yet, but it is known that it will be resumed. > > > > > > > > > If so, isn't that really the root cause for this problem, > > > > > > > > No, it is not. > > > > > > > > > or what am I missing? > > > > > > > > Essentially, what I said above. > > > > > > > > If a device that was suspended prior to a system-wide suspend > > > > wants/needs to be resumed during the subsequent system-wide resume, > > > > and it was runtime-PM-enabled before the suspend transition, it needs > > > > to (a) be runtime-PM-enabled during the subsequent system-wide resume > > > > transition and (b) it also needs to be marked as RPM_ACTIVE because in > > > > fact it is not suspended any more. The existing code before the patch > > > > takes care of this for the device itself, but not for the devices it > > > > depends on which also need to be resumed (which happens) and marked as > > > > RPM_ACTIVE (which doesn't happen) and the patch just makes sure that > > > > the latter will happen. > > > > > > Thanks for clarifying! > > > > > > > > > > > Actually, what happens now is that the actual state of the parent > > > > during the system-wide resume, right before re-enabling runtime PM for > > > > it, does not match its runtime PM status which is still RPM_SUSPENDED. > > > > That's what is fixed here and it applies to the parent as well as to > > > > all of the other devices depended on by the child and the parent. > > > > > > Well, unfortunately I don't think it will work to call > > > pm_runtime_set_active() for parents/suppliers like this. > > > > As stated above, if a device with DPM_FLAG_SMART_SUSPEND has > > power.must_resume set, it must be resumed. Therefore, all of the > > devices depended on by it must be resumed (literally, they need to be > > powered up and configured to work). This is already a rule without > > the patch. > > > > Accordingly, they all effectively will be "active" and so their > > runtime PM status must reflect this. > > From a theoretical point of view, yes I agree. > > > > > I don't see anything that cannot work, but I see why it is broken > > without this patch. > > You are right that the behaviour is broken and needs to be fixed, but > I am not convinced that $subject patch is the way forward. See more > below. > > > > > > I think we need the drivers for the parents/suppliers to be in > > > agreement with the behaviour of DPM_FLAG_SMART_SUSPEND to allow the > > > propagation. Not sure how to best achieve this though. > > > > It would be good to actually identify the cases in which it may not > > work and they can be fixed on top of this patch. > > The problem with $subject patch is that drivers/buses are required to > check the runtime PM status, with pm_runtime_suspended(), > pm_runtime_status_suspended() or pm_runtime_active() in one of its > system suspend/resume callbacks , to synchronize it with the HW state > for its device (turn on/off clocks for example). Well, I'm kind of unaware of this requirement. It clearly is not even followed by the code without the $subject patch. The real requirement is that the runtime PM status at the point when runtime PM is re-enabled, that is in device_resume_early(), must reflect the current actual HW state. > Certainly, we can not rely on drivers to conform to this behaviour and > there are plenty of cases where they really don't. For example, we > have drivers that only implements runtime PM support or simply don't > care about the runtime PM status during system resume, but just leaves > the device in the state it is already in. Drivers that only support runtime PM are broken with respect to system sleep ATM. They need to be made to support system sleep or they cannot be used on systems that use system sleep. There may be a way around this for system suspend/resume (see below), but not for hibernation. > Moreover, we have the users of pm_runtime_force_suspend|resume(), > which we also know *not* to work with DPM_FLAG_SMART_SUSPEND and thus > $subject patch too. I am less worried about these cases though, as I > believe we should be able to fix them, by taking into account the > suggested "->power.set_active flag", or something along those lines. Yes, and that's what I'm going to do. > That said, it seems like we need another way forward. I still don't see why, sorry. I guess the concern is that if a device suddenly needs to be resumed during system resume even though it was runtime-suspended before the preceding system suspend, there is no way to tell its driver/bus type/etc that this is the case if they all decide to leave the device as is, but as I have said for multiple times in this thread, leaving a device as is during system resume may not be an option unless it is a leaf device without any subordinates. This has always been the case. We'll see if there is any damage resulting from the $subject change and we'll fix it if so. In the future, though, I'd like to integrate system resume with runtime PM more than it is now. Namely, it should be possible to move the re-enabling of runtime PM to the front of device_resume_early() and then use pm_runtime_resume() for resuming devices whose drivers support runtime PM (I don't see any fundamental obstacles to this). One benefit of doing this would be that pm_runtime_resume() would automatically trigger a runtime resume for all of the "superior" devices, so they could be left in whatever state they had been in before the preceding system suspend regardless of what happens to their "subordinates". It is likely that some kind of driver opt-in will be needed for this or maybe the core can figure it out by itself. It can look at what callbacks are implemented etc. For example, if a driver only implements :runtime_suspend() and :runtime_resume() and no other PM callbacks, it is reasonable to assume that the devices handled by it should be suspended and resumed with the help of the runtime PM infrastructure even during system-wide suspend/resume (that doesn't apply to hibernation, though).
[...] > > > > The problem with $subject patch is that drivers/buses are required to > > check the runtime PM status, with pm_runtime_suspended(), > > pm_runtime_status_suspended() or pm_runtime_active() in one of its > > system suspend/resume callbacks , to synchronize it with the HW state > > for its device (turn on/off clocks for example). > > Well, I'm kind of unaware of this requirement. > > It clearly is not even followed by the code without the $subject patch. > > The real requirement is that the runtime PM status at the point when > runtime PM is re-enabled, that is in device_resume_early(), must > reflect the current actual HW state. Right. Seems like we are in agreement, just that there seems to be multiple ways to describe the similar problem. > > > Certainly, we can not rely on drivers to conform to this behaviour and > > there are plenty of cases where they really don't. For example, we > > have drivers that only implements runtime PM support or simply don't > > care about the runtime PM status during system resume, but just leaves > > the device in the state it is already in. > > Drivers that only support runtime PM are broken with respect to system > sleep ATM. They need to be made to support system sleep or they > cannot be used on systems that use system sleep. There may be a way > around this for system suspend/resume (see below), but not for > hibernation. I think calling them broken may be to take this a step too far. While I certainly agree that these drivers have room for some improvements, it looks to me that these drivers work today, but may not with $subject patch. > > > Moreover, we have the users of pm_runtime_force_suspend|resume(), > > which we also know *not* to work with DPM_FLAG_SMART_SUSPEND and thus > > $subject patch too. I am less worried about these cases though, as I > > believe we should be able to fix them, by taking into account the > > suggested "->power.set_active flag", or something along those lines. > > Yes, and that's what I'm going to do. > > > That said, it seems like we need another way forward. > > I still don't see why, sorry. > > I guess the concern is that if a device suddenly needs to be resumed > during system resume even though it was runtime-suspended before the > preceding system suspend, there is no way to tell its driver/bus > type/etc that this is the case if they all decide to leave the device > as is, but as I have said for multiple times in this thread, leaving a > device as is during system resume may not be an option unless it is a > leaf device without any subordinates. This has always been the case. > > We'll see if there is any damage resulting from the $subject change > and we'll fix it if so. > > In the future, though, I'd like to integrate system resume with > runtime PM more than it is now. Namely, it should be possible to move > the re-enabling of runtime PM to the front of device_resume_early() > and then use pm_runtime_resume() for resuming devices whose drivers > support runtime PM (I don't see any fundamental obstacles to this). > One benefit of doing this would be that pm_runtime_resume() would > automatically trigger a runtime resume for all of the "superior" > devices, so they could be left in whatever state they had been in > before the preceding system suspend regardless of what happens to > their "subordinates". It is likely that some kind of driver opt-in > will be needed for this or maybe the core can figure it out by itself. Right, I am certainly interested to discuss this topic too. It's not an easy thing and the biggest problem is that we can't really just change the behaviour without a big risk of breaking things. Some kind of opt-in behaviour is the only thing that can work, in my opinion. Anyway, I am here to review and discuss. :-) > > It can look at what callbacks are implemented etc. For example, if a > driver only implements :runtime_suspend() and :runtime_resume() and no > other PM callbacks, it is reasonable to assume that the devices > handled by it should be suspended and resumed with the help of the > runtime PM infrastructure even during system-wide suspend/resume (that > doesn't apply to hibernation, though). Maybe this can work in some opt-in/out way, but certainly not as a solution for all. For example, we have subsystems that deal with system suspend/resume quite differently, where drivers instead implement some subsystem specific callbacks, rather than the common system suspend/resume ops. Kind regards Uffe
--- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -656,13 +656,15 @@ * so change its status accordingly. * * Otherwise, the device is going to be resumed, so set its PM-runtime - * status to "active", but do that only if DPM_FLAG_SMART_SUSPEND is set - * to avoid confusing drivers that don't use it. + * status to "active" unless its power.set_active flag is clear, in + * which case it is not necessary to update its PM-runtime status. */ - if (skip_resume) + if (skip_resume) { pm_runtime_set_suspended(dev); - else if (dev_pm_skip_suspend(dev)) + } else if (dev->power.set_active) { pm_runtime_set_active(dev); + dev->power.set_active = false; + } if (dev->pm_domain) { info = "noirq power domain "; @@ -1189,18 +1191,24 @@ return PMSG_ON; } -static void dpm_superior_set_must_resume(struct device *dev) +static void dpm_superior_set_must_resume(struct device *dev, bool set_active) { struct device_link *link; int idx; - if (dev->parent) + if (dev->parent) { dev->parent->power.must_resume = true; + if (set_active) + dev->parent->power.set_active = true; + } idx = device_links_read_lock(); - list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) + list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) { link->supplier->power.must_resume = true; + if (set_active) + link->supplier->power.set_active = true; + } device_links_read_unlock(idx); } @@ -1278,8 +1286,11 @@ dev->power.may_skip_resume)) dev->power.must_resume = true; - if (dev->power.must_resume) - dpm_superior_set_must_resume(dev); + if (dev->power.must_resume) { + dev->power.set_active = dev->power.set_active || + dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND); + dpm_superior_set_must_resume(dev, dev->power.set_active); + } Complete: complete_all(&dev->power.completion); --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -683,6 +683,7 @@ bool no_pm_callbacks:1; /* Owned by the PM core */ bool async_in_progress:1; /* Owned by the PM core */ bool must_resume:1; /* Owned by the PM core */ + bool set_active:1; /* Owned by the PM core */ bool may_skip_resume:1; /* Set by subsystems */ #else bool should_wakeup:1;