[PATCH 0/4] PCI/PM: Always disable PTM for all devices during

[PATCH 0/4] PCI/PM: Always disable PTM for all devices during

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

We currently disable PTM for Root Ports during suspend.  Leaving PTM
enabled for downstream devices causes UR errors if they send PTM Requests.
The intent of this series is to:

  - Unconditionally disable PTM during suspend (even if the driver saves
    its own state) by moving the disable from pci_prepare_to_sleep() to
    pci_pm_suspend().

  - Disable PTM for all devices by removing the Root Port condition.

  - Manually set PTM Enable when restoring PTM state because suspend saves
    the PTM state *after* disabling PTM.

This series is intended to replace Rajvi's second patch, so we would end
up with this:

  Rajvi  PCI/PM: Simplify pci_pm_suspend_noirq()
  Bjorn  PCI/PTM: Preserve PTM Root Select
  Bjorn  PCI/PTM: Enable PTM when restoring state
  Bjorn  PCI/PM: Always disable PTM for all devices during suspend
  Bjorn  PCI/PTM: Cache PTM Capability offset

Please comment!

Bjorn Helgaas (4):
  PCI/PTM: Preserve PTM Root Select
  PCI/PTM: Enable PTM when restoring state
  PCI/PM: Always disable PTM for all devices during suspend
  PCI/PTM: Cache PTM Capability offset

 drivers/pci/pci-driver.c |  8 ++++++
 drivers/pci/pci.c        | 20 --------------
 drivers/pci/pcie/ptm.c   | 56 +++++++++++++++++++---------------------
 include/linux/pci.h      |  1 +
 4 files changed, 35 insertions(+), 50 deletions(-)

-- 
2.25.1

[PATCH 1/4] PCI/PTM: Preserve PTM Root Select

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

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@google.com>
Cc: David E. Box <david.e.box@linux.intel.com>
---
 drivers/pci/pcie/ptm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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);
 }
 
-- 
2.25.1

[PATCH 2/4] PCI/PTM: Enable PTM when restoring state

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

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@google.com>
---
 drivers/pci/pcie/ptm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

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);
 }
 
-- 
2.25.1

[PATCH 3/4] PCI/PM: Always disable PTM for all devices during suspend

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

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.  PCIe r6.0,
sec 2.2.8, recommends that functions support generation of messages in
non-D0 states, so we have to 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.

Here's a sample of errors that occur when PTM is disabled only on the Root
Port.  With this topology:

  0000:00:1d.0 Root Port            to [bus 08-71]
  0000:08:00.0 Switch Upstream Port to [bus 09-71]

Kai-Heng reported errors like this:

  pcieport 0000:00:1d.0: AER: Uncorrected (Non-Fatal) error received: 0000:00:1d.0
  pcieport 0000:00:1d.0: PCIe Bus Error: severity=Uncorrected (Non-Fatal), type=Transaction Layer, (Requester ID)
  pcieport 0000:00:1d.0:   device [8086:7ab0] error status/mask=00100000/00004000
  pcieport 0000:00:1d.0:    [20] UnsupReq               (First)
  pcieport 0000:00:1d.0: AER:   TLP Header: 34000000 08000052 00000000 00000000

Decoding TLP header 0x34...... (0011 0100b) and 0x08000052:

  Fmt                         001b  4 DW header, no data
  Type                     1 0100b  Msg (Local - Terminate at Receiver)
  Requester ID  0x0800              Bus 08 Devfn 00.0
  Message Code    0x52  0101 0010b  PTM Request

The 00:1d.0 Root Port logged an Unsupported Request error when it received
a PTM Request with Requester ID 08:00.0.

Fixes: a697f072f5da ("PCI: Disable PTM during suspend to save power")
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215453
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216210
Based-on: https://lore.kernel.org/r/20220706123244.18056-1-kai.heng.feng@canonical.com
Based-on-patch-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci-driver.c |  8 ++++++++
 drivers/pci/pci.c        | 20 --------------------
 2 files changed, 8 insertions(+), 20 deletions(-)

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);
-- 
2.25.1

[PATCH 4/4] PCI/PTM: Cache PTM Capability offset

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Cache the PTM Capability offset in struct pci_dev so we don't have to
search for it every time we enable/disable/save/restore.  No functional
change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/ptm.c | 46 ++++++++++++++++--------------------------
 include/linux/pci.h    |  1 +
 2 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
index 3115601a85ef..8f38ba7b386c 100644
--- a/drivers/pci/pcie/ptm.c
+++ b/drivers/pci/pcie/ptm.c
@@ -31,14 +31,10 @@ static void pci_ptm_info(struct pci_dev *dev)
 
 void pci_disable_ptm(struct pci_dev *dev)
 {
-	int ptm;
+	u16 ptm = dev->ptm_cap;
 	u16 ctrl;
 
-	if (!pci_is_pcie(dev))
-		return;
-
-	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
-	if (!ptm)
+	if (!ptm || !dev->ptm_enabled)
 		return;
 
 	pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl);
@@ -48,14 +44,10 @@ void pci_disable_ptm(struct pci_dev *dev)
 
 void pci_save_ptm_state(struct pci_dev *dev)
 {
-	int ptm;
+	u16 ptm = dev->ptm_cap;
 	struct pci_cap_saved_state *save_state;
 	u16 *cap;
 
-	if (!pci_is_pcie(dev))
-		return;
-
-	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
 	if (!ptm)
 		return;
 
@@ -69,16 +61,15 @@ void pci_save_ptm_state(struct pci_dev *dev)
 
 void pci_restore_ptm_state(struct pci_dev *dev)
 {
+	u16 ptm = dev->ptm_cap;
 	struct pci_cap_saved_state *save_state;
-	int ptm;
 	u16 *cap;
 
-	if (!pci_is_pcie(dev))
+	if (!ptm)
 		return;
 
 	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_PTM);
-	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
-	if (!save_state || !ptm)
+	if (!save_state)
 		return;
 
 	cap = (u16 *)&save_state->cap.data[0];
@@ -95,10 +86,10 @@ void pci_restore_ptm_state(struct pci_dev *dev)
 
 void pci_ptm_init(struct pci_dev *dev)
 {
-	int pos;
+	struct pci_dev *ups;
+	u16 ptm;
 	u32 cap, ctrl;
 	u8 local_clock;
-	struct pci_dev *ups;
 
 	if (!pci_is_pcie(dev))
 		return;
@@ -125,13 +116,14 @@ void pci_ptm_init(struct pci_dev *dev)
 		return;
 	}
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
-	if (!pos)
+	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
+	dev->ptm_cap = ptm;
+	if (!ptm)
 		return;
 
 	pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_PTM, sizeof(u16));
 
-	pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
+	pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
 	local_clock = (cap & PCI_PTM_GRANULARITY_MASK) >> 8;
 
 	/*
@@ -156,7 +148,7 @@ void pci_ptm_init(struct pci_dev *dev)
 	}
 
 	ctrl |= dev->ptm_granularity << 8;
-	pci_write_config_dword(dev, pos + PCI_PTM_CTRL, ctrl);
+	pci_write_config_dword(dev, ptm + PCI_PTM_CTRL, ctrl);
 	dev->ptm_enabled = 1;
 
 	pci_ptm_info(dev);
@@ -164,18 +156,14 @@ void pci_ptm_init(struct pci_dev *dev)
 
 int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 {
-	int pos;
+	u16 ptm = dev->ptm_cap;
 	u32 cap, ctrl;
 	struct pci_dev *ups;
 
-	if (!pci_is_pcie(dev))
+	if (!ptm)
 		return -EINVAL;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
-	if (!pos)
-		return -EINVAL;
-
-	pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
+	pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
 	if (!(cap & PCI_PTM_CAP_REQ))
 		return -EINVAL;
 
@@ -200,7 +188,7 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 
 	ctrl = PCI_PTM_CTRL_ENABLE;
 	ctrl |= dev->ptm_granularity << 8;
-	pci_write_config_dword(dev, pos + PCI_PTM_CTRL, ctrl);
+	pci_write_config_dword(dev, ptm + PCI_PTM_CTRL, ctrl);
 	dev->ptm_enabled = 1;
 
 	pci_ptm_info(dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 060af91bafcd..f6c162d06bff 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -475,6 +475,7 @@ struct pci_dev {
 	unsigned int	broken_cmd_compl:1;	/* No compl for some cmds */
 #endif
 #ifdef CONFIG_PCIE_PTM
