On Tue, Sep 9, 2025 at 9:16 PM Mario Limonciello (AMD) <supe...@kernel.org> 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.
You should say why you need/want to unify the suspend and hibernate codepaths because this is kind of relevant for this patch. It isn't entirely valid to use suspend-specific code in the hibernate/power down code paths. Also, I'd consider reordering the series so that this patch goes immediately before patch [9/12] in which the new function is used for the first time. > Tested-by: Eric Naim <dn...@cachyos.org> > Signed-off-by: Mario Limonciello (AMD) <supe...@kernel.org> > --- > v7: > * Reword title > v5: > * Split from earlier patches > * Add tags > v4: > * > https://lore.kernel.org/linux-pci/20250616175019.3471583-1-supe...@kernel.org/ > --- > drivers/pci/pci-driver.c | 81 +++++++++++++++++++++++++--------------- > 1 file changed, 51 insertions(+), 30 deletions(-) > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > index f201d298d7173..571a3809f163a 100644 > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -762,6 +762,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) I guess you want to propagate pci_dev->skip_bus_pm to the parent bridge if set for pci_dev in the hibernation code path so that bridges can go into D3hot/cold in that case. Fair enough, but I'd give the common function a somewhat less neutral name, like for example pci_pm_noirq_prepare_to_sleep(). > +{ > + 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()) { And pm_suspend_no_platform() is suspend-specific and in the hibernation/power down cases it is always false because the platform is going to take over. This means that *skip_bus_pm will not be updated in those cases, so why do you need it in this function in the first place? > + pci_dbg(pci_dev, "PCI PM: Skipped\n"); > + *skip_bus_pm = true; > + return; > + } > + > + pci_pm_set_unknown_state(pci_dev); So this should stay outside the common part I think. > +} > +#endif /* CONFIG_SUSPEND */ > + > #ifdef CONFIG_SUSPEND > static void pcie_pme_root_status_cleanup(struct pci_dev *pci_dev) > { > @@ -851,6 +899,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; And it doesn't look like this variable is necessary after all. > > if (dev_pm_skip_suspend(dev)) > return 0; > @@ -881,38 +930,10 @@ 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; > - } > - > - pci_pm_set_unknown_state(pci_dev); > > /* > * Some BIOSes from ASUS have a bug: If a USB EHCI host controller's > --
