linux-kernel - Re: [PATCH 2/2] PCI: Disable Precision Time Measurement during 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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0hGhyPySUdabwW5_LhyAKC3A4zdgj7H=55R=Xk3jvt3Yw@mail.gmail.com>
Date:   Thu, 19 Nov 2020 13:01:53 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     "David E. Box" <david.e.box@...ux.intel.com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <len.brown@...el.com>,
        Linux PCI <linux-pci@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] PCI: Disable Precision Time Measurement during suspend

On Thu, Nov 19, 2020 at 1:17 AM David E. Box
<david.e.box@...ux.intel.com> wrote:
>
> On Intel client platforms that support suspend-to-idle, like Ice Lake,
> root ports that have Precision Time Management (PTM) enabled can prevent
> the port from being fully power gated, causing higher power consumption
> while suspended.  To prevent this, after saving the PTM control register,
> disable the feature.  The feature will be returned to its previous state
> during restore.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=209361
> Reported-by: Len Brown <len.brown@...el.com>
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> Signed-off-by: David E. Box <david.e.box@...ux.intel.com>
> ---
>  drivers/pci/pci.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 6fd4ae910a88..a2b40497d443 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -21,6 +21,7 @@
>  #include <linux/module.h>
>  #include <linux/spinlock.h>
>  #include <linux/string.h>
> +#include <linux/suspend.h>
>  #include <linux/log2.h>
>  #include <linux/logic_pio.h>
>  #include <linux/pm_wakeup.h>
> @@ -1543,7 +1544,7 @@ static void pci_save_ptm_state(struct pci_dev *dev)
>  {
>         int ptm;
>         struct pci_cap_saved_state *save_state;
> -       u16 *cap;
> +       u16 *cap, ctrl;
>
>         if (!pci_is_pcie(dev))
>                 return;
> @@ -1560,6 +1561,17 @@ static void pci_save_ptm_state(struct pci_dev *dev)
>
>         cap = (u16 *)&save_state->cap.data[0];
>         pci_read_config_word(dev, ptm + PCI_PTM_CTRL, cap);
> +
> +       /*
> +        * On Intel systems that support suspend-to-idle, additional
> +        * power savings can be gained by disabling PTM on root ports,
> +        * as this allows the port to enter a deeper pm state.

I would say "There are systems (for example, ...) 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 (pm_suspend_target_state == PM_SUSPEND_TO_IDLE &&

AFAICS the target sleep state doesn't matter here, so I'd skip the
check above, but otherwise it LGTM.

> +           pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> +               ctrl = *cap & ~(PCI_PTM_CTRL_ENABLE | PCI_PTM_CTRL_ROOT);
> +               pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
> +       }
>  }
>
>  static void pci_restore_ptm_state(struct pci_dev *dev)
> --