[PATCH v14.c 0/4] Use LPS0 constraints to opt devices into D3

[PATCH v14.c 0/4] Use LPS0 constraints to opt devices into D3

[Mario Limonciello]

LPS0 constraints specified by the platform can be used as a method to
opt devices into D3.

In v14 this series has been split into 3 parts.
 part A: Immediate fix for AMD issue.
 part B: LPS0 export improvements
 part C: Long term solution for all vendors

This is part C, and applying it is dependent upon parts A and B.
It is sent separately to make it easier to follow and aid with review.

Mario Limonciello (4):
  PCI: ACPI: Add helper functions for converting ACPI <-> PCI states
  PCI: ACPI: Consider non-hotplug bridges for D3 in acpi_pci_bridge_d3()
  PCI: ACPI: Use device constraints to opt devices into D3 support
  PCI: ACPI: Limit the Intel specific opt-in to D3 to 2024

 drivers/pci/pci-acpi.c | 101 +++++++++++++++++++++++++++--------------
 drivers/pci/pci.c      |  14 ++++--
 2 files changed, 76 insertions(+), 39 deletions(-)

-- 
2.34.1

[PATCH v14.c 1/4] PCI: ACPI: Add helper functions for converting ACPI <-> PCI states

[Mario Limonciello]

Several functions do internal mappings in either direction. Add
helpers for this functionality.  No intended functional changes.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v10->v11:
 * New patch
---
 drivers/pci/pci-acpi.c | 87 +++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 34 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index a05350a4e49cb..b5b65cdfa3b8b 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -884,6 +884,56 @@ acpi_status pci_acpi_add_pm_notifier(struct acpi_device *dev,
 	return acpi_add_pm_notifier(dev, &pci_dev->dev, pci_acpi_wake_dev);
 }
 
+/**
+ * map_pci_to_acpi - map a PCI power state to an ACPI D-state
+ * @state: PCI power state to map
+ *
+ * Returns: Corresponding ACPI D-state, otherwise ACPI_STATE_UNKNOWN
+ */
+static u8 map_pci_to_acpi(pci_power_t state)
+{
+	switch (state) {
+	case PCI_D0:
+		return ACPI_STATE_D0;
+	case PCI_D1:
+		return ACPI_STATE_D1;
+	case PCI_D2:
+		return ACPI_STATE_D2;
+	case PCI_D3hot:
+		return ACPI_STATE_D3_HOT;
+	case PCI_D3cold:
+		return ACPI_STATE_D3_COLD;
+	}
+
+	return ACPI_STATE_UNKNOWN;
+}
+
+/**
+ * map_acpi_to_pci - map an ACPI D-state to a PCI power state
+ * @state: ACPI D-state to map
+ *
+ * Returns: Corresponding PCI power state, otherwise PCI_POWER_ERROR.
+ */
+static pci_power_t map_acpi_to_pci(int state)
+{
+	switch (state) {
+	case ACPI_STATE_D0:
+		return PCI_D0;
+	case ACPI_STATE_D1:
+		return PCI_D1;
+	case ACPI_STATE_D2:
+		return PCI_D2;
+	case ACPI_STATE_D3_HOT:
+		return PCI_D3hot;
+	case ACPI_STATE_D3_COLD:
+		return PCI_D3cold;
+	case ACPI_STATE_UNKNOWN:
+		return PCI_UNKNOWN;
+	}
+
+	return PCI_POWER_ERROR;
+}
+
 /*
  * _SxD returns the D-state with the highest power
  * (lowest D-state number) supported in the S-state "x".
@@ -919,19 +969,7 @@ pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
 	if (acpi_state < 0)
 		return PCI_POWER_ERROR;
 
-	switch (acpi_state) {
-	case ACPI_STATE_D0:
-		return PCI_D0;
-	case ACPI_STATE_D1:
-		return PCI_D1;
-	case ACPI_STATE_D2:
-		return PCI_D2;
-	case ACPI_STATE_D3_HOT:
-		return PCI_D3hot;
-	case ACPI_STATE_D3_COLD:
-		return PCI_D3cold;
-	}
-	return PCI_POWER_ERROR;
+	return map_acpi_to_pci(acpi_state);
 }
 
 static struct acpi_device *acpi_pci_find_companion(struct device *dev);
@@ -1056,13 +1094,6 @@ static void acpi_pci_config_space_access(struct pci_dev *dev, bool enable)
 int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
-	static const u8 state_conv[] = {
-		[PCI_D0] = ACPI_STATE_D0,
-		[PCI_D1] = ACPI_STATE_D1,
-		[PCI_D2] = ACPI_STATE_D2,
-		[PCI_D3hot] = ACPI_STATE_D3_HOT,
-		[PCI_D3cold] = ACPI_STATE_D3_COLD,
-	};
 	int error;
 
 	/* If the ACPI device has _EJ0, ignore the device */
@@ -1089,7 +1120,7 @@ int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 		acpi_pci_config_space_access(dev, false);
 	}
 
-	error = acpi_device_set_power(adev, state_conv[state]);
+	error = acpi_device_set_power(adev, map_pci_to_acpi(state));
 	if (error)
 		return error;
 
@@ -1111,23 +1142,11 @@ int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
-	static const pci_power_t state_conv[] = {
-		[ACPI_STATE_D0]      = PCI_D0,
-		[ACPI_STATE_D1]      = PCI_D1,
-		[ACPI_STATE_D2]      = PCI_D2,
-		[ACPI_STATE_D3_HOT]  = PCI_D3hot,
-		[ACPI_STATE_D3_COLD] = PCI_D3cold,
-	};
-	int state;
 
 	if (!adev || !acpi_device_power_manageable(adev))
 		return PCI_UNKNOWN;
 
-	state = adev->power.state;
-	if (state == ACPI_STATE_UNKNOWN)
-		return PCI_UNKNOWN;
-
-	return state_conv[state];
+	return map_acpi_to_pci(adev->power.state);
 }
 
 void acpi_pci_refresh_power_state(struct pci_dev *dev)
-- 
2.34.1

Re: [PATCH v14.c 1/4] PCI: ACPI: Add helper functions for converting ACPI <-> PCI states

[Rafael J. Wysocki]

On Fri, Aug 18, 2023 at 9:40 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> Several functions do internal mappings in either direction. Add
> helpers for this functionality.  No intended functional changes.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v10->v11:
>  * New patch
> ---
>  drivers/pci/pci-acpi.c | 87 +++++++++++++++++++++++++-----------------
>  1 file changed, 53 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index a05350a4e49cb..b5b65cdfa3b8b 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -884,6 +884,56 @@ acpi_status pci_acpi_add_pm_notifier(struct acpi_device *dev,
>         return acpi_add_pm_notifier(dev, &pci_dev->dev, pci_acpi_wake_dev);
>  }
>
> +/**
> + * map_pci_to_acpi - map a PCI power state to an ACPI D-state
> + * @state: PCI power state to map
> + *
> + * Returns: Corresponding ACPI D-state, otherwise ACPI_STATE_UNKNOWN
> + */
> +static u8 map_pci_to_acpi(pci_power_t state)

I would prefer to call this pci_power_state_to_acpi() and analogously
for the other one.

With this addressed, please feel free to add my ACK to the patch.

