[PATCH v3 0/4] PCI/ACPI: Simplify PCIe _OSC feature negotiation

[PATCH v3 0/4] PCI/ACPI: Simplify PCIe _OSC feature negotiation

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Hi,

here is the third version of my patches to simplify the _OSC
negotiation of PCIe features between Linux and the firmware.

This version is a complete rewrite, so there is no changelog to the
previous version. Patches 1-3 are cleanups and small restructurings of
the code as a preparation for patch 4.

The last patch gets rid of the dedicated _OSC query to check for _OSC
support and merges that functionality into acpi_pci_osc_control_set().

This allows to simplify and/or remove other functions and consilidate
error handling in negotiate_os_control().

I have tested the patches with and without 'pcie_ports=compat' and
found no regressions on my test machine.

Please review.

Thanks,

	Joerg



Joerg Roedel (4):
  PCI/ACPI: Remove OSC_PCI_SUPPORT_MASKS and OSC_PCI_CONTROL_MASKS
  PCI/ACPI: Move supported and control calculations to separaten
    functions
  PCI/ACPI: Move _OSC query checks to separate function
  PCI/ACPI: Check for _OSC support in acpi_pci_osc_control_set()

 drivers/acpi/pci_root.c | 161 +++++++++++++++++++++-------------------
 include/linux/acpi.h    |   2 -
 2 files changed, 84 insertions(+), 79 deletions(-)

-- 
2.32.0

[PATCH v3 1/4] PCI/ACPI: Remove OSC_PCI_SUPPORT_MASKS and OSC_PCI_CONTROL_MASKS

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

These masks are only used internally in the PCI Host Bridge _OSC
negotiation code, which already makes sure nothing outside of these
masks is set. Remove the masks and simplify the code.

Suggested-by: Bjorn Helgaas <helgass@kernel.org>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/acpi/pci_root.c | 9 +++------
 include/linux/acpi.h    | 2 --
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index d7deedf3548e..0c3030a58219 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -199,18 +199,15 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
 	acpi_status status;
 	u32 result, capbuf[3];
 
-	support &= OSC_PCI_SUPPORT_MASKS;
 	support |= root->osc_support_set;
 
 	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
 	capbuf[OSC_SUPPORT_DWORD] = support;
-	if (control) {
-		*control &= OSC_PCI_CONTROL_MASKS;
+	if (control)
 		capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set;
-	} else {
+	else
 		/* Run _OSC query only with existing controls. */
 		capbuf[OSC_CONTROL_DWORD] = root->osc_control_set;
-	}
 
 	status = acpi_pci_run_osc(root->device->handle, capbuf, &result);
 	if (ACPI_SUCCESS(status)) {
@@ -357,7 +354,7 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 r
 	if (!mask)
 		return AE_BAD_PARAMETER;
 
-	ctrl = *mask & OSC_PCI_CONTROL_MASKS;
+	ctrl = *mask;
 	if ((ctrl & req) != req)
 		return AE_TYPE;
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 72e4f7fd268c..c6dba5f21384 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -577,7 +577,6 @@ extern u32 osc_sb_native_usb4_control;
 #define OSC_PCI_MSI_SUPPORT			0x00000010
 #define OSC_PCI_EDR_SUPPORT			0x00000080
 #define OSC_PCI_HPX_TYPE_3_SUPPORT		0x00000100
-#define OSC_PCI_SUPPORT_MASKS			0x0000019f
 
 /* PCI Host Bridge _OSC: Capabilities DWORD 3: Control Field */
 #define OSC_PCI_EXPRESS_NATIVE_HP_CONTROL	0x00000001
@@ -587,7 +586,6 @@ extern u32 osc_sb_native_usb4_control;
 #define OSC_PCI_EXPRESS_CAPABILITY_CONTROL	0x00000010
 #define OSC_PCI_EXPRESS_LTR_CONTROL		0x00000020
 #define OSC_PCI_EXPRESS_DPC_CONTROL		0x00000080
-#define OSC_PCI_CONTROL_MASKS			0x000000bf
 
 #define ACPI_GSB_ACCESS_ATTRIB_QUICK		0x00000002
 #define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV         0x00000004
-- 
2.32.0

[PATCH v3 2/4] PCI/ACPI: Move supported and control calculations to separaten functions

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Move the calculations of supported and controled _OSC features out of
negotiate_os_control into separate functions.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/acpi/pci_root.c | 93 ++++++++++++++++++++++++-----------------
 1 file changed, 55 insertions(+), 38 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 0c3030a58219..ed4e6b55e9bc 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -396,6 +396,59 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 r
 	return AE_OK;
 }
 
