[PATCH v6 1/2] PCI/PM: refactor pci_pm_suspend_noirq()

[PATCH v6 1/2] PCI/PM: refactor pci_pm_suspend_noirq()

[Rajvi Jingar]

The state of the device is saved during pci_pm_suspend_noirq(), if it
has not already been saved, regardless of the skip_bus_pm flag value. So
skip_bus_pm check is removed before saving the device state.

Signed-off-by: Rajvi Jingar <rajvi.jingar@linux.intel.com>
Suggested-by: David E. Box <david.e.box@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 v1 -> v2: add comments to the changes
 v2 -> v3: move changelog after "---" marker
 v3 -> v4: add "---" marker after changelog
 v4 -> v5: no change
 v5 -> v6: no change
---
 drivers/pci/pci-driver.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 49238ddd39ee..1f64de3e5280 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -867,20 +867,14 @@ static int pci_pm_suspend_noirq(struct device *dev)
 		}
 	}
 
-	if (pci_dev->skip_bus_pm) {
+	if (!pci_dev->state_saved) {
+		pci_save_state(pci_dev);
 		/*
-		 * Either the device is a bridge with a child in D0 below it, or
-		 * the function is running for the second time in a row without
-		 * going through full resume, which is possible only during
-		 * suspend-to-idle in a spurious wakeup case.  The device should
-		 * be in D0 at this point, but if it is a bridge, it may be
-		 * necessary to save its state.
+		 * 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->state_saved)
-			pci_save_state(pci_dev);
-	} else if (!pci_dev->state_saved) {
-		pci_save_state(pci_dev);
-		if (pci_power_manageable(pci_dev))
+		if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
 			pci_prepare_to_sleep(pci_dev);
 	}
 
-- 
2.25.1

[PATCH v6 2/2] PCI/PM: disable PTM on all devices

[Rajvi Jingar]

On receiving a PTM Request from a downstream device, if PTM is disabled
on the root port, as per PCIe specification, such request would cause
an Unsupported Request error. So disable PTM for any downstream devices.
PTM state needs to be saved before disabling it to be restored later.

set ptm_enabled from 'struct pci_dev' to 0 in pci_ptm_disable() and
it is used in pci_save_state() before saving PTM state to avoid
double save.

Fixes: a697f072f5da ("PCI: Disable PTM during suspend to save power")
Signed-off-by: Rajvi Jingar <rajvi.jingar@linux.intel.com>
Suggested-by: David E. Box <david.e.box@linux.intel.com>
---
 v1 -> v2: add Fixes tag in commit message
 v2 -> v3: move changelog after "---" marker
 v3 -> v4: add "---" marker after changelog
 v4 -> v5: move pci_disable_ptm() out of the pci_dev->state_saved check.
	   disable PTM for all devices, not just root ports.
 v5 -> v6: move pci_disable_ptm() to pci_pm_suspend()
	   set pci_dev->ptm_enabled to 0 in pci_ptm_disable() and it is
	   used in pci_save_state() before saving PTM state to avoid
	   double save.
---
 drivers/pci/pci-driver.c | 21 ++++++++++++++++++++-
 drivers/pci/pci.c        | 26 +++++++++++---------------
 drivers/pci/pcie/ptm.c   |  1 +
 3 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 1f64de3e5280..db4d7835d7ae 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -803,14 +803,33 @@ static int pci_pm_suspend(struct device *dev)
 		pci_dev_adjust_pme(pci_dev);
 	}
 
+	/*
+	 * If a PTM Requester is put in a low-power state, a PTM Responder
+	 * upstream from it may also be put in a low-power state. Putting a
+	 * Port in D1, D2, or D3hot does not prohibit it from sending or
+	 * responding to PTM Requests. We want to disable PTM on Responders
+	 * when they are in a low-power state. Per 6.21.3, a PTM Requester
+	 * must not be enabled when the upstream PTM Responder is disabled.
+	 * Therefore, we must disable all PTM on all downstream PTM
+	 * Requesters before disabling it on the PTM Responder, e.g., a Root
+	 * Port.
+	 *
+	 * Also, to restore the PTM state, it needs to be saved before
+	 * disabling it for all devices.
+	 */
+	pci_save_ptm_state(pci_dev);
+	pci_disable_ptm(pci_dev);
+
 	if (pm->suspend) {
 		pci_power_t prev = pci_dev->current_state;
 		int error;
 
 		error = pm->suspend(dev);
 		suspend_report_result(dev, pm->suspend, error);
-		if (error)
+		if (error) {
+			pci_restore_ptm_state(pci_dev);
 			return error;
+		}
 
 		if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
 		    && pci_dev->current_state != PCI_UNKNOWN) {
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index cfaf40a540a8..0df9b783621e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1669,7 +1669,13 @@ int pci_save_state(struct pci_dev *dev)
 	pci_save_ltr_state(dev);
 	pci_save_dpc_state(dev);
 	pci_save_aer_state(dev);
-	pci_save_ptm_state(dev);
+	/*
+	 * PCI PM core disables PTM during suspend and saves PTM state before
+	 * that to be able to restore the ptm state restored later. So PCI core
+	 * needs this check to avoid double save.
+	 */
+	if (dev->ptm_enabled)
+		pci_save_ptm_state(dev);
 	return pci_save_vc_state(dev);
 }
 EXPORT_SYMBOL(pci_save_state);
@@ -2710,24 +2716,12 @@ 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);
 
-	if (error) {
+	if (error)
 		pci_enable_wake(dev, target_state, false);
-		pci_restore_ptm_state(dev);
-	}
 
 	return error;
 }