> +{
> +       switch (state) {
> +       case PCI_D0:
> +               return ACPI_STATE_D0;
> +       case PCI_D1:
> +               return ACPI_STATE_D1;
> +       case PCI_D2:
> +               return ACPI_STATE_D2;
> +       case PCI_D3hot:
> +               return ACPI_STATE_D3_HOT;
> +       case PCI_D3cold:
> +               return ACPI_STATE_D3_COLD;
> +       }
> +
> +       return ACPI_STATE_UNKNOWN;
> +}
> +
> +/**
> + * map_acpi_to_pci - map an ACPI D-state to a PCI power state
> + * @state: ACPI D-state to map
> + *
> + * Returns: Corresponding PCI power state, otherwise PCI_POWER_ERROR.
> + */
> +static pci_power_t map_acpi_to_pci(int state)
> +{
> +       switch (state) {
> +       case ACPI_STATE_D0:
> +               return PCI_D0;
> +       case ACPI_STATE_D1:
> +               return PCI_D1;
> +       case ACPI_STATE_D2:
> +               return PCI_D2;
> +       case ACPI_STATE_D3_HOT:
> +               return PCI_D3hot;
> +       case ACPI_STATE_D3_COLD:
> +               return PCI_D3cold;
> +       case ACPI_STATE_UNKNOWN:
> +               return PCI_UNKNOWN;
> +       }
> +
> +       return PCI_POWER_ERROR;
> +}
> +
>  /*
>   * _SxD returns the D-state with the highest power
>   * (lowest D-state number) supported in the S-state "x".
> @@ -919,19 +969,7 @@ pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
>         if (acpi_state < 0)
>                 return PCI_POWER_ERROR;
>
> -       switch (acpi_state) {
> -       case ACPI_STATE_D0:
> -               return PCI_D0;
> -       case ACPI_STATE_D1:
> -               return PCI_D1;
> -       case ACPI_STATE_D2:
> -               return PCI_D2;
> -       case ACPI_STATE_D3_HOT:
> -               return PCI_D3hot;
> -       case ACPI_STATE_D3_COLD:
> -               return PCI_D3cold;
> -       }
> -       return PCI_POWER_ERROR;
> +       return map_acpi_to_pci(acpi_state);
>  }
>
>  static struct acpi_device *acpi_pci_find_companion(struct device *dev);
> @@ -1056,13 +1094,6 @@ static void acpi_pci_config_space_access(struct pci_dev *dev, bool enable)
>  int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>  {
>         struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
> -       static const u8 state_conv[] = {
> -               [PCI_D0] = ACPI_STATE_D0,
> -               [PCI_D1] = ACPI_STATE_D1,
> -               [PCI_D2] = ACPI_STATE_D2,
> -               [PCI_D3hot] = ACPI_STATE_D3_HOT,
> -               [PCI_D3cold] = ACPI_STATE_D3_COLD,
> -       };
>         int error;
>
>         /* If the ACPI device has _EJ0, ignore the device */
> @@ -1089,7 +1120,7 @@ int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>                 acpi_pci_config_space_access(dev, false);
>         }
>
> -       error = acpi_device_set_power(adev, state_conv[state]);
> +       error = acpi_device_set_power(adev, map_pci_to_acpi(state));
>         if (error)
>                 return error;
>
> @@ -1111,23 +1142,11 @@ int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>  pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)
>  {
>         struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
> -       static const pci_power_t state_conv[] = {
> -               [ACPI_STATE_D0]      = PCI_D0,
> -               [ACPI_STATE_D1]      = PCI_D1,
> -               [ACPI_STATE_D2]      = PCI_D2,
> -               [ACPI_STATE_D3_HOT]  = PCI_D3hot,
> -               [ACPI_STATE_D3_COLD] = PCI_D3cold,
> -       };
> -       int state;
>
>         if (!adev || !acpi_device_power_manageable(adev))
>                 return PCI_UNKNOWN;
>
> -       state = adev->power.state;
> -       if (state == ACPI_STATE_UNKNOWN)
> -               return PCI_UNKNOWN;
> -
> -       return state_conv[state];
> +       return map_acpi_to_pci(adev->power.state);
>  }
>
>  void acpi_pci_refresh_power_state(struct pci_dev *dev)
> --
> 2.34.1
>

[PATCH v14.c 2/4] PCI: ACPI: Consider non-hotplug bridges for D3 in acpi_pci_bridge_d3()

[Mario Limonciello]

acpi_pci_bridge_d3() only runs on hotplug bridges. Two checks are made
for _S0W and for whether a device is power manageable contingent upon
an ACPI companion being present.

