linux-kernel - Re: [PATCH v13 11/12] PCI: ACPI: Use device constraints to opt devices into D3 support

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: <CAJZ5v0iowLso0Wvg7gnQfXikvn_TJcpPMAW_WPNmSW6zxHUVpQ@mail.gmail.com>
Date:   Fri, 18 Aug 2023 18:06:04 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Mario Limonciello <mario.limonciello@....com>
Cc:     "Rafael J . Wysocki" <rafael@...nel.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Bjorn Helgaas <helgaas@...nel.org>, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-acpi@...r.kernel.org,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Iain Lane <iain@...ngesquash.org.uk>,
        Shyam-sundar S-k <Shyam-sundar.S-k@....com>
Subject: Re: [PATCH v13 11/12] PCI: ACPI: Use device constraints to opt
 devices into D3 support

On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello
<mario.limonciello@....com> wrote:
>
> In Windows, systems that support Modern Standby specify hardware
> pre-conditions for the SoC to be able to achieve the lowest power state
> by using 'device constraints' in a SOC specific "Power Engine
> Plugin" (PEP) [1] [2].

The connection between the device power states and the "PEP
constraints" is still rather unclear after reading the above.  I would
add something like

"For each device, the constraint is the minimum (shallowest) power
state in which the device can be for the platform to be still able to
achieve significant energy conservation in a system-wide low-power
idle configuration."

> Device constraints are specified in the return value for a _DSM of
> a PNP0D80 device, and Linux enumerates the constraints during probing.
>
> In cases that the existing logic for pci_bridge_d3_possible() may not
> enable D3 support query for any constraints specified by the device.
> If the constraints specify D3 support, then use D3 for the PCI device.

The above paragraph is not particularly clear IMO.  I would say
something like this instead:

"For PCI bridges (including PCIe ports), the constraints may be
regarded as an additional source of information regarding the power
state to put the device into when it is suspended.  In particular, if
the constraint for a given PCI bridge is D3hot, the platform regards
D3hot as a valid power state for the bridge and it is reasonable to
expect that there won't be any side effects caused by putting the
bridge into that power state.

Accordingly, take the low-power S0 idle (LPS0) constraints into
account when deciding whether or not to allow a given PCI bridge to be
put into D3."

> Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/platform-design-for-modern-standby#low-power-core-silicon-cpu-soc-dram [1]
> Link: https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf [2]
> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> ---
> v12->v13:
>  * Move back to PCI code
>  * Trim commit message
> v11->v12:
>  * Adjust for dropped patch 8/9 from v11.
>  * Update comment
> v10->v11:
>  * Fix kernel kernel build robot errors and various sparse warnings
>    related to new handling of pci_power_t.
>  * Use the new helpers introduced in previous patches
> ---
>  drivers/pci/pci-acpi.c | 14 ++++++++++++++
>  drivers/pci/pci.c      | 12 ++++++++++++
>  drivers/pci/pci.h      |  5 +++++
>  3 files changed, 31 insertions(+)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index b5b65cdfa3b8b..bcc76e9d399c5 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1081,6 +1081,20 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>         return false;
>  }
>
> +/**
> + * acpi_pci_check_d3_constraint - Check if device specifies a D3 platform constraint
> + * @dev: PCI device to check
> + *
> + * This function checks if the platform specifies that a given PCI device should
> + * be put into D3 to satisfy a low power platform constraint
> + *
> + * Returns: TRUE if constraint for D3hot or deeper, FALSE otherwise.
> + */
> +bool acpi_pci_check_d3_constraint(struct pci_dev *dev)
> +{
> +       return acpi_get_lps0_constraint(&dev->dev) >= ACPI_STATE_D3_HOT;
> +}
> +
>  static void acpi_pci_config_space_access(struct pci_dev *dev, bool enable)
>  {
>         int val = enable ? ACPI_REG_CONNECT : ACPI_REG_DISCONNECT;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 051e88ee64c63..0fc8d35154f97 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1082,6 +1082,14 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
>         return acpi_pci_bridge_d3(dev);
>  }
>
> +static inline bool platform_check_d3_constraint(struct pci_dev *dev)
> +{
> +       if (pci_use_mid_pm())
> +               return false;
> +
> +       return acpi_pci_check_d3_constraint(dev);
> +}

As I said elsewhere, the above looks redundant to me.  I would rather
make acpi_pci_bridge_d3() use the PEP constraints as an additional
source of information.

> +
>  /**
>   * pci_update_current_state - Read power state of given device and cache it
>   * @dev: PCI device to handle.
> @@ -3036,6 +3044,10 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>                 if (dmi_check_system(bridge_d3_blacklist))
>                         return false;
>
> +               /* the platform specifies in LPS0 constraints to use D3 */
> +               if (platform_check_d3_constraint(bridge))
> +                       return true;
> +
>                 /*
>                  * It is safe to put Intel PCIe ports from 2015 or newer
>                  * to D3.