On Thu, Sep 1, 2022 at 11:26 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> On Wed, Aug 31, 2022 at 07:53:05PM +0200, Rafael J. Wysocki wrote:
> > ...
>
> > In the meantime, I recalled that nvme wanted to leave the device in D0
> > and program it into an internal low-power state in some cases which
> > would be disturbed by disabling PTM later on (a config space write
> > would kick the device out of the internal low-power state).
> >
> > So it looks like it would be better to disable PTM as the first thing
> > in pci_pm_suspend() before calling the driver's suspend callback
> > (which may be nvme suspend),
>
> Yes, I was thinking the same thing. There's no reason we need to wait
> until interrupts are disabled to disable PTM.
>
> > but then we'd need to save the original PTM status and restore it
> > during the subsequent resume. That could be done as early as in
> > pci_pm_resume_noirq(), but I think the cleanest way would be to add
> > a new bit to struct pci_device for that.
>
> > Alternatively, we can drop the $subject patch, so ptm_enabled still
> > only means that it has been enabled during enumeration and it can be
> > used to restore the original PTM status during resume.
>
> I like this second idea of dropping this "PCI/PTM: fix to maintain
> pci_dev->ptm_enabled" patch and using "dev->ptm_enabled" to set the
> PTM Enable bit on restore, as in the patches below. Then we don't
> need to do anything explicit to re-enable PTM.
>
> If this makes sense, I'll add a few cleanups on top and post as a
> formal series.
It does to me.
Thanks for taking care of this!
> commit 73690aa361a7 ("PCI/PM: Always disable PTM for all devices during suspend")
> Author: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Date: Thu Sep 1 16:14:45 2022 -0500
>
> PCI/PM: Always disable PTM for all devices during suspend
>
> We want to disable PTM on Root Ports because that allows some chips, e.g.,
> Intel mobile chips since Coffee Lake, to enter a lower-power PM state.
>
> That means we also have to disable PTM on downstream devices because PCIe
> r6.0, sec 2.2.8, strongly recommends that functions support generation of
> messages in non-D0 states, so we assume Switch Upstream Ports or Endpoints
> may send PTM Requests while in D1, D2, and D3hot. A PTM message received
> by a Downstream Port (including a Root Port) with PTM disabled must be
> treated as an Unsupported Request (sec 6.21.3).
>
> PTM was previously disabled only for Root Ports, and it was disabled in
> pci_prepare_to_sleep(), which is not called at all if a driver supports
> legacy PM or does its own state saving.
>
> Instead, disable PTM early in pci_pm_suspend() and pci_pm_runtime_suspend()
> so we do it in all cases.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 2815922ac525..f07399a94807 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -772,6 +772,12 @@ static int pci_pm_suspend(struct device *dev)
> struct pci_dev *pci_dev = to_pci_dev(dev);
> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>
> + /*
> + * Disabling PTM allows some systems, e.g., Intel mobile chips
> + * since Coffee Lake, to enter a lower-power PM state.
> + */
> + pci_disable_ptm(pci_dev);
> +
> pci_dev->skip_bus_pm = false;
>
> if (pci_has_legacy_pm_support(pci_dev))
> @@ -1269,6 +1275,8 @@ static int pci_pm_runtime_suspend(struct device *dev)
> pci_power_t prev = pci_dev->current_state;
> int error;
>
> + pci_disable_ptm(pci_dev);
> +
> /*
> * If pci_dev->driver is not set (unbound), we leave the device in D0,
> * but it may go to D3cold when the bridge above it runtime suspends.
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 95bc329e74c0..b0e2968c8cca 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2706,16 +2706,6 @@ int pci_prepare_to_sleep(struct pci_dev *dev)
> if (target_state == PCI_POWER_ERROR)
> return -EIO;
>
> - /*
> - * There are systems (for example, Intel mobile chips since Coffee
> - * Lake) where the power drawn while suspended can be significantly
> - * reduced by disabling PTM on PCIe root ports as this allows the
> - * port to enter a lower-power PM state and the SoC to reach a
> - * lower-power idle state as a whole.
> - */
> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
> - pci_disable_ptm(dev);
> -
> pci_enable_wake(dev, target_state, wakeup);
>
> error = pci_set_power_state(dev, target_state);
> @@ -2764,16 +2754,6 @@ int pci_finish_runtime_suspend(struct pci_dev *dev)
> if (target_state == PCI_POWER_ERROR)
> return -EIO;
>
> - /*
> - * There are systems (for example, Intel mobile chips since Coffee
> - * Lake) where the power drawn while suspended can be significantly
> - * reduced by disabling PTM on PCIe root ports as this allows the
> - * port to enter a lower-power PM state and the SoC to reach a
> - * lower-power idle state as a whole.
> - */
> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
> - pci_disable_ptm(dev);
> -
> __pci_enable_wake(dev, target_state, pci_dev_run_wake(dev));
>
> error = pci_set_power_state(dev, target_state);
>
> commit f84a7e954e37 ("PCI/PTM: Enable PTM when restoring state")
> Author: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Date: Thu Sep 1 15:51:23 2022 -0500
>
> PCI/PTM: Enable PTM when restoring state
>
> The suspend path may disable PTM before saving config state, which means
> the PCI_PTM_CTRL_ENABLE bit in the saved state may be cleared even though
> we want PTM to be enabled when resuming.
>
> If "dev->ptm_enabled" is set, it means PTM should be enabled, so make sure
> PCI_PTM_CTRL_ENABLE is set when restoring the PTM state.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>
> diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> index b6a417247ce3..3115601a85ef 100644
> --- a/drivers/pci/pcie/ptm.c
> +++ b/drivers/pci/pcie/ptm.c
> @@ -82,6 +82,14 @@ void pci_restore_ptm_state(struct pci_dev *dev)
> return;
>
> cap = (u16 *)&save_state->cap.data[0];
> +
> + /*
> + * The suspend path may disable PTM before saving config state.
> + * Make sure PCI_PTM_CTRL_ENABLE is set if PTM should be enabled.
> + */
> + if (dev->ptm_enabled)
> + *cap |= PCI_PTM_CTRL_ENABLE;
> +
> pci_write_config_word(dev, ptm + PCI_PTM_CTRL, *cap);
> }
>
>
> commit 1d7d32a35df0 ("PCI/PTM: Preserve PTM Root Select")
> Author: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Date: Thu Sep 1 15:54:15 2022 -0500
>
> PCI/PTM: Preserve PTM Root Select
>
> When disabling PTM, there's no need to clear the Root Select bit. We
> disable PTM during suspend, and we want to re-enable it during resume.
> Clearing Root Select here makes re-enabling more complicated.
>
> Per PCIe r6.0, sec 7.9.15.3, "When set, if the PTM Enable bit is also Set,
> this Time Source is the PTM Root," so if PTM Enable is cleared, the value
> of Root Select should be irrelevant.
>
> Preserve Root Select to simplify re-enabling PTM.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Cc: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
>
> diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> index 368a254e3124..b6a417247ce3 100644
> --- a/drivers/pci/pcie/ptm.c
> +++ b/drivers/pci/pcie/ptm.c
> @@ -42,7 +42,7 @@ void pci_disable_ptm(struct pci_dev *dev)
> return;
>
> pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl);
> - ctrl &= ~(PCI_PTM_CTRL_ENABLE | PCI_PTM_CTRL_ROOT);
> + ctrl &= ~PCI_PTM_CTRL_ENABLE;
> pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
> }
>