+static u32 calculate_support(void)
+{
+	u32 support;
+
+	/*
+	 * All supported architectures that use ACPI have support for
+	 * PCI domains, so we indicate this in _OSC support capabilities.
+	 */
+	support = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
+	support |= OSC_PCI_HPX_TYPE_3_SUPPORT;
+	if (pci_ext_cfg_avail())
+		support |= OSC_PCI_EXT_CONFIG_SUPPORT;
+	if (pcie_aspm_support_enabled())
+		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
+	if (pci_msi_enabled())
+		support |= OSC_PCI_MSI_SUPPORT;
+	if (IS_ENABLED(CONFIG_PCIE_EDR))
+		support |= OSC_PCI_EDR_SUPPORT;
+
+	return support;
+}
+
+static u32 calculate_control(void)
+{
+	u32 control;
+
+	control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
+		| OSC_PCI_EXPRESS_PME_CONTROL;
+
+	if (IS_ENABLED(CONFIG_PCIEASPM))
+		control |= OSC_PCI_EXPRESS_LTR_CONTROL;
+
+	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
+		control |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
+
+	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
+		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
+
+	if (pci_aer_available())
+		control |= OSC_PCI_EXPRESS_AER_CONTROL;
+
+	/*
+	 * Per the Downstream Port Containment Related Enhancements ECN to
+	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
+	 * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
+	 * and EDR.
+	 */
+	if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
+		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+
+	return control;
+}
+
 static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 				 bool is_pcie)
 {
@@ -416,20 +469,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 		return;
 	}
 
-	/*
-	 * All supported architectures that use ACPI have support for
-	 * PCI domains, so we indicate this in _OSC support capabilities.
-	 */
-	support = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
-	support |= OSC_PCI_HPX_TYPE_3_SUPPORT;
-	if (pci_ext_cfg_avail())
-		support |= OSC_PCI_EXT_CONFIG_SUPPORT;
-	if (pcie_aspm_support_enabled())
-		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
-	if (pci_msi_enabled())
-		support |= OSC_PCI_MSI_SUPPORT;
-	if (IS_ENABLED(CONFIG_PCIE_EDR))
-		support |= OSC_PCI_EDR_SUPPORT;
+	support = calculate_support();
 
 	decode_osc_support(root, "OS supports", support);
 	status = acpi_pci_osc_support(root, support);
@@ -456,31 +496,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 		return;
 	}
 
-	control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
-		| OSC_PCI_EXPRESS_PME_CONTROL;
-
-	if (IS_ENABLED(CONFIG_PCIEASPM))
-		control |= OSC_PCI_EXPRESS_LTR_CONTROL;
-
-	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
-		control |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
-
-	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
-		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
-
-	if (pci_aer_available())
-		control |= OSC_PCI_EXPRESS_AER_CONTROL;
-
-	/*
-	 * Per the Downstream Port Containment Related Enhancements ECN to
-	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
-	 * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
-	 * and EDR.
-	 */
-	if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
-		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+	requested = control = calculate_control();
 