These checks also make sense for non-hotplug bridges, so move the
check for hotplug bridge after the acpi companion checks.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/pci-acpi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index b5b65cdfa3b8b..64e6ada024235 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1017,7 +1017,7 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
 	struct acpi_device *adev, *rpadev;
 	const union acpi_object *obj;
 
-	if (acpi_pci_disabled || !dev->is_hotplug_bridge)
+	if (acpi_pci_disabled)
 		return false;
 
 	adev = ACPI_COMPANION(&dev->dev);
@@ -1039,6 +1039,9 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
 			return true;
 	}
 
+	if (!dev->is_hotplug_bridge)
+		return false;
+
 	rpdev = pcie_find_root_port(dev);
 	if (!rpdev)
 		return false;
-- 
2.34.1

Re: [PATCH v14.c 2/4] PCI: ACPI: Consider non-hotplug bridges for D3 in acpi_pci_bridge_d3()

[Rafael J. Wysocki]

On Fri, Aug 18, 2023 at 9:40 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> acpi_pci_bridge_d3() only runs on hotplug bridges. Two checks are made
> for _S0W and for whether a device is power manageable contingent upon
> an ACPI companion being present.
>
> These checks also make sense for non-hotplug bridges, so move the
> check for hotplug bridge after the acpi companion checks.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Looks reasonable to me.

Acked-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
>  drivers/pci/pci-acpi.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index b5b65cdfa3b8b..64e6ada024235 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1017,7 +1017,7 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>         struct acpi_device *adev, *rpadev;
>         const union acpi_object *obj;
>
> -       if (acpi_pci_disabled || !dev->is_hotplug_bridge)
> +       if (acpi_pci_disabled)
>                 return false;
>
>         adev = ACPI_COMPANION(&dev->dev);
> @@ -1039,6 +1039,9 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>                         return true;
>         }
>
> +       if (!dev->is_hotplug_bridge)
> +               return false;
> +
>         rpdev = pcie_find_root_port(dev);
>         if (!rpdev)
>                 return false;
> --

[PATCH v14.c 3/4] PCI: ACPI: Use device constraints to opt devices into D3 support