@@ -2775,8 +2769,10 @@ int pci_finish_runtime_suspend(struct pci_dev *dev)
 	 * 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)
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
+		pci_save_ptm_state(dev);
 		pci_disable_ptm(dev);
+	}
 
 	__pci_enable_wake(dev, target_state, pci_dev_run_wake(dev));
 
diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
index 368a254e3124..746e29779c27 100644
--- a/drivers/pci/pcie/ptm.c
+++ b/drivers/pci/pcie/ptm.c
@@ -44,6 +44,7 @@ void pci_disable_ptm(struct pci_dev *dev)
 	pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl);
 	ctrl &= ~(PCI_PTM_CTRL_ENABLE | PCI_PTM_CTRL_ROOT);
 	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
+	dev->ptm_enabled = 0;
 }
 
 void pci_save_ptm_state(struct pci_dev *dev)
-- 
2.25.1

Re: [PATCH v6 2/2] PCI/PM: disable PTM on all devices

[kernel test robot]

Hi Rajvi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on linus/master v5.19-rc1 next-20220608]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Rajvi-Jingar/PCI-PM-refactor-pci_pm_suspend_noirq/20220608-092412
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-randconfig-a001 (https://download.01.org/0day-ci/archive/20220609/202206090155.uxFOFYd0-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/1bf4649d5fa01aa9d1ce606461791344adfaa2ab
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Rajvi-Jingar/PCI-PM-refactor-pci_pm_suspend_noirq/20220608-092412
        git checkout 1bf4649d5fa01aa9d1ce606461791344adfaa2ab
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/pci/pci.c: In function 'pci_save_state':
>> drivers/pci/pci.c:1677:18: error: 'struct pci_dev' has no member named 'ptm_enabled'; did you mean 'ats_enabled'?
    1677 |         if (dev->ptm_enabled)
         |                  ^~~~~~~~~~~
         |                  ats_enabled


vim +1677 drivers/pci/pci.c

  1644	
  1645	/**
  1646	 * pci_save_state - save the PCI configuration space of a device before
  1647	 *		    suspending
  1648	 * @dev: PCI device that we're dealing with
  1649	 */
  1650	int pci_save_state(struct pci_dev *dev)
  1651	{
  1652		int i;
  1653		/* XXX: 100% dword access ok here? */
  1654		for (i = 0; i < 16; i++) {
  1655			pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]);
  1656			pci_dbg(dev, "saving config space at offset %#x (reading %#x)\n",
  1657				i * 4, dev->saved_config_space[i]);
  1658		}
  1659		dev->state_saved = true;
  1660	
  1661		i = pci_save_pcie_state(dev);
  1662		if (i != 0)
  1663			return i;
  1664	
  1665		i = pci_save_pcix_state(dev);
  1666		if (i != 0)
  1667			return i;
  1668	
  1669		pci_save_ltr_state(dev);
  1670		pci_save_dpc_state(dev);
  1671		pci_save_aer_state(dev);
  1672		/*
  1673		 * PCI PM core disables PTM during suspend and saves PTM state before
  1674		 * that to be able to restore the ptm state restored later. So PCI core
  1675		 * needs this check to avoid double save.
  1676		 */
> 1677		if (dev->ptm_enabled)
  1678			pci_save_ptm_state(dev);
  1679		return pci_save_vc_state(dev);
  1680	}
  1681	EXPORT_SYMBOL(pci_save_state);
  1682	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v6 2/2] PCI/PM: disable PTM on all devices

[Bjorn Helgaas]

On Thu, Jun 09, 2022 at 02:14:32AM +0800, kernel test robot wrote:
> Hi Rajvi,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on helgaas-pci/next]
> [also build test ERROR on linus/master v5.19-rc1 next-20220608]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]

I'll wait for the v7 to fix the warning.

At the same time, please capitalize the subject lines per convention,
and also the "set ptm_enabled" in 2/2 commit log.

> url:    https://github.com/intel-lab-lkp/linux/commits/Rajvi-Jingar/PCI-PM-refactor-pci_pm_suspend_noirq/20220608-092412
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
> config: i386-randconfig-a001 (https://download.01.org/0day-ci/archive/20220609/202206090155.uxFOFYd0-lkp@intel.com/config)
> compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
> reproduce (this is a W=1 build):
>         # https://github.com/intel-lab-lkp/linux/commit/1bf4649d5fa01aa9d1ce606461791344adfaa2ab
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Rajvi-Jingar/PCI-PM-refactor-pci_pm_suspend_noirq/20220608-092412
>         git checkout 1bf4649d5fa01aa9d1ce606461791344adfaa2ab
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/pci/pci.c: In function 'pci_save_state':
> >> drivers/pci/pci.c:1677:18: error: 'struct pci_dev' has no member named 'ptm_enabled'; did you mean 'ats_enabled'?
>     1677 |         if (dev->ptm_enabled)
>          |                  ^~~~~~~~~~~
>          |                  ats_enabled
> 
> 
> vim +1677 drivers/pci/pci.c
> 
>   1644	
>   1645	/**
>   1646	 * pci_save_state - save the PCI configuration space of a device before
>   1647	 *		    suspending
>   1648	 * @dev: PCI device that we're dealing with
>   1649	 */
>   1650	int pci_save_state(struct pci_dev *dev)
>   1651	{
>   1652		int i;
>   1653		/* XXX: 100% dword access ok here? */
>   1654		for (i = 0; i < 16; i++) {
>   1655			pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]);
>   1656			pci_dbg(dev, "saving config space at offset %#x (reading %#x)\n",
>   1657				i * 4, dev->saved_config_space[i]);
>   1658		}
>   1659		dev->state_saved = true;
>   1660	
>   1661		i = pci_save_pcie_state(dev);
>   1662		if (i != 0)
>   1663			return i;
>   1664	
>   1665		i = pci_save_pcix_state(dev);
>   1666		if (i != 0)
>   1667			return i;
>   1668	
>   1669		pci_save_ltr_state(dev);
>   1670		pci_save_dpc_state(dev);
>   1671		pci_save_aer_state(dev);
>   1672		/*
>   1673		 * PCI PM core disables PTM during suspend and saves PTM state before
>   1674		 * that to be able to restore the ptm state restored later. So PCI core
>   1675		 * needs this check to avoid double save.
>   1676		 */
> > 1677		if (dev->ptm_enabled)
>   1678			pci_save_ptm_state(dev);
>   1679		return pci_save_vc_state(dev);
>   1680	}
>   1681	EXPORT_SYMBOL(pci_save_state);
>   1682	
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://01.org/lkp