+	u16		ptm_cap;		/* PTM Capability offset */
 	unsigned int	ptm_root:1;
 	unsigned int	ptm_enabled:1;
 	u8		ptm_granularity;
-- 
2.25.1

Re: [PATCH 1/4] PCI/PTM: Preserve PTM Root Select

[Sathyanarayanan Kuppuswamy]

On 9/2/22 7:58 AM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> 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.

Currently, it looks like we disable PCI_PTM_CTRL_ROOT in pci_disable_ptm(),
but not enable it in pci_enable_ptm(). Do you know this did not trigger an
issue?

Also, you mentioned that it is complicated to enable it, can you add some
details?


> 
> 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@google.com>
> Cc: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/pci/pcie/ptm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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);
>  }
>  

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 2/4] PCI/PTM: Enable PTM when restoring state

[Sathyanarayanan Kuppuswamy]

On 9/2/22 7:58 AM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> 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 suspend is disabling PTM separately, why not enable it during the resume
operation? Why club it with PTM state restoration?

> 
> 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@google.com>
> ---
>  drivers/pci/pcie/ptm.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> 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);
>  }
>  

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 1/4] PCI/PTM: Preserve PTM Root Select

[Bjorn Helgaas]

On Fri, Sep 02, 2022 at 10:24:05AM -0700, Sathyanarayanan Kuppuswamy wrote:
> On 9/2/22 7:58 AM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > 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.
> 
> Currently, it looks like we disable PCI_PTM_CTRL_ROOT in pci_disable_ptm(),
> but not enable it in pci_enable_ptm(). Do you know this did not trigger an
> issue?

For Root Ports and Switches, we enable PTM (and set Root Select when
appropriate) during enumeration in pci_ptm_init().  This is based on
the assumption that enabling PTM in Root Ports and Switches is a no-op
unless there's an Endpoint that generates PTM Requests.  (It turns out
that's not quite true, because Kai-Heng's bug report [1] shows the
08:00.0 Switch sending PTM Requests even though no Endpoint even has a
PTM Capability.)

If we didn't enable PTM in Root Ports and Switches during enumeration,
we'd have to walk the whole path and enable them when enabling PTM for
an Endpoint.

pci_enable_ptm() currently only works for Endpoints, which cannot be
PTM Roots, so it never has to set PCI_PTM_CTRL_ROOT.

If we clear PCI_PTM_CTRL_ROOT in pci_disable_ptm(), it will never get
set again unless we re-enumerate the Root Port.

Thanks for asking this, because it reminds me why I didn't add
pci_enable_ptm() calls in the resume paths!  That would make them
parallel with the suspend paths, which would definitely be nice.  But
we would have to rework pci_enable_ptm() to work for Root Ports and
Switch Ports as well.  I think we *could* do that.  What do you think?

Regardless of that question, I think it's unnecessary to clear
PCI_PTM_CTRL_ROOT in pci_disable_ptm(), so we should leave it alone.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=215453

> Also, you mentioned that it is complicated to enable it, can you add some
> details?
> 
> > 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@google.com>
> > Cc: David E. Box <david.e.box@linux.intel.com>
> > ---
> >  drivers/pci/pcie/ptm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 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);
> >  }
> >  
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer

Re: [PATCH 2/4] PCI/PTM: Enable PTM when restoring state

[Bjorn Helgaas]

On Fri, Sep 02, 2022 at 10:25:54AM -0700, Sathyanarayanan Kuppuswamy wrote:
> On 9/2/22 7:58 AM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > 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 suspend is disabling PTM separately, why not enable it during the resume
> operation? Why club it with PTM state restoration?

The long answer is in my previous reply [1].  The short answer is that
pci_enable_ptm() only works with Endpoints, so if we enable PTM in the
resume path, we need to rework it to handle Root Ports and Switch
Ports as well.

[1] https://lore.kernel.org/r/20220902203848.GA370638@bhelgaas

> > 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@google.com>
> > ---
> >  drivers/pci/pcie/ptm.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > 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);
> >  }
> >  
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer

Re: [PATCH 1/4] PCI/PTM: Preserve PTM Root Select

[Sathyanarayanan Kuppuswamy]

Hi Bjorn,

On 9/2/22 1:38 PM, Bjorn Helgaas wrote:
> On Fri, Sep 02, 2022 at 10:24:05AM -0700, Sathyanarayanan Kuppuswamy wrote:
>> On 9/2/22 7:58 AM, Bjorn Helgaas wrote:
>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> 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.
>>
>> Currently, it looks like we disable PCI_PTM_CTRL_ROOT in pci_disable_ptm(),
>> but not enable it in pci_enable_ptm(). Do you know this did not trigger an
>> issue?
> 
> For Root Ports and Switches, we enable PTM (and set Root Select when
> appropriate) during enumeration in pci_ptm_init().  This is based on
> the assumption that enabling PTM in Root Ports and Switches is a no-op
> unless there's an Endpoint that generates PTM Requests.  (It turns out
> that's not quite true, because Kai-Heng's bug report [1] shows the
> 08:00.0 Switch sending PTM Requests even though no Endpoint even has a
> PTM Capability.)
> 
> If we didn't enable PTM in Root Ports and Switches during enumeration,
> we'd have to walk the whole path and enable them when enabling PTM for
> an Endpoint.
> 
> pci_enable_ptm() currently only works for Endpoints, which cannot be
> PTM Roots, so it never has to set PCI_PTM_CTRL_ROOT.
> 
> If we clear PCI_PTM_CTRL_ROOT in pci_disable_ptm(), it will never get
> set again unless we re-enumerate the Root Port.

Thanks for clarifying.

> 
> Thanks for asking this, because it reminds me why I didn't add
> pci_enable_ptm() calls in the resume paths!  That would make them
> parallel with the suspend paths, which would definitely be nice.  But
> we would have to rework pci_enable_ptm() to work for Root Ports and
> Switch Ports as well.  I think we *could* do that.  What do you think?

IMO, the code will look better if we keep the suspend and resume paths in
sync. Since we are calling pci_disable_ptm() in suspend path, it makes
sense to call pci_enable_ptm() in resume path.

Making the pci_enable_ptm() handle root and upstream ports should not
be very complicated, right?

> 
> Regardless of that question, I think it's unnecessary to clear
> PCI_PTM_CTRL_ROOT in pci_disable_ptm(), so we should leave it alone.

I agree with you. We should not touch PCI_PTM_CTRL_ROOT in pci_disable_ptm().

> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=215453
> 
>> Also, you mentioned that it is complicated to enable it, can you add some
>> details?
>>
>>> 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@google.com>
>>> Cc: David E. Box <david.e.box@linux.intel.com>
>>> ---
>>>  drivers/pci/pcie/ptm.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> 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);
>>>  }
>>>  
>>
>> -- 
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 1/4] PCI/PTM: Preserve PTM Root Select

[Bjorn Helgaas]

On Fri, Sep 02, 2022 at 02:11:12PM -0700, Sathyanarayanan Kuppuswamy wrote:
> On 9/2/22 1:38 PM, Bjorn Helgaas wrote:
> > On Fri, Sep 02, 2022 at 10:24:05AM -0700, Sathyanarayanan Kuppuswamy wrote:
> >> On 9/2/22 7:58 AM, Bjorn Helgaas wrote:
> >>> From: Bjorn Helgaas <bhelgaas@google.com>
> >>>
> >>> 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.
> >>
> >> Currently, it looks like we disable PCI_PTM_CTRL_ROOT in pci_disable_ptm(),
> >> but not enable it in pci_enable_ptm(). Do you know this did not trigger an
> >> issue?
> ...

> > Thanks for asking this, because it reminds me why I didn't add
> > pci_enable_ptm() calls in the resume paths!  That would make them
> > parallel with the suspend paths, which would definitely be nice.  But
> > we would have to rework pci_enable_ptm() to work for Root Ports and
> > Switch Ports as well.  I think we *could* do that.  What do you think?
> 
> IMO, the code will look better if we keep the suspend and resume paths in
> sync. Since we are calling pci_disable_ptm() in suspend path, it makes
> sense to call pci_enable_ptm() in resume path.
> 
> Making the pci_enable_ptm() handle root and upstream ports should not
> be very complicated, right?

I took a stab at it.  pci_enable_ptm() is getting kind of ugly, but
maybe it's better overall.  I'll post it and you can see what you
think.