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

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

[Krishna chaitanya chundru]

It is a property of the runtime PM framework that it can only
follow continuous dependency chains.  That is, if there is a device
with runtime PM disabled in a dependency chain, runtime PM cannot be
enabled for devices below it and above it in that chain both at the
same time.

Since runtime PM is disabled for host bridge, the state of the child
devices under the host bridge is not taken into account by PM framework
for the top level parent, PCIe controller. So PM framework, allows
the controller driver to enter runtime PM irrespective of the state
of the devices under the host bridge.

So enable runtime pm of the host bridge device, so that dependency
chain in maintained between endpoint devices and the controller driver.

PM framework expectes parent runtime pm enabled before enabling runtime
pm of the child. As PCIe starfive device is enabling runtime pm after
the pci_host_probe which enables runtime pm of the child device i.e for
the bridge device a warning is shown saying "pcie-starfive 940000000.pcie:
Enabling runtime PM for inactive device with active children" and also
shows possible circular locking dependency detected message.

As it is must to enable parent device's runtime PM before enabling child's
runtime pm as the pcie-starfive device runtime pm is enabled after child
runtime starfive device is seeing the warning.

In the first patch fix the pcie-starfive driver by enabling runtime
pm before calling pci_host_probe().

All other PCIe controller drivers are enabling runtime pm before
calling pci_host_probe() which is as expected so don't require any
fix like pcie-starfive driver.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
Changes in v7:
- Bring the cover letter details to commit message as suggested by bjorn.
- Add a comment in the driver to ensure pm_runtime_enable is called
  before host bridge pm_runtime_enable().
- Link to v6: https://lore.kernel.org/r/20241017-runtime_pm-v6-0-55eab5c2c940@quicinc.com
Changes in v6:
- include the patch by mayank which fixes runtime pm enable order for
  pcie starfive driver.
Changes in v5:
- call pm_runtime_no_callbacks() as suggested by Rafael.
- include the commit texts as suggested by Rafael.
- Link to v4: https://lore.kernel.org/linux-pci/20240708-runtime_pm-v4-1-c02a3663243b@quicinc.com/
Changes in v4:
- Changed pm_runtime_enable() to devm_pm_runtime_enable() (suggested by mayank)
- Link to v3: https://lore.kernel.org/lkml/20240609-runtime_pm-v3-1-3d0460b49d60@quicinc.com/
Changes in v3:
- Moved the runtime API call's from the dwc driver to PCI framework
  as it is applicable for all (suggested by mani)
- Updated the commit message.
- Link to v2: https://lore.kernel.org/all/20240305-runtime_pm_enable-v2-1-a849b74091d1@quicinc.com
Changes in v2:
- Updated commit message as suggested by mani.
- Link to v1: https://lore.kernel.org/r/20240219-runtime_pm_enable-v1-1-d39660310504@quicinc.com

---
Krishna chaitanya chundru (1):
      PCI: Enable runtime pm of the host bridge

Mayank Rana (1):
      PCI: starfive: Enable PCIe controller's runtime PM before probing host bridge

 drivers/pci/controller/plda/pcie-starfive.c | 10 +++++++---
 drivers/pci/probe.c                         | 11 +++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)
---
base-commit: 2f87d0916ce0d2925cedbc9e8f5d6291ba2ac7b2
change-id: 20241016-runtime_pm-d3dbf41736b6

Best regards,
-- 
Krishna chaitanya chundru <quic_krichai@quicinc.com>

[PATCH v7 1/2] PCI: starfive: Enable PCIe controller's runtime PM before probing host bridge

[Krishna chaitanya chundru]

From: Mayank Rana <quic_mrana@quicinc.com>

PCIe controller device (i.e. PCIe starfive device) is parent to PCIe host
bridge device. To enable runtime PM of PCIe host bridge device (child
device), it is must to enable parent device's runtime PM to avoid seeing
the below warning from PM core:

pcie-starfive 940000000.pcie: Enabling runtime PM for inactive device
with active children

Fix this issue by enabling starfive pcie controller device's runtime PM
before calling pci_host_probe() in plda_pcie_host_init().

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
v3->v6:
- no change
Link to v3: https://patchwork.kernel.org/project/linux-pci/patch/20241014162607.1247611-1-quic_mrana@quicinc.com/
v2->v3:
- Update commit description based on Mani's feedback
- Updated Reviewed-by tag
Link to v2: https://patchwork.kernel.org/project/linux-pci/patch/20241011235530.3919347-1-quic_mrana@quicinc.com/

v1->v2: Updated commit description based on Bjorn's feedback
Link to v1: https://patchwork.kernel.org/project/linux-pci/patch/20241010202950.3263899-1-quic_mrana@quicinc.com/
---
 drivers/pci/controller/plda/pcie-starfive.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c
index c9933ecf6833..0564fdce47c2 100644
--- a/drivers/pci/controller/plda/pcie-starfive.c
+++ b/drivers/pci/controller/plda/pcie-starfive.c
@@ -404,6 +404,9 @@ static int starfive_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+
 	plda->host_ops = &sf_host_ops;
 	plda->num_events = PLDA_MAX_EVENT_NUM;
 	/* mask doorbell event */
@@ -413,11 +416,12 @@ static int starfive_pcie_probe(struct platform_device *pdev)
 	plda->events_bitmap <<= PLDA_NUM_DMA_EVENTS;
 	ret = plda_pcie_host_init(&pcie->plda, &starfive_pcie_ops,
 				  &stf_pcie_event);
-	if (ret)
+	if (ret) {
+		pm_runtime_put_sync(&pdev->dev);
+		pm_runtime_disable(&pdev->dev);
 		return ret;
+	}
 
-	pm_runtime_enable(&pdev->dev);
-	pm_runtime_get_sync(&pdev->dev);
 	platform_set_drvdata(pdev, pcie);
 
 	return 0;

-- 
2.34.1

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

[Krishna chaitanya chundru]

The Controller driver is the parent device of the PCIe host bridge,
PCI-PCI bridge and PCIe endpoint as shown below.

        PCIe controller(Top level parent & parent of host bridge)
                        |
                        v
        PCIe Host bridge(Parent of PCI-PCI bridge)
                        |
                        v
        PCI-PCI bridge(Parent of endpoint driver)
                        |
                        v
                PCIe endpoint driver

Now, when the controller device goes to runtime suspend, PM framework
will check the runtime PM state of the child device (host bridge) and
will find it to be disabled. So it will allow the parent (controller
device) to go to runtime suspend. Only if the child device's state was
'active' it will prevent the parent to get suspended.

It is a property of the runtime PM framework that it can only
follow continuous dependency chains.  That is, if there is a device
with runtime PM disabled in a dependency chain, runtime PM cannot be
enabled for devices below it and above it in that chain both at the
same time.

Since runtime PM is disabled for host bridge, the state of the child
devices under the host bridge is not taken into account by PM framework
for the top level parent, PCIe controller. So PM framework, allows
the controller driver to enter runtime PM irrespective of the state
of the devices under the host bridge. And this causes the topology
breakage and also possible PM issues like controller driver goes to
runtime suspend while endpoint driver is doing some transfers.

Because of the above, in order to enable runtime PM for a PCIe
controller device, one needs to ensure that runtime PM is enabled for
all devices in every dependency chain between it and any PCIe endpoint
(as runtime PM is enabled for PCIe endpoints).

This means that runtime PM needs to be enabled for the host bridge
device, which is present in all of these dependency chains.

After this change, the host bridge device will be runtime-suspended
by the runtime PM framework automatically after suspending its last
child and it will be runtime-resumed automatically before resuming its
first child which will allow the runtime PM framework to track
dependencies between the host bridge device and all of its
descendants.

PM framework expectes parent runtime pm enabled before enabling runtime
pm of the child. 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.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
Changes in v7:
- update commit text & and add a comment in the driver as suggested by bjorn.
Changes in v6:
- no change
Changes in v5:
- call pm_runtime_no_callbacks() as suggested by Rafael.
- include the commit texts as suggested by Rafael.
- Link to v4: https://lore.kernel.org/linux-pci/20240708-runtime_pm-v4-1-c02a3663243b@quicinc.com/
Changes in v4:
- Changed pm_runtime_enable() to devm_pm_runtime_enable() (suggested by mayank)
- Link to v3: https://lore.kernel.org/lkml/20240609-runtime_pm-v3-1-3d0460b49d60@quicinc.com/
Changes in v3:
- Moved the runtime API call's from the dwc driver to PCI framework
  as it is applicable for all (suggested by mani)
- Updated the commit message.
- Link to v2: https://lore.kernel.org/all/20240305-runtime_pm_enable-v2-1-a849b74091d1@quicinc.com
Changes in v2:
- Updated commit message as suggested by mani.
- Link to v1: https://lore.kernel.org/r/20240219-runtime_pm_enable-v1-1-d39660310504@quicinc.com
---
 drivers/pci/probe.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 4f68414c3086..ee30e6024f52 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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);

-- 
2.34.1

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

[Markus Elfring]

…
> PM framework expectes parent runtime pm enabled before enabling runtime

               expects?                PM?


> pm of the child. …

  PM?


> drivers, before calling pci_host_probe() as pm frameworks expects if the

                                              PM framework?


> parent device supports runtime pm then it needs to enabled before child

                                 PM?


> runtime pm.

          PM?


Can any tags (like “Fixes” and “Cc”) become helpful for the proposed change?

Regards,
Markus

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

[Bjorn Helgaas]

On Mon, Nov 11, 2024 at 02:11:51PM +0530, Krishna chaitanya chundru wrote:
> It is a property of the runtime PM framework that it can only
> follow continuous dependency chains.  That is, if there is a device
> with runtime PM disabled in a dependency chain, runtime PM cannot be
> enabled for devices below it and above it in that chain both at the
> same time.
> 
> Since runtime PM is disabled for host bridge, the state of the child
> devices under the host bridge is not taken into account by PM framework
> for the top level parent, PCIe controller. So PM framework, allows
> the controller driver to enter runtime PM irrespective of the state
> of the devices under the host bridge.
> 
> So enable runtime pm of the host bridge device, so that dependency
> chain in maintained between endpoint devices and the controller driver.
> 
> PM framework expectes parent runtime pm enabled before enabling runtime
> pm of the child. As PCIe starfive device is enabling runtime pm after
> the pci_host_probe which enables runtime pm of the child device i.e for
> the bridge device a warning is shown saying "pcie-starfive 940000000.pcie:
> Enabling runtime PM for inactive device with active children" and also
> shows possible circular locking dependency detected message.
> 
> As it is must to enable parent device's runtime PM before enabling child's
> runtime pm as the pcie-starfive device runtime pm is enabled after child
> runtime starfive device is seeing the warning.
> 
> In the first patch fix the pcie-starfive driver by enabling runtime
> pm before calling pci_host_probe().
> 
> All other PCIe controller drivers are enabling runtime pm before
> calling pci_host_probe() which is as expected so don't require any
> fix like pcie-starfive driver.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>

Provisionally applied to pci/pm for v6.13.  I see a typo or two, so
the commit logs will likely be updated, but I pushed the branch to get
build testing started.

> ---
> Changes in v7:
> - Bring the cover letter details to commit message as suggested by bjorn.
> - Add a comment in the driver to ensure pm_runtime_enable is called
>   before host bridge pm_runtime_enable().
> - Link to v6: https://lore.kernel.org/r/20241017-runtime_pm-v6-0-55eab5c2c940@quicinc.com
> Changes in v6:
> - include the patch by mayank which fixes runtime pm enable order for
>   pcie starfive driver.
> Changes in v5:
> - call pm_runtime_no_callbacks() as suggested by Rafael.
> - include the commit texts as suggested by Rafael.
> - Link to v4: https://lore.kernel.org/linux-pci/20240708-runtime_pm-v4-1-c02a3663243b@quicinc.com/
> Changes in v4:
> - Changed pm_runtime_enable() to devm_pm_runtime_enable() (suggested by mayank)
> - Link to v3: https://lore.kernel.org/lkml/20240609-runtime_pm-v3-1-3d0460b49d60@quicinc.com/
> Changes in v3:
> - Moved the runtime API call's from the dwc driver to PCI framework
>   as it is applicable for all (suggested by mani)
> - Updated the commit message.
> - Link to v2: https://lore.kernel.org/all/20240305-runtime_pm_enable-v2-1-a849b74091d1@quicinc.com
> Changes in v2:
> - Updated commit message as suggested by mani.
> - Link to v1: https://lore.kernel.org/r/20240219-runtime_pm_enable-v1-1-d39660310504@quicinc.com
> 
> ---
> Krishna chaitanya chundru (1):
>       PCI: Enable runtime pm of the host bridge
> 
> Mayank Rana (1):
>       PCI: starfive: Enable PCIe controller's runtime PM before probing host bridge
> 
>  drivers/pci/controller/plda/pcie-starfive.c | 10 +++++++---
>  drivers/pci/probe.c                         | 11 +++++++++++
>  2 files changed, 18 insertions(+), 3 deletions(-)
> ---
> base-commit: 2f87d0916ce0d2925cedbc9e8f5d6291ba2ac7b2
> change-id: 20241016-runtime_pm-d3dbf41736b6
> 
> Best regards,
> -- 
> Krishna chaitanya chundru <quic_krichai@quicinc.com>
> 

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

[Krishna Chaitanya Chundru]

On 11/11/2024 10:16 PM, Markus Elfring wrote:
> …
>> PM framework expectes parent runtime pm enabled before enabling runtime
> 
>                 expects?                PM?
> 
> 
>> pm of the child. …
> 
>    PM?
> 
> 
>> drivers, before calling pci_host_probe() as pm frameworks expects if the
> 
>                                                PM framework?
> 
> 
>> parent device supports runtime pm then it needs to enabled before child
> 
>                                   PM?
> 
> 
>> runtime pm.
> 
>            PM?
> 
> 
> Can any tags (like “Fixes” and “Cc”) become helpful for the proposed change?
Bjorn,  This problem was present from starting on wards not sure what is 
the fix tag we need to use here, is it fine if we use fix tag as below.
as at this function only we are trying add the fix.

Fixes: 49b8e3f3ed1d4 ("PCI: Add generic function to probe PCI host 
controllers")

- Krishna Chaitanya.
> 
> Regards,
> Markus

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

[Johan Hovold]

On Mon, Nov 11, 2024 at 02:11:53PM +0530, Krishna chaitanya chundru wrote:
> The Controller driver is the parent device of the PCIe host bridge,
> PCI-PCI bridge and PCIe endpoint as shown below.
> 
>         PCIe controller(Top level parent & parent of host bridge)
>                         |
>                         v
>         PCIe Host bridge(Parent of PCI-PCI bridge)
>                         |
>                         v
>         PCI-PCI bridge(Parent of endpoint driver)
>                         |
>                         v
>                 PCIe endpoint driver
> 
> Now, when the controller device goes to runtime suspend, PM framework
> will check the runtime PM state of the child device (host bridge) and
> will find it to be disabled. So it will allow the parent (controller
> device) to go to runtime suspend. Only if the child device's state was
> 'active' it will prevent the parent to get suspended.

> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>

> @@ -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:

	pci0004:00: pcie4: Enabling runtime PM for inactive device with active children

which may have unpopulated ports (this laptop SKU does not have a modem).

Reverting dc421bb3c0db ("PCI: Enable runtime PM of the host bridge")
makes the warning go away.

Johan


#regzbot introduced: dc421bb3c0db

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

[Krishna Chaitanya Chundru]

On 1/7/2025 6:49 PM, Johan Hovold wrote:
> On Mon, Nov 11, 2024 at 02:11:53PM +0530, Krishna chaitanya chundru wrote:
>> The Controller driver is the parent device of the PCIe host bridge,
>> PCI-PCI bridge and PCIe endpoint as shown below.
>>
>>          PCIe controller(Top level parent & parent of host bridge)
>>                          |
>>                          v
>>          PCIe Host bridge(Parent of PCI-PCI bridge)
>>                          |
>>                          v
>>          PCI-PCI bridge(Parent of endpoint driver)
>>                          |
>>                          v
>>                  PCIe endpoint driver
>>
>> Now, when the controller device goes to runtime suspend, PM framework
>> will check the runtime PM state of the child device (host bridge) and
>> will find it to be disabled. So it will allow the parent (controller
>> device) to go to runtime suspend. Only if the child device's state was
>> 'active' it will prevent the parent to get suspended.
> 
>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> 
>> @@ -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:
>
Hi Johan,

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.

> 	pci0004:00: pcie4: Enabling runtime PM for inactive device with active children
> 
I believe this is not causing any functional issues.
> 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.

- Krishna Chaitanya.
> Reverting dc421bb3c0db ("PCI: Enable runtime PM of the host bridge")
> makes the warning go away.
> 
> Johan
> 
> 
> #regzbot introduced: dc421bb3c0db

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

[Johan Hovold]

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.

Johan

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

[Manivannan Sadhasivam]

+ 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

I'm not sure of what the fix is in this case. But removing the
DPM_FLAG_SMART_SUSPEND flag from PCI bridge driver (portdrv) makes the warning
go away. This indicates that something is wrong with the DPM_FLAG_SMART_SUSPEND
flag handling in PM core.

Ulf/Rafael, thoughts?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Bjorn Helgaas]

On Mon, Jan 13, 2025 at 09:55:49PM +0530, Manivannan Sadhasivam wrote:
> 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
> 
> I'm not sure of what the fix is in this case. But removing the
> DPM_FLAG_SMART_SUSPEND flag from PCI bridge driver (portdrv) makes
> the warning go away. This indicates that something is wrong with the
> DPM_FLAG_SMART_SUSPEND flag handling in PM core.
> 
> Ulf/Rafael, thoughts?

What's the plan for this?  Does anybody have a proposal?

IIUC there is no functional issue, but the new warning must be fixed,
and it would sure be nice to do it before v6.13.  If there *is* a
functional problem, we need to consider a revert ASAP.

Bjorn

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

[Manivannan Sadhasivam]

On Tue, Jan 14, 2025 at 03:16:53PM -0600, Bjorn Helgaas wrote:
> On Mon, Jan 13, 2025 at 09:55:49PM +0530, Manivannan Sadhasivam wrote:
> > 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
> > 
> > I'm not sure of what the fix is in this case. But removing the
> > DPM_FLAG_SMART_SUSPEND flag from PCI bridge driver (portdrv) makes
> > the warning go away. This indicates that something is wrong with the
> > DPM_FLAG_SMART_SUSPEND flag handling in PM core.
> > 
> > Ulf/Rafael, thoughts?
> 
> What's the plan for this?  Does anybody have a proposal?
> 

TBH, I don't know how to fix this issue in a proper way. I need inputs from
Rafael/Ulf.

> IIUC there is no functional issue, but the new warning must be fixed,
> and it would sure be nice to do it before v6.13.  If there *is* a
> functional problem, we need to consider a revert ASAP.
> 

There is no functional problem that I'm aware of, so revert is not warranted.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Johan Hovold]

On Sun, Jan 19, 2025 at 08:59:40PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jan 14, 2025 at 03:16:53PM -0600, Bjorn Helgaas wrote:
> > On Mon, Jan 13, 2025 at 09:55:49PM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Jan 07, 2025 at 03:27:59PM +0100, Johan Hovold wrote:

> > > > > > 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:

> > > > > > 	pci0004:00: pcie4: Enabling runtime PM for inactive device with active children

> > > > > > which may have unpopulated ports (this laptop SKU does not
> > > > > > have a modem).

> > What's the plan for this?  Does anybody have a proposal?
> > 
> 
> TBH, I don't know how to fix this issue in a proper way. I need inputs from
> Rafael/Ulf.
> 
> > IIUC there is no functional issue, but the new warning must be fixed,
> > and it would sure be nice to do it before v6.13.  If there *is* a
> > functional problem, we need to consider a revert ASAP.
> > 
> 
> There is no functional problem that I'm aware of, so revert is not warranted.

I'd argue for reverting the offending commit as that is the only way to
make sure that the new warning is ever addressed.

Vendors unfortunately do not a have a good track record of following up
and fixing issues like this.

Judging from a quick look at the code (and the commit message of the
patch in question), no host controller driver depends on the commit in
question as the ones that do enable runtime PM just resume
unconditionally at probe() currently (i.e. effectively ignores the state
of their children).

Johan

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

[Manivannan Sadhasivam]

On Mon, Jan 20, 2025 at 11:29:41AM +0100, Johan Hovold wrote:
> On Sun, Jan 19, 2025 at 08:59:40PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Jan 14, 2025 at 03:16:53PM -0600, Bjorn Helgaas wrote:
> > > On Mon, Jan 13, 2025 at 09:55:49PM +0530, Manivannan Sadhasivam wrote:
> > > > On Tue, Jan 07, 2025 at 03:27:59PM +0100, Johan Hovold wrote:
> 
> > > > > > > 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:
> 
> > > > > > > 	pci0004:00: pcie4: Enabling runtime PM for inactive device with active children
> 
> > > > > > > which may have unpopulated ports (this laptop SKU does not
> > > > > > > have a modem).
> 
> > > What's the plan for this?  Does anybody have a proposal?
> > > 
> > 
> > TBH, I don't know how to fix this issue in a proper way. I need inputs from
> > Rafael/Ulf.
> > 
> > > IIUC there is no functional issue, but the new warning must be fixed,
> > > and it would sure be nice to do it before v6.13.  If there *is* a
> > > functional problem, we need to consider a revert ASAP.
> > > 
> > 
> > There is no functional problem that I'm aware of, so revert is not warranted.
> 
> I'd argue for reverting the offending commit as that is the only way to
> make sure that the new warning is ever addressed.
> 

How come reverting becomes the *only* way to address the issue? There seems to
be nothing wrong with the commit in question and the same pattern in being used
in other drivers as well. The issue looks to be in the PM core.

Moreover, the warning is not causing any functional issue as far as I know. So
just reverting the commit that took so much effort to get merged for the sake of
hiding a warning doesn't feel right to me.

> Vendors unfortunately do not a have a good track record of following up
> and fixing issues like this.
> 

I'm not relying on vendors here. I am looking into this issue and would like to
fix the warning/issue on my own. That's why I shared my observation with
Ulf/Rafael.

> Judging from a quick look at the code (and the commit message of the
> patch in question), no host controller driver depends on the commit in
> question as the ones that do enable runtime PM just resume
> unconditionally at probe() currently (i.e. effectively ignores the state
> of their children).
> 

Right. There are a couple of pieces that needs to be fixed to have the runtime
PM working from top to bottom in the PCIe hierarchy. This is also one more
reason why I believe that the commit wouldn't have caused any functional issue.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Johan Hovold]

On Mon, Jan 20, 2025 at 08:58:29PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Jan 20, 2025 at 11:29:41AM +0100, Johan Hovold wrote:

> > I'd argue for reverting the offending commit as that is the only way to
> > make sure that the new warning is ever addressed.

> How come reverting becomes the *only* way to address the issue?

I didn't say it was the only way to address the issue, just that it's
the only way to make sure that the new warning gets addressed. Once code
is upstream, some vendors tend to lose interest.

> There seems to
> be nothing wrong with the commit in question and the same pattern in being used
> in other drivers as well. The issue looks to be in the PM core.

After taking a closer look now, I agree that the underlying issue seems
to be in PM core.

Possibly introduced by Rafael's commit 6e176bf8d461 ("PM: sleep: core:
Do not skip callbacks in the resume phase") which moved the set_active()
call from resume_noirq() to resume_early().

> Moreover, the warning is not causing any functional issue as far as I know. So
> just reverting the commit that took so much effort to get merged for the sake of
> hiding a warning doesn't feel right to me.

My point was simply that this commit introduced a new warning in 6.13,
and there is still no fix available. The code is also effectively dead,
well aside from triggering the warning, and runtime suspending the host
controller cannot even be tested with mainline yet (and this was
unfortunately not made clear in the commit message).

The change should have been part of a series that actually enabled the
functionality and not just a potential piece of it which cannot yet be
tested. Also, for Qualcomm controllers, we don't even yet have proper
suspend so it's probably not a good idea to throw runtime PM into the
mix there just yet.

But, sure, a revert would have made more sense last week. I guess you
have a few more weeks to address this now. We can always backport a
revert once rc1 is out.

Johan

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

[Johan Hovold]

On Tue, Jan 21, 2025 at 02:18:49PM +0100, Johan Hovold wrote:
> After taking a closer look now, I agree that the underlying issue seems
> to be in PM core.
> 
> Possibly introduced by Rafael's commit 6e176bf8d461 ("PM: sleep: core:
> Do not skip callbacks in the resume phase") which moved the set_active()
> call from resume_noirq() to resume_early().

Scratch the last paragraph, I misread the diff, sorry.

Johan

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

[Manivannan Sadhasivam]

On Tue, Jan 21, 2025 at 02:18:49PM +0100, Johan Hovold wrote:
> On Mon, Jan 20, 2025 at 08:58:29PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Jan 20, 2025 at 11:29:41AM +0100, Johan Hovold wrote:
> 
> > > I'd argue for reverting the offending commit as that is the only way to
> > > make sure that the new warning is ever addressed.
> 
> > How come reverting becomes the *only* way to address the issue?
> 
> I didn't say it was the only way to address the issue, just that it's
> the only way to make sure that the new warning gets addressed. Once code
> is upstream, some vendors tend to lose interest.
> 
> > There seems to
> > be nothing wrong with the commit in question and the same pattern in being used
> > in other drivers as well. The issue looks to be in the PM core.
> 
> After taking a closer look now, I agree that the underlying issue seems
> to be in PM core.
> 
> Possibly introduced by Rafael's commit 6e176bf8d461 ("PM: sleep: core:
> Do not skip callbacks in the resume phase") which moved the set_active()
> call from resume_noirq() to resume_early().
> 
> > Moreover, the warning is not causing any functional issue as far as I know. So
> > just reverting the commit that took so much effort to get merged for the sake of
> > hiding a warning doesn't feel right to me.
> 
> My point was simply that this commit introduced a new warning in 6.13,
> and there is still no fix available. The code is also effectively dead,
> well aside from triggering the warning, and runtime suspending the host
> controller cannot even be tested with mainline yet (and this was
> unfortunately not made clear in the commit message).
> 
> The change should have been part of a series that actually enabled the
> functionality and not just a potential piece of it which cannot yet be
> tested. Also, for Qualcomm controllers, we don't even yet have proper
> suspend so it's probably not a good idea to throw runtime PM into the
> mix there just yet.
> 

There are multiple pieces needed to be stitch together to enable runtime PM in
the PCIe topology. And each one of them seemed to be controversial enough (due
to common code etc...). So that's the reason they were sent separately. Though
I must admit that the full picture and the limitation of not being able to
exercise the runtime PM should've been mentioned.

> But, sure, a revert would have made more sense last week. I guess you
> have a few more weeks to address this now. We can always backport a
> revert once rc1 is out.
> 

Sure. I've pinged Ulf offline and he promised to look into it asap.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Ulf Hansson]

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.

>
> I'm not sure of what the fix is in this case. But removing the
> DPM_FLAG_SMART_SUSPEND flag from PCI bridge driver (portdrv) makes the warning
> go away. This indicates that something is wrong with the DPM_FLAG_SMART_SUSPEND
> flag handling in PM core.

As an intermediate step, perhaps the easiest thing here is to remove
the DPM_FLAG_SMART_SUSPEND flag from the PCI bridge? Ideally it should
not break anything, but we probably would lose some optimizations.

Another option would be to use pm_suspend_ignore_children(), but I am
not sure if that would be the correct thing to do here.

>
> Ulf/Rafael, thoughts?
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Kind regards
Uffe

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

[Rafael J. Wysocki]

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

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.

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

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

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -656,12 +656,12 @@
 	 * 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.must_resume flag is clear, in
+	 * which case it is not necessary to update its PM-runtime status.
 	 */
 	if (skip_resume)
 		pm_runtime_set_suspended(dev);
-	else if (dev_pm_skip_suspend(dev))
+	else if (dev->power.must_resume)
 		pm_runtime_set_active(dev);
 
 	if (dev->pm_domain) {

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

[Rafael J. Wysocki]

[-- 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;

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

[Manivannan Sadhasivam]

On Tue, Jan 28, 2025 at 12:47:09PM +0100, Rafael J. Wysocki wrote:
> 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.
> 

This patch indeed makes the warning go away and I don't spot any other issues.
So you can add my Tested-by tag while submitting the fix.

Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks a lot!

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Rafael J. Wysocki]

On Tue, Jan 28, 2025 at 4:58 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Tue, Jan 28, 2025 at 12:47:09PM +0100, Rafael J. Wysocki wrote:
> > 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.
> >
>
> This patch indeed makes the warning go away and I don't spot any other issues.
> So you can add my Tested-by tag while submitting the fix.
>
> Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> Thanks a lot!

Thank you!