[Mario Limonciello]

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].

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.

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@amd.com>
---
v13->v14:
 * Move code as another way to satisfy acpi_pci_bridge_d3() instead
 * Update commit with Rafael's suggestions
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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 64e6ada024235..8331aea22d327 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1022,6 +1022,15 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
 
 	adev = ACPI_COMPANION(&dev->dev);
 	if (adev) {
+		int acpi_state;
+
+		/* Check if the platform specifies an LPS0 constraint of D3. */
+		acpi_state = acpi_get_lps0_constraint(adev);
+		pci_dbg(dev, "LPS0 constraint: %d\n", acpi_state);
+		if (acpi_state != ACPI_STATE_UNKNOWN &&
+		    acpi_state >= ACPI_STATE_D3_HOT)
+			return true;
+
 		/*
 		 * If the bridge has _S0W, whether or not it can go into D3
 		 * depends on what is returned by that object.  In particular,
-- 
2.34.1

Re: [PATCH v14.c 3/4] PCI: ACPI: Use device constraints to opt devices into D3 support

[Rafael J. Wysocki]

On Fri, Aug 18, 2023 at 9:40 PM Mario Limonciello
<mario.limonciello@amd.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].
>
> 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.
>
> 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@amd.com>

All feedback addressed AFAICS, so

Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
> v13->v14:
>  * Move code as another way to satisfy acpi_pci_bridge_d3() instead
>  * Update commit with Rafael's suggestions
> 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 | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 64e6ada024235..8331aea22d327 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1022,6 +1022,15 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>
>         adev = ACPI_COMPANION(&dev->dev);
>         if (adev) {
> +               int acpi_state;
> +
> +               /* Check if the platform specifies an LPS0 constraint of D3. */
> +               acpi_state = acpi_get_lps0_constraint(adev);
> +               pci_dbg(dev, "LPS0 constraint: %d\n", acpi_state);
> +               if (acpi_state != ACPI_STATE_UNKNOWN &&
> +                   acpi_state >= ACPI_STATE_D3_HOT)
> +                       return true;
> +
>                 /*
>                  * If the bridge has _S0W, whether or not it can go into D3
>                  * depends on what is returned by that object.  In particular,
> --
> 2.34.1
>

[PATCH v14.c 4/4] PCI: ACPI: Limit the Intel specific opt-in to D3 to 2024

[Mario Limonciello]

Intel systems that need to have PCIe ports in D3 for low power idle
specify this by constraints on the ACPI PNP0D80 device. As this information
is queried by acpi_pci_bridge_d3(), limit the DMI BIOS year check to stop
at 2024. This will allow future systems to rely on the constraints check
and ACPI checks to set up policy like non-Intel systems do.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v13->v14:
 * Use a variable instead
v12->v13:
 * New patch
---
 drivers/pci/pci.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bfdad2eb36d13..c8787d898c377 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3037,16 +3037,22 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
 			return false;
 
 		/*
-		 * Allow Intel PCIe ports from 2015 onward to go into D3 to
+		 * Allow Intel PCIe ports from 2015 to 2024 to go into D3 to
 		 * achieve additional energy conservation on some platforms.
 		 *
+		 * Intel systems from 2025 onward that need this are expected
+		 * to specify this in an LPS0 device constraint instead.
+		 *
 		 * This is only set for Intel PCIe ports as it causes problems
 		 * on both AMD Rembrandt and Phoenix platforms where USB keyboards
 		 * can not be used to wake the system from suspend.
 		 */
-		if (bridge->vendor == PCI_VENDOR_ID_INTEL &&
-		    dmi_get_bios_year() >= 2015)
-			return true;
+		if (bridge->vendor == PCI_VENDOR_ID_INTEL) {
+			int year = dmi_get_bios_year();
+
+			if (year >= 2015 && year <= 2024)
+				return true;
+		}
 		break;
 	}
 
-- 
2.34.1

Re: [PATCH v14.c 4/4] PCI: ACPI: Limit the Intel specific opt-in to D3 to 2024

[Rafael J. Wysocki]

On Fri, Aug 18, 2023 at 9:40 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> Intel systems that need to have PCIe ports in D3 for low power idle
> specify this by constraints on the ACPI PNP0D80 device. As this information
> is queried by acpi_pci_bridge_d3(), limit the DMI BIOS year check to stop
> at 2024. This will allow future systems to rely on the constraints check
> and ACPI checks to set up policy like non-Intel systems do.

So I'm not sure about the value of this change.

The behavior is made Intel-specific in [14a 1/1] and it will be that
way at least for some time.  This change only sets the date after
which it won't be Intel-specific any more, but for what reason
exactly?

And why is 2024 the cut-off year (and not 2025, for example)?

If Intel platforms continue to be OK with putting all PCIe ports into
D3hot beyond 2024, why restrict the kernel from doing so on them?

> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v13->v14:
>  * Use a variable instead
> v12->v13:
>  * New patch
> ---
>  drivers/pci/pci.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index bfdad2eb36d13..c8787d898c377 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3037,16 +3037,22 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>                         return false;
>
>                 /*
> -                * Allow Intel PCIe ports from 2015 onward to go into D3 to
> +                * Allow Intel PCIe ports from 2015 to 2024 to go into D3 to
>                  * achieve additional energy conservation on some platforms.
>                  *
> +                * Intel systems from 2025 onward that need this are expected
> +                * to specify this in an LPS0 device constraint instead.
> +                *
>                  * This is only set for Intel PCIe ports as it causes problems
>                  * on both AMD Rembrandt and Phoenix platforms where USB keyboards
>                  * can not be used to wake the system from suspend.
>                  */
> -               if (bridge->vendor == PCI_VENDOR_ID_INTEL &&
> -                   dmi_get_bios_year() >= 2015)
> -                       return true;
> +               if (bridge->vendor == PCI_VENDOR_ID_INTEL) {
> +                       int year = dmi_get_bios_year();
> +
> +                       if (year >= 2015 && year <= 2024)
> +                               return true;
> +               }
>                 break;
>         }
>
> --
> 2.34.1
>

Re: [PATCH v14.c 4/4] PCI: ACPI: Limit the Intel specific opt-in to D3 to 2024

[Limonciello, Mario]

On 8/21/2023 1:46 PM, Rafael J. Wysocki wrote:
> On Fri, Aug 18, 2023 at 9:40 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> Intel systems that need to have PCIe ports in D3 for low power idle
>> specify this by constraints on the ACPI PNP0D80 device. As this information
>> is queried by acpi_pci_bridge_d3(), limit the DMI BIOS year check to stop
>> at 2024. This will allow future systems to rely on the constraints check
>> and ACPI checks to set up policy like non-Intel systems do.
> 
> So I'm not sure about the value of this change.
> 
> The behavior is made Intel-specific in [14a 1/1] and it will be that
> way at least for some time.  This change only sets the date after
> which it won't be Intel-specific any more, but for what reason
> exactly?
> 
> And why is 2024 the cut-off year (and not 2025, for example)?

No particular reason other than it's a few kernel cycles to get this 
tested and working or revert it if it's a bad idea after all.

> 
> If Intel platforms continue to be OK with putting all PCIe ports into
> D3hot beyond 2024, why restrict the kernel from doing so on them?

OK let me try to explain my thought process.

The reason that root ports were put into D3 on Intel systems was that 
it's required for the system to get into the deepest state.

At the time that it was introduced there wasn't a way for the firmware
to express this desire for root ports that were not power manageable by 
ACPI.

Constraints are a good way to express it, so by limiting the Intel 
hardcode to a number of years gets everyone onto the same codepaths.

But that being said - if you would rather keep Intel as hardcode forever 
this patch can be dropped from the series.

> 
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> v13->v14:
>>   * Use a variable instead
>> v12->v13:
>>   * New patch
>> ---
>>   drivers/pci/pci.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index bfdad2eb36d13..c8787d898c377 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3037,16 +3037,22 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>>                          return false;
>>
>>                  /*
>> -                * Allow Intel PCIe ports from 2015 onward to go into D3 to
>> +                * Allow Intel PCIe ports from 2015 to 2024 to go into D3 to
>>                   * achieve additional energy conservation on some platforms.
>>                   *
>> +                * Intel systems from 2025 onward that need this are expected
>> +                * to specify this in an LPS0 device constraint instead.
>> +                *
>>                   * This is only set for Intel PCIe ports as it causes problems
>>                   * on both AMD Rembrandt and Phoenix platforms where USB keyboards
>>                   * can not be used to wake the system from suspend.
>>                   */
>> -               if (bridge->vendor == PCI_VENDOR_ID_INTEL &&
>> -                   dmi_get_bios_year() >= 2015)
>> -                       return true;
>> +               if (bridge->vendor == PCI_VENDOR_ID_INTEL) {
>> +                       int year = dmi_get_bios_year();
>> +
>> +                       if (year >= 2015 && year <= 2024)
>> +                               return true;
>> +               }
>>                  break;
>>          }
>>
>> --
>> 2.34.1
>>

Re: [PATCH v14.c 4/4] PCI: ACPI: Limit the Intel specific opt-in to D3 to 2024

[Rafael J. Wysocki]

On Mon, Aug 21, 2023 at 9:18 PM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
>
>
> On 8/21/2023 1:46 PM, Rafael J. Wysocki wrote:
> > On Fri, Aug 18, 2023 at 9:40 PM Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> >>
> >> Intel systems that need to have PCIe ports in D3 for low power idle
> >> specify this by constraints on the ACPI PNP0D80 device. As this information
> >> is queried by acpi_pci_bridge_d3(), limit the DMI BIOS year check to stop
> >> at 2024. This will allow future systems to rely on the constraints check
> >> and ACPI checks to set up policy like non-Intel systems do.
> >
> > So I'm not sure about the value of this change.
> >
> > The behavior is made Intel-specific in [14a 1/1] and it will be that
> > way at least for some time.  This change only sets the date after
> > which it won't be Intel-specific any more, but for what reason
> > exactly?
> >
> > And why is 2024 the cut-off year (and not 2025, for example)?
>
> No particular reason other than it's a few kernel cycles to get this
> tested and working or revert it if it's a bad idea after all.
>
> >
> > If Intel platforms continue to be OK with putting all PCIe ports into
> > D3hot beyond 2024, why restrict the kernel from doing so on them?
>
> OK let me try to explain my thought process.
>
> The reason that root ports were put into D3 on Intel systems was that
> it's required for the system to get into the deepest state.
>
> At the time that it was introduced there wasn't a way for the firmware
> to express this desire for root ports that were not power manageable by
> ACPI.
>
> Constraints are a good way to express it, so by limiting the Intel
> hardcode to a number of years gets everyone onto the same codepaths.

Assuming that the will be used in future systems, but that is beyond
the control of anyone involved here I think.

> But that being said - if you would rather keep Intel as hardcode forever
> this patch can be dropped from the series.

This change can be made at any time and I don't see a particular
reason for making it right now.

Re: [PATCH v14.c 4/4] PCI: ACPI: Limit the Intel specific opt-in to D3 to 2024

[Limonciello, Mario]

On 8/21/2023 2:24 PM, Rafael J. Wysocki wrote:
> On Mon, Aug 21, 2023 at 9:18 PM Limonciello, Mario
> <mario.limonciello@amd.com> wrote:
>>
>>
>>
>> On 8/21/2023 1:46 PM, Rafael J. Wysocki wrote:
>>> On Fri, Aug 18, 2023 at 9:40 PM Mario Limonciello
>>> <mario.limonciello@amd.com> wrote:
>>>>
>>>> Intel systems that need to have PCIe ports in D3 for low power idle
>>>> specify this by constraints on the ACPI PNP0D80 device. As this information
>>>> is queried by acpi_pci_bridge_d3(), limit the DMI BIOS year check to stop
>>>> at 2024. This will allow future systems to rely on the constraints check
>>>> and ACPI checks to set up policy like non-Intel systems do.
>>>
>>> So I'm not sure about the value of this change.
>>>
>>> The behavior is made Intel-specific in [14a 1/1] and it will be that
>>> way at least for some time.  This change only sets the date after
>>> which it won't be Intel-specific any more, but for what reason
>>> exactly?
>>>
>>> And why is 2024 the cut-off year (and not 2025, for example)?
>>
>> No particular reason other than it's a few kernel cycles to get this
>> tested and working or revert it if it's a bad idea after all.
>>
>>>
>>> If Intel platforms continue to be OK with putting all PCIe ports into
>>> D3hot beyond 2024, why restrict the kernel from doing so on them?
>>
>> OK let me try to explain my thought process.
>>
>> The reason that root ports were put into D3 on Intel systems was that
>> it's required for the system to get into the deepest state.
>>
>> At the time that it was introduced there wasn't a way for the firmware
>> to express this desire for root ports that were not power manageable by
>> ACPI.
>>
>> Constraints are a good way to express it, so by limiting the Intel
>> hardcode to a number of years gets everyone onto the same codepaths.
> 
> Assuming that the will be used in future systems, but that is beyond
> the control of anyone involved here I think.
> 
>> But that being said - if you would rather keep Intel as hardcode forever
>> this patch can be dropped from the series.
> 
> This change can be made at any time and I don't see a particular
> reason for making it right now.

OK, then after Bjorn reviews the other patches of 14.a and 14.c I'll 
drop this patch.

Thanks!