[PATCH] PCI/ACPI: Improve _OSC control request granularity

[PATCH] PCI/ACPI: Improve _OSC control request granularity

[Aaron Sierra]

This patch reorganizes negotiate_os_control() to be less ASPM-centric in
order to:

    1. allow other features (notably AER) to work without enabling ASPM
    2. better isolate feature-specific tests for readability/maintenance

Each feature (ASPM, PCIe hotplug, SHPC hotplug, and AER) now has its own
inline function for setting its _OSC control requests.

Part of making this function more generic, required eliminating a test
for overall success/failure that previously caused two different types
of messages to be printed. Now, printed messages are streamlined to
always show requested _OSC control versus what was granted.

Previous output (success):

  acpi PNP0A08:00: _OSC: OS now controls [PME AER PCIeCapability LTR]

Previous output (failure):

  acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
  acpi PNP0A08:00: _OSC: platform willing to grant []

Now:

  acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
  acpi PNP0A08:00: _OSC: platform granted [PME AER PCIeCapability LTR]

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/acpi/pci_root.c | 161 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 116 insertions(+), 45 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 7433035..8dd6720 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -53,9 +53,10 @@ static int acpi_pci_root_scan_dependent(struct acpi_device *adev)
 }
 
 #define ACPI_PCIE_REQ_SUPPORT (OSC_PCI_EXT_CONFIG_SUPPORT \
-				| OSC_PCI_ASPM_SUPPORT \
-				| OSC_PCI_CLOCK_PM_SUPPORT \
 				| OSC_PCI_MSI_SUPPORT)
+#define ACPI_PCIE_ASPM_SUPPORT (ACPI_PCIE_REQ_SUPPORT \
+				| OSC_PCI_ASPM_SUPPORT \
+				| OSC_PCI_CLOCK_PM_SUPPORT)
 
 static const struct acpi_device_id root_device_ids[] = {
 	{"PNP0A03", 0},
@@ -421,10 +422,101 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
 }
 EXPORT_SYMBOL(acpi_pci_osc_control_set);
 
+static int __osc_check_support(struct acpi_pci_root *root, u32 support,
+			       u32 required, const char *feature)
+{
+	acpi_status status;
+
+	if ((support & required) != required)
+		return -ENODEV;
+
+	status = acpi_pci_osc_support(root, support);
+	if (ACPI_FAILURE(status)) {
+		dev_info(&root->device->dev, "_OSC failed (%s); disabling %s\n",
+			 acpi_format_exception(status), feature);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static inline int __osc_set_aspm_control(struct acpi_pci_root *root,
+					 u32 support, u32 *control)
+{
+	int rc;
+
+	if (!IS_ENABLED(CONFIG_PCIEASPM))
+		return -ENODEV;
+
+	rc = __osc_check_support(root, support, ACPI_PCIE_ASPM_SUPPORT, "ASPM");
+	if (rc)
+		return rc;
+
+	*control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
+		    OSC_PCI_EXPRESS_LTR_CONTROL |
+		    OSC_PCI_EXPRESS_PME_CONTROL;
+
+	return 0;
+}
+
+static inline bool __osc_have_aspm_control(u32 control)
+{
+	u32 required = OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
+		       OSC_PCI_EXPRESS_LTR_CONTROL |
+		       OSC_PCI_EXPRESS_PME_CONTROL;
+
+	return (control & required) == required;
+}
+
+static inline void __osc_set_pciehp_control(struct acpi_pci_root *root,
+					    u32 support, u32 *control)
+{
+	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
+		return;
+
+	if (__osc_check_support(root, support, ACPI_PCIE_REQ_SUPPORT,
+				"PCIeHotplug"))
+		return;
+
+	*control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
+		    OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
+}
+
+static inline void __osc_set_shpchp_control(struct acpi_pci_root *root,
+					    u32 support, u32 *control)
+{
+	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
+		return;
+
+	if (__osc_check_support(root, support, ACPI_PCIE_REQ_SUPPORT,
+				"SHPCHotplug"))
+		return;
+
+	*control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
+		    OSC_PCI_SHPC_NATIVE_HP_CONTROL;
+}
+
+static inline void __osc_set_aer_control(struct acpi_pci_root *root,
+					 u32 support, u32 *control)
+{
+	if (!pci_aer_available())
+		return;
+
+	if (aer_acpi_firmware_first()) {
+		dev_info(&root->device->dev, "PCIe AER handled by firmware\n");
+		return;
+	}
+
+	if (__osc_check_support(root, support, ACPI_PCIE_REQ_SUPPORT, "AER"))
+		return;
+
+	*control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
+		    OSC_PCI_EXPRESS_AER_CONTROL;
+}
+
 static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
 {
 	u32 support, control, requested;
-	acpi_status status;
 	struct acpi_device *device = root->device;
 	acpi_handle handle = device->handle;
 
@@ -440,6 +532,12 @@ 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");
+		return;
+	}
+
 	/*
 	 * All supported architectures that use ACPI have support for
 	 * PCI domains, so we indicate this in _OSC support capabilities.
@@ -453,50 +551,26 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
 		support |= OSC_PCI_MSI_SUPPORT;
 
 	decode_osc_support(root, "OS supports", support);
-	status = acpi_pci_osc_support(root, support);
-	if (ACPI_FAILURE(status)) {
-		dev_info(&device->dev, "_OSC failed (%s); disabling ASPM\n",
-			 acpi_format_exception(status));
+
+	control = 0;
+	if (__osc_set_aspm_control(root, support, &control))
 		*no_aspm = 1;
-		return;
-	}
 
-	if (pcie_ports_disabled) {
-		dev_info(&device->dev, "PCIe port services disabled; not requesting _OSC control\n");
-		return;
-	}
+	__osc_set_pciehp_control(root, support, &control);
+	__osc_set_shpchp_control(root, support, &control);
+	__osc_set_aer_control(root, support, &control);
 
-	if ((support & ACPI_PCIE_REQ_SUPPORT) != ACPI_PCIE_REQ_SUPPORT) {
-		decode_osc_support(root, "not requesting OS control; OS requires",
-				   ACPI_PCIE_REQ_SUPPORT);
+	if (!control) {
+		dev_info(&device->dev, "_OSC: not requesting OS control\n");
 		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()) {
-		if (aer_acpi_firmware_first())
-			dev_info(&device->dev,
-				 "PCIe AER handled by firmware\n");
-		else
-			control |= OSC_PCI_EXPRESS_AER_CONTROL;
-	}
-
 	requested = control;
-	status = acpi_pci_osc_control_set(handle, &control,
-					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
-	if (ACPI_SUCCESS(status)) {
-		decode_osc_control(root, "OS now controls", control);
+	acpi_pci_osc_control_set(handle, &control, 0);
+	decode_osc_control(root, "OS requested", requested);
+	decode_osc_control(root, "platform granted", control);
+
+	if (__osc_have_aspm_control(control)) {
 		if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
 			/*
 			 * We have ASPM control, but the FADT indicates that
@@ -506,11 +580,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
 			dev_info(&device->dev, "FADT indicates ASPM is unsupported, using BIOS configuration\n");
 			*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 failed (%s); disabling ASPM\n",
-			acpi_format_exception(status));
+	} else if (!*no_aspm) {
+		dev_info(&device->dev, "_OSC failed; disabling ASPM\n");
 
 		/*
 		 * We want to disable ASPM here, but aspm_disabled
-- 
2.7.4

[PATCH v2 0/2] Improve _OSC control request granularity

[Aaron Sierra]

These patches allow PCIe AER to function in a kernel without
CONFIG_PCIEASPM enabled. There is no explicit Kbuild dependency between
the two and there are, in my opinion, legitimate reasons for no implicit
dependency to exist either.

Previously with ASPM:

acpi PNP0A03:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
acpi PNP0A03:00: _OSC: OS now controls [PME AER PCIeCapability LTR]

Previously without ASPM:

acpi PNP0A03:00: _OSC: OS supports [ExtendedConfig Segments MSI]
acpi PNP0A03:00: _OSC: not requesting OS control; OS requires [ExtendedConfig ASPM ClockPM MSI]

Now with ASPM:

acpi PNP0A03:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
acpi PNP0A03:00: _OSC: OS requested [PME AER PCIeCapability LTR]
acpi PNP0A03:00: _OSC: platform granted [PME AER PCIeCapability LTR]

Now without ASPM:

acpi PNP0A03:00: _OSC: OS supports [ExtendedConfig Segments MSI]
acpi PNP0A03:00: _OSC: OS requested [AER PCIeCapability]
acpi PNP0A03:00: _OSC: platform granted [AER PCIeCapability]

This is v2 of the patchset, which includes these changes from v1:
  * Rebased from the mainline kernel (4.19-rc7) to Bjorn Helgaas's
    pci/aspm branch [1]
  * Factored moving the pcie_ports_disabled check to the top of
    negotiate_os_control into its own patch
  * Simplified new __osc_check_support function and renamed it to
    __osc_have_support
  * No longer messes with _OSC general availability test and related
    error message (i.e. _OSC failed ... disabling ASPM)

[1] https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git

Aaron Sierra (2):
  PCI/ACPI: Move _OSC test for native services to top
  PCI/ACPI: Improve _OSC control request granularity

 drivers/acpi/pci_root.c | 129 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 91 insertions(+), 38 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] PCI/ACPI: Move _OSC test for native services to top

[Aaron Sierra]

Move the simple test for when PCIe native services are disabled
closer to the top, prior to where things get more complicated.

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/acpi/pci_root.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 707aafc..eb9f14e 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -441,6 +441,12 @@ 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");
+		return;
+	}
+
 	/*
 	 * All supported architectures that use ACPI have support for
 	 * PCI domains, so we indicate this in _OSC support capabilities.
@@ -468,11 +474,6 @@ 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");
-		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);
-- 
2.7.4

[PATCH v2 2/2] PCI/ACPI: Improve _OSC control request granularity

[Aaron Sierra]

This patch reorganizes negotiate_os_control() to be less ASPM-centric in
order to:

    1. allow other features (notably AER) to work without enabling ASPM
    2. better isolate feature-specific tests for readability/maintenance

Each feature (ASPM, PCIe hotplug, SHPC hotplug, and AER) now has its own
inline function for setting its _OSC control requests.

Part of making this function more generic, required eliminating a test
for overall success/failure that previously caused two different types
of messages to be printed. Now, printed messages are streamlined to
always show requested _OSC control versus what was granted.

Previous output (success):

  acpi PNP0A08:00: _OSC: OS now controls [PME AER PCIeCapability LTR]

Previous output (failure):

  acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
  acpi PNP0A08:00: _OSC: platform willing to grant []

Now:

  acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
  acpi PNP0A08:00: _OSC: platform granted [PME AER PCIeCapability LTR]

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/acpi/pci_root.c | 118 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 85 insertions(+), 33 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index eb9f14e..9685aba 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -53,9 +53,10 @@ static int acpi_pci_root_scan_dependent(struct acpi_device *adev)
 }
 
 #define ACPI_PCIE_REQ_SUPPORT (OSC_PCI_EXT_CONFIG_SUPPORT \
-				| OSC_PCI_ASPM_SUPPORT \
-				| OSC_PCI_CLOCK_PM_SUPPORT \
 				| OSC_PCI_MSI_SUPPORT)
+#define ACPI_PCIE_ASPM_SUPPORT (ACPI_PCIE_REQ_SUPPORT \
+				| OSC_PCI_ASPM_SUPPORT \
+				| OSC_PCI_CLOCK_PM_SUPPORT)
 
 static const struct acpi_device_id root_device_ids[] = {
 	{"PNP0A03", 0},
@@ -421,6 +422,72 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
 }
 EXPORT_SYMBOL(acpi_pci_osc_control_set);
 
+static inline bool __osc_have_support(u32 support, u32 required)
+{
+	return ((support & required) == required);
+}
+
+static inline int __osc_set_aspm_control(struct acpi_pci_root *root,
+					 u32 support, u32 *control)
+{
+	if (!IS_ENABLED(CONFIG_PCIEASPM) ||
+	    !__osc_have_support(support, ACPI_PCIE_ASPM_SUPPORT))
+		return -ENODEV;
+
+	*control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
+		    OSC_PCI_EXPRESS_LTR_CONTROL |
+		    OSC_PCI_EXPRESS_PME_CONTROL;
+
+	return 0;
+}
+
+static inline bool __osc_have_aspm_control(u32 control)
+{
+	u32 required = OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
+		       OSC_PCI_EXPRESS_LTR_CONTROL |
+		       OSC_PCI_EXPRESS_PME_CONTROL;
+
+	return __osc_have_support(control, required);
+}
+
+static inline void __osc_set_pciehp_control(struct acpi_pci_root *root,
+					    u32 support, u32 *control)
+{
+	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE) ||
+	    !__osc_have_support(support, ACPI_PCIE_REQ_SUPPORT))
+		return;
+
+	*control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
+		    OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
+}
+
+static inline void __osc_set_shpchp_control(struct acpi_pci_root *root,
+					    u32 support, u32 *control)
+{
+	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC) ||
+	    !__osc_have_support(support, ACPI_PCIE_REQ_SUPPORT))
+		return;
+
+	*control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
+		    OSC_PCI_SHPC_NATIVE_HP_CONTROL;
+}
+
+static inline void __osc_set_aer_control(struct acpi_pci_root *root,
+					 u32 support, u32 *control)
+{
+	if (!pci_aer_available() ||
+	    !__osc_have_support(support, ACPI_PCIE_REQ_SUPPORT))
+		return;
+
+	if (aer_acpi_firmware_first()) {
+		dev_info(&root->device->dev, "PCIe AER handled by firmware\n");
+		return;
+	}
+
+	*control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
+		    OSC_PCI_EXPRESS_AER_CONTROL;
+}
+
 static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 				 bool is_pcie)
 {
@@ -474,37 +541,25 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 		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;
-	}
-
-	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;
+	control = 0;
+	if (__osc_set_aspm_control(root, support, &control))
+		*no_aspm = 1;
 
-	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
-		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
+	__osc_set_pciehp_control(root, support, &control);
+	__osc_set_shpchp_control(root, support, &control);
+	__osc_set_aer_control(root, support, &control);
 
-	if (pci_aer_available()) {
-		if (aer_acpi_firmware_first())
-			dev_info(&device->dev,
-				 "PCIe AER handled by firmware\n");
-		else
-			control |= OSC_PCI_EXPRESS_AER_CONTROL;
+	if (!control) {
+		dev_info(&device->dev, "_OSC: not requesting OS control\n");
+		return;
 	}
 
 	requested = control;
-	status = acpi_pci_osc_control_set(handle, &control,
-					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
-	if (ACPI_SUCCESS(status)) {
-		decode_osc_control(root, "OS now controls", control);
+	acpi_pci_osc_control_set(handle, &control, 0);
+	decode_osc_control(root, "OS requested", requested);
+	decode_osc_control(root, "platform granted", control);
+
+	if (__osc_have_aspm_control(control)) {
 		if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
 			/*
 			 * We have ASPM control, but the FADT indicates that
@@ -514,11 +569,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 			dev_info(&device->dev, "FADT indicates ASPM is unsupported, using BIOS configuration\n");
 			*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 failed (%s); disabling ASPM\n",
-			acpi_format_exception(status));
+	} else if (!*no_aspm) {
+		dev_info(&device->dev, "_OSC failed; disabling ASPM\n");
 
 		/*
 		 * We want to disable ASPM here, but aspm_disabled
-- 
2.7.4

Re: [PATCH v2 1/2] PCI/ACPI: Move _OSC test for native services to top

[Bjorn Helgaas]

Hi Aaron,

On Thu, Oct 25, 2018 at 11:01:31AM -0500, Aaron Sierra wrote:
> Move the simple test for when PCIe native services are disabled
> closer to the top, prior to where things get more complicated.
> 
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
>  drivers/acpi/pci_root.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 707aafc..eb9f14e 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -441,6 +441,12 @@ 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");
> +		return;
> +	}

Today we always set "*no_aspm = 1" if _OSC fails, which means we later
call pcie_no_aspm().

After this patch, when pcie_ports_disabled is "true", we don't even try to
evaluate _OSC, and we will never set *no_aspm, so we will never call
pcie_no_aspm() when pcie_ports_disabled is "true", which happens in these
cases:

  1) CONFIG_PCIEPORTBUS is unset, or
  2) CONFIG_PCIEPORTBUS=y and we booted with "pcie_ports=compat"

Case 1) isn't a problem because pcie_no_aspm() is only implemented when
CONFIG_PCIEASPM=y, and CONFIG_PCIEASPM depends on CONFIG_PCIEPORTBUS, so in
this case today we only call the empty stub pcie_no_aspm() function.

But case 2) is a behavior change that seems unintended.

Even though CONFIG_PCIEASPM depends on CONFIG_PCIEPORTBUS, ASPM doesn't
actually *use* anything provided by PCIEPORTBUS, so I think the ASPM code
is still active and useful even when we boot with "pcie_ports=compat".

Whether CONFIG_PCIEASPM should depend on CONFIG_PCIEPORTBUS is another
question.  I tend to think maybe it should not, but that's an orthogonal
question.

>  	/*
>  	 * All supported architectures that use ACPI have support for
>  	 * PCI domains, so we indicate this in _OSC support capabilities.
> @@ -468,11 +474,6 @@ 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");
> -		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);
> -- 
> 2.7.4
> 

Re: [PATCH v2 2/2] PCI/ACPI: Improve _OSC control request granularity

[Bjorn Helgaas]

On Thu, Oct 25, 2018 at 11:01:32AM -0500, Aaron Sierra wrote:
> This patch reorganizes negotiate_os_control() to be less ASPM-centric in
> order to:
> 
>     1. allow other features (notably AER) to work without enabling ASPM
>     2. better isolate feature-specific tests for readability/maintenance

I really like this idea; thanks for working it up!

> Each feature (ASPM, PCIe hotplug, SHPC hotplug, and AER) now has its own
> inline function for setting its _OSC control requests.
> 
> Part of making this function more generic, required eliminating a test
> for overall success/failure that previously caused two different types
> of messages to be printed. Now, printed messages are streamlined to
> always show requested _OSC control versus what was granted.
> 
> Previous output (success):
> 
>   acpi PNP0A08:00: _OSC: OS now controls [PME AER PCIeCapability LTR]
> 
> Previous output (failure):
> 
>   acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
>   acpi PNP0A08:00: _OSC: platform willing to grant []
> 
> Now:
> 
>   acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
>   acpi PNP0A08:00: _OSC: platform granted [PME AER PCIeCapability LTR]
> 
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
>  drivers/acpi/pci_root.c | 118 ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 85 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index eb9f14e..9685aba 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -53,9 +53,10 @@ static int acpi_pci_root_scan_dependent(struct acpi_device *adev)
>  }
>  
>  #define ACPI_PCIE_REQ_SUPPORT (OSC_PCI_EXT_CONFIG_SUPPORT \
> -				| OSC_PCI_ASPM_SUPPORT \
> -				| OSC_PCI_CLOCK_PM_SUPPORT \
>  				| OSC_PCI_MSI_SUPPORT)
> +#define ACPI_PCIE_ASPM_SUPPORT (ACPI_PCIE_REQ_SUPPORT \
> +				| OSC_PCI_ASPM_SUPPORT \
> +				| OSC_PCI_CLOCK_PM_SUPPORT)
>  
>  static const struct acpi_device_id root_device_ids[] = {
>  	{"PNP0A03", 0},
> @@ -421,6 +422,72 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
>  }
>  EXPORT_SYMBOL(acpi_pci_osc_control_set);
>  
> +static inline bool __osc_have_support(u32 support, u32 required)
> +{
> +	return ((support & required) == required);
> +}

I'm not really a fan of function names with leading underscores, except
maybe for "raw" things that don't acquire locks.

> +static inline int __osc_set_aspm_control(struct acpi_pci_root *root,
> +					 u32 support, u32 *control)
> +{
> +	if (!IS_ENABLED(CONFIG_PCIEASPM) ||
> +	    !__osc_have_support(support, ACPI_PCIE_ASPM_SUPPORT))
> +		return -ENODEV;
> +
> +	*control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
> +		    OSC_PCI_EXPRESS_LTR_CONTROL |
> +		    OSC_PCI_EXPRESS_PME_CONTROL;

I think this would be more readable if we could avoid the double
negatives, e.g.,

  if (IS_ENABLED(CONFIG_PCIEASPM) &&
      __osc_have_support(support, ACPI_PCIE_ASPM_SUPPORT)) {
          *control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
                      OSC_PCI_EXPRESS_LTR_CONTROL |
                      OSC_PCI_EXPRESS_PME_CONTROL;
          return 0;
  }

  return -ENODEV;

Since the caller ignores the return values anyway, I wonder if this
could also work by *returning* the new mask bits instead of using
"control" as a reference parameter, e.g.,

  if (IS_ENABLED(...))
    return OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
           OSC_PCI_EXPRESS_LTR_CONTROL |
           OSC_PCI_EXPRESS_PME_CONTROL;

  return 0;

Then the calls would look like:

  control |= __osc_set_pciehp_control(root, support);
  control |= __osc_set_shpchp_control(root, support);
  ...

> +	return 0;
> +}
> +
> +static inline bool __osc_have_aspm_control(u32 control)
> +{
> +	u32 required = OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
> +		       OSC_PCI_EXPRESS_LTR_CONTROL |
> +		       OSC_PCI_EXPRESS_PME_CONTROL;
> +
> +	return __osc_have_support(control, required);
> +}
> +
> +static inline void __osc_set_pciehp_control(struct acpi_pci_root *root,
> +					    u32 support, u32 *control)
> +{
> +	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE) ||
> +	    !__osc_have_support(support, ACPI_PCIE_REQ_SUPPORT))
> +		return;
> +
> +	*control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
> +		    OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
> +}
> +
> +static inline void __osc_set_shpchp_control(struct acpi_pci_root *root,
> +					    u32 support, u32 *control)
> +{
> +	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC) ||
> +	    !__osc_have_support(support, ACPI_PCIE_REQ_SUPPORT))
> +		return;
> +
> +	*control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
> +		    OSC_PCI_SHPC_NATIVE_HP_CONTROL;
> +}
> +
> +static inline void __osc_set_aer_control(struct acpi_pci_root *root,
> +					 u32 support, u32 *control)
> +{
> +	if (!pci_aer_available() ||
> +	    !__osc_have_support(support, ACPI_PCIE_REQ_SUPPORT))
> +		return;
> +
> +	if (aer_acpi_firmware_first()) {
> +		dev_info(&root->device->dev, "PCIe AER handled by firmware\n");
> +		return;
> +	}
> +
> +	*control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
> +		    OSC_PCI_EXPRESS_AER_CONTROL;
> +}
> +
>  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>  				 bool is_pcie)
>  {
> @@ -474,37 +541,25 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>  		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;
> -	}
> -
> -	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;
> +	control = 0;
> +	if (__osc_set_aspm_control(root, support, &control))
> +		*no_aspm = 1;
>  
> -	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
> -		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
> +	__osc_set_pciehp_control(root, support, &control);
> +	__osc_set_shpchp_control(root, support, &control);
> +	__osc_set_aer_control(root, support, &control);
>  
> -	if (pci_aer_available()) {
> -		if (aer_acpi_firmware_first())
> -			dev_info(&device->dev,
> -				 "PCIe AER handled by firmware\n");
> -		else
> -			control |= OSC_PCI_EXPRESS_AER_CONTROL;
> +	if (!control) {
> +		dev_info(&device->dev, "_OSC: not requesting OS control\n");
> +		return;
>  	}
>  
>  	requested = control;
> -	status = acpi_pci_osc_control_set(handle, &control,
> -					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
> -	if (ACPI_SUCCESS(status)) {
> -		decode_osc_control(root, "OS now controls", control);
> +	acpi_pci_osc_control_set(handle, &control, 0);
> +	decode_osc_control(root, "OS requested", requested);
> +	decode_osc_control(root, "platform granted", control);
> +
> +	if (__osc_have_aspm_control(control)) {
>  		if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
>  			/*
>  			 * We have ASPM control, but the FADT indicates that
> @@ -514,11 +569,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>  			dev_info(&device->dev, "FADT indicates ASPM is unsupported, using BIOS configuration\n");
>  			*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 failed (%s); disabling ASPM\n",
> -			acpi_format_exception(status));
> +	} else if (!*no_aspm) {
> +		dev_info(&device->dev, "_OSC failed; disabling ASPM\n");
>  
>  		/*
>  		 * We want to disable ASPM here, but aspm_disabled
> -- 
> 2.7.4
> 

Re: [PATCH v2 1/2] PCI/ACPI: Move _OSC test for native services to top

[Aaron Sierra]

----- Original Message -----
> From: "Bjorn Helgaas" <helgaas@kernel.org>
> To: "Aaron Sierra" <asierra@xes-inc.com>
> Sent: Wednesday, January 30, 2019 4:44:15 PM
> Subject: Re: [PATCH v2 1/2] PCI/ACPI: Move _OSC test for native services to top

> Hi Aaron,
> 
> On Thu, Oct 25, 2018 at 11:01:31AM -0500, Aaron Sierra wrote:
>> Move the simple test for when PCIe native services are disabled
>> closer to the top, prior to where things get more complicated.
>> 
>> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
>> ---
>>  drivers/acpi/pci_root.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 707aafc..eb9f14e 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -441,6 +441,12 @@ 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");
>> +		return;
>> +	}
> 
> Today we always set "*no_aspm = 1" if _OSC fails, which means we later
> call pcie_no_aspm().
> 
> After this patch, when pcie_ports_disabled is "true", we don't even try to
> evaluate _OSC, and we will never set *no_aspm, so we will never call
> pcie_no_aspm() when pcie_ports_disabled is "true", which happens in these
> cases:
> 
>  1) CONFIG_PCIEPORTBUS is unset, or
>  2) CONFIG_PCIEPORTBUS=y and we booted with "pcie_ports=compat"
> 
> Case 1) isn't a problem because pcie_no_aspm() is only implemented when
> CONFIG_PCIEASPM=y, and CONFIG_PCIEASPM depends on CONFIG_PCIEPORTBUS, so in
> this case today we only call the empty stub pcie_no_aspm() function.
> 
> But case 2) is a behavior change that seems unintended.
> 
> Even though CONFIG_PCIEASPM depends on CONFIG_PCIEPORTBUS, ASPM doesn't
> actually *use* anything provided by PCIEPORTBUS, so I think the ASPM code
> is still active and useful even when we boot with "pcie_ports=compat".
> 
> Whether CONFIG_PCIEASPM should depend on CONFIG_PCIEPORTBUS is another
> question.  I tend to think maybe it should not, but that's an orthogonal
> question.
> 

Bjorn, thanks for the review. I certainly did not mean to change behavior
to the extent that you describe. This patch is also not really needed by
the second patch in the series, so I will drop this from v3. Sorry for
the noise.

-Aaron

Re: [PATCH v2 2/2] PCI/ACPI: Improve _OSC control request granularity

[Aaron Sierra]

----- Original Message -----
> From: "Bjorn Helgaas" <helgaas@kernel.org>
> To: "Aaron Sierra" <asierra@xes-inc.com>
> Sent: Wednesday, January 30, 2019 4:57:53 PM
> Subject: Re: [PATCH v2 2/2] PCI/ACPI: Improve _OSC control request granularity

> On Thu, Oct 25, 2018 at 11:01:32AM -0500, Aaron Sierra wrote:
>> This patch reorganizes negotiate_os_control() to be less ASPM-centric in
>> order to:
>> 
>>     1. allow other features (notably AER) to work without enabling ASPM
>>     2. better isolate feature-specific tests for readability/maintenance
> 
> I really like this idea; thanks for working it up!

Hi Bjorn,

Thanks for reviewing this. I've added follow-ups to each of your comments
below.

>> Each feature (ASPM, PCIe hotplug, SHPC hotplug, and AER) now has its own
>> inline function for setting its _OSC control requests.
>> 
>> Part of making this function more generic, required eliminating a test
>> for overall success/failure that previously caused two different types
>> of messages to be printed. Now, printed messages are streamlined to
>> always show requested _OSC control versus what was granted.
>> 
>> Previous output (success):
>> 
>>   acpi PNP0A08:00: _OSC: OS now controls [PME AER PCIeCapability LTR]
>> 
>> Previous output (failure):
>> 
>>   acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
>>   acpi PNP0A08:00: _OSC: platform willing to grant []
>> 
>> Now:
>> 
>>   acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
>>   acpi PNP0A08:00: _OSC: platform granted [PME AER PCIeCapability LTR]
>> 
>> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
>> ---
>>  drivers/acpi/pci_root.c | 118 ++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 85 insertions(+), 33 deletions(-)
>> 
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index eb9f14e..9685aba 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -53,9 +53,10 @@ static int acpi_pci_root_scan_dependent(struct acpi_device
>> *adev)
>>  }
>>  
>>  #define ACPI_PCIE_REQ_SUPPORT (OSC_PCI_EXT_CONFIG_SUPPORT \
>> -				| OSC_PCI_ASPM_SUPPORT \
>> -				| OSC_PCI_CLOCK_PM_SUPPORT \
>>  				| OSC_PCI_MSI_SUPPORT)
>> +#define ACPI_PCIE_ASPM_SUPPORT (ACPI_PCIE_REQ_SUPPORT \
>> +				| OSC_PCI_ASPM_SUPPORT \
>> +				| OSC_PCI_CLOCK_PM_SUPPORT)
>>  
>>  static const struct acpi_device_id root_device_ids[] = {
>>  	{"PNP0A03", 0},
>> @@ -421,6 +422,72 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle,
>> u32 *mask, u32 req)
>>  }
>>  EXPORT_SYMBOL(acpi_pci_osc_control_set);
>>  
>> +static inline bool __osc_have_support(u32 support, u32 required)
>> +{
>> +	return ((support & required) == required);
>> +}
> 
> I'm not really a fan of function names with leading underscores, except
> maybe for "raw" things that don't acquire locks.

No problem, I will strip the underscores in v3.

>> +static inline int __osc_set_aspm_control(struct acpi_pci_root *root,
>> +					 u32 support, u32 *control)
>> +{
>> +	if (!IS_ENABLED(CONFIG_PCIEASPM) ||
>> +	    !__osc_have_support(support, ACPI_PCIE_ASPM_SUPPORT))
>> +		return -ENODEV;
>> +
>> +	*control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
>> +		    OSC_PCI_EXPRESS_LTR_CONTROL |
>> +		    OSC_PCI_EXPRESS_PME_CONTROL;
> 
> I think this would be more readable if we could avoid the double
> negatives, e.g.,
> 
>  if (IS_ENABLED(CONFIG_PCIEASPM) &&
>      __osc_have_support(support, ACPI_PCIE_ASPM_SUPPORT)) {
>          *control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
>                      OSC_PCI_EXPRESS_LTR_CONTROL |
>                      OSC_PCI_EXPRESS_PME_CONTROL;
>          return 0;
>  }
> 
>  return -ENODEV;

I tend to try to avoid avoidable indentation and to get out as early as
possible. In the case of these tiny functions, the style you suggested doesn't
cause any additional line wrapping or complexity. I will make this change,
except for __osc_set_aer_control(), where my original structure seems to be a
better fit.

> Since the caller ignores the return values anyway, I wonder if this
> could also work by *returning* the new mask bits instead of using
> "control" as a reference parameter, e.g.,
> 
>  if (IS_ENABLED(...))
>    return OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
>           OSC_PCI_EXPRESS_LTR_CONTROL |
>           OSC_PCI_EXPRESS_PME_CONTROL;
> 
>  return 0;
> 
> Then the calls would look like:
> 
>  control |= __osc_set_pciehp_control(root, support);
>  control |= __osc_set_shpchp_control(root, support);
>  ...

Yes, I do like that better. I had a draft with that structure, but I changed
for some reason. FYI, I'm inclined to rename these bit-setting functions to
"osc_get_X_control_bits()" to try to avoid confusion. Hopefully that sits well
with you.

-Aaron

Re: [PATCH v2 2/2] PCI/ACPI: Improve _OSC control request granularity

[Bjorn Helgaas]

On Wed, Feb 13, 2019 at 11:31:07AM -0600, Aaron Sierra wrote:
> ----- Original Message -----
> > From: "Bjorn Helgaas" <helgaas@kernel.org>
> > To: "Aaron Sierra" <asierra@xes-inc.com>
> > Sent: Wednesday, January 30, 2019 4:57:53 PM
> > Subject: Re: [PATCH v2 2/2] PCI/ACPI: Improve _OSC control request granularity
> 
> > On Thu, Oct 25, 2018 at 11:01:32AM -0500, Aaron Sierra wrote:

> > Then the calls would look like:
> > 
> >  control |= __osc_set_pciehp_control(root, support);
> >  control |= __osc_set_shpchp_control(root, support);
> >  ...
> 
> Yes, I do like that better. I had a draft with that structure, but I changed
> for some reason. FYI, I'm inclined to rename these bit-setting functions to
> "osc_get_X_control_bits()" to try to avoid confusion. Hopefully that sits well
> with you.

Sounds good!

Bjorn

[PATCH v3] PCI/ACPI: Improve _OSC control request granularity

[Aaron Sierra]

This patch reorganizes negotiate_os_control() to be less ASPM-centric in
order to:

    1. allow other features (notably AER) to work without enabling ASPM
    2. better isolate feature-specific tests for readability/maintenance

Each feature (ASPM, PCIe hotplug, SHPC hotplug, and AER) now has its own
inline function for setting its _OSC control requests.

Part of making this function more generic, required eliminating a test
for overall success/failure that previously caused two different types
of messages to be printed. Now, printed messages are streamlined to
always show requested _OSC control versus what was granted.

Previous output (success):

  acpi PNP0A08:00: _OSC: OS now controls [PME AER PCIeCapability LTR]

Previous output (failure):

  acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
  acpi PNP0A08:00: _OSC: platform willing to grant []

New output:

  acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
  acpi PNP0A08:00: _OSC: platform granted [PME AER PCIeCapability LTR]

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---

v3:
  * Dropped patch moving the pcie_ports_disabled check
  * Removed underscore prefix from new inline functions
  * Defined ASPM_OSC_CONTROL_BITS and removed __osc_have_aspm_control()
  * Refactored OSC control bit-setting functions to return the bits they want
    to set, instead of an ignored error code, and converted most conditionals
    from inverted logic, (!x || !y), to positive logic, (x && y)
  * Renamed OSC control bit-setting functions from __osc_set_X_control()
    to osc_get_X_control_bits()

v2:
  * Rebased from the mainline kernel (4.19-rc7) to Bjorn Helgaas's
    pci/aspm branch [1]
  * Factored moving the pcie_ports_disabled check to the top of
    negotiate_os_control into its own patch
  * Simplified new __osc_check_support function and renamed it to
    __osc_have_support
  * No longer messes with _OSC general availability test and related
    error message (i.e. _OSC failed ... disabling ASPM)

[1] https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git

 drivers/acpi/pci_root.c | 111 ++++++++++++++++++++++++++++++----------
 1 file changed, 83 insertions(+), 28 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 707aafc7c2aa..ac74bd42399d 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -53,9 +53,13 @@ static int acpi_pci_root_scan_dependent(struct acpi_device *adev)
 }
 
 #define ACPI_PCIE_REQ_SUPPORT (OSC_PCI_EXT_CONFIG_SUPPORT \
-				| OSC_PCI_ASPM_SUPPORT \
-				| OSC_PCI_CLOCK_PM_SUPPORT \
 				| OSC_PCI_MSI_SUPPORT)
+#define ACPI_PCIE_ASPM_SUPPORT (ACPI_PCIE_REQ_SUPPORT \
+				| OSC_PCI_ASPM_SUPPORT \
+				| OSC_PCI_CLOCK_PM_SUPPORT)
+#define OSC_CONTROL_BITS_ASPM (OSC_PCI_EXPRESS_CAPABILITY_CONTROL \
+				| OSC_PCI_EXPRESS_LTR_CONTROL \
+				| OSC_PCI_EXPRESS_PME_CONTROL)
 
 static const struct acpi_device_id root_device_ids[] = {
 	{"PNP0A03", 0},
@@ -421,6 +425,67 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
 }
 EXPORT_SYMBOL(acpi_pci_osc_control_set);
 
+static inline bool osc_have_support(u32 support, u32 required)
+{
+	return ((support & required) == required);
+}
+
+static inline u32 osc_get_aspm_control_bits(struct acpi_pci_root *root,
+					    u32 support)
+{
+	u32 control = 0;
+
+	if (IS_ENABLED(CONFIG_PCIEASPM) &&
+	    osc_have_support(support, ACPI_PCIE_ASPM_SUPPORT)) {
+		control = OSC_CONTROL_BITS_ASPM;
+	}
+
+	return control;
+}
+
+static inline u32 osc_get_pciehp_control_bits(struct acpi_pci_root *root,
+					      u32 support)
+{
+	u32 control = 0;
+
+	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE) &&
+	    osc_have_support(support, ACPI_PCIE_REQ_SUPPORT)) {
+		control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
+			  OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
+	}
+
+	return control;
+}
+
+static inline u32 osc_get_shpchp_control_bits(struct acpi_pci_root *root,
+					      u32 support)
+{
+	u32 control = 0;
+
+	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC) &&
+	    osc_have_support(support, ACPI_PCIE_REQ_SUPPORT)) {
+		control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
+			  OSC_PCI_SHPC_NATIVE_HP_CONTROL;
+	}
+
+	return control;
+}
+
+static inline u32 osc_get_aer_control_bits(struct acpi_pci_root *root,
+					   u32 support)
+{
+	if (!pci_aer_available() ||
+	    !osc_have_support(support, ACPI_PCIE_REQ_SUPPORT))
+		return 0;
+
+	if (aer_acpi_firmware_first()) {
+		dev_info(&root->device->dev, "PCIe AER handled by firmware\n");
+		return 0;
+	}
+
+	return OSC_PCI_EXPRESS_CAPABILITY_CONTROL | OSC_PCI_EXPRESS_AER_CONTROL;
+}
+
 static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 				 bool is_pcie)
 {
@@ -479,31 +544,24 @@ 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;
+	control = osc_get_aspm_control_bits(root, support);
+	if (!control)
+		*no_aspm = 1;
 
-	if (pci_aer_available()) {
-		if (aer_acpi_firmware_first())
-			dev_info(&device->dev,
-				 "PCIe AER handled by firmware\n");
-		else
-			control |= OSC_PCI_EXPRESS_AER_CONTROL;
+	control |= osc_get_pciehp_control_bits(root, support);
+	control |= osc_get_shpchp_control_bits(root, support);
+	control |= osc_get_aer_control_bits(root, support);
+	if (!control) {
+		dev_info(&device->dev, "_OSC: not requesting OS control\n");
+		return;
 	}
 
 	requested = control;
-	status = acpi_pci_osc_control_set(handle, &control,
-					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
-	if (ACPI_SUCCESS(status)) {
-		decode_osc_control(root, "OS now controls", control);
+	acpi_pci_osc_control_set(handle, &control, 0);
+	decode_osc_control(root, "OS requested", requested);
+	decode_osc_control(root, "platform granted", control);
+
+	if (osc_have_support(control, OSC_CONTROL_BITS_ASPM)) {
 		if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
 			/*
 			 * We have ASPM control, but the FADT indicates that
@@ -513,11 +571,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 			dev_info(&device->dev, "FADT indicates ASPM is unsupported, using BIOS configuration\n");
 			*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 failed (%s); disabling ASPM\n",
-			acpi_format_exception(status));
+	} else if (!*no_aspm) {
+		dev_info(&device->dev, "_OSC failed; disabling ASPM\n");
 
 		/*
 		 * We want to disable ASPM here, but aspm_disabled
-- 
2.17.1

Re: [PATCH v3] PCI/ACPI: Improve _OSC control request granularity

[Aaron Sierra]

Bjorn,

Just checking if this is still on your radar. Has it been superseded or
complicated by other work?

Aaron

Re: [PATCH v3] PCI/ACPI: Improve _OSC control request granularity

[Bjorn Helgaas]

On Tue, Apr 16, 2019 at 12:52:53PM -0500, Aaron Sierra wrote:
> Bjorn,
> 
> Just checking if this is still on your radar. Has it been superseded or
> complicated by other work?

Still on my radar, sorry for the delay.  I use the patchwork instance at
https://patchwork.ozlabs.org/project/linux-pci/list/, and as long as it's
"New" there, I haven't forgotten about it.

Bjorn

Re: [PATCH v3] PCI/ACPI: Improve _OSC control request granularity

[Bjorn Helgaas]

Hi Aaron,

On Wed, Feb 13, 2019 at 03:32:42PM -0600, Aaron Sierra wrote:
> This patch reorganizes negotiate_os_control() to be less ASPM-centric in
> order to:
> 
>     1. allow other features (notably AER) to work without enabling ASPM
>     2. better isolate feature-specific tests for readability/maintenance
> 
> Each feature (ASPM, PCIe hotplug, SHPC hotplug, and AER) now has its own
> inline function for setting its _OSC control requests.

Can you split this into three patches?

IIUC, 1) above is a functional change that allows us to use AER even
if CONFIG_PCIEASPM is unset or we booted with "pcie_aspm=off".  This
seems like a reasonable thing to do, although I would want to dig out
the commit that added the requirement in the first place to see if
there's some reason for requiring Linux support for all those
features.  It would also be nice to have the commit log for this patch
mention the use case.

And 2) seems like basically a cleanup with no functional change?  It
does make the code in negotiate_os_control() a little prettier, but I
have to admit that while reviewing this, I found the additional
indirection made it harder to untangle the dependencies, so I'm not
100% convinced yet.  _OSC is just such an ugly interface to begin with
that I'm not sure how it can really be improved.

> Part of making this function more generic, required eliminating a test
> for overall success/failure that previously caused two different types
> of messages to be printed. Now, printed messages are streamlined to
> always show requested _OSC control versus what was granted.
> 
> Previous output (success):
> 
>   acpi PNP0A08:00: _OSC: OS now controls [PME AER PCIeCapability LTR]
> 
> Previous output (failure):
> 
>   acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
>   acpi PNP0A08:00: _OSC: platform willing to grant []
> 
> New output:
> 
>   acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
>   acpi PNP0A08:00: _OSC: platform granted [PME AER PCIeCapability LTR]

I like this output change.  Can it be split into a separate third
patch that only changes the output, without changing the actual
behavior?

> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
> 
> v3:
>   * Dropped patch moving the pcie_ports_disabled check
>   * Removed underscore prefix from new inline functions
>   * Defined ASPM_OSC_CONTROL_BITS and removed __osc_have_aspm_control()
>   * Refactored OSC control bit-setting functions to return the bits they want
>     to set, instead of an ignored error code, and converted most conditionals
>     from inverted logic, (!x || !y), to positive logic, (x && y)
>   * Renamed OSC control bit-setting functions from __osc_set_X_control()
>     to osc_get_X_control_bits()
> 
> v2:
>   * Rebased from the mainline kernel (4.19-rc7) to Bjorn Helgaas's
>     pci/aspm branch [1]
>   * Factored moving the pcie_ports_disabled check to the top of
>     negotiate_os_control into its own patch
>   * Simplified new __osc_check_support function and renamed it to
>     __osc_have_support
>   * No longer messes with _OSC general availability test and related
>     error message (i.e. _OSC failed ... disabling ASPM)
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git
> 
>  drivers/acpi/pci_root.c | 111 ++++++++++++++++++++++++++++++----------
>  1 file changed, 83 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 707aafc7c2aa..ac74bd42399d 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -53,9 +53,13 @@ static int acpi_pci_root_scan_dependent(struct acpi_device *adev)
>  }
>  
>  #define ACPI_PCIE_REQ_SUPPORT (OSC_PCI_EXT_CONFIG_SUPPORT \
> -				| OSC_PCI_ASPM_SUPPORT \
> -				| OSC_PCI_CLOCK_PM_SUPPORT \
>  				| OSC_PCI_MSI_SUPPORT)
> +#define ACPI_PCIE_ASPM_SUPPORT (ACPI_PCIE_REQ_SUPPORT \
> +				| OSC_PCI_ASPM_SUPPORT \
> +				| OSC_PCI_CLOCK_PM_SUPPORT)
> +#define OSC_CONTROL_BITS_ASPM (OSC_PCI_EXPRESS_CAPABILITY_CONTROL \
> +				| OSC_PCI_EXPRESS_LTR_CONTROL \
> +				| OSC_PCI_EXPRESS_PME_CONTROL)
>  
>  static const struct acpi_device_id root_device_ids[] = {
>  	{"PNP0A03", 0},
> @@ -421,6 +425,67 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
>  }
>  EXPORT_SYMBOL(acpi_pci_osc_control_set);
>  
> +static inline bool osc_have_support(u32 support, u32 required)
> +{
> +	return ((support & required) == required);
> +}
> +
> +static inline u32 osc_get_aspm_control_bits(struct acpi_pci_root *root,
> +					    u32 support)
> +{
> +	u32 control = 0;
> +
> +	if (IS_ENABLED(CONFIG_PCIEASPM) &&
> +	    osc_have_support(support, ACPI_PCIE_ASPM_SUPPORT)) {
> +		control = OSC_CONTROL_BITS_ASPM;
> +	}
> +
> +	return control;

If we end up keeping these wrappers, they would be a little simpler
as:

    static inline u32 osc_get_aspm_control_bits(...)
    {
      if (IS_ENABLED(CONFIG_PCIEASPM) &&
	  osc_have_support(support, ACPI_PCIE_ASPM_SUPPORT))
	    return OSC_CONTROL_BITS_ASPM;
      return 0;
    }

But really, it seems a little convoluted to have to pass in "support",
since it doesn't depend on any _OSC results or even on which host
bridge this is.  It's just a function of config options and sometimes
global kernel boot parameters.  So I'm not sure whether the overall
readability is better.

> +}
> +
> +static inline u32 osc_get_pciehp_control_bits(struct acpi_pci_root *root,
> +					      u32 support)
> +{
> +	u32 control = 0;
> +
> +	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE) &&
> +	    osc_have_support(support, ACPI_PCIE_REQ_SUPPORT)) {
> +		control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
> +			  OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
> +	}
> +
> +	return control;
> +}
> +
> +static inline u32 osc_get_shpchp_control_bits(struct acpi_pci_root *root,
> +					      u32 support)
> +{
> +	u32 control = 0;
> +
> +	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC) &&
> +	    osc_have_support(support, ACPI_PCIE_REQ_SUPPORT)) {
> +		control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
> +			  OSC_PCI_SHPC_NATIVE_HP_CONTROL;
> +	}
> +
> +	return control;
> +}
> +
> +static inline u32 osc_get_aer_control_bits(struct acpi_pci_root *root,
> +					   u32 support)
> +{
> +	if (!pci_aer_available() ||
> +	    !osc_have_support(support, ACPI_PCIE_REQ_SUPPORT))
> +		return 0;
> +
> +	if (aer_acpi_firmware_first()) {
> +		dev_info(&root->device->dev, "PCIe AER handled by firmware\n");
> +		return 0;
> +	}
> +
> +	return OSC_PCI_EXPRESS_CAPABILITY_CONTROL | OSC_PCI_EXPRESS_AER_CONTROL;
> +}
> +
>  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>  				 bool is_pcie)
>  {
> @@ -479,31 +544,24 @@ 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;
> +	control = osc_get_aspm_control_bits(root, support);
> +	if (!control)
> +		*no_aspm = 1;
>  
> -	if (pci_aer_available()) {
> -		if (aer_acpi_firmware_first())
> -			dev_info(&device->dev,
> -				 "PCIe AER handled by firmware\n");
> -		else
> -			control |= OSC_PCI_EXPRESS_AER_CONTROL;
> +	control |= osc_get_pciehp_control_bits(root, support);
> +	control |= osc_get_shpchp_control_bits(root, support);
> +	control |= osc_get_aer_control_bits(root, support);
> +	if (!control) {
> +		dev_info(&device->dev, "_OSC: not requesting OS control\n");
> +		return;
>  	}
>  
>  	requested = control;
> -	status = acpi_pci_osc_control_set(handle, &control,
> -					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
> -	if (ACPI_SUCCESS(status)) {
> -		decode_osc_control(root, "OS now controls", control);
> +	acpi_pci_osc_control_set(handle, &control, 0);
> +	decode_osc_control(root, "OS requested", requested);
> +	decode_osc_control(root, "platform granted", control);
> +
> +	if (osc_have_support(control, OSC_CONTROL_BITS_ASPM)) {
>  		if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
>  			/*
>  			 * We have ASPM control, but the FADT indicates that
> @@ -513,11 +571,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>  			dev_info(&device->dev, "FADT indicates ASPM is unsupported, using BIOS configuration\n");
>  			*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 failed (%s); disabling ASPM\n",
> -			acpi_format_exception(status));
> +	} else if (!*no_aspm) {
> +		dev_info(&device->dev, "_OSC failed; disabling ASPM\n");
>  
>  		/*
>  		 * We want to disable ASPM here, but aspm_disabled
> -- 
> 2.17.1
> 

Re: [PATCH v3] PCI/ACPI: Improve _OSC control request granularity

[Aaron Sierra]

----- Original Message -----
> From: "Bjorn Helgaas" <helgaas@kernel.org>
> Sent: Wednesday, June 26, 2019 12:20:13 PM

> Hi Aaron,
> 
> On Wed, Feb 13, 2019 at 03:32:42PM -0600, Aaron Sierra wrote:
>> This patch reorganizes negotiate_os_control() to be less ASPM-centric in
>> order to:
>> 
>>     1. allow other features (notably AER) to work without enabling ASPM
>>     2. better isolate feature-specific tests for readability/maintenance
>> 
>> Each feature (ASPM, PCIe hotplug, SHPC hotplug, and AER) now has its own
>> inline function for setting its _OSC control requests.

Hi Bjorn,

Thanks for the review.

> Can you split this into three patches?

Sure.

> IIUC, 1) above is a functional change that allows us to use AER even
> if CONFIG_PCIEASPM is unset or we booted with "pcie_aspm=off".  This
> seems like a reasonable thing to do, although I would want to dig out
> the commit that added the requirement in the first place to see if
> there's some reason for requiring Linux support for all those
> features.  It would also be nice to have the commit log for this patch
> mention the use case.

That is correct. I will elaborate on the use case in the new patch for
this change. It boils down to wanting to keep PCIe links fully active
for reliability while still being able to know about error conditions on
the links.

> And 2) seems like basically a cleanup with no functional change?  It
> does make the code in negotiate_os_control() a little prettier, but I
> have to admit that while reviewing this, I found the additional
> indirection made it harder to untangle the dependencies, so I'm not
> 100% convinced yet.  _OSC is just such an ugly interface to begin with
> that I'm not sure how it can really be improved.

That is a fair assessment. I point out a functional change following your
last comment below.

>> Part of making this function more generic, required eliminating a test
>> for overall success/failure that previously caused two different types
>> of messages to be printed. Now, printed messages are streamlined to
>> always show requested _OSC control versus what was granted.
>> 
>> Previous output (success):
>> 
>>   acpi PNP0A08:00: _OSC: OS now controls [PME AER PCIeCapability LTR]
>> 
>> Previous output (failure):
>> 
>>   acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
>>   acpi PNP0A08:00: _OSC: platform willing to grant []
>> 
>> New output:
>> 
>>   acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
>>   acpi PNP0A08:00: _OSC: platform granted [PME AER PCIeCapability LTR]
> 
> I like this output change.  Can it be split into a separate third
> patch that only changes the output, without changing the actual
> behavior?

OK
 
>> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
>> ---
>> 
>> v3:
>>   * Dropped patch moving the pcie_ports_disabled check
>>   * Removed underscore prefix from new inline functions
>>   * Defined ASPM_OSC_CONTROL_BITS and removed __osc_have_aspm_control()
>>   * Refactored OSC control bit-setting functions to return the bits they want
>>     to set, instead of an ignored error code, and converted most conditionals
>>     from inverted logic, (!x || !y), to positive logic, (x && y)
>>   * Renamed OSC control bit-setting functions from __osc_set_X_control()
>>     to osc_get_X_control_bits()
>> 
>> v2:
>>   * Rebased from the mainline kernel (4.19-rc7) to Bjorn Helgaas's
>>     pci/aspm branch [1]
>>   * Factored moving the pcie_ports_disabled check to the top of
>>     negotiate_os_control into its own patch
>>   * Simplified new __osc_check_support function and renamed it to
>>     __osc_have_support
>>   * No longer messes with _OSC general availability test and related
>>     error message (i.e. _OSC failed ... disabling ASPM)
>> 
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git
>> 
>>  drivers/acpi/pci_root.c | 111 ++++++++++++++++++++++++++++++----------
>>  1 file changed, 83 insertions(+), 28 deletions(-)
>> 
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 707aafc7c2aa..ac74bd42399d 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -53,9 +53,13 @@ static int acpi_pci_root_scan_dependent(struct acpi_device
>> *adev)
>>  }
>>  
>>  #define ACPI_PCIE_REQ_SUPPORT (OSC_PCI_EXT_CONFIG_SUPPORT \
>> -				| OSC_PCI_ASPM_SUPPORT \
>> -				| OSC_PCI_CLOCK_PM_SUPPORT \
>>  				| OSC_PCI_MSI_SUPPORT)
>> +#define ACPI_PCIE_ASPM_SUPPORT (ACPI_PCIE_REQ_SUPPORT \
>> +				| OSC_PCI_ASPM_SUPPORT \
>> +				| OSC_PCI_CLOCK_PM_SUPPORT)
>> +#define OSC_CONTROL_BITS_ASPM (OSC_PCI_EXPRESS_CAPABILITY_CONTROL \
>> +				| OSC_PCI_EXPRESS_LTR_CONTROL \
>> +				| OSC_PCI_EXPRESS_PME_CONTROL)
>>  
>>  static const struct acpi_device_id root_device_ids[] = {
>>  	{"PNP0A03", 0},
>> @@ -421,6 +425,67 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle,
>> u32 *mask, u32 req)
>>  }
>>  EXPORT_SYMBOL(acpi_pci_osc_control_set);
>>  
>> +static inline bool osc_have_support(u32 support, u32 required)
>> +{
>> +	return ((support & required) == required);
>> +}
>> +
>> +static inline u32 osc_get_aspm_control_bits(struct acpi_pci_root *root,
>> +					    u32 support)
>> +{
>> +	u32 control = 0;
>> +
>> +	if (IS_ENABLED(CONFIG_PCIEASPM) &&
>> +	    osc_have_support(support, ACPI_PCIE_ASPM_SUPPORT)) {
>> +		control = OSC_CONTROL_BITS_ASPM;
>> +	}
>> +
>> +	return control;
> 
> If we end up keeping these wrappers, they would be a little simpler
> as:
> 
>    static inline u32 osc_get_aspm_control_bits(...)
>    {
>      if (IS_ENABLED(CONFIG_PCIEASPM) &&
>	  osc_have_support(support, ACPI_PCIE_ASPM_SUPPORT))
>	    return OSC_CONTROL_BITS_ASPM;
>      return 0;
>    }
> 
> But really, it seems a little convoluted to have to pass in "support",
> since it doesn't depend on any _OSC results or even on which host
> bridge this is.  It's just a function of config options and sometimes
> global kernel boot parameters.  So I'm not sure whether the overall
> readability is better.

Doesn't pci_ext_cfg_avail() at least depend on the host bridge? I see what
you mean with respect to support flags set based on pci_msi_enable() and
pcie_aspm_support_enabled().

I think you've highlighted that this one can be simplified even more due
to overlap with pcie_aspm_support_enabled():

    static inline u32 osc_get_aspm_control_bits(...)
    {
    	if (osc_have_support(support, ACPI_PCIE_ASPM_SUPPORT))
		return OSC_CONTROL_BITS_ASPM;
    	return 0;
    }

To be clear, the key change that allows AER to work without ASPM is altering
the flags set in ACPI_PCIE_REQ_SUPPORT to the least restrictive set needed
to satisfy at least one kernel feature. Without that change, this test
causes us to bail out without requesting any _OSC control:

	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;
	}

The inline functions attempt to better show the relationship between support
bits (support needed by kernel features enabled at runtime) and the control
bits (access requests to support those features) for a given feature.

In the case of ASPM, there is a change in behavior related to weakening the
blanket "support" test. The control bits it depends on can now be omitted
from the request set, so we don't request more than the kernel intends to
use.

-Aaron

>> +}
>> +
>> +static inline u32 osc_get_pciehp_control_bits(struct acpi_pci_root *root,
>> +					      u32 support)
>> +{
>> +	u32 control = 0;
>> +
>> +	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE) &&
>> +	    osc_have_support(support, ACPI_PCIE_REQ_SUPPORT)) {
>> +		control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
>> +			  OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
>> +	}
>> +
>> +	return control;
>> +}
>> +
>> +static inline u32 osc_get_shpchp_control_bits(struct acpi_pci_root *root,
>> +					      u32 support)
>> +{
>> +	u32 control = 0;
>> +
>> +	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC) &&
>> +	    osc_have_support(support, ACPI_PCIE_REQ_SUPPORT)) {
>> +		control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
>> +			  OSC_PCI_SHPC_NATIVE_HP_CONTROL;
>> +	}
>> +
>> +	return control;
>> +}
>> +
>> +static inline u32 osc_get_aer_control_bits(struct acpi_pci_root *root,
>> +					   u32 support)
>> +{
>> +	if (!pci_aer_available() ||
>> +	    !osc_have_support(support, ACPI_PCIE_REQ_SUPPORT))
>> +		return 0;
>> +
>> +	if (aer_acpi_firmware_first()) {
>> +		dev_info(&root->device->dev, "PCIe AER handled by firmware\n");
>> +		return 0;
>> +	}
>> +
>> +	return OSC_PCI_EXPRESS_CAPABILITY_CONTROL | OSC_PCI_EXPRESS_AER_CONTROL;
>> +}
>> +
>>  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>>  				 bool is_pcie)
>>  {
>> @@ -479,31 +544,24 @@ 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;
>> +	control = osc_get_aspm_control_bits(root, support);
>> +	if (!control)
>> +		*no_aspm = 1;
>>  
>> -	if (pci_aer_available()) {
>> -		if (aer_acpi_firmware_first())
>> -			dev_info(&device->dev,
>> -				 "PCIe AER handled by firmware\n");
>> -		else
>> -			control |= OSC_PCI_EXPRESS_AER_CONTROL;
>> +	control |= osc_get_pciehp_control_bits(root, support);
>> +	control |= osc_get_shpchp_control_bits(root, support);
>> +	control |= osc_get_aer_control_bits(root, support);
>> +	if (!control) {
>> +		dev_info(&device->dev, "_OSC: not requesting OS control\n");
>> +		return;
>>  	}
>>  
>>  	requested = control;
>> -	status = acpi_pci_osc_control_set(handle, &control,
>> -					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
>> -	if (ACPI_SUCCESS(status)) {
>> -		decode_osc_control(root, "OS now controls", control);
>> +	acpi_pci_osc_control_set(handle, &control, 0);
>> +	decode_osc_control(root, "OS requested", requested);
>> +	decode_osc_control(root, "platform granted", control);
>> +
>> +	if (osc_have_support(control, OSC_CONTROL_BITS_ASPM)) {
>>  		if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
>>  			/*
>>  			 * We have ASPM control, but the FADT indicates that
>> @@ -513,11 +571,8 @@ static void negotiate_os_control(struct acpi_pci_root
>> *root, int *no_aspm,
>>  			dev_info(&device->dev, "FADT indicates ASPM is unsupported, using BIOS
>>  			configuration\n");
>>  			*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 failed (%s); disabling ASPM\n",
>> -			acpi_format_exception(status));
>> +	} else if (!*no_aspm) {
>> +		dev_info(&device->dev, "_OSC failed; disabling ASPM\n");
>>  
>>  		/*
>>  		 * We want to disable ASPM here, but aspm_disabled
>> --
>> 2.17.1