linux-kernel - Re: [PATCH v5 2/2] PCI/PM: Fix pci_pm_suspend

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <70d0c0d4-093f-ae8a-9654-5a433285ab12@intel.com>
Date:   Thu, 12 May 2022 15:49:18 +0200
From:   "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
To:     Rajvi Jingar <rajvi.jingar@...el.com>, <bhelgaas@...gle.com>
CC:     <david.e.box@...ux.intel.com>, <linux-pci@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v5 2/2] PCI/PM: Fix pci_pm_suspend_noirq() to disable PTM

On 4/29/2022 11:05 PM, Rajvi Jingar wrote:
> For the PCIe devices (like nvme) that do not go into D3 state still need to
> disable PTM to allow the port to enter a lower-power PM state and the SoC
> to reach a lower-power idle state as a whole. Move the pci_disable_ptm()
> out of pci_prepare_to_sleep() as this code path is not followed for devices
> that do not go into D3. This fixes the issue seen on Dell XPS 9300 with
> Ice Lake CPU and Dell Precision 5530 with Coffee Lake CPU platforms to get
> improved residency in low power idle states.
>
> Also, on receiving a PTM Request from a downstream device, if PTM is
> disabled on the root port, as per PCIe r6.0, sec 6.21.3, such a request
> would cause an Unsupported Request error. So it must first disable PTM in
> any downstream devices.
>
> Fixes: a697f072f5da ("PCI: Disable PTM during suspend to save power")
> Signed-off-by: Rajvi Jingar <rajvi.jingar@...el.com>
> Suggested-by: David E. Box <david.e.box@...ux.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.
> ---
>   drivers/pci/pci-driver.c | 28 +++++++++++++++++++---------
>   drivers/pci/pci.c        | 10 ----------
>   2 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 8b55a90126a2..400dd18a9cf5 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -813,6 +813,7 @@ static int pci_pm_suspend_late(struct device *dev)
>   
>   static int pci_pm_suspend_noirq(struct device *dev)
>   {
> +	unsigned int dev_state_saved;
>   	struct pci_dev *pci_dev = to_pci_dev(dev);
>   	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>   
> @@ -845,16 +846,25 @@ static int pci_pm_suspend_noirq(struct device *dev)
>   		}
>   	}
>   
> -	if (!pci_dev->state_saved) {
> +	dev_state_saved = pci_dev->state_saved;

If pci_dev->state_saved is set here, the device may be in D3cold already 
and disabling PTM for it will not work.  Of course, it is not necessary 
to disable PTM for it then, but this case need to be taken care of.

> +	if (!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);
> -	}
> +
> +	/*
> +	 * There are systems (for example, Intel mobile chips since Coffee
> +	 * Lake) where the power drawn while suspended can be significantly
> +	 * reduced by disabling PTM as this allows the SoC to reach a
> +	 * lower-power idle state as a whole.
> +	 */

Something like this should suffice IMV:

if (!dev_state_saved || pci_dev->current_state != PCI_D3cold)

         pci_disable_ptm(pci_dev);


> +	pci_disable_ptm(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 (!dev_state_saved && !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));
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 9ecce435fb3f..f8768672c064 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2660,16 +2660,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);

And the restoration of the PTM state on errors in pci_prepare_to_sleep() 
is not needed any more.