Re: [PATCH v7 2/2] PCI: Enable runtime pm of the host bridge

Rafael J. Wysocki
2025-01-28T11:47:21Z

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

On Mon, Jan 27, 2025 at 8:57 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Jan 27, 2025 at 3:32 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Mon, 13 Jan 2025 at 17:25, Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > >
> > > + Ulf (for the runtime PM related question)
> > >
> > > On Tue, Jan 07, 2025 at 03:27:59PM +0100, Johan Hovold wrote:
> > > > On Tue, Jan 07, 2025 at 07:40:39PM +0530, Krishna Chaitanya Chundru wrote:
> > > > > On 1/7/2025 6:49 PM, Johan Hovold wrote:
> > > >
> > > > > >> @@ -3106,6 +3106,17 @@ int pci_host_probe(struct pci_host_bridge *bridge)
> > > > > >> pcie_bus_configure_settings(child);
> > > > > >>
> > > > > >> pci_bus_add_devices(bus);
> > > > > >> +
> > > > > >> + /*
> > > > > >> + * Ensure pm_runtime_enable() is called for the controller drivers,
> > > > > >> + * before calling pci_host_probe() as pm frameworks expects if the
> > > > > >> + * parent device supports runtime pm then it needs to enabled before
> > > > > >> + * child runtime pm.
> > > > > >> + */
> > > > > >> + pm_runtime_set_active(&bridge->dev);
> > > > > >> + pm_runtime_no_callbacks(&bridge->dev);
> > > > > >> + devm_pm_runtime_enable(&bridge->dev);
> > > > > >> +
> > > > > >> return 0;
> > > > > >> }
> > > > > >> EXPORT_SYMBOL_GPL(pci_host_probe);
> > > > > >
> > > > > > I just noticed that this change in 6.13-rc1 is causing the following
> > > > > > warning on resume from suspend on machines like the Lenovo ThinkPad
> > > > > > X13s:
> > > >
> > > > > Can you confirm if you are seeing this issue is seen in the boot-up
> > > > > case also. As this part of the code executes only at the boot time and
> > > > > will not have effect in resume from suspend.
> > > >
> > > > No, I only see it during resume. And enabling runtime PM can (and in
> > > > this case, obviously does) impact system suspend as well.
> > > >
> > > > > > pci0004:00: pcie4: Enabling runtime PM for inactive device with active children
> > > >
> > > > > I believe this is not causing any functional issues.
> > > >
> > > > It still needs to be fixed.
> > > >
> > > > > > which may have unpopulated ports (this laptop SKU does not have a modem).
> > > >
> > > > > Can you confirm if this warning goes away if there is some endpoint
> > > > > connected to it.
> > > >
> > > > I don't have anything to connect to the slot in this machine, but this
> > > > seems to be the case as I do not see this warning for the populated
> > > > slots, nor on the CRD reference design which has a modem on PCIe4.
> > > >
> > >
> > > Yes, this is only happening for unpopulated slots and the warning shows up only
> > > if runtime PM is enabled for both PCI bridge and host bridge. This patch enables
> > > the runtime PM for host bridge and if the PCI bridge runtime PM is also enabled
> > > (only happens now for ACPI/BIOS based platforms), then the warning shows up only
> > > if the PCI bridge was RPM suspended (mostly happens if there was no device
> > > connected) during the system wide resume time.
> > >
> > > For the sake of reference, PCI host bridge is the parent of PCI bridge.
> > >
> > > Looking at where the warning gets triggered (in pm_runtime_enable()), we have
> > > the below checks:
> > >
> > > dev->power.runtime_status == RPM_SUSPENDED
> > > !dev->power.ignore_children
> > > atomic_read(&dev->power.child_count) > 0
> > >
> > > When pm_runtime_enable() gets called for PCI host bridge:
> > >
> > > dev->power.runtime_status = RPM_SUSPENDED
> > > dev->power.ignore_children = 0
> > > dev->power.child_count = 1
> > >
> > > First 2 passes seem legit, but the issue is with the 3rd one. Here, the
> > > child_count of 1 means that the PCI host bridge has an 'active' child (which is
> > > the PCI bridge). The PCI bridge was supposed to be RPM_SUSPENDED as the resume
> > > process should first resume the parent (PCI host bridge). But this is not the
> > > case here.
> > >
> > > Then looking at where the child_count gets incremented, it leads to
> > > pm_runtime_set_active() of device_resume_noirq(). pm_runtime_set_active() is
> > > only called for a device if dev_pm_skip_suspend() succeeds, which requires
> > > DPM_FLAG_SMART_SUSPEND flag to be set and the device to be runtime suspended.
> > >
> > > This criteria matches for PCI bridge. So its status was set to 'RPM_ACTIVE' even
> > > though the parent PCI host bridge was still in the RPM_SUSPENDED state. I don't
> > > think this is a valid condition as seen from the warning triggered for PCI host
> > > bridge when pm_runtime_enable() is called from device_resume_early():
> > >
> > > pci0004:00: pcie4: Enabling runtime PM for inactive device with active children
> >
> > Thanks for the detailed analysis, much appreciated.
> >
> > So this seems to boil down to the fact that the PM core calls
> > pm_runtime_set_active() for a device, when it really should not. If
> > there is a clever way to avoid that, I think we need Rafael's opinion
> > on.
>
> Actually, not really.
>
> The status of the child and the child count of the parent have no
> meaning until runtime PM is enabled for the parent. They can be
> manipulated freely before this happens with no consequences and all
> will be fine as long as those settings are consistent when runtime PM
> is enabled for the parent.
>
> Now, they aren't consistent at that point because
> dev_pm_skip_suspend() returns false for the parent as it has
> DPM_FLAG_SMART_SUSPEND clear.
>
> To me, this looks like a coding mistake because all devices that have
> power.must_resume set should also be set to RPM_ACTIVE before
> re-enabling runtime PM for them, so the attached patch should work.

Having reflected on it a bit I think that it's better to avoid
changing the existing behavior too much, so attached is a new version
of the patch.

It is along the same lines as before, but it doesn't go as far as the
previous version. Namely, in addition to what the existing code does,
it will cause the runtime PM status to be set to RPM_ACTIVE for the
devices whose dependents will have it set which should address the
problem at hand if I'm not mistaken.

I'd appreciated giving it a go on a system where the warning is printed.

Thanks!

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

---
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;
+ }

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;