Re: [RFC PATCH] PCI/APCI: Move acpi_pci_osc_support() check to negotiation phase

"Kuppuswamy, Sathyanarayanan" <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> · Wed, 28 Apr 2021 10:21:12 -0700

On 4/28/21 1:18 AM, Joerg Roedel wrote:
From: Joerg Roedel <jroedel@xxxxxxx>

The acpi_pci_osc_support() does an _OSC query with _OSC supported set
to what the OS supports but a zero _OSC control value. This is
problematic on some platforms where the firmware allows to configure
whether DPC is under OS or Firmware control.

Do we run acpi_pci_osc_support() only to check whether _OSC is
supported ? Or does it serve any other purpose.



When DPC is configured to be under OS control these platforms will
issue a warning in the firmware log that the OS does not support DPC.

Also, is there any other benefit from this patch other than fixing
a warning message in firmware?


Avoid an _OSC query with _OSC control set to zero by moving the
supported check into the acpi_pci_osc_control_set() path. This is
still early enough to fail as nothing before that depends on the
results of acpi_pci_osc_support().

As a result the acpi_pci_osc_support() function can be removed and
acpi_pci_query_osc() be simplified because it no longer called with a
NULL pointer for *control.

Signed-off-by: Joerg Roedel <jroedel@xxxxxxx>
---
  drivers/acpi/pci_root.c | 50 ++++++++++++++++-------------------------
  1 file changed, 19 insertions(+), 31 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index dcd593766a64..530ecf4970b1 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -199,16 +199,11 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
  
  	support &= OSC_PCI_SUPPORT_MASKS;
  	support |= root->osc_support_set;
+	*control &= OSC_PCI_CONTROL_MASKS;
  
  	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
  	capbuf[OSC_SUPPORT_DWORD] = support;
-	if (control) {
-		*control &= OSC_PCI_CONTROL_MASKS;
-		capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set;
-	} else {
-		/* Run _OSC query only with existing controls. */
-		capbuf[OSC_CONTROL_DWORD] = root->osc_control_set;
-	}
+	capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set;
  
  	status = acpi_pci_run_osc(root->device->handle, capbuf, &result);
  	if (ACPI_SUCCESS(status)) {
@@ -219,11 +214,6 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
  	return status;
  }
  
-static acpi_status acpi_pci_osc_support(struct acpi_pci_root *root, u32 flags)
-{
-	return acpi_pci_query_osc(root, flags, NULL);
-}
-
  struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle)
  {
  	struct acpi_pci_root *root;
@@ -346,7 +336,8 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev);
   * _OSC bits the BIOS has granted control of, but its contents are meaningless
   * on failure.
   **/
-static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
+static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32
+					    *mask, u32 req, u32 support)
  {
  	struct acpi_pci_root *root;
  	acpi_status status;
@@ -370,7 +361,7 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 r
  
  	/* Need to check the available controls bits before requesting them. */
  	while (*mask) {
-		status = acpi_pci_query_osc(root, root->osc_support_set, mask);
+		status = acpi_pci_query_osc(root, support, mask);
  		if (ACPI_FAILURE(status))
  			return status;
  		if (ctrl == *mask)
@@ -433,18 +424,6 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
  		support |= OSC_PCI_EDR_SUPPORT;
  
  	decode_osc_support(root, "OS supports", support);
-	status = acpi_pci_osc_support(root, support);
-	if (ACPI_FAILURE(status)) {
-		*no_aspm = 1;
-
-		/* _OSC is optional for PCI host bridges */
-		if ((status == AE_NOT_FOUND) && !is_pcie)
-			return;
-
-		dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
-			 acpi_format_exception(status));
-		return;
-	}
  
  	if (pcie_ports_disabled) {
  		dev_info(&device->dev, "PCIe port services disabled; not requesting _OSC control\n");
@@ -483,7 +462,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
  
  	requested = control;
  	status = acpi_pci_osc_control_set(handle, &control,
-					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
+					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL,
+					  support);
  	if (ACPI_SUCCESS(status)) {
  		decode_osc_control(root, "OS now controls", control);
  		if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
@@ -496,10 +476,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
  			*no_aspm = 1;
  		}
  	} else {
-		decode_osc_control(root, "OS requested", requested);
-		decode_osc_control(root, "platform willing to grant", control);
-		dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
-			acpi_format_exception(status));
+		/* Platform wants to control PCIe features */
+		root->osc_support_set = 0;
  
  		/*
  		 * We want to disable ASPM here, but aspm_disabled
@@ -509,6 +487,16 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
  		 * root scan.
  		 */
  		*no_aspm = 1;
+
+		/* _OSC is optional for PCI host bridges */
+		if ((status == AE_NOT_FOUND) && !is_pcie)
+			return;
+
+		decode_osc_control(root, "OS requested", requested);
+		decode_osc_control(root, "platform willing to grant", control);
+		dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
+			acpi_format_exception(status));
+
  	}
  }
  


--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer






