On Thu, Aug 1, 2024 at 10:33 PM Manivannan Sadhasivam
<manivannan.sadhasivam@xxxxxxxxxx> wrote:
>
> On Thu, Aug 01, 2024 at 02:07:41PM -0700, Hsin-Yi Wang wrote:
> > On Fri, Jul 26, 2024 at 4:02 PM Manivannan Sadhasivam
> > <manivannan.sadhasivam@xxxxxxxxxx> wrote:
> > >
> > > Currently, there is no proper distinction between D3Hot and D3Cold while
> > > handling the power management for PCI bridges. For instance,
> > > pci_bridge_d3_allowed() API decides whether it is allowed to put the
> > > bridge in D3, but it doesn't explicitly specify whether D3Hot or D3Cold
> > > is allowed in a scenario. This often leads to confusion and may be prone
> > > to errors.
> > >
> > > So let's split the D3Hot and D3Cold handling where possible. The current
> > > pci_bridge_d3_allowed() API is now split into pci_bridge_d3hot_allowed()
> > > and pci_bridge_d3cold_allowed() APIs and used in relevant places.
> > >
> > > Also, pci_bridge_d3_update() API is now renamed to
> > > pci_bridge_d3cold_update() since it was only used to check the possibility
> > > of D3Cold.
> > >
> > > Note that it is assumed that only D3Hot needs to be checked while
> > > transitioning the bridge during runtime PM and D3Cold in other places. In
> > > the ACPI case, wakeup is now only enabled if both D3Hot and D3Cold are
> > > allowed for the bridge.
> > >
> > > Still, there are places where just 'd3' is used opaquely, but those are
> > > hard to distinguish, hence left for future cleanups.
> > >
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > > ---
> > > drivers/pci/bus.c | 2 +-
> > > drivers/pci/pci-acpi.c | 5 +--
> > > drivers/pci/pci-sysfs.c | 2 +-
> > > drivers/pci/pci.c | 78 ++++++++++++++++++++++++++++++----------------
> > > drivers/pci/pci.h | 12 ++++---
> > > drivers/pci/pcie/portdrv.c | 16 +++++-----
> > > drivers/pci/remove.c | 2 +-
> > > include/linux/pci.h | 3 +-
> > > 8 files changed, 75 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > > index 826b5016a101..cb1a1aaefa90 100644
> > > --- a/drivers/pci/bus.c
> > > +++ b/drivers/pci/bus.c
> > > @@ -346,7 +346,7 @@ void pci_bus_add_device(struct pci_dev *dev)
> > > of_pci_make_dev_node(dev);
> > > pci_create_sysfs_dev_files(dev);
> > > pci_proc_attach_device(dev);
> > > - pci_bridge_d3_update(dev);
> > > + pci_bridge_d3cold_update(dev);
> > >
> > > dev->match_driver = !dn || of_device_is_available(dn);
> > > retval = device_attach(&dev->dev);
> > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > > index 0f260cdc4592..aaf5a68e7984 100644
> > > --- a/drivers/pci/pci-acpi.c
> > > +++ b/drivers/pci/pci-acpi.c
> > > @@ -1434,7 +1434,7 @@ void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
> > > * reason is that the bridge may have additional methods such as
> > > * _DSW that need to be called.
> > > */
> > > - if (pci_dev->bridge_d3_allowed)
> > > + if (pci_dev->bridge_d3cold_allowed && pci_dev->bridge_d3hot_allowed)
> > > device_wakeup_enable(dev);
> > >
> > > acpi_pci_wakeup(pci_dev, false);
> > > @@ -1452,7 +1452,8 @@ void pci_acpi_cleanup(struct device *dev, struct acpi_device *adev)
> > > pci_acpi_remove_pm_notifier(adev);
> > > if (adev->wakeup.flags.valid) {
> > > acpi_device_power_remove_dependent(adev, dev);
> > > - if (pci_dev->bridge_d3_allowed)
> > > + if (pci_dev->bridge_d3cold_allowed &&
> > > + pci_dev->bridge_d3hot_allowed)
> > > device_wakeup_disable(dev);
> > >
> > > device_set_wakeup_capable(dev, false);
> > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > index 40cfa716392f..45628b0dd116 100644
> > > --- a/drivers/pci/pci-sysfs.c
> > > +++ b/drivers/pci/pci-sysfs.c
> > > @@ -529,7 +529,7 @@ static ssize_t d3cold_allowed_store(struct device *dev,
> > > return -EINVAL;
> > >
> > > pdev->d3cold_allowed = !!val;
> > > - pci_bridge_d3_update(pdev);
> > > + pci_bridge_d3cold_update(pdev);
> > >
> > > pm_runtime_resume(dev);
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 0edc4e448c2d..48e2ca0cd8a0 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -166,9 +166,9 @@ bool pci_ats_disabled(void)
> > > }
> > > EXPORT_SYMBOL_GPL(pci_ats_disabled);
> > >
> > > -/* Disable bridge_d3 for all PCIe ports */
> > > +/* Disable both D3Hot and D3Cold for all PCIe ports */
> > > static bool pci_bridge_d3_disable;
> > > -/* Force bridge_d3 for all PCIe ports */
> > > +/* Force both D3Hot and D3Cold for all PCIe ports */
> > > static bool pci_bridge_d3_force;
> > >
> > > static int __init pcie_port_pm_setup(char *str)
> > > @@ -2966,14 +2966,11 @@ static const struct dmi_system_id bridge_d3_blacklist[] = {
> > > { }
> > > };
> > >
> > > -/**
> > > - * pci_bridge_d3_allowed - Is it allowed to put the bridge into D3
> > > - * @bridge: Bridge to check
> > > - *
> > > - * This function checks if the bridge is allowed to move to D3.
> > > - * Currently we only allow D3 for recent enough PCIe ports and Thunderbolt.
> > > +/*
> > > + * Helper function to check whether it is allowed to put the bridge into D3
> > > + * states (D3Hot and D3Cold).
> > > */
> > > -bool pci_bridge_d3_allowed(struct pci_dev *bridge)
> > > +static bool pci_bridge_d3_allowed(struct pci_dev *bridge, pci_power_t state)
> > > {
> > > if (!pci_is_pcie(bridge))
> > > return false;
> > > @@ -3026,6 +3023,32 @@ bool pci_bridge_d3_allowed(struct pci_dev *bridge)
> > > return false;
> > > }
> > >
> > > +/**
> > > + * pci_bridge_d3cold_allowed - Is it allowed to put the bridge into D3Cold
> > > + * @bridge: Bridge to check
> > > + *
> > > + * This function checks if the bridge is allowed to move to D3Cold.
> > > + * Currently we only allow D3Cold for recent enough PCIe ports on ACPI based
> > > + * platforms and Thunderbolt.
> > > + */
> > > +bool pci_bridge_d3cold_allowed(struct pci_dev *bridge)
> > > +{
> > > + return pci_bridge_d3_allowed(bridge, PCI_D3cold);
> > > +}
> > > +
> > > +/**
> > > + * pci_bridge_d3cold_allowed - Is it allowed to put the bridge into D3Hot
> >
> > typo? pci_bridge_d3hot_allowed.
> >
>
> Yep, nice catch!
>
> > > + * @bridge: Bridge to check
> > > + *
> > > + * This function checks if the bridge is allowed to move to D3Hot.
> > > + * Currently we only allow D3Hot for recent enough PCIe ports on ACPI based
> > > + * platforms and Thunderbolt.
> > > + */
> > > +bool pci_bridge_d3hot_allowed(struct pci_dev *bridge)
> > > +{
> > > + return pci_bridge_d3_allowed(bridge, PCI_D3hot);
> > > +}
> > > +
> > > static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
> > > {
> > > bool *d3cold_ok = data;
> > > @@ -3046,55 +3069,55 @@ static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
> > > }
> > >
> > > /*
> > > - * pci_bridge_d3_update - Update bridge D3 capabilities
> > > + * pci_bridge_d3cold_update - Update bridge D3Cold capabilities
> > > * @dev: PCI device which is changed
> > > *
> > > * Update upstream bridge PM capabilities accordingly depending on if the
> > > * device PM configuration was changed or the device is being removed. The
> > > * change is also propagated upstream.
> > > */
> > > -void pci_bridge_d3_update(struct pci_dev *dev)
> > > +void pci_bridge_d3cold_update(struct pci_dev *dev)
> > > {
> > > bool remove = !device_is_registered(&dev->dev);
> > > struct pci_dev *bridge;
> > > bool d3cold_ok = true;
> > >
> > > bridge = pci_upstream_bridge(dev);
> > > - if (!bridge || !pci_bridge_d3_allowed(bridge))
> > > + if (!bridge || !pci_bridge_d3cold_allowed(bridge))
> > > return;
> > >
> > > /*
> > > - * If D3 is currently allowed for the bridge, removing one of its
> > > + * If D3Cold is currently allowed for the bridge, removing one of its
> > > * children won't change that.
> > > */
> > > - if (remove && bridge->bridge_d3_allowed)
> > > + if (remove && bridge->bridge_d3cold_allowed)
> > > return;
> > >
> > > /*
> > > - * If D3 is currently allowed for the bridge and a child is added or
> > > - * changed, disallowance of D3 can only be caused by that child, so
> > > + * If D3Cold is currently allowed for the bridge and a child is added or
> > > + * changed, disallowance of D3Cold can only be caused by that child, so
> > > * we only need to check that single device, not any of its siblings.
> > > *
> > > - * If D3 is currently not allowed for the bridge, checking the device
> > > - * first may allow us to skip checking its siblings.
> > > + * If D3Cold is currently not allowed for the bridge, checking the
> > > + * device first may allow us to skip checking its siblings.
> > > */
> > > if (!remove)
> > > pci_dev_check_d3cold(dev, &d3cold_ok);
> > >
> > > /*
> > > - * If D3 is currently not allowed for the bridge, this may be caused
> > > + * If D3Cold is currently not allowed for the bridge, this may be caused
> > > * either by the device being changed/removed or any of its siblings,
> > > * so we need to go through all children to find out if one of them
> > > - * continues to block D3.
> > > + * continues to block D3Cold.
> > > */
> > > - if (d3cold_ok && !bridge->bridge_d3_allowed)
> > > + if (d3cold_ok && !bridge->bridge_d3cold_allowed)
> > > pci_walk_bus(bridge->subordinate, pci_dev_check_d3cold,
> > > &d3cold_ok);
> > >
> > > - if (bridge->bridge_d3_allowed != d3cold_ok) {
> > > - bridge->bridge_d3_allowed = d3cold_ok;
> > > + if (bridge->bridge_d3cold_allowed != d3cold_ok) {
> > > + bridge->bridge_d3cold_allowed = d3cold_ok;
> > > /* Propagate change to upstream bridges */
> > > - pci_bridge_d3_update(bridge);
> > > + pci_bridge_d3cold_update(bridge);
> > > }
> > > }
> > >
> > > @@ -3110,7 +3133,7 @@ void pci_d3cold_enable(struct pci_dev *dev)
> > > {
> > > if (dev->no_d3cold) {
> > > dev->no_d3cold = false;
> > > - pci_bridge_d3_update(dev);
> > > + pci_bridge_d3cold_update(dev);
> > > }
> > > }
> > > EXPORT_SYMBOL_GPL(pci_d3cold_enable);
> > > @@ -3127,7 +3150,7 @@ void pci_d3cold_disable(struct pci_dev *dev)
> > > {
> > > if (!dev->no_d3cold) {
> > > dev->no_d3cold = true;
> > > - pci_bridge_d3_update(dev);
> > > + pci_bridge_d3cold_update(dev);
> > > }
> > > }
> > > EXPORT_SYMBOL_GPL(pci_d3cold_disable);
> > > @@ -3167,7 +3190,8 @@ void pci_pm_init(struct pci_dev *dev)
> > > dev->pm_cap = pm;
> > > dev->d3hot_delay = PCI_PM_D3HOT_WAIT;
> > > dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
> > > - dev->bridge_d3_allowed = pci_bridge_d3_allowed(dev);
> > > + dev->bridge_d3cold_allowed = pci_bridge_d3cold_allowed(dev);
> > > + dev->bridge_d3hot_allowed = pci_bridge_d3hot_allowed(dev);
> > > dev->d3cold_allowed = true;
> > >
> > > dev->d1_support = false;
> > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > index 53ca75639201..f819eab793fc 100644
> > > --- a/drivers/pci/pci.h
> > > +++ b/drivers/pci/pci.h
> > > @@ -92,8 +92,9 @@ void pci_pm_init(struct pci_dev *dev);
> > > void pci_ea_init(struct pci_dev *dev);
> > > void pci_msi_init(struct pci_dev *dev);
> > > void pci_msix_init(struct pci_dev *dev);
> > > -bool pci_bridge_d3_allowed(struct pci_dev *dev);
> > > -void pci_bridge_d3_update(struct pci_dev *dev);
> > > +bool pci_bridge_d3cold_allowed(struct pci_dev *dev);
> > > +bool pci_bridge_d3hot_allowed(struct pci_dev *dev);
> > > +void pci_bridge_d3cold_update(struct pci_dev *dev);
> > > int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type);
> > >
> > > static inline void pci_wakeup_event(struct pci_dev *dev)
> > > @@ -111,9 +112,12 @@ static inline bool pci_power_manageable(struct pci_dev *pci_dev)
> > > {
> > > /*
> > > * Currently we allow normal PCI devices and PCI bridges transition
> > > - * into D3 if their bridge_d3 is set.
> > > + * into D3 states if both bridge_d3cold_allowed and bridge_d3hot_allowed
> > > + * are set.
> > > */
> >
> > If pm requires both D3hot and D3cold, can we add a flag for DT to
> > support D3cold? Otherwise during suspend, pcieport still stays at D0.
> >
>
> You mean D3hot?
>
> > [ 42.202016] mt7921e 0000:01:00.0: PM: calling
> > pci_pm_suspend_noirq+0x0/0x300 @ 77, parent: 0000:00:00.0
> > [ 42.231681] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
>
> Here I can see that the port entered D3hot
>
This one is the wifi device connected to the port.
> > [ 42.238048] mt7921e 0000:01:00.0: PM:
> > pci_pm_suspend_noirq+0x0/0x300 returned 0 after 26583 usecs
> > [ 42.247083] pcieport 0000:00:00.0: PM: calling
> > pci_pm_suspend_noirq+0x0/0x300 @ 3196, parent: pci0000:00
> > [ 42.296325] pcieport 0000:00:00.0: PCI PM: Suspend power state: D0
>
This is the port suspended with D0. If we hack power_manageable to
only consider D3hot, the state here for pcieport will become D3hot
(compared in below).
If it's D0 (and s2idle), in resume it won't restore config:
https://elixir.bootlin.com/linux/v6.10/source/drivers/pci/pci-driver.c#L959,
and in resume it would be an issue.
Comparison:
1. pcieport can go to D3:
(suspend)
[ 61.645809] mt7921e 0000:01:00.0: PM: calling
pci_pm_suspend_noirq+0x0/0x2f8 @ 1139, parent: 0000:00:00.0
[ 61.675562] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
[ 61.681931] mt7921e 0000:01:00.0: PM:
pci_pm_suspend_noirq+0x0/0x2f8 returned 0 after 26502 usecs
[ 61.690959] pcieport 0000:00:00.0: PM: calling
pci_pm_suspend_noirq+0x0/0x2f8 @ 3248, parent: pci0000:00
[ 61.755359] pcieport 0000:00:00.0: PCI PM: Suspend power state: D3hot
[ 61.761832] pcieport 0000:00:00.0: PM:
pci_pm_suspend_noirq+0x0/0x2f8 returned 0 after 61345 usecs
(resume)
[ 65.243981] pcieport 0000:00:00.0: PM: calling
pci_pm_resume_noirq+0x0/0x190 @ 3258, parent: pci0000:00
[ 65.253122] mtk-pcie-phy 16930000.phy: CKM_38=0x13040500,
GLB_20=0x0, GLB_30=0x0, GLB_38=0x30453fc, GLB_F4=0x1453b000
[ 65.262725] pcieport 0000:00:00.0: PM:
pci_pm_resume_noirq+0x0/0x190 returned 0 after 175 usecs
[ 65.273159] mtk-pcie-phy 16930000.phy: No calibration info
[ 65.281903] mt7921e 0000:01:00.0: PM: calling
pci_pm_resume_noirq+0x0/0x190 @ 3259, parent: 0000:00:00.0
[ 65.297108] mt7921e 0000:01:00.0: PM: pci_pm_resume_noirq+0x0/0x190
returned 0 after 329 usecs
2. pcieport stays at D0 due to power_manageable returns false:
(suspend)
[ 52.435375] mt7921e 0000:01:00.0: PM: calling
pci_pm_suspend_noirq+0x0/0x300 @ 2040, parent: 0000:00:00.0
[ 52.465235] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
[ 52.471610] mt7921e 0000:01:00.0: PM:
pci_pm_suspend_noirq+0x0/0x300 returned 0 after 26602 usecs
[ 52.480674] pcieport 0000:00:00.0: PM: calling
pci_pm_suspend_noirq+0x0/0x300 @ 143, parent: pci0000:00
[ 52.529876] pcieport 0000:00:00.0: PCI PM: Suspend power state: D0
<-- port is still D0
[ 52.536056] pcieport 0000:00:00.0: PCI PM: Skipped
(resume)
[ 56.026298] pcieport 0000:00:00.0: PM: calling
pci_pm_resume_noirq+0x0/0x190 @ 3243, parent: pci0000:00
[ 56.035379] mtk-pcie-phy 16930000.phy: CKM_38=0x13040500,
GLB_20=0x0, GLB_30=0x0, GLB_38=0x30453fc, GLB_F4=0x1453b000
[ 56.044776] pcieport 0000:00:00.0: PM:
pci_pm_resume_noirq+0x0/0x190 returned 0 after 13 usecs
[ 56.055409] mtk-pcie-phy 16930000.phy: No calibration info
[ 56.064098] mt7921e 0000:01:00.0: PM: calling
pci_pm_resume_noirq+0x0/0x190 @ 3244, parent: 0000:00:00.0
[ 56.078962] mt7921e 0000:01:00.0: Unable to change power state from
D3hot to D0, device inaccessible <-- resume failed.
[ 56.152363] mt7921e 0000:01:00.0: PM: pci_pm_resume_noirq+0x0/0x190
returned 0 after 73406 usecs
> And resuming with D0.
>
> Problem with D3Cold is, it is a power off state. If a driver wants a device/port
> to enter D3Cold, then it needs to know the power supply. Otherwise, techinically
> the driver cannot put the device into D3Cold. And there is no power supply
> exposed in DT for most of the rootports/bridges.
>
I think maybe it can be fixed in the power_manageable checking? Since
suspend flow still considers this state. If this returns false due to
being unable to D3cold, pcieport still stays at D0 and would cause
device resume failure.
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்