-	requested = control;
 	status = acpi_pci_osc_control_set(handle, &control,
 					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
 	if (ACPI_SUCCESS(status)) {
-- 
2.32.0

[PATCH v3 3/4] PCI/ACPI: Move _OSC query checks to separate function

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Move the checks about whether the _OSC controls are requested from the
firmware to a separate function.

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

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index ed4e6b55e9bc..f12e512bcddc 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -449,6 +449,24 @@ static u32 calculate_control(void)
 	return control;
 }
 
+static bool os_control_query_checks(struct acpi_pci_root *root, u32 support)
+{
+	struct acpi_device *device = root->device;
+
+	if (pcie_ports_disabled) {
+		dev_info(&device->dev, "PCIe port services disabled; not requesting _OSC control\n");
+		return false;
+	}
+
+	if ((support & ACPI_PCIE_REQ_SUPPORT) != ACPI_PCIE_REQ_SUPPORT) {
+		decode_osc_support(root, "not requesting OS control; OS requires",
+				   ACPI_PCIE_REQ_SUPPORT);
+		return false;
+	}
+
+	return true;
+}
+
 static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 				 bool is_pcie)
 {
@@ -485,16 +503,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 		return;
 	}
 
-	if (pcie_ports_disabled) {
-		dev_info(&device->dev, "PCIe port services disabled; not requesting _OSC control\n");
+	if (!os_control_query_checks(root, support))
 		return;
-	}
-
-	if ((support & ACPI_PCIE_REQ_SUPPORT) != ACPI_PCIE_REQ_SUPPORT) {
-		decode_osc_support(root, "not requesting OS control; OS requires",
-				   ACPI_PCIE_REQ_SUPPORT);
-		return;
-	}
 
 	requested = control = calculate_control();
 
-- 
2.32.0

[PATCH v3 4/4] PCI/ACPI: Check for _OSC support in acpi_pci_osc_control_set()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Get rid of acpi_pci_osc_support() and check for _OSC supported features
directly in acpi_pci_osc_control_set(). There is no point in doing an
unconditional _OSC query with control=0 even when the kernel later wants
to take control over more features.

This safes one _OSC query and simplifies the code by getting rid of
the acpi_pci_osc_support() function. As a side effect, the !control
checks in acpi_pci_query_osc() can also be removed.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/acpi/pci_root.c | 81 ++++++++++++++++-------------------------
 1 file changed, 32 insertions(+), 49 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index f12e512bcddc..ab2f7dfb0c44 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -203,26 +203,16 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
 
 	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
 	capbuf[OSC_SUPPORT_DWORD] = support;
-	if (control)
-		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)) {
 		root->osc_support_set = support;
-		if (control)
-			*control = result;
+		*control = result;
 	}
 	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;
@@ -345,8 +335,9 @@ 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 support)
 {
+	u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL;
 	struct acpi_pci_root *root;
 	acpi_status status;
 	u32 ctrl, capbuf[3];
@@ -354,22 +345,16 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 r
 	if (!mask)
 		return AE_BAD_PARAMETER;
 
-	ctrl = *mask;
-	if ((ctrl & req) != req)
-		return AE_TYPE;
-
 	root = acpi_pci_find_root(handle);
 	if (!root)
 		return AE_NOT_EXIST;
 
-	*mask = ctrl | root->osc_control_set;
-	/* No need to evaluate _OSC if the control was already granted. */
-	if ((root->osc_control_set & ctrl) == ctrl)
-		return AE_OK;
+	ctrl   = *mask;
+	*mask |= root->osc_control_set;
 
 	/* Need to check the available controls bits before requesting them. */
-	while (*mask) {
-		status = acpi_pci_query_osc(root, root->osc_support_set, mask);
+	do {
+		status = acpi_pci_query_osc(root, support, mask);
 		if (ACPI_FAILURE(status))
 			return status;
 		if (ctrl == *mask)
@@ -377,7 +362,11 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 r
 		decode_osc_control(root, "platform does not support",
 				   ctrl & ~(*mask));
 		ctrl = *mask;
-	}
+	} while (*mask);
+
+	/* No need to request _OSC if the control was already granted. */
+	if ((root->osc_control_set & ctrl) == ctrl)
+		return AE_OK;
 
 	if ((ctrl & req) != req) {
 		decode_osc_control(root, "not requesting control; platform does not support",
@@ -470,7 +459,7 @@ static bool os_control_query_checks(struct acpi_pci_root *root, u32 support)
 static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 				 bool is_pcie)
 {
-	u32 support, control, requested;
+	u32 support, control = 0, requested = 0;
 	acpi_status status;
 	struct acpi_device *device = root->device;
 	acpi_handle handle = device->handle;
@@ -490,28 +479,15 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 	support = calculate_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 (!os_control_query_checks(root, support))
-		return;
 
-	requested = control = calculate_control();
+	if (os_control_query_checks(root, support))
+		requested = control = calculate_control();
 
-	status = acpi_pci_osc_control_set(handle, &control,
-					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
+	status = acpi_pci_osc_control_set(handle, &control, support);
 	if (ACPI_SUCCESS(status)) {
-		decode_osc_control(root, "OS now controls", control);
+		if (control)
+			decode_osc_control(root, "OS now controls", control);
+
 		if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
 			/*
 			 * We have ASPM control, but the FADT indicates that
@@ -522,11 +498,6 @@ 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));
-
 		/*
 		 * We want to disable ASPM here, but aspm_disabled
 		 * needs to remain in its state from boot so that we
@@ -535,6 +506,18 @@ 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;
+
+		if (control) {
+			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.32.0

Re: [PATCH v3 1/4] PCI/ACPI: Remove OSC_PCI_SUPPORT_MASKS and OSC_PCI_CONTROL_MASKS

[Rafael J. Wysocki]

On Tue, Aug 24, 2021 at 2:21 PM Joerg Roedel <joro@8bytes.org> wrote:
>
> From: Joerg Roedel <jroedel@suse.de>
>
> These masks are only used internally in the PCI Host Bridge _OSC
> negotiation code, which already makes sure nothing outside of these
> masks is set. Remove the masks and simplify the code.
>
> Suggested-by: Bjorn Helgaas <helgass@kernel.org>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>

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

> ---
>  drivers/acpi/pci_root.c | 9 +++------
>  include/linux/acpi.h    | 2 --
>  2 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index d7deedf3548e..0c3030a58219 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -199,18 +199,15 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
>         acpi_status status;
>         u32 result, capbuf[3];
>
> -       support &= OSC_PCI_SUPPORT_MASKS;
>         support |= root->osc_support_set;
>
>         capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
>         capbuf[OSC_SUPPORT_DWORD] = support;
> -       if (control) {
> -               *control &= OSC_PCI_CONTROL_MASKS;
> +       if (control)
>                 capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set;
> -       } else {
> +       else
>                 /* Run _OSC query only with existing controls. */
>                 capbuf[OSC_CONTROL_DWORD] = root->osc_control_set;
> -       }
>
>         status = acpi_pci_run_osc(root->device->handle, capbuf, &result);
>         if (ACPI_SUCCESS(status)) {
> @@ -357,7 +354,7 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 r
>         if (!mask)
>                 return AE_BAD_PARAMETER;
>
> -       ctrl = *mask & OSC_PCI_CONTROL_MASKS;
> +       ctrl = *mask;
>         if ((ctrl & req) != req)
>                 return AE_TYPE;
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 72e4f7fd268c..c6dba5f21384 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -577,7 +577,6 @@ extern u32 osc_sb_native_usb4_control;
>  #define OSC_PCI_MSI_SUPPORT                    0x00000010
>  #define OSC_PCI_EDR_SUPPORT                    0x00000080
>  #define OSC_PCI_HPX_TYPE_3_SUPPORT             0x00000100
> -#define OSC_PCI_SUPPORT_MASKS                  0x0000019f
>
>  /* PCI Host Bridge _OSC: Capabilities DWORD 3: Control Field */
>  #define OSC_PCI_EXPRESS_NATIVE_HP_CONTROL      0x00000001
> @@ -587,7 +586,6 @@ extern u32 osc_sb_native_usb4_control;
>  #define OSC_PCI_EXPRESS_CAPABILITY_CONTROL     0x00000010
>  #define OSC_PCI_EXPRESS_LTR_CONTROL            0x00000020
>  #define OSC_PCI_EXPRESS_DPC_CONTROL            0x00000080
> -#define OSC_PCI_CONTROL_MASKS                  0x000000bf
>
>  #define ACPI_GSB_ACCESS_ATTRIB_QUICK           0x00000002
>  #define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV         0x00000004
> --
> 2.32.0
>

Re: [PATCH v3 2/4] PCI/ACPI: Move supported and control calculations to separaten functions

[Rafael J. Wysocki]

On Tue, Aug 24, 2021 at 2:21 PM Joerg Roedel <joro@8bytes.org> wrote:
>
> From: Joerg Roedel <jroedel@suse.de>
>
> Move the calculations of supported and controled _OSC features out of
> negotiate_os_control into separate functions.
>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>

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

> ---
>  drivers/acpi/pci_root.c | 93 ++++++++++++++++++++++++-----------------
>  1 file changed, 55 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 0c3030a58219..ed4e6b55e9bc 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -396,6 +396,59 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 r
>         return AE_OK;
>  }
>
> +static u32 calculate_support(void)
> +{
> +       u32 support;
> +
> +       /*
> +        * All supported architectures that use ACPI have support for
> +        * PCI domains, so we indicate this in _OSC support capabilities.
> +        */
> +       support = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
> +       support |= OSC_PCI_HPX_TYPE_3_SUPPORT;
> +       if (pci_ext_cfg_avail())
> +               support |= OSC_PCI_EXT_CONFIG_SUPPORT;
> +       if (pcie_aspm_support_enabled())
> +               support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
> +       if (pci_msi_enabled())
> +               support |= OSC_PCI_MSI_SUPPORT;
> +       if (IS_ENABLED(CONFIG_PCIE_EDR))
> +               support |= OSC_PCI_EDR_SUPPORT;
> +
> +       return support;
> +}
> +
> +static u32 calculate_control(void)
> +{
> +       u32 control;
> +
> +       control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
> +               | OSC_PCI_EXPRESS_PME_CONTROL;
> +
> +       if (IS_ENABLED(CONFIG_PCIEASPM))
> +               control |= OSC_PCI_EXPRESS_LTR_CONTROL;
> +
> +       if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
> +               control |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
> +
> +       if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
> +               control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
> +
> +       if (pci_aer_available())
> +               control |= OSC_PCI_EXPRESS_AER_CONTROL;
> +
> +       /*
> +        * Per the Downstream Port Containment Related Enhancements ECN to
> +        * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
> +        * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
> +        * and EDR.
> +        */
> +       if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
> +               control |= OSC_PCI_EXPRESS_DPC_CONTROL;
> +
> +       return control;
> +}
> +
>  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>                                  bool is_pcie)
>  {
> @@ -416,20 +469,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>                 return;
>         }
>
> -       /*
> -        * All supported architectures that use ACPI have support for
> -        * PCI domains, so we indicate this in _OSC support capabilities.
> -        */
> -       support = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
> -       support |= OSC_PCI_HPX_TYPE_3_SUPPORT;
> -       if (pci_ext_cfg_avail())
> -               support |= OSC_PCI_EXT_CONFIG_SUPPORT;
> -       if (pcie_aspm_support_enabled())
> -               support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
> -       if (pci_msi_enabled())
> -               support |= OSC_PCI_MSI_SUPPORT;
> -       if (IS_ENABLED(CONFIG_PCIE_EDR))
> -               support |= OSC_PCI_EDR_SUPPORT;
> +       support = calculate_support();
>
>         decode_osc_support(root, "OS supports", support);
>         status = acpi_pci_osc_support(root, support);
> @@ -456,31 +496,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>                 return;
>         }
>
> -       control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
> -               | OSC_PCI_EXPRESS_PME_CONTROL;
> -
> -       if (IS_ENABLED(CONFIG_PCIEASPM))
> -               control |= OSC_PCI_EXPRESS_LTR_CONTROL;
> -
> -       if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
> -               control |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
> -
> -       if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
> -               control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
> -
> -       if (pci_aer_available())
> -               control |= OSC_PCI_EXPRESS_AER_CONTROL;
> -
> -       /*
> -        * Per the Downstream Port Containment Related Enhancements ECN to
> -        * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
> -        * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
> -        * and EDR.
> -        */
> -       if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
> -               control |= OSC_PCI_EXPRESS_DPC_CONTROL;
> +       requested = control = calculate_control();
>
> -       requested = control;
>         status = acpi_pci_osc_control_set(handle, &control,
>                                           OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
>         if (ACPI_SUCCESS(status)) {
> --
> 2.32.0
>

Re: [PATCH v3 3/4] PCI/ACPI: Move _OSC query checks to separate function

[Rafael J. Wysocki]

On Tue, Aug 24, 2021 at 2:21 PM Joerg Roedel <joro@8bytes.org> wrote:
>
> From: Joerg Roedel <jroedel@suse.de>
>
> Move the checks about whether the _OSC controls are requested from the
> firmware to a separate function.
>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>

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

> ---
>  drivers/acpi/pci_root.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index ed4e6b55e9bc..f12e512bcddc 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -449,6 +449,24 @@ static u32 calculate_control(void)
>         return control;
>  }
>
> +static bool os_control_query_checks(struct acpi_pci_root *root, u32 support)
> +{
> +       struct acpi_device *device = root->device;
> +
> +       if (pcie_ports_disabled) {
> +               dev_info(&device->dev, "PCIe port services disabled; not requesting _OSC control\n");
> +               return false;
> +       }
> +
> +       if ((support & ACPI_PCIE_REQ_SUPPORT) != ACPI_PCIE_REQ_SUPPORT) {
> +               decode_osc_support(root, "not requesting OS control; OS requires",
> +                                  ACPI_PCIE_REQ_SUPPORT);
> +               return false;
> +       }
> +
> +       return true;
> +}
> +
>  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>                                  bool is_pcie)
>  {
> @@ -485,16 +503,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>                 return;
>         }
>
> -       if (pcie_ports_disabled) {
> -               dev_info(&device->dev, "PCIe port services disabled; not requesting _OSC control\n");
> +       if (!os_control_query_checks(root, support))
>                 return;
> -       }
> -
> -       if ((support & ACPI_PCIE_REQ_SUPPORT) != ACPI_PCIE_REQ_SUPPORT) {
> -               decode_osc_support(root, "not requesting OS control; OS requires",
> -                                  ACPI_PCIE_REQ_SUPPORT);
> -               return;
> -       }
>
>         requested = control = calculate_control();
>
> --
> 2.32.0
>

Re: [PATCH v3 4/4] PCI/ACPI: Check for _OSC support in acpi_pci_osc_control_set()

[Rafael J. Wysocki]

On Tue, Aug 24, 2021 at 2:21 PM Joerg Roedel <joro@8bytes.org> wrote:
>
> From: Joerg Roedel <jroedel@suse.de>
>
> Get rid of acpi_pci_osc_support() and check for _OSC supported features
> directly in acpi_pci_osc_control_set(). There is no point in doing an
> unconditional _OSC query with control=0 even when the kernel later wants
> to take control over more features.
>
> This safes one _OSC query and simplifies the code by getting rid of

s/safes/saves/

> the acpi_pci_osc_support() function. As a side effect, the !control
> checks in acpi_pci_query_osc() can also be removed.
>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>

Apart from the typo above, I haven't found any issues in the patch, so

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

> ---
>  drivers/acpi/pci_root.c | 81 ++++++++++++++++-------------------------
>  1 file changed, 32 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index f12e512bcddc..ab2f7dfb0c44 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -203,26 +203,16 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
>
>         capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
>         capbuf[OSC_SUPPORT_DWORD] = support;
> -       if (control)
> -               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)) {
>                 root->osc_support_set = support;
> -               if (control)
> -                       *control = result;
> +               *control = result;
>         }
>         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;
> @@ -345,8 +335,9 @@ 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 support)
>  {
> +       u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL;
>         struct acpi_pci_root *root;
>         acpi_status status;
>         u32 ctrl, capbuf[3];
> @@ -354,22 +345,16 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 r
>         if (!mask)
>                 return AE_BAD_PARAMETER;
>
> -       ctrl = *mask;
> -       if ((ctrl & req) != req)
> -               return AE_TYPE;
> -
>         root = acpi_pci_find_root(handle);
>         if (!root)
>                 return AE_NOT_EXIST;
>
> -       *mask = ctrl | root->osc_control_set;
> -       /* No need to evaluate _OSC if the control was already granted. */
> -       if ((root->osc_control_set & ctrl) == ctrl)
> -               return AE_OK;
> +       ctrl   = *mask;
> +       *mask |= root->osc_control_set;
>
>         /* Need to check the available controls bits before requesting them. */
> -       while (*mask) {
> -               status = acpi_pci_query_osc(root, root->osc_support_set, mask);
> +       do {
> +               status = acpi_pci_query_osc(root, support, mask);
>                 if (ACPI_FAILURE(status))
>                         return status;
>                 if (ctrl == *mask)
> @@ -377,7 +362,11 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 r
>                 decode_osc_control(root, "platform does not support",
>                                    ctrl & ~(*mask));
>                 ctrl = *mask;
> -       }
> +       } while (*mask);
> +
> +       /* No need to request _OSC if the control was already granted. */
> +       if ((root->osc_control_set & ctrl) == ctrl)
> +               return AE_OK;
>
>         if ((ctrl & req) != req) {
>                 decode_osc_control(root, "not requesting control; platform does not support",
> @@ -470,7 +459,7 @@ static bool os_control_query_checks(struct acpi_pci_root *root, u32 support)
>  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>                                  bool is_pcie)
>  {
> -       u32 support, control, requested;
> +       u32 support, control = 0, requested = 0;
>         acpi_status status;
>         struct acpi_device *device = root->device;
>         acpi_handle handle = device->handle;
> @@ -490,28 +479,15 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>         support = calculate_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 (!os_control_query_checks(root, support))
> -               return;
>
> -       requested = control = calculate_control();
> +       if (os_control_query_checks(root, support))
> +               requested = control = calculate_control();
>
> -       status = acpi_pci_osc_control_set(handle, &control,
> -                                         OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
> +       status = acpi_pci_osc_control_set(handle, &control, support);
>         if (ACPI_SUCCESS(status)) {
> -               decode_osc_control(root, "OS now controls", control);
> +               if (control)
> +                       decode_osc_control(root, "OS now controls", control);
> +
>                 if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
>                         /*
>                          * We have ASPM control, but the FADT indicates that
> @@ -522,11 +498,6 @@ 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));
> -
>                 /*
>                  * We want to disable ASPM here, but aspm_disabled
>                  * needs to remain in its state from boot so that we
> @@ -535,6 +506,18 @@ 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;
> +
> +               if (control) {
> +                       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.32.0
>

Re: [PATCH v3 0/4] PCI/ACPI: Simplify PCIe _OSC feature negotiation

[Rafael J. Wysocki]

On Tue, Aug 24, 2021 at 2:21 PM Joerg Roedel <joro@8bytes.org> wrote:
>
> From: Joerg Roedel <jroedel@suse.de>
>
> Hi,
>
> here is the third version of my patches to simplify the _OSC
> negotiation of PCIe features between Linux and the firmware.
>
> This version is a complete rewrite, so there is no changelog to the
> previous version. Patches 1-3 are cleanups and small restructurings of
> the code as a preparation for patch 4.
>
> The last patch gets rid of the dedicated _OSC query to check for _OSC
> support and merges that functionality into acpi_pci_osc_control_set().
>
> This allows to simplify and/or remove other functions and consilidate
> error handling in negotiate_os_control().
>
> I have tested the patches with and without 'pcie_ports=compat' and
> found no regressions on my test machine.

I have reviewed the patches, so if you want me to queue up this
series, please let me know.

Thanks,
Rafael


> Joerg Roedel (4):
>   PCI/ACPI: Remove OSC_PCI_SUPPORT_MASKS and OSC_PCI_CONTROL_MASKS
>   PCI/ACPI: Move supported and control calculations to separaten
>     functions
>   PCI/ACPI: Move _OSC query checks to separate function
>   PCI/ACPI: Check for _OSC support in acpi_pci_osc_control_set()
>
>  drivers/acpi/pci_root.c | 161 +++++++++++++++++++++-------------------
>  include/linux/acpi.h    |   2 -
>  2 files changed, 84 insertions(+), 79 deletions(-)
>
> --
> 2.32.0
>

Re: [PATCH v3 0/4] PCI/ACPI: Simplify PCIe _OSC feature negotiation

[Rafael J. Wysocki]

On Wed, Sep 1, 2021 at 9:33 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Aug 24, 2021 at 2:21 PM Joerg Roedel <joro@8bytes.org> wrote:
> >
> > From: Joerg Roedel <jroedel@suse.de>
> >
> > Hi,
> >
> > here is the third version of my patches to simplify the _OSC
> > negotiation of PCIe features between Linux and the firmware.
> >
> > This version is a complete rewrite, so there is no changelog to the
> > previous version. Patches 1-3 are cleanups and small restructurings of
> > the code as a preparation for patch 4.
> >
> > The last patch gets rid of the dedicated _OSC query to check for _OSC
> > support and merges that functionality into acpi_pci_osc_control_set().
> >
> > This allows to simplify and/or remove other functions and consilidate
> > error handling in negotiate_os_control().
> >
> > I have tested the patches with and without 'pcie_ports=compat' and
> > found no regressions on my test machine.
>
> I have reviewed the patches, so if you want me to queue up this
> series, please let me know.

Should I assume that Bjorn will be taking it?

Re: [PATCH v3 0/4] PCI/ACPI: Simplify PCIe _OSC feature negotiation

[Joerg Roedel]

On Mon, Sep 13, 2021 at 06:14:38PM +0200, Rafael J. Wysocki wrote:
> Should I assume that Bjorn will be taking it?

No idea, checking git-log on the file shows mixed Signed-off-bys from
both of you. Bjorn?


	Joerg

Re: [PATCH v3 0/4] PCI/ACPI: Simplify PCIe _OSC feature negotiation

[Bjorn Helgaas]

On Tue, Sep 14, 2021 at 8:50 AM Joerg Roedel <joro@8bytes.org> wrote:
>
> On Mon, Sep 13, 2021 at 06:14:38PM +0200, Rafael J. Wysocki wrote:
> > Should I assume that Bjorn will be taking it?
>
> No idea, checking git-log on the file shows mixed Signed-off-bys from
> both of you. Bjorn?

I'll merge it, hopefully this week.  Thanks!

Bjorn

Re: [PATCH v3 0/4] PCI/ACPI: Simplify PCIe _OSC feature negotiation

[Bjorn Helgaas]

On Tue, Aug 24, 2021 at 02:20:50PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Hi,
> 
> here is the third version of my patches to simplify the _OSC
> negotiation of PCIe features between Linux and the firmware.
> 
> This version is a complete rewrite, so there is no changelog to the
> previous version. Patches 1-3 are cleanups and small restructurings of
> the code as a preparation for patch 4.
> 
> The last patch gets rid of the dedicated _OSC query to check for _OSC
> support and merges that functionality into acpi_pci_osc_control_set().
> 
> This allows to simplify and/or remove other functions and consilidate
> error handling in negotiate_os_control().
> 
> I have tested the patches with and without 'pcie_ports=compat' and
> found no regressions on my test machine.
> 
> Please review.
> 
> Thanks,
> 
> 	Joerg
> 
> 
> 
> Joerg Roedel (4):
>   PCI/ACPI: Remove OSC_PCI_SUPPORT_MASKS and OSC_PCI_CONTROL_MASKS
>   PCI/ACPI: Move supported and control calculations to separaten
>     functions
>   PCI/ACPI: Move _OSC query checks to separate function
>   PCI/ACPI: Check for _OSC support in acpi_pci_osc_control_set()
> 
>  drivers/acpi/pci_root.c | 161 +++++++++++++++++++++-------------------
>  include/linux/acpi.h    |   2 -
>  2 files changed, 84 insertions(+), 79 deletions(-)

Applied with Rafael's reviewed-by to pci/acpi for v5.16, thanks!

Re: [PATCH v3 0/4] PCI/ACPI: Simplify PCIe _OSC feature negotiation

[Joerg Roedel]

On Tue, Sep 28, 2021 at 04:21:19PM -0500, Bjorn Helgaas wrote:
> Applied with Rafael's reviewed-by to pci/acpi for v5.16, thanks!

Thanks Bjorn!