Re: [PATCH v4 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
>
> --
> மணிவண்ணன் சதாசிவம்





[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux