Re: [PATCH v2 3/6] PCI/PM: Split out code from pci_pm_suspend_noirq() into helper

[Rafael J. Wysocki]

On Mon, Apr 27, 2026 at 10:50 PM Mario Limonciello (AMD)
<superm1@xxxxxxxxxx> wrote:
>
> In order to unify suspend and hibernate codepaths without code duplication
> the common code should be in common helpers.  Move it from
> pci_pm_suspend_noirq() into a helper.  No intended functional changes.
>
> Tested-by: Eric Naim <dnaim@xxxxxxxxxxx>
> Signed-off-by: Mario Limonciello (AMD) <superm1@xxxxxxxxxx>
> ---
>  drivers/pci/pci-driver.c | 86 +++++++++++++++++++++++++---------------
>  1 file changed, 54 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index a43ee7bbfb3f5..bfb521eb0eed7 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -788,6 +788,54 @@ static void pci_pm_complete(struct device *dev)
>
>  #endif /* !CONFIG_PM_SLEEP */
>
> +#if defined(CONFIG_SUSPEND)
> +/**
> + * pci_pm_suspend_noirq_common
> + * @pci_dev: pci device
> + * @skip_bus_pm: pointer to a boolean indicating whether to skip bus PM
> + *
> + * Prepare the device to go into a low power state by saving state and
> + * deciding whether to skip bus PM.
> + *
> + */
> +static void pci_pm_suspend_noirq_common(struct pci_dev *pci_dev, bool *skip_bus_pm)
> +{
> +       if (!pci_dev->state_saved) {
> +               pci_save_state(pci_dev);
> +
> +               /*
> +                * If the device is a bridge with a child in D0 below it,
> +                * it needs to stay in D0, so check skip_bus_pm to avoid
> +                * putting it into a low-power state in that case.
> +                */
> +               if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
> +                       pci_prepare_to_sleep(pci_dev);
> +       }
> +
> +       pci_dbg(pci_dev, "PCI PM: Sleep power state: %s\n",
> +               pci_power_name(pci_dev->current_state));
> +
> +       if (pci_dev->current_state == PCI_D0) {
> +               pci_dev->skip_bus_pm = true;
> +               /*
> +                * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
> +                * downstream device is in D0, so avoid changing the power state
> +                * of the parent bridge by setting the skip_bus_pm flag for it.
> +                */
> +               if (pci_dev->bus->self)
> +                       pci_dev->bus->self->skip_bus_pm = true;
> +       }
> +
> +       if (pci_dev->skip_bus_pm && pm_suspend_no_platform()) {
> +               pci_dbg(pci_dev, "PCI PM: Skipped\n");
> +               *skip_bus_pm = true;
> +               return;
> +       }
> +
> +       pci_pm_set_unknown_state(pci_dev);

Can this go back to pci_pm_suspend_noirq()?  It wouldn't be necessary
to change labels then.

> +}
> +#endif /* CONFIG_SUSPEND */
> +
>  #ifdef CONFIG_SUSPEND
>  static void pcie_pme_root_status_cleanup(struct pci_dev *pci_dev)
>  {
> @@ -877,6 +925,7 @@ static int pci_pm_suspend_noirq(struct device *dev)
>  {
>         struct pci_dev *pci_dev = to_pci_dev(dev);
>         const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       bool skip_bus_pm = false;
>
>         if (dev_pm_skip_suspend(dev))
>                 return 0;
> @@ -886,7 +935,8 @@ static int pci_pm_suspend_noirq(struct device *dev)
>
>         if (!pm) {
>                 pci_save_state(pci_dev);
> -               goto set_unknown;
> +               pci_pm_set_unknown_state(pci_dev);
> +               goto Ehci_workaround;
>         }
>
>         if (pm->suspend_noirq) {
> @@ -907,40 +957,12 @@ static int pci_pm_suspend_noirq(struct device *dev)
>                 }
>         }
>
> -       if (!pci_dev->state_saved) {
> -               pci_save_state(pci_dev);
> -
> -               /*
> -                * If the device is a bridge with a child in D0 below it,
> -                * it needs to stay in D0, so check skip_bus_pm to avoid
> -                * putting it into a low-power state in that case.
> -                */
> -               if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
> -                       pci_prepare_to_sleep(pci_dev);
> -       }
> -
> -       pci_dbg(pci_dev, "PCI PM: Suspend power state: %s\n",
> -               pci_power_name(pci_dev->current_state));
> +       pci_pm_suspend_noirq_common(pci_dev, &skip_bus_pm);
>
> -       if (pci_dev->current_state == PCI_D0) {
> -               pci_dev->skip_bus_pm = true;
> -               /*
> -                * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
> -                * downstream device is in D0, so avoid changing the power state
> -                * of the parent bridge by setting the skip_bus_pm flag for it.
> -                */
> -               if (pci_dev->bus->self)
> -                       pci_dev->bus->self->skip_bus_pm = true;
> -       }
> -
> -       if (pci_dev->skip_bus_pm && pm_suspend_no_platform()) {
> -               pci_dbg(pci_dev, "PCI PM: Skipped\n");
> +       if (skip_bus_pm)
>                 goto Fixup;
> -       }
> -
> -set_unknown:
> -       pci_pm_set_unknown_state(pci_dev);
>
> +Ehci_workaround:
>         /*
>          * Some BIOSes from ASUS have a bug: If a USB EHCI host controller's
>          * PCI COMMAND register isn't 0, the BIOS assumes that the controller
> --
> 2.53.0
>