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

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

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

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.

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.

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@suse.de>
---
 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));
+
 	}
 }
 
-- 
2.31.1

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

[Kuppuswamy, Sathyanarayanan]

On 4/28/21 1:18 AM, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> 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@suse.de>
> ---
>   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

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

[Joerg Roedel]

On Wed, Apr 28, 2021 at 10:21:12AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> 
> 
> On 4/28/21 1:18 AM, Joerg Roedel wrote:
> > From: Joerg Roedel <jroedel@suse.de>
> > 
> > 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.

I am not 100% sure, but to me it looks like the pure purpose of the
acpi_pci_osc_support() call was indeed to check whether the firmware is
willing to grant the OS control over some PCIe features.

> > 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?

Not much other benefit, besides some removed code. But those messages
can confuse the system owner and are worth getting rid of imho.

Regards,

	Joerg