[PATCH 0/3] Add API for missing PCI firmware specification funcs

[PATCH 0/3] Add API for missing PCI firmware specification funcs

[Mario Limonciello]

The PCI firmware specification includes support for methods for adjusting
AUX power in D3cold as well as the PERST# assertion delay for a PCI
device.

In some hardware designs these methods might be useful for drivers to
call to configure the platform properly for the hardware's expected
behavior.

This series introduces two new symbols as API that drivers can call if
necessary.

In the Microsoft Windows ecosystem, Microsoft offers a similar API wrapping
these functions for device drivers to utilize.

Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/ns-wdm-_d3cold_aux_power_and_timing_interface
Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nc-wdm-d3cold_request_aux_power
Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nc-wdm-d3cold_request_perst_delay

Mario Limonciello (3):
  PCI: Call PCI ACPI _DSM with consistent revision argument
  PCI/ACPI: Add API for specifying aux power in D3cold
  PCI/ACPI: Add API for specifying a PERST# assertion delay

 drivers/acpi/pci_root.c  |   3 +-
 drivers/pci/pci-acpi.c   | 100 ++++++++++++++++++++++++++++++++++++++-
 drivers/pci/pci-label.c  |   4 +-
 drivers/pci/pcie/edr.c   |  13 ++---
 include/linux/pci-acpi.h |  10 ++++
 5 files changed, 119 insertions(+), 11 deletions(-)


base-commit: 89cdf9d556016a54ff6ddd62324aa5ec790c05cc
-- 
2.34.1

[PATCH 1/3] PCI: Call PCI ACPI _DSM with consistent revision argument

[Mario Limonciello]

The PCI ACPI _DSM is called across multiple places in the PCI core
with different arguments for the revision.

The PCI firmware specification specifies that this is an incorrect
behavior.
"OSPM must invoke all Functions other than Function 0 with the
 same Revision ID value"

Link: https://members.pcisig.com/wg/PCI-SIG/document/15350
      PCI Firmware specification 3.3, section 4.6
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/pci_root.c  |  3 ++-
 drivers/pci/pci-acpi.c   |  6 ++++--
 drivers/pci/pci-label.c  |  4 ++--
 drivers/pci/pcie/edr.c   | 13 +++++++------
 include/linux/pci-acpi.h |  1 +
 5 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 58b89b8d950e..bca2270a93d4 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -1055,7 +1055,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 	 * exists and returns 0, we must preserve any PCI resource
 	 * assignments made by firmware for this host bridge.
 	 */
-	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1,
+	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge),
+				      &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
 				      DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER);
 	if (obj && obj->integer.value == 0)
 		host_bridge->preserve_config = 1;
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 004575091596..bea72e807817 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -28,6 +28,7 @@
 const guid_t pci_acpi_dsm_guid =
 	GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a,
 		  0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d);
+const int pci_acpi_dsm_rev = 5;
 
 #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64)
 static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res)
@@ -1215,7 +1216,8 @@ void acpi_pci_add_bus(struct pci_bus *bus)
 	if (!pci_is_root_bus(bus))
 		return;
 
-	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 3,
+	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge),
+				      &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
 				      DSM_PCI_POWER_ON_RESET_DELAY, NULL, ACPI_TYPE_INTEGER);
 	if (!obj)
 		return;
@@ -1376,7 +1378,7 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
 	if (bridge->ignore_reset_delay)
 		pdev->d3cold_delay = 0;
 
-	obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 3,
+	obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
 				      DSM_PCI_DEVICE_READINESS_DURATIONS, NULL,
 				      ACPI_TYPE_PACKAGE);
 	if (!obj)
diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
index 0c6446519640..91bdd04029f0 100644
--- a/drivers/pci/pci-label.c
+++ b/drivers/pci/pci-label.c
@@ -41,7 +41,7 @@ static bool device_has_acpi_name(struct device *dev)
 	if (!handle)
 		return false;
 
-	return acpi_check_dsm(handle, &pci_acpi_dsm_guid, 0x2,
+	return acpi_check_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
 			      1 << DSM_PCI_DEVICE_NAME);
 #else
 	return false;
@@ -162,7 +162,7 @@ static int dsm_get_label(struct device *dev, char *buf,
 	if (!handle)
 		return -1;
 
-	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 0x2,
+	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
 				DSM_PCI_DEVICE_NAME, NULL);
 	if (!obj)
 		return -1;
diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c
index 5f4914d313a1..ab6a50201124 100644
--- a/drivers/pci/pcie/edr.c
+++ b/drivers/pci/pcie/edr.c
@@ -35,7 +35,7 @@ static int acpi_enable_dpc(struct pci_dev *pdev)
 	 * Behavior when calling unsupported _DSM functions is undefined,
 	 * so check whether EDR_PORT_DPC_ENABLE_DSM is supported.
 	 */
-	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
+	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
 			    1ULL << EDR_PORT_DPC_ENABLE_DSM))
 		return 0;
 
@@ -51,8 +51,9 @@ static int acpi_enable_dpc(struct pci_dev *pdev)
 	 * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is
 	 * optional.  Return success if it's not implemented.
 	 */
-	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
-				EDR_PORT_DPC_ENABLE_DSM, &argv4);
+	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid,
+				pci_acpi_dsm_rev, EDR_PORT_DPC_ENABLE_DSM,
+				&argv4);
 	if (!obj)
 		return 0;
 
@@ -88,12 +89,12 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev)
 	 * Behavior when calling unsupported _DSM functions is undefined,
 	 * so check whether EDR_PORT_DPC_ENABLE_DSM is supported.
 	 */
-	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
+	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
 			    1ULL << EDR_PORT_LOCATE_DSM))
 		return pci_dev_get(pdev);
 
-	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
-				EDR_PORT_LOCATE_DSM, NULL);
+	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid,
+				pci_acpi_dsm_rev, EDR_PORT_LOCATE_DSM, NULL);
 	if (!obj)
 		return pci_dev_get(pdev);
 
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 078225b514d4..7966ef8f14b3 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -115,6 +115,7 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
 #endif
 
 extern const guid_t pci_acpi_dsm_guid;
+extern const int pci_acpi_dsm_rev;
 
 /* _DSM Definitions for PCI */
 #define DSM_PCI_PRESERVE_BOOT_CONFIG		0x05
-- 
2.34.1

[PATCH 2/3] PCI/ACPI: Add API for specifying aux power in D3cold

[Mario Limonciello]

The PCI firmware specification includes support for a _DSM function
to negotiate aux power to be provided to the device while it is in
D3cold.

"The Request D3cold Aux Power Limit function describes how a device
driver conveys its auxiliary  power requirements to the host platform
when the device is in D3cold. The system firmware responds with a value
indicating whether the request can be supported. The power request is
specific to the Auxiliary power supply; main power may be removed while
in D3cold. A device must not draw any more power than what has been
negotiated via this mechanism after entering D3cold"

Add API for callers to utilize this _DSM.

Link: https://members.pcisig.com/wg/PCI-SIG/document/15350
      PCI Firmware specification 3.3, section 4.6.10
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/pci-acpi.c   | 54 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h |  7 ++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index bea72e807817..257858a6719e 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1348,6 +1348,60 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
 	return adev;
 }
 
+/**
+ * pci_acpi_request_aux_power_for_d3cold - Request auxiliary power for D3cold
+ * @pdev: PCI device to request power for
+ * @arg: mW requested
+ *
+ * This function requests auxiliary power for a PCI device in D3cold state
+ * when main power is removed above the value guaranteed by PCI specification.
+ *
+ * Return:
+ *  0 on success
+ *  -ENODEV when the device does not support the _DSM
+ *  -EIO on ACPI call failure
+ *  -EACCESS when the request was denied
+ *  Positive value corresponding to platform requested retry interval in seconds
+ */
+int pci_acpi_request_aux_power_for_d3cold(struct pci_dev *pdev, int arg)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	union acpi_object *obj;
+	union acpi_object argv4 = {
+		.integer.type = ACPI_TYPE_INTEGER,
+		.integer.value = arg,
+	};
+	int val;
+
+	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid,
+			    pci_acpi_dsm_rev,
+			    1 << DSM_PCI_REQUEST_D3COLD_AUX_POWER))
+		return -ENODEV;
+
+	obj = acpi_evaluate_dsm_typed(adev->handle, &pci_acpi_dsm_guid,
+				      pci_acpi_dsm_rev,
+				      DSM_PCI_REQUEST_D3COLD_AUX_POWER,
+				      &argv4, ACPI_TYPE_INTEGER);
+	if (!obj)
+		return -EIO;
+
+	val = obj->integer.value;
+	ACPI_FREE(obj);
+
+	switch (val) {
+	case PCI_D3COLD_AUX_DENIED:
+		return -EACCES;
+	case PCI_D3COLD_AUX_GRANTED:
+	case PCI_D3COLD_AUX_NO_MAIN_POWER_REMOVAL:
+		return 0;
+	default:
+		break;
+	}
+
+	return val;
+}
+EXPORT_SYMBOL_GPL(pci_acpi_request_aux_power_for_d3cold);
+
 /**
  * pci_acpi_optimize_delay - optimize PCI D3 and D3cold delay from ACPI
  * @pdev: the PCI device whose delay is to be updated
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 7966ef8f14b3..bc6372bcb7d6 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -122,6 +122,13 @@ extern const int pci_acpi_dsm_rev;
 #define DSM_PCI_DEVICE_NAME			0x07
 #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
 #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
+#define DSM_PCI_REQUEST_D3COLD_AUX_POWER	0x0A
+
+#define PCI_D3COLD_AUX_DENIED			0
+#define PCI_D3COLD_AUX_GRANTED			1
+#define PCI_D3COLD_AUX_NO_MAIN_POWER_REMOVAL	2
+
+int pci_acpi_request_aux_power_for_d3cold(struct pci_dev *pdev, int arg);
 
 #ifdef CONFIG_PCIE_EDR
 void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
-- 
2.34.1

[PATCH 3/3] PCI/ACPI: Add API for specifying a PERST# assertion delay

[Mario Limonciello]

The PCI firmware specification includes support for a _DSM function
to specify a PERST# assertion delay.

"The Add PERST# Assertion Delay function is used to convey the requirement
for a fixed delay in timing between the time the PME_TO_Ack message is
received at the PCI Express Downstream Port that originated the
PME_Turn_Off message, and the time the platform asserts PERST# to the slot
during the corresponding Endpoint’s or PCI Express Upstream Port’s
transition to D3cold while the system is in an ACPI operational state."

Add API for callers to utilize this _DSM.

Link: https://members.pcisig.com/wg/PCI-SIG/document/15350
      PCI Firmware specification 3.3, section 4.6.11
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/pci-acpi.c   | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h |  2 ++
 2 files changed, 42 insertions(+)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 257858a6719e..82e04808040d 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1402,6 +1402,46 @@ int pci_acpi_request_aux_power_for_d3cold(struct pci_dev *pdev, int arg)
 }
 EXPORT_SYMBOL_GPL(pci_acpi_request_aux_power_for_d3cold);
 
+/**
+ * pci_acpi_set_perst_delay - Set the assertion delay for the PERST# signal
+ * @pdev: PCI device to set the delay for
+ * @arg: Delay value in microseconds
+ *
+ * This function sets the delay for the PERST# signal of the given PCI device.
+ * The delay value is specified in microseconds through the @arg parameter.
+ * The maximum delay value is 10000 microseconds.
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+int pci_acpi_set_perst_delay(struct pci_dev *pdev, int arg)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	union acpi_object *obj;
+	union acpi_object argv4 = {
+		.integer.type = ACPI_TYPE_INTEGER,
+		.integer.value = arg,
+	};
+	int val;
+
+	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid,
+			    pci_acpi_dsm_rev, 1 << DSM_PCI_ADD_PERST_DELAY))
+		return -ENODEV;
+
+	if (arg > 10000)
+		return -EINVAL;
+
+	obj = acpi_evaluate_dsm_typed(adev->handle, &pci_acpi_dsm_guid,
+				      pci_acpi_dsm_rev, DSM_PCI_ADD_PERST_DELAY,
+				      &argv4, ACPI_TYPE_INTEGER);
+	if (!obj)
+		return -EIO;
+
+	val = obj->integer.value;
+	ACPI_FREE(obj);
+	return (val == arg) ? 0 : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(pci_acpi_set_perst_delay);
+
 /**
  * pci_acpi_optimize_delay - optimize PCI D3 and D3cold delay from ACPI
  * @pdev: the PCI device whose delay is to be updated
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index bc6372bcb7d6..bdce83c3afbf 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -123,12 +123,14 @@ extern const int pci_acpi_dsm_rev;
 #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
 #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
 #define DSM_PCI_REQUEST_D3COLD_AUX_POWER	0x0A
+#define DSM_PCI_ADD_PERST_DELAY			0x0B
 
 #define PCI_D3COLD_AUX_DENIED			0
 #define PCI_D3COLD_AUX_GRANTED			1
 #define PCI_D3COLD_AUX_NO_MAIN_POWER_REMOVAL	2
 
 int pci_acpi_request_aux_power_for_d3cold(struct pci_dev *pdev, int arg);
+int pci_acpi_set_perst_delay(struct pci_dev *pdev, int arg);
 
 #ifdef CONFIG_PCIE_EDR
 void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
-- 
2.34.1

Re: [PATCH 1/3] PCI: Call PCI ACPI _DSM with consistent revision argument

[Rafael J. Wysocki]

On Fri, Nov 10, 2023 at 9:32 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> The PCI ACPI _DSM is called across multiple places in the PCI core
> with different arguments for the revision.
>
> The PCI firmware specification specifies that this is an incorrect
> behavior.
> "OSPM must invoke all Functions other than Function 0 with the
>  same Revision ID value"
>
> Link: https://members.pcisig.com/wg/PCI-SIG/document/15350
>       PCI Firmware specification 3.3, section 4.6
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

and I haven't seen much activity related to this series, so I'm not
sure what's happening to it.

Regardless, I think that the remaining two patches are better sent
along with the first users of the new APIs.

> ---
>  drivers/acpi/pci_root.c  |  3 ++-
>  drivers/pci/pci-acpi.c   |  6 ++++--
>  drivers/pci/pci-label.c  |  4 ++--
>  drivers/pci/pcie/edr.c   | 13 +++++++------
>  include/linux/pci-acpi.h |  1 +
>  5 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 58b89b8d950e..bca2270a93d4 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -1055,7 +1055,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>          * exists and returns 0, we must preserve any PCI resource
>          * assignments made by firmware for this host bridge.
>          */
> -       obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1,
> +       obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge),
> +                                     &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>                                       DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER);
>         if (obj && obj->integer.value == 0)
>                 host_bridge->preserve_config = 1;
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 004575091596..bea72e807817 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -28,6 +28,7 @@
>  const guid_t pci_acpi_dsm_guid =
>         GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a,
>                   0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d);
> +const int pci_acpi_dsm_rev = 5;
>
>  #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64)
>  static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res)
> @@ -1215,7 +1216,8 @@ void acpi_pci_add_bus(struct pci_bus *bus)
>         if (!pci_is_root_bus(bus))
>                 return;
>
> -       obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 3,
> +       obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge),
> +                                     &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>                                       DSM_PCI_POWER_ON_RESET_DELAY, NULL, ACPI_TYPE_INTEGER);
>         if (!obj)
>                 return;
> @@ -1376,7 +1378,7 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
>         if (bridge->ignore_reset_delay)
>                 pdev->d3cold_delay = 0;
>
> -       obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 3,
> +       obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>                                       DSM_PCI_DEVICE_READINESS_DURATIONS, NULL,
>                                       ACPI_TYPE_PACKAGE);
>         if (!obj)
> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
> index 0c6446519640..91bdd04029f0 100644
> --- a/drivers/pci/pci-label.c
> +++ b/drivers/pci/pci-label.c
> @@ -41,7 +41,7 @@ static bool device_has_acpi_name(struct device *dev)
>         if (!handle)
>                 return false;
>
> -       return acpi_check_dsm(handle, &pci_acpi_dsm_guid, 0x2,
> +       return acpi_check_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>                               1 << DSM_PCI_DEVICE_NAME);
>  #else
>         return false;
> @@ -162,7 +162,7 @@ static int dsm_get_label(struct device *dev, char *buf,
>         if (!handle)
>                 return -1;
>
> -       obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 0x2,
> +       obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>                                 DSM_PCI_DEVICE_NAME, NULL);
>         if (!obj)
>                 return -1;
> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c
> index 5f4914d313a1..ab6a50201124 100644
> --- a/drivers/pci/pcie/edr.c
> +++ b/drivers/pci/pcie/edr.c
> @@ -35,7 +35,7 @@ static int acpi_enable_dpc(struct pci_dev *pdev)
>          * Behavior when calling unsupported _DSM functions is undefined,
>          * so check whether EDR_PORT_DPC_ENABLE_DSM is supported.
>          */
> -       if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
> +       if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>                             1ULL << EDR_PORT_DPC_ENABLE_DSM))
>                 return 0;
>
> @@ -51,8 +51,9 @@ static int acpi_enable_dpc(struct pci_dev *pdev)
>          * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is
>          * optional.  Return success if it's not implemented.
>          */
> -       obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
> -                               EDR_PORT_DPC_ENABLE_DSM, &argv4);
> +       obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid,
> +                               pci_acpi_dsm_rev, EDR_PORT_DPC_ENABLE_DSM,
> +                               &argv4);
>         if (!obj)
>                 return 0;
>
> @@ -88,12 +89,12 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev)
>          * Behavior when calling unsupported _DSM functions is undefined,
>          * so check whether EDR_PORT_DPC_ENABLE_DSM is supported.
>          */
> -       if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
> +       if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>                             1ULL << EDR_PORT_LOCATE_DSM))
>                 return pci_dev_get(pdev);
>
> -       obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
> -                               EDR_PORT_LOCATE_DSM, NULL);
> +       obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid,
> +                               pci_acpi_dsm_rev, EDR_PORT_LOCATE_DSM, NULL);
>         if (!obj)
>                 return pci_dev_get(pdev);
>
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 078225b514d4..7966ef8f14b3 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -115,6 +115,7 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
>  #endif
>
>  extern const guid_t pci_acpi_dsm_guid;
> +extern const int pci_acpi_dsm_rev;
>
>  /* _DSM Definitions for PCI */
>  #define DSM_PCI_PRESERVE_BOOT_CONFIG           0x05
> --
> 2.34.1
>
>

Re: [PATCH 1/3] PCI: Call PCI ACPI _DSM with consistent revision argument

[Mario Limonciello]

On 11/30/2023 07:34, Rafael J. Wysocki wrote:
> On Fri, Nov 10, 2023 at 9:32 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> The PCI ACPI _DSM is called across multiple places in the PCI core
>> with different arguments for the revision.
>>
>> The PCI firmware specification specifies that this is an incorrect
>> behavior.
>> "OSPM must invoke all Functions other than Function 0 with the
>>   same Revision ID value"
>>
>> Link: https://members.pcisig.com/wg/PCI-SIG/document/15350
>>        PCI Firmware specification 3.3, section 4.6
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 

Thanks!

> and I haven't seen much activity related to this series, so I'm not
> sure what's happening to it.
> 
> Regardless, I think that the remaining two patches are better sent
> along with the first users of the new APIs.

Bjorn,

If you agree, can you please queue up the first patch?  I'll shuffle the 
others into the back of my mind for if/when they're needed.

Thanks!
> 
>> ---
>>   drivers/acpi/pci_root.c  |  3 ++-
>>   drivers/pci/pci-acpi.c   |  6 ++++--
>>   drivers/pci/pci-label.c  |  4 ++--
>>   drivers/pci/pcie/edr.c   | 13 +++++++------
>>   include/linux/pci-acpi.h |  1 +
>>   5 files changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 58b89b8d950e..bca2270a93d4 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -1055,7 +1055,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>>           * exists and returns 0, we must preserve any PCI resource
>>           * assignments made by firmware for this host bridge.
>>           */
>> -       obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1,
>> +       obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge),
>> +                                     &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>                                        DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER);
>>          if (obj && obj->integer.value == 0)
>>                  host_bridge->preserve_config = 1;
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 004575091596..bea72e807817 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -28,6 +28,7 @@
>>   const guid_t pci_acpi_dsm_guid =
>>          GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a,
>>                    0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d);
>> +const int pci_acpi_dsm_rev = 5;
>>
>>   #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64)
>>   static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res)
>> @@ -1215,7 +1216,8 @@ void acpi_pci_add_bus(struct pci_bus *bus)
>>          if (!pci_is_root_bus(bus))
>>                  return;
>>
>> -       obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 3,
>> +       obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge),
>> +                                     &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>                                        DSM_PCI_POWER_ON_RESET_DELAY, NULL, ACPI_TYPE_INTEGER);
>>          if (!obj)
>>                  return;
>> @@ -1376,7 +1378,7 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
>>          if (bridge->ignore_reset_delay)
>>                  pdev->d3cold_delay = 0;
>>
>> -       obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 3,
>> +       obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>                                        DSM_PCI_DEVICE_READINESS_DURATIONS, NULL,
>>                                        ACPI_TYPE_PACKAGE);
>>          if (!obj)
>> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
>> index 0c6446519640..91bdd04029f0 100644
>> --- a/drivers/pci/pci-label.c
>> +++ b/drivers/pci/pci-label.c
>> @@ -41,7 +41,7 @@ static bool device_has_acpi_name(struct device *dev)
>>          if (!handle)
>>                  return false;
>>
>> -       return acpi_check_dsm(handle, &pci_acpi_dsm_guid, 0x2,
>> +       return acpi_check_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>                                1 << DSM_PCI_DEVICE_NAME);
>>   #else
>>          return false;
>> @@ -162,7 +162,7 @@ static int dsm_get_label(struct device *dev, char *buf,
>>          if (!handle)
>>                  return -1;
>>
>> -       obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 0x2,
>> +       obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>                                  DSM_PCI_DEVICE_NAME, NULL);
>>          if (!obj)
>>                  return -1;
>> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c
>> index 5f4914d313a1..ab6a50201124 100644
>> --- a/drivers/pci/pcie/edr.c
>> +++ b/drivers/pci/pcie/edr.c
>> @@ -35,7 +35,7 @@ static int acpi_enable_dpc(struct pci_dev *pdev)
>>           * Behavior when calling unsupported _DSM functions is undefined,
>>           * so check whether EDR_PORT_DPC_ENABLE_DSM is supported.
>>           */
>> -       if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
>> +       if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>                              1ULL << EDR_PORT_DPC_ENABLE_DSM))
>>                  return 0;
>>
>> @@ -51,8 +51,9 @@ static int acpi_enable_dpc(struct pci_dev *pdev)
>>           * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is
>>           * optional.  Return success if it's not implemented.
>>           */
>> -       obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
>> -                               EDR_PORT_DPC_ENABLE_DSM, &argv4);
>> +       obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid,
>> +                               pci_acpi_dsm_rev, EDR_PORT_DPC_ENABLE_DSM,
>> +                               &argv4);
>>          if (!obj)
>>                  return 0;
>>
>> @@ -88,12 +89,12 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev)
>>           * Behavior when calling unsupported _DSM functions is undefined,
>>           * so check whether EDR_PORT_DPC_ENABLE_DSM is supported.
>>           */
>> -       if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
>> +       if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>                              1ULL << EDR_PORT_LOCATE_DSM))
>>                  return pci_dev_get(pdev);
>>
>> -       obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
>> -                               EDR_PORT_LOCATE_DSM, NULL);
>> +       obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid,
>> +                               pci_acpi_dsm_rev, EDR_PORT_LOCATE_DSM, NULL);
>>          if (!obj)
>>                  return pci_dev_get(pdev);
>>
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 078225b514d4..7966ef8f14b3 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -115,6 +115,7 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
>>   #endif
>>
>>   extern const guid_t pci_acpi_dsm_guid;
>> +extern const int pci_acpi_dsm_rev;
>>
>>   /* _DSM Definitions for PCI */
>>   #define DSM_PCI_PRESERVE_BOOT_CONFIG           0x05
>> --
>> 2.34.1
>>
>>

Re: [PATCH 1/3] PCI: Call PCI ACPI _DSM with consistent revision argument

[Bjorn Helgaas]

On Fri, Nov 10, 2023 at 12:55:01PM -0600, Mario Limonciello wrote:
> The PCI ACPI _DSM is called across multiple places in the PCI core
> with different arguments for the revision.
> 
> The PCI firmware specification specifies that this is an incorrect
> behavior.
> "OSPM must invoke all Functions other than Function 0 with the
>  same Revision ID value"

This patch passes the same or a larger Revision ID than before, so
everything should work the same because the spec requires backwards
compatibility.  But it's conceivable that it could break on firmware
that does the revision check incorrectly.

Is this fixing a problem other than the spec compliance issue?

I agree the PCI FW spec says this.  It was added in r3.3 by the ECN at
https://members.pcisig.com/wg/Firmware/document/previewpdf/13988, but
I don't quite understand that ECN.

ACPI r6.5, sec 9.1.1, doesn't include the "must invoke all Functions
with same Revision ID" language, and the ASL example there clearly
treats revisions higher than those implemented by firmware as valid,
although new Functions added by those higher revisions are obviously
not supported.

PCI FW also says OSPM should not use a fixed Revision ID, but should
start with the highest known revision and "successively invoke
Function 0 with decremented values of Revision ID until system
firmware returns a value indicating support for more than Function 0"
(added by the same ECN), and I don't think Linux does this part.

So I think the fixed "pci_acpi_dsm_rev" value as in your patch works
fine with the ACPI ASL example, but it doesn't track the "successively
decrement" part of PCI FW.  I don't know the reason for that part of
the ECN.

Unrelated to this patch, I think it's a bug that Linux fails to invoke
Function 0 in a few cases, e.g., DSM_PCI_PRESERVE_BOOT_CONFIG,
DSM_PCI_POWER_ON_RESET_DELAY, and DSM_PCI_DEVICE_READINESS_DURATIONS.

Per spec, OSPM must invoke Function 0 to learn which other Functions
are supported.  It's not explicitly stated, but I think this is
required because a supported non-zero Function may return "any data
object", so there's no return value that would mean "this Function
Index is not supported." 

Bjorn

> Link: https://members.pcisig.com/wg/PCI-SIG/document/15350
>       PCI Firmware specification 3.3, section 4.6
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/pci_root.c  |  3 ++-
>  drivers/pci/pci-acpi.c   |  6 ++++--
>  drivers/pci/pci-label.c  |  4 ++--
>  drivers/pci/pcie/edr.c   | 13 +++++++------
>  include/linux/pci-acpi.h |  1 +
>  5 files changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 58b89b8d950e..bca2270a93d4 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -1055,7 +1055,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  	 * exists and returns 0, we must preserve any PCI resource
>  	 * assignments made by firmware for this host bridge.
>  	 */
> -	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1,
> +	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge),
> +				      &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>  				      DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER);
>  	if (obj && obj->integer.value == 0)
>  		host_bridge->preserve_config = 1;
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 004575091596..bea72e807817 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -28,6 +28,7 @@
>  const guid_t pci_acpi_dsm_guid =
>  	GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a,
>  		  0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d);
> +const int pci_acpi_dsm_rev = 5;
>
>  #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64)
>  static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res)
> @@ -1215,7 +1216,8 @@ void acpi_pci_add_bus(struct pci_bus *bus)
>  	if (!pci_is_root_bus(bus))
>  		return;
>  
> -	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 3,
> +	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge),
> +				      &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>  				      DSM_PCI_POWER_ON_RESET_DELAY, NULL, ACPI_TYPE_INTEGER);
>  	if (!obj)
>  		return;
> @@ -1376,7 +1378,7 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
>  	if (bridge->ignore_reset_delay)
>  		pdev->d3cold_delay = 0;
>  
> -	obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 3,
> +	obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>  				      DSM_PCI_DEVICE_READINESS_DURATIONS, NULL,
>  				      ACPI_TYPE_PACKAGE);
>  	if (!obj)
> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
> index 0c6446519640..91bdd04029f0 100644
> --- a/drivers/pci/pci-label.c
> +++ b/drivers/pci/pci-label.c
> @@ -41,7 +41,7 @@ static bool device_has_acpi_name(struct device *dev)
>  	if (!handle)
>  		return false;
>  
> -	return acpi_check_dsm(handle, &pci_acpi_dsm_guid, 0x2,
> +	return acpi_check_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>  			      1 << DSM_PCI_DEVICE_NAME);
>  #else
>  	return false;
> @@ -162,7 +162,7 @@ static int dsm_get_label(struct device *dev, char *buf,
>  	if (!handle)
>  		return -1;
>  
> -	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 0x2,
> +	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>  				DSM_PCI_DEVICE_NAME, NULL);
>  	if (!obj)
>  		return -1;
> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c
> index 5f4914d313a1..ab6a50201124 100644
> --- a/drivers/pci/pcie/edr.c
> +++ b/drivers/pci/pcie/edr.c
> @@ -35,7 +35,7 @@ static int acpi_enable_dpc(struct pci_dev *pdev)
>  	 * Behavior when calling unsupported _DSM functions is undefined,
>  	 * so check whether EDR_PORT_DPC_ENABLE_DSM is supported.
>  	 */
> -	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
> +	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>  			    1ULL << EDR_PORT_DPC_ENABLE_DSM))
>  		return 0;
>  
> @@ -51,8 +51,9 @@ static int acpi_enable_dpc(struct pci_dev *pdev)
>  	 * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is
>  	 * optional.  Return success if it's not implemented.
>  	 */
> -	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
> -				EDR_PORT_DPC_ENABLE_DSM, &argv4);
> +	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid,
> +				pci_acpi_dsm_rev, EDR_PORT_DPC_ENABLE_DSM,
> +				&argv4);
>  	if (!obj)
>  		return 0;
>  
> @@ -88,12 +89,12 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev)
>  	 * Behavior when calling unsupported _DSM functions is undefined,
>  	 * so check whether EDR_PORT_DPC_ENABLE_DSM is supported.
>  	 */
> -	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
> +	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>  			    1ULL << EDR_PORT_LOCATE_DSM))
>  		return pci_dev_get(pdev);
>  
> -	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
> -				EDR_PORT_LOCATE_DSM, NULL);
> +	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid,
> +				pci_acpi_dsm_rev, EDR_PORT_LOCATE_DSM, NULL);
>  	if (!obj)
>  		return pci_dev_get(pdev);
>  
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 078225b514d4..7966ef8f14b3 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -115,6 +115,7 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
>  #endif
>  
>  extern const guid_t pci_acpi_dsm_guid;
> +extern const int pci_acpi_dsm_rev;
>  
>  /* _DSM Definitions for PCI */
>  #define DSM_PCI_PRESERVE_BOOT_CONFIG		0x05
> -- 
> 2.34.1
> 

Re: [PATCH 1/3] PCI: Call PCI ACPI _DSM with consistent revision argument

[Mario Limonciello]

On 11/30/2023 16:29, Bjorn Helgaas wrote:
> On Fri, Nov 10, 2023 at 12:55:01PM -0600, Mario Limonciello wrote:
>> The PCI ACPI _DSM is called across multiple places in the PCI core
>> with different arguments for the revision.
>>
>> The PCI firmware specification specifies that this is an incorrect
>> behavior.
>> "OSPM must invoke all Functions other than Function 0 with the
>>   same Revision ID value"
> 
> This patch passes the same or a larger Revision ID than before, so
> everything should work the same because the spec requires backwards
> compatibility.  But it's conceivable that it could break on firmware
> that does the revision check incorrectly.
> 
> Is this fixing a problem other than the spec compliance issue?

It was just a spec compliance issue I noticed when implementing the 
other two patches.

> 
> I agree the PCI FW spec says this.  It was added in r3.3 by the ECN at
> https://members.pcisig.com/wg/Firmware/document/previewpdf/13988, but
> I don't quite understand that ECN.
> 
> ACPI r6.5, sec 9.1.1, doesn't include the "must invoke all Functions
> with same Revision ID" language, and the ASL example there clearly
> treats revisions higher than those implemented by firmware as valid,
> although new Functions added by those higher revisions are obviously
> not supported.
> 
> PCI FW also says OSPM should not use a fixed Revision ID, but should
> start with the highest known revision and "successively invoke
> Function 0 with decremented values of Revision ID until system
> firmware returns a value indicating support for more than Function 0"
> (added by the same ECN), and I don't think Linux does this part.
> 
> So I think the fixed "pci_acpi_dsm_rev" value as in your patch works
> fine with the ACPI ASL example, but it doesn't track the "successively
> decrement" part of PCI FW.  I don't know the reason for that part of
> the ECN.
> 

Do you think it's better to respin to take this into account and be more 
stringent or "do nothing"?

> Unrelated to this patch, I think it's a bug that Linux fails to invoke
> Function 0 in a few cases, e.g., DSM_PCI_PRESERVE_BOOT_CONFIG,
> DSM_PCI_POWER_ON_RESET_DELAY, and DSM_PCI_DEVICE_READINESS_DURATIONS.
> 
> Per spec, OSPM must invoke Function 0 to learn which other Functions
> are supported.  It's not explicitly stated, but I think this is
> required because a supported non-zero Function may return "any data
> object", so there's no return value that would mean "this Function
> Index is not supported."
> 

> Bjorn

What are your thoughts on the other two patches in the series?
Should they wait for a consumer or prepare the API to match the spec.

> 
>> Link: https://members.pcisig.com/wg/PCI-SIG/document/15350
>>        PCI Firmware specification 3.3, section 4.6
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/acpi/pci_root.c  |  3 ++-
>>   drivers/pci/pci-acpi.c   |  6 ++++--
>>   drivers/pci/pci-label.c  |  4 ++--
>>   drivers/pci/pcie/edr.c   | 13 +++++++------
>>   include/linux/pci-acpi.h |  1 +
>>   5 files changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 58b89b8d950e..bca2270a93d4 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -1055,7 +1055,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>>   	 * exists and returns 0, we must preserve any PCI resource
>>   	 * assignments made by firmware for this host bridge.
>>   	 */
>> -	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1,
>> +	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge),
>> +				      &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>   				      DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER);
>>   	if (obj && obj->integer.value == 0)
>>   		host_bridge->preserve_config = 1;
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 004575091596..bea72e807817 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -28,6 +28,7 @@
>>   const guid_t pci_acpi_dsm_guid =
>>   	GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a,
>>   		  0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d);
>> +const int pci_acpi_dsm_rev = 5;
>>
>>   #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64)
>>   static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res)
>> @@ -1215,7 +1216,8 @@ void acpi_pci_add_bus(struct pci_bus *bus)
>>   	if (!pci_is_root_bus(bus))
>>   		return;
>>   
>> -	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 3,
>> +	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge),
>> +				      &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>   				      DSM_PCI_POWER_ON_RESET_DELAY, NULL, ACPI_TYPE_INTEGER);
>>   	if (!obj)
>>   		return;
>> @@ -1376,7 +1378,7 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
>>   	if (bridge->ignore_reset_delay)
>>   		pdev->d3cold_delay = 0;
>>   
>> -	obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 3,
>> +	obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>   				      DSM_PCI_DEVICE_READINESS_DURATIONS, NULL,
>>   				      ACPI_TYPE_PACKAGE);
>>   	if (!obj)
>> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
>> index 0c6446519640..91bdd04029f0 100644
>> --- a/drivers/pci/pci-label.c
>> +++ b/drivers/pci/pci-label.c
>> @@ -41,7 +41,7 @@ static bool device_has_acpi_name(struct device *dev)
>>   	if (!handle)
>>   		return false;
>>   
>> -	return acpi_check_dsm(handle, &pci_acpi_dsm_guid, 0x2,
>> +	return acpi_check_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>   			      1 << DSM_PCI_DEVICE_NAME);
>>   #else
>>   	return false;
>> @@ -162,7 +162,7 @@ static int dsm_get_label(struct device *dev, char *buf,
>>   	if (!handle)
>>   		return -1;
>>   
>> -	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 0x2,
>> +	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>   				DSM_PCI_DEVICE_NAME, NULL);
>>   	if (!obj)
>>   		return -1;
>> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c
>> index 5f4914d313a1..ab6a50201124 100644
>> --- a/drivers/pci/pcie/edr.c
>> +++ b/drivers/pci/pcie/edr.c
>> @@ -35,7 +35,7 @@ static int acpi_enable_dpc(struct pci_dev *pdev)
>>   	 * Behavior when calling unsupported _DSM functions is undefined,
>>   	 * so check whether EDR_PORT_DPC_ENABLE_DSM is supported.
>>   	 */
>> -	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
>> +	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>   			    1ULL << EDR_PORT_DPC_ENABLE_DSM))
>>   		return 0;
>>   
>> @@ -51,8 +51,9 @@ static int acpi_enable_dpc(struct pci_dev *pdev)
>>   	 * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is
>>   	 * optional.  Return success if it's not implemented.
>>   	 */
>> -	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
>> -				EDR_PORT_DPC_ENABLE_DSM, &argv4);
>> +	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid,
>> +				pci_acpi_dsm_rev, EDR_PORT_DPC_ENABLE_DSM,
>> +				&argv4);
>>   	if (!obj)
>>   		return 0;
>>   
>> @@ -88,12 +89,12 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev)
>>   	 * Behavior when calling unsupported _DSM functions is undefined,
>>   	 * so check whether EDR_PORT_DPC_ENABLE_DSM is supported.
>>   	 */
>> -	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
>> +	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>   			    1ULL << EDR_PORT_LOCATE_DSM))
>>   		return pci_dev_get(pdev);
>>   
>> -	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
>> -				EDR_PORT_LOCATE_DSM, NULL);
>> +	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid,
>> +				pci_acpi_dsm_rev, EDR_PORT_LOCATE_DSM, NULL);
>>   	if (!obj)
>>   		return pci_dev_get(pdev);
>>   
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 078225b514d4..7966ef8f14b3 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -115,6 +115,7 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
>>   #endif
>>   
>>   extern const guid_t pci_acpi_dsm_guid;
>> +extern const int pci_acpi_dsm_rev;
>>   
>>   /* _DSM Definitions for PCI */
>>   #define DSM_PCI_PRESERVE_BOOT_CONFIG		0x05
>> -- 
>> 2.34.1
>>

Re: [PATCH 1/3] PCI: Call PCI ACPI _DSM with consistent revision argument

[Bjorn Helgaas]

On Tue, Dec 05, 2023 at 02:12:54PM -0600, Mario Limonciello wrote:
> On 11/30/2023 16:29, Bjorn Helgaas wrote:
> > On Fri, Nov 10, 2023 at 12:55:01PM -0600, Mario Limonciello wrote:
> > > The PCI ACPI _DSM is called across multiple places in the PCI core
> > > with different arguments for the revision.
> > > 
> > > The PCI firmware specification specifies that this is an incorrect
> > > behavior.
> > > "OSPM must invoke all Functions other than Function 0 with the
> > >   same Revision ID value"
> > 
> > This patch passes the same or a larger Revision ID than before, so
> > everything should work the same because the spec requires backwards
> > compatibility.  But it's conceivable that it could break on firmware
> > that does the revision check incorrectly.
> > 
> > Is this fixing a problem other than the spec compliance issue?
> 
> It was just a spec compliance issue I noticed when implementing the other
> two patches.
> 
> > I agree the PCI FW spec says this.  It was added in r3.3 by the ECN at
> > https://members.pcisig.com/wg/Firmware/document/previewpdf/13988, but
> > I don't quite understand that ECN.
> > 
> > ACPI r6.5, sec 9.1.1, doesn't include the "must invoke all Functions
> > with same Revision ID" language, and the ASL example there clearly
> > treats revisions higher than those implemented by firmware as valid,
> > although new Functions added by those higher revisions are obviously
> > not supported.
> > 
> > PCI FW also says OSPM should not use a fixed Revision ID, but should
> > start with the highest known revision and "successively invoke
> > Function 0 with decremented values of Revision ID until system
> > firmware returns a value indicating support for more than Function 0"
> > (added by the same ECN), and I don't think Linux does this part.
> > 
> > So I think the fixed "pci_acpi_dsm_rev" value as in your patch works
> > fine with the ACPI ASL example, but it doesn't track the "successively
> > decrement" part of PCI FW.  I don't know the reason for that part of
> > the ECN.
> 
> Do you think it's better to respin to take this into account and be more
> stringent or "do nothing"?

To me, it seems better to do nothing unless a change would solve a
problem.  I raised it as a question to the PCI Firmware workgroup
(https://members.pcisig.com/wg/Firmware/mail/thread/32031), but I
haven't heard anything.

Regrettably, that link only works for PCI-SIG members; here's the text
of my question:

  Sorry to reopen this old topic.  This ECN was approved and appears
  in r3.3.  We're contemplating Linux changes to conform to it.

  I think I understand the ACPI requirement for OSPM to invoke _DSM
  Function 0 to learn whether a Function is supported (because a
  non-zero Function may have completely arbitrary return values, so
  invoking that Function has no way to return "this Function Index
  isn't supported").

  I don't understand why it's important for OSPM to "invoke all
  Functions other than Function 0 with the same Revision ID."  That
  idea doesn't appear in ACPI r6.5, sec 9.1.1 or in the sample ASL
  there.  Is there a benefit to using the same Revision ID for all
  Functions?  (Of course OSPM must invoke Function 0 with Revision ID
  N to learn whether Function X is supported for Revision ID N.)

  I also don't understand why "OSPM should successively invoke
  Function 0 with decremented values of Revision ID until system
  firmware returns a value indicating support for more than Function
  0."  ACPI r6.5 doesn't suggest that, and the sample ASL returns
  different bitmasks depending on the Revision ID supplied by OSPM,
  including a default case that returns a bitmask including all
  Functions implemented by the firmware if OSPM supplied a higher
  Revision ID from the future.  What is the benefit of probing with
  decremented Revision IDs?

  Is there something PCI-specific here, or should these requirements
  be in the ACPI spec instead of the PCI Firmware spec?

> > Unrelated to this patch, I think it's a bug that Linux fails to invoke
> > Function 0 in a few cases, e.g., DSM_PCI_PRESERVE_BOOT_CONFIG,
> > DSM_PCI_POWER_ON_RESET_DELAY, and DSM_PCI_DEVICE_READINESS_DURATIONS.
> > 
> > Per spec, OSPM must invoke Function 0 to learn which other Functions
> > are supported.  It's not explicitly stated, but I think this is
> > required because a supported non-zero Function may return "any data
> > object", so there's no return value that would mean "this Function
> > Index is not supported."
> 
> What are your thoughts on the other two patches in the series?
> Should they wait for a consumer or prepare the API to match the spec.

I'd prefer to wait until there are users of the new functions.
There's no real benefit to adding functions that are never called.

Bjorn

Re: [PATCH 1/3] PCI: Call PCI ACPI _DSM with consistent revision argument

[Mario Limonciello]

On 12/6/2023 17:04, Bjorn Helgaas wrote:
> On Tue, Dec 05, 2023 at 02:12:54PM -0600, Mario Limonciello wrote:
>> On 11/30/2023 16:29, Bjorn Helgaas wrote:
>>> On Fri, Nov 10, 2023 at 12:55:01PM -0600, Mario Limonciello wrote:
>>>> The PCI ACPI _DSM is called across multiple places in the PCI core
>>>> with different arguments for the revision.
>>>>
>>>> The PCI firmware specification specifies that this is an incorrect
>>>> behavior.
>>>> "OSPM must invoke all Functions other than Function 0 with the
>>>>    same Revision ID value"
>>>
>>> This patch passes the same or a larger Revision ID than before, so
>>> everything should work the same because the spec requires backwards
>>> compatibility.  But it's conceivable that it could break on firmware
>>> that does the revision check incorrectly.
>>>
>>> Is this fixing a problem other than the spec compliance issue?
>>
>> It was just a spec compliance issue I noticed when implementing the other
>> two patches.
>>
>>> I agree the PCI FW spec says this.  It was added in r3.3 by the ECN at
>>> https://members.pcisig.com/wg/Firmware/document/previewpdf/13988, but
>>> I don't quite understand that ECN.
>>>
>>> ACPI r6.5, sec 9.1.1, doesn't include the "must invoke all Functions
>>> with same Revision ID" language, and the ASL example there clearly
>>> treats revisions higher than those implemented by firmware as valid,
>>> although new Functions added by those higher revisions are obviously
>>> not supported.
>>>
>>> PCI FW also says OSPM should not use a fixed Revision ID, but should
>>> start with the highest known revision and "successively invoke
>>> Function 0 with decremented values of Revision ID until system
>>> firmware returns a value indicating support for more than Function 0"
>>> (added by the same ECN), and I don't think Linux does this part.
>>>
>>> So I think the fixed "pci_acpi_dsm_rev" value as in your patch works
>>> fine with the ACPI ASL example, but it doesn't track the "successively
>>> decrement" part of PCI FW.  I don't know the reason for that part of
>>> the ECN.
>>
>> Do you think it's better to respin to take this into account and be more
>> stringent or "do nothing"?
> 
> To me, it seems better to do nothing unless a change would solve a
> problem.  I raised it as a question to the PCI Firmware workgroup
> (https://members.pcisig.com/wg/Firmware/mail/thread/32031), but I
> haven't heard anything.
> 
> Regrettably, that link only works for PCI-SIG members; here's the text
> of my question:
> 
>    Sorry to reopen this old topic.  This ECN was approved and appears
>    in r3.3.  We're contemplating Linux changes to conform to it.
> 
>    I think I understand the ACPI requirement for OSPM to invoke _DSM
>    Function 0 to learn whether a Function is supported (because a
>    non-zero Function may have completely arbitrary return values, so
>    invoking that Function has no way to return "this Function Index
>    isn't supported").
> 
>    I don't understand why it's important for OSPM to "invoke all
>    Functions other than Function 0 with the same Revision ID."  That
>    idea doesn't appear in ACPI r6.5, sec 9.1.1 or in the sample ASL
>    there.  Is there a benefit to using the same Revision ID for all
>    Functions?  (Of course OSPM must invoke Function 0 with Revision ID
>    N to learn whether Function X is supported for Revision ID N.)
> 
>    I also don't understand why "OSPM should successively invoke
>    Function 0 with decremented values of Revision ID until system
>    firmware returns a value indicating support for more than Function
>    0."  ACPI r6.5 doesn't suggest that, and the sample ASL returns
>    different bitmasks depending on the Revision ID supplied by OSPM,
>    including a default case that returns a bitmask including all
>    Functions implemented by the firmware if OSPM supplied a higher
>    Revision ID from the future.  What is the benefit of probing with
>    decremented Revision IDs?
> 
>    Is there something PCI-specific here, or should these requirements
>    be in the ACPI spec instead of the PCI Firmware spec?
> 
>>> Unrelated to this patch, I think it's a bug that Linux fails to invoke
>>> Function 0 in a few cases, e.g., DSM_PCI_PRESERVE_BOOT_CONFIG,
>>> DSM_PCI_POWER_ON_RESET_DELAY, and DSM_PCI_DEVICE_READINESS_DURATIONS.
>>>
>>> Per spec, OSPM must invoke Function 0 to learn which other Functions
>>> are supported.  It's not explicitly stated, but I think this is
>>> required because a supported non-zero Function may return "any data
>>> object", so there's no return value that would mean "this Function
>>> Index is not supported."
>>
>> What are your thoughts on the other two patches in the series?
>> Should they wait for a consumer or prepare the API to match the spec.
> 
> I'd prefer to wait until there are users of the new functions.
> There's no real benefit to adding functions that are never called.
> 
> Bjorn

OK thanks - I'll put aside the whole series for now.  The first 
immediate consumers of this would be GPU driver, but it's only needed in 
very specific circumstances that haven't come up yet in practice.