[PATCH 00/14] _OSC simplification

[PATCH 00/14] _OSC simplification

[Bjorn Helgaas]

This is a long series of mostly minor changes with the goal of
simplifying the _OSC-related code and making the negotiation between
Linux and the platform more understandable.

My intent is that this doesn't change any behavior except for the text
in dmesg.

However, this restructuring does raise some questions in my mind, such
as the fact that we do not call pcie_no_aspm() to disable ASPM in two
cases where it seems like we might want to:

  1) When pcie_ports_disabled, e.g., when booting with
     "pcie_ports=compat".

  2) When Linux doesn't support all the required services, e.g., if
     compiled with ASPM support but not MSI support.

I haven't looked deeply, so maybe there are other constraints that
keep us from using ASPM in these cases.  In any case, this series
doesn't change any behavior in this regard.

Below are samples of the dmesg changes from this series.  Typical case
where we successfully get control of PCIe services:

  -acpi PNP0A08:00: Requesting ACPI _OSC control (0x1d)
  -acpi PNP0A08:00: ACPI _OSC control (0x1d) granted
  +acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
  +acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME AER PCIeCapability]

Failure case where the system has no _OSC method:

  -acpi PNP0A03:00: ACPI _OSC support notification failed, disabling PCIe ASPM
  -acpi PNP0A03:00: Unable to request _OSC control (_OSC support mask: 0x08)
  +acpi PNP0A03:00: _OSC: OS supports [ASPM ClockPM Segments MSI]
  +acpi PNP0A03:00: _OSC failed (AE_NOT_FOUND); disabling ASPM

Linux only requests _OSC control of features as a group; if Linux
doesn't support *all* of those features, we don't request control of
any of them (per 415e12b237).  Here's an example booted with
"pci=nomsi":

  -acpi PNP0A08:00: Unable to request _OSC control (_OSC support mask: 0x0f)
  +acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments]
  +acpi PNP0A08:00: _OSC: not requesting OS control; OS requires [ExtendedConfig ASPM ClockPM MSI]

Here's a system that doesn't grant OS control of the PCIe Capability.
This is another case where we don't want control of anything unless we
have control of the PCIe Capability:

  -acpi PNP0A08:00: Requesting ACPI _OSC control (0x1d)
  -acpi PNP0A08:00: ACPI _OSC request failed (AE_SUPPORT), returned control mask: 0x0d
  -acpi PNP0A08:00: ACPI _OSC control for PCIe not granted, disabling ASPM
  +acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
  +acpi PNP0A08:00: _OSC: platform does not support [PCIeCapability]
  +acpi PNP0A08:00: _OSC: not requesting control; platform does not support [PCIeCapability]
  +acpi PNP0A08:00: _OSC: OS requested [PCIeHotplug PME AER PCIeCapability]
  +acpi PNP0A08:00: _OSC: platform willing to grant [PCIeHotplug PME AER]
  +acpi PNP0A08:00: _OSC failed (AE_SUPPORT); disabling ASPM


This is based on a923874198 ("Merge tag 'pci-v3.12-changes' of
git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci").

---

Bjorn Helgaas (14):
      ACPI: Write _OSC bit field definitions in hex
      ACPI: Rename OSC_QUERY_TYPE to OSC_QUERY_DWORD
      ACPI: Tidy acpi_run_osc() declarations
      ACPI: Remove unused OSC_PCI_NATIVE_HOTPLUG
      ACPI: Write OSC_PCI_CONTROL_MASKS like OSC_PCI_SUPPORT_MASKS
      PCI/ACPI: Name _OSC #defines more consistently
      PCI/ACPI: Drop unnecessary _OSC existence tests
      PCI/ACPI: Move _OSC stuff from acpi_pci_root_add() to negotiate_os_control()
      PCI/ACPI: Split _OSC "support" and "control" flags into separate variables
      PCI/ACPI: Run _OSC only once for OSPM feature support
      PCI/ACPI: Skip _OSC control tests if _OSC support call failed
      PCI/ACPI: Separate out _OSC "PCIe port services disabled" path
      PCI/ACPI: Separate out _OSC "we don't support enough services" path
      PCI/ACPI: Decode _OSC bitmasks symbolically


 drivers/acpi/apei/apei-base.c    |    6 -
 drivers/acpi/bus.c               |   18 +--
 drivers/acpi/pci_root.c          |  246 +++++++++++++++++++++++---------------
 drivers/pci/hotplug/acpi_pcihp.c |    2 
 drivers/pci/hotplug/shpchp.h     |    2 
 include/linux/acpi.h             |   81 ++++++-------
 6 files changed, 199 insertions(+), 156 deletions(-)

[PATCH 01/14] ACPI: Write _OSC bit field definitions in hex

[Bjorn Helgaas]

Update _OSC definition comments to correspond to the 1-based spec wording
(DWORD 1, etc.)  Write _OSC field #defines as hex to make clear that they
are bits in a 32-bit DWORD, not arbitrary values.  No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 include/linux/acpi.h |   54 +++++++++++++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a5db4ae..285de6f 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -304,39 +304,39 @@ struct acpi_osc_context {
 #define OSC_SUPPORT_TYPE 		1
 #define OSC_CONTROL_TYPE		2
 
-/* _OSC DW0 Definition */
-#define OSC_QUERY_ENABLE		1
-#define OSC_REQUEST_ERROR		2
-#define OSC_INVALID_UUID_ERROR		4
-#define OSC_INVALID_REVISION_ERROR	8
-#define OSC_CAPABILITIES_MASK_ERROR	16
+/* _OSC Capabilities DWORD 1: Query/Control and Error Returns (generic) */
+#define OSC_QUERY_ENABLE			0x00000001  /* input */
+#define OSC_REQUEST_ERROR			0x00000002  /* return */
+#define OSC_INVALID_UUID_ERROR			0x00000004  /* return */
+#define OSC_INVALID_REVISION_ERROR		0x00000008  /* return */
+#define OSC_CAPABILITIES_MASK_ERROR		0x00000010  /* return */
 
 acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);
 
-/* platform-wide _OSC bits */
-#define OSC_SB_PAD_SUPPORT		1
-#define OSC_SB_PPC_OST_SUPPORT		2
-#define OSC_SB_PR3_SUPPORT		4
-#define OSC_SB_HOTPLUG_OST_SUPPORT	8
-#define OSC_SB_APEI_SUPPORT		16
+/* Platform-Wide Capabilities _OSC: Capabilities DWORD 2: Support Field */
+#define OSC_SB_PAD_SUPPORT			0x00000001
+#define OSC_SB_PPC_OST_SUPPORT			0x00000002
+#define OSC_SB_PR3_SUPPORT			0x00000004
+#define OSC_SB_HOTPLUG_OST_SUPPORT		0x00000008
+#define OSC_SB_APEI_SUPPORT			0x00000010
+#define OSC_SB_CPC_SUPPORT			0x00000020
 
 extern bool osc_sb_apei_support_acked;
 
-/* PCI defined _OSC bits */
-/* _OSC DW1 Definition (OS Support Fields) */
-#define OSC_EXT_PCI_CONFIG_SUPPORT		1
-#define OSC_ACTIVE_STATE_PWR_SUPPORT 		2
-#define OSC_CLOCK_PWR_CAPABILITY_SUPPORT	4
-#define OSC_PCI_SEGMENT_GROUPS_SUPPORT		8
-#define OSC_MSI_SUPPORT				16
-#define OSC_PCI_SUPPORT_MASKS			0x1f
-
-/* _OSC DW1 Definition (OS Control Fields) */
-#define OSC_PCI_EXPRESS_NATIVE_HP_CONTROL	1
-#define OSC_SHPC_NATIVE_HP_CONTROL 		2
-#define OSC_PCI_EXPRESS_PME_CONTROL		4
-#define OSC_PCI_EXPRESS_AER_CONTROL		8
-#define OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL	16
+/* PCI Host Bridge _OSC: Capabilities DWORD 2: Support Field */
+#define OSC_EXT_PCI_CONFIG_SUPPORT		0x00000001
+#define OSC_ACTIVE_STATE_PWR_SUPPORT 		0x00000002
+#define OSC_CLOCK_PWR_CAPABILITY_SUPPORT	0x00000004
+#define OSC_PCI_SEGMENT_GROUPS_SUPPORT		0x00000008
+#define OSC_MSI_SUPPORT				0x00000010
+#define OSC_PCI_SUPPORT_MASKS			0x0000001f
+
+/* PCI Host Bridge _OSC: Capabilities DWORD 3: Control Field */
+#define OSC_PCI_EXPRESS_NATIVE_HP_CONTROL	0x00000001
+#define OSC_SHPC_NATIVE_HP_CONTROL 		0x00000002
+#define OSC_PCI_EXPRESS_PME_CONTROL		0x00000004
+#define OSC_PCI_EXPRESS_AER_CONTROL		0x00000008
+#define OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL	0x00000010
 
 #define OSC_PCI_CONTROL_MASKS 	(OSC_PCI_EXPRESS_NATIVE_HP_CONTROL | 	\
 				OSC_SHPC_NATIVE_HP_CONTROL | 		\

[PATCH 02/14] ACPI: Rename OSC_QUERY_TYPE to OSC_QUERY_DWORD

[Bjorn Helgaas]

OSC_QUERY_TYPE isn't a "type"; it's an index into the _OSC Capabilities
Buffer of DWORDs.  Rename OSC_QUERY_TYPE, OSC_SUPPORT_TYPE, and
OSC_CONTROL_TYPE to OSC_QUERY_DWORD, etc., to make this clear.
No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/apei/apei-base.c |    6 +++---
 drivers/acpi/bus.c            |   18 +++++++++---------
 drivers/acpi/pci_root.c       |   14 +++++++-------
 include/linux/acpi.h          |    7 ++++---
 4 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 46f80e2..6d2c49b 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -758,9 +758,9 @@ int apei_osc_setup(void)
 		.cap.pointer	= capbuf,
 	};
 
-	capbuf[OSC_QUERY_TYPE] = OSC_QUERY_ENABLE;
-	capbuf[OSC_SUPPORT_TYPE] = 1;
-	capbuf[OSC_CONTROL_TYPE] = 0;
+	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
+	capbuf[OSC_SUPPORT_DWORD] = 1;
+	capbuf[OSC_CONTROL_DWORD] = 0;
 
 	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle))
 	    || ACPI_FAILURE(acpi_run_osc(handle, &context)))
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index b587ec8..fbcfaa6 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -255,7 +255,7 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
 			acpi_print_osc_error(handle, context,
 				"_OSC invalid revision");
 		if (errors & OSC_CAPABILITIES_MASK_ERROR) {
-			if (((u32 *)context->cap.pointer)[OSC_QUERY_TYPE]
+			if (((u32 *)context->cap.pointer)[OSC_QUERY_DWORD]
 			    & OSC_QUERY_ENABLE)
 				goto out_success;
 			status = AE_SUPPORT;
@@ -295,30 +295,30 @@ static void acpi_bus_osc_support(void)
 	};
 	acpi_handle handle;
 
-	capbuf[OSC_QUERY_TYPE] = OSC_QUERY_ENABLE;
-	capbuf[OSC_SUPPORT_TYPE] = OSC_SB_PR3_SUPPORT; /* _PR3 is in use */
+	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
+	capbuf[OSC_SUPPORT_DWORD] = OSC_SB_PR3_SUPPORT; /* _PR3 is in use */
 #if defined(CONFIG_ACPI_PROCESSOR_AGGREGATOR) ||\
 			defined(CONFIG_ACPI_PROCESSOR_AGGREGATOR_MODULE)
-	capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_PAD_SUPPORT;
+	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PAD_SUPPORT;
 #endif
 
 #if defined(CONFIG_ACPI_PROCESSOR) || defined(CONFIG_ACPI_PROCESSOR_MODULE)
-	capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_PPC_OST_SUPPORT;
+	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PPC_OST_SUPPORT;
 #endif
 
 #ifdef ACPI_HOTPLUG_OST
-	capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_HOTPLUG_OST_SUPPORT;
+	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_HOTPLUG_OST_SUPPORT;
 #endif
 
 	if (!ghes_disable)
-		capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_APEI_SUPPORT;
+		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
 	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
 		return;
 	if (ACPI_SUCCESS(acpi_run_osc(handle, &context))) {
 		u32 *capbuf_ret = context.ret.pointer;
-		if (context.ret.length > OSC_SUPPORT_TYPE)
+		if (context.ret.length > OSC_SUPPORT_DWORD)
 			osc_sb_apei_support_acked =
-				capbuf_ret[OSC_SUPPORT_TYPE] & OSC_SB_APEI_SUPPORT;
+				capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
 		kfree(context.ret.pointer);
 	}
 	/* do we need to check other returned cap? Sounds no */
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index d3874f4..323afd7 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -158,14 +158,14 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
 	support &= OSC_PCI_SUPPORT_MASKS;
 	support |= root->osc_support_set;
 
-	capbuf[OSC_QUERY_TYPE] = OSC_QUERY_ENABLE;
-	capbuf[OSC_SUPPORT_TYPE] = support;
+	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
+	capbuf[OSC_SUPPORT_DWORD] = support;
 	if (control) {
 		*control &= OSC_PCI_CONTROL_MASKS;
-		capbuf[OSC_CONTROL_TYPE] = *control | root->osc_control_set;
+		capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set;
 	} else {
 		/* Run _OSC query only with existing controls. */
-		capbuf[OSC_CONTROL_TYPE] = root->osc_control_set;
+		capbuf[OSC_CONTROL_DWORD] = root->osc_control_set;
 	}
 
 	status = acpi_pci_run_osc(root->device->handle, capbuf, &result);
@@ -357,9 +357,9 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
 		goto out;
 	}
 
-	capbuf[OSC_QUERY_TYPE] = 0;
-	capbuf[OSC_SUPPORT_TYPE] = root->osc_support_set;
-	capbuf[OSC_CONTROL_TYPE] = ctrl;
+	capbuf[OSC_QUERY_DWORD] = 0;
+	capbuf[OSC_SUPPORT_DWORD] = root->osc_support_set;
+	capbuf[OSC_CONTROL_DWORD] = ctrl;
 	status = acpi_pci_run_osc(handle, capbuf, mask);
 	if (ACPI_SUCCESS(status))
 		root->osc_control_set = *mask;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 285de6f..3ba9fe3 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -300,9 +300,10 @@ struct acpi_osc_context {
 	struct acpi_buffer ret; /* free by caller if success */
 };
 
-#define OSC_QUERY_TYPE			0
-#define OSC_SUPPORT_TYPE 		1
-#define OSC_CONTROL_TYPE		2
+/* Indexes into _OSC Capabilities Buffer (DWORDs 2 & 3 are device-specific) */
+#define OSC_QUERY_DWORD				0	/* DWORD 1 */
+#define OSC_SUPPORT_DWORD 			1	/* DWORD 2 */
+#define OSC_CONTROL_DWORD			2	/* DWORD 3 */
 
 /* _OSC Capabilities DWORD 1: Query/Control and Error Returns (generic) */
 #define OSC_QUERY_ENABLE			0x00000001  /* input */

[PATCH 03/14] ACPI: Tidy acpi_run_osc() declarations

[Bjorn Helgaas]

Move the acpi_run_osc() prototype next to the related structure and
update comments.  No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 include/linux/acpi.h |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 3ba9fe3..902cccf 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -294,12 +294,14 @@ void __init acpi_nvs_nosave_s3(void);
 #endif /* CONFIG_PM_SLEEP */
 
 struct acpi_osc_context {
-	char *uuid_str; /* uuid string */
+	char *uuid_str;			/* UUID string */
 	int rev;
-	struct acpi_buffer cap; /* arg2/arg3 */
-	struct acpi_buffer ret; /* free by caller if success */
+	struct acpi_buffer cap;		/* list of DWORD capabilities */
+	struct acpi_buffer ret;		/* free by caller if success */
 };
 
+acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);
+
 /* Indexes into _OSC Capabilities Buffer (DWORDs 2 & 3 are device-specific) */
 #define OSC_QUERY_DWORD				0	/* DWORD 1 */
 #define OSC_SUPPORT_DWORD 			1	/* DWORD 2 */
@@ -312,8 +314,6 @@ struct acpi_osc_context {
 #define OSC_INVALID_REVISION_ERROR		0x00000008  /* return */
 #define OSC_CAPABILITIES_MASK_ERROR		0x00000010  /* return */
 
-acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);
-
 /* Platform-Wide Capabilities _OSC: Capabilities DWORD 2: Support Field */
 #define OSC_SB_PAD_SUPPORT			0x00000001
 #define OSC_SB_PPC_OST_SUPPORT			0x00000002

[PATCH 04/14] ACPI: Remove unused OSC_PCI_NATIVE_HOTPLUG

[Bjorn Helgaas]

OSC_PCI_NATIVE_HOTPLUG is completely unused, so remove it.  No functional
change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 include/linux/acpi.h |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 902cccf..94e5bca 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -345,9 +345,6 @@ extern bool osc_sb_apei_support_acked;
 				OSC_PCI_EXPRESS_AER_CONTROL |		\
 				OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL)
 
-#define OSC_PCI_NATIVE_HOTPLUG	(OSC_PCI_EXPRESS_NATIVE_HP_CONTROL |	\
-				OSC_SHPC_NATIVE_HP_CONTROL)
-
 extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
 					     u32 *mask, u32 req);
 

[PATCH 05/14] ACPI: Write OSC_PCI_CONTROL_MASKS like OSC_PCI_SUPPORT_MASKS

[Bjorn Helgaas]

We write OSC_PCI_SUPPORT_MASKS as a simple 0x1f, so do the same
for OSC_PCI_CONTROL_MASKS.  No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 include/linux/acpi.h |    7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 94e5bca..7bda6a2 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -338,12 +338,7 @@ extern bool osc_sb_apei_support_acked;
 #define OSC_PCI_EXPRESS_PME_CONTROL		0x00000004
 #define OSC_PCI_EXPRESS_AER_CONTROL		0x00000008
 #define OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL	0x00000010
-
-#define OSC_PCI_CONTROL_MASKS 	(OSC_PCI_EXPRESS_NATIVE_HP_CONTROL | 	\
-				OSC_SHPC_NATIVE_HP_CONTROL | 		\
-				OSC_PCI_EXPRESS_PME_CONTROL |		\
-				OSC_PCI_EXPRESS_AER_CONTROL |		\
-				OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL)
+#define OSC_PCI_CONTROL_MASKS			0x0000001f
 
 extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
 					     u32 *mask, u32 req);

[PATCH 06/14] PCI/ACPI: Name _OSC #defines more consistently

[Bjorn Helgaas]

Make PCI Host Bridge _OSC #defines more consistent.  No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/pci_root.c          |   19 +++++++++----------
 drivers/pci/hotplug/acpi_pcihp.c |    2 +-
 drivers/pci/hotplug/shpchp.h     |    2 +-
 include/linux/acpi.h             |   12 ++++++------
 4 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 323afd7..28dd555 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -49,10 +49,10 @@ static int acpi_pci_root_add(struct acpi_device *device,
 			     const struct acpi_device_id *not_used);
 static void acpi_pci_root_remove(struct acpi_device *device);
 
-#define ACPI_PCIE_REQ_SUPPORT (OSC_EXT_PCI_CONFIG_SUPPORT \
-				| OSC_ACTIVE_STATE_PWR_SUPPORT \
-				| OSC_CLOCK_PWR_CAPABILITY_SUPPORT \
-				| OSC_MSI_SUPPORT)
+#define ACPI_PCIE_REQ_SUPPORT (OSC_PCI_EXT_CONFIG_SUPPORT \
+				| OSC_PCI_ASPM_SUPPORT \
+				| OSC_PCI_CLOCK_PM_SUPPORT \
+				| OSC_PCI_MSI_SUPPORT)
 
 static const struct acpi_device_id root_device_ids[] = {
 	{"PNP0A03", 0},
@@ -439,13 +439,12 @@ static int acpi_pci_root_add(struct acpi_device *device,
 	acpi_pci_osc_support(root, flags);
 
 	if (pci_ext_cfg_avail())
-		flags |= OSC_EXT_PCI_CONFIG_SUPPORT;
+		flags |= OSC_PCI_EXT_CONFIG_SUPPORT;
 	if (pcie_aspm_support_enabled()) {
-		flags |= OSC_ACTIVE_STATE_PWR_SUPPORT |
-		OSC_CLOCK_PWR_CAPABILITY_SUPPORT;
+		flags |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
 	}
 	if (pci_msi_enabled())
-		flags |= OSC_MSI_SUPPORT;
+		flags |= OSC_PCI_MSI_SUPPORT;
 	if (flags != base_flags) {
 		status = acpi_pci_osc_support(root, flags);
 		if (ACPI_FAILURE(status)) {
@@ -458,7 +457,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
 
 	if (!pcie_ports_disabled
 	    && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
-		flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL
+		flags = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
 			| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
 			| OSC_PCI_EXPRESS_PME_CONTROL;
 
@@ -474,7 +473,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
 			"Requesting ACPI _OSC control (0x%02x)\n", flags);
 
 		status = acpi_pci_osc_control_set(handle, &flags,
-				       OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL);
+				       OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
 		if (ACPI_SUCCESS(status)) {
 			dev_info(&device->dev,
 				"ACPI _OSC control (0x%02x) granted\n", flags);
diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
index 2a47e82..f814016 100644
--- a/drivers/pci/hotplug/acpi_pcihp.c
+++ b/drivers/pci/hotplug/acpi_pcihp.c
@@ -338,7 +338,7 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev, u32 flags)
 	acpi_handle chandle, handle;
 	struct acpi_buffer string = { ACPI_ALLOCATE_BUFFER, NULL };
 
-	flags &= OSC_SHPC_NATIVE_HP_CONTROL;
+	flags &= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
 	if (!flags) {
 		err("Invalid flags %u specified!\n", flags);
 		return -EINVAL;
diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h
index e260f20..d876e4b 100644
--- a/drivers/pci/hotplug/shpchp.h
+++ b/drivers/pci/hotplug/shpchp.h
@@ -191,7 +191,7 @@ static inline const char *slot_name(struct slot *slot)
 #include <linux/pci-acpi.h>
 static inline int get_hp_hw_control_from_firmware(struct pci_dev *dev)
 {
-	u32 flags = OSC_SHPC_NATIVE_HP_CONTROL;
+	u32 flags = OSC_PCI_SHPC_NATIVE_HP_CONTROL;
 	return acpi_get_hp_hw_control_from_firmware(dev, flags);
 }
 #else
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 7bda6a2..9974a49 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -325,19 +325,19 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);
 extern bool osc_sb_apei_support_acked;
 
 /* PCI Host Bridge _OSC: Capabilities DWORD 2: Support Field */
-#define OSC_EXT_PCI_CONFIG_SUPPORT		0x00000001
-#define OSC_ACTIVE_STATE_PWR_SUPPORT 		0x00000002
-#define OSC_CLOCK_PWR_CAPABILITY_SUPPORT	0x00000004
+#define OSC_PCI_EXT_CONFIG_SUPPORT		0x00000001
+#define OSC_PCI_ASPM_SUPPORT			0x00000002
+#define OSC_PCI_CLOCK_PM_SUPPORT		0x00000004
 #define OSC_PCI_SEGMENT_GROUPS_SUPPORT		0x00000008
-#define OSC_MSI_SUPPORT				0x00000010
+#define OSC_PCI_MSI_SUPPORT			0x00000010
 #define OSC_PCI_SUPPORT_MASKS			0x0000001f
 
 /* PCI Host Bridge _OSC: Capabilities DWORD 3: Control Field */
 #define OSC_PCI_EXPRESS_NATIVE_HP_CONTROL	0x00000001
-#define OSC_SHPC_NATIVE_HP_CONTROL 		0x00000002
+#define OSC_PCI_SHPC_NATIVE_HP_CONTROL 		0x00000002
 #define OSC_PCI_EXPRESS_PME_CONTROL		0x00000004
 #define OSC_PCI_EXPRESS_AER_CONTROL		0x00000008
-#define OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL	0x00000010
+#define OSC_PCI_EXPRESS_CAPABILITY_CONTROL	0x00000010
 #define OSC_PCI_CONTROL_MASKS			0x0000001f
 
 extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,

[PATCH 07/14] PCI/ACPI: Drop unnecessary _OSC existence tests

[Bjorn Helgaas]

There's no need to check whether _OSC exists here; we eventually
call acpi_evaluate_object(..., "_OSC", ...), and that will fail
gracefully if _OSC doesn't exist.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/pci_root.c |   11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 28dd555..cc87cc4 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -180,11 +180,7 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
 static acpi_status acpi_pci_osc_support(struct acpi_pci_root *root, u32 flags)
 {
 	acpi_status status;
-	acpi_handle tmp;
 
-	status = acpi_get_handle(root->device->handle, "_OSC", &tmp);
-	if (ACPI_FAILURE(status))
-		return status;
 	mutex_lock(&osc_lock);
 	status = acpi_pci_query_osc(root, flags, NULL);
 	mutex_unlock(&osc_lock);
@@ -316,9 +312,8 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev);
 acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
 {
 	struct acpi_pci_root *root;
-	acpi_status status;
+	acpi_status status = AE_OK;
 	u32 ctrl, capbuf[3];
-	acpi_handle tmp;
 
 	if (!mask)
 		return AE_BAD_PARAMETER;
@@ -331,10 +326,6 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
 	if (!root)
 		return AE_NOT_EXIST;
 
-	status = acpi_get_handle(handle, "_OSC", &tmp);
-	if (ACPI_FAILURE(status))
-		return status;
-
 	mutex_lock(&osc_lock);
 
 	*mask = ctrl | root->osc_control_set;

[PATCH 08/14] PCI/ACPI: Move _OSC stuff from acpi_pci_root_add() to negotiate_os_control()

[Bjorn Helgaas]

This doesn't change any of the _OSC code; it just moves it out into
a new function so it doesn't clutter acpi_pci_root_add() so much.  This
also enables future simplifications.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/pci_root.c |  132 +++++++++++++++++++++++++----------------------
 1 file changed, 71 insertions(+), 61 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index cc87cc4..3e57f10 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -360,67 +360,13 @@ out:
 }
 EXPORT_SYMBOL(acpi_pci_osc_control_set);
 
-static int acpi_pci_root_add(struct acpi_device *device,
-			     const struct acpi_device_id *not_used)
+static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
+				 int *clear_aspm)
 {
-	unsigned long long segment, bus;
-	acpi_status status;
-	int result;
-	struct acpi_pci_root *root;
 	u32 flags, base_flags;
+	acpi_status status;
+	struct acpi_device *device = root->device;
 	acpi_handle handle = device->handle;
-	bool no_aspm = false, clear_aspm = false;
-
-	root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
-	if (!root)
-		return -ENOMEM;
-
-	segment = 0;
-	status = acpi_evaluate_integer(handle, METHOD_NAME__SEG, NULL,
-				       &segment);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		dev_err(&device->dev,  "can't evaluate _SEG\n");
-		result = -ENODEV;
-		goto end;
-	}
-
-	/* Check _CRS first, then _BBN.  If no _BBN, default to zero. */
-	root->secondary.flags = IORESOURCE_BUS;
-	status = try_get_root_bridge_busnr(handle, &root->secondary);
-	if (ACPI_FAILURE(status)) {
-		/*
-		 * We need both the start and end of the downstream bus range
-		 * to interpret _CBA (MMCONFIG base address), so it really is
-		 * supposed to be in _CRS.  If we don't find it there, all we
-		 * can do is assume [_BBN-0xFF] or [0-0xFF].
-		 */
-		root->secondary.end = 0xFF;
-		dev_warn(&device->dev,
-			 FW_BUG "no secondary bus range in _CRS\n");
-		status = acpi_evaluate_integer(handle, METHOD_NAME__BBN,
-					       NULL, &bus);
-		if (ACPI_SUCCESS(status))
-			root->secondary.start = bus;
-		else if (status == AE_NOT_FOUND)
-			root->secondary.start = 0;
-		else {
-			dev_err(&device->dev, "can't evaluate _BBN\n");
-			result = -ENODEV;
-			goto end;
-		}
-	}
-
-	root->device = device;
-	root->segment = segment & 0xFFFF;
-	strcpy(acpi_device_name(device), ACPI_PCI_ROOT_DEVICE_NAME);
-	strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
-	device->driver_data = root;
-
-	pr_info(PREFIX "%s [%s] (domain %04x %pR)\n",
-	       acpi_device_name(device), acpi_device_bid(device),
-	       root->segment, &root->secondary);
-
-	root->mcfg_addr = acpi_pci_root_get_mcfg_addr(handle);
 
 	/*
 	 * All supported architectures that use ACPI have support for
@@ -441,7 +387,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
 		if (ACPI_FAILURE(status)) {
 			dev_info(&device->dev, "ACPI _OSC support "
 				"notification failed, disabling PCIe ASPM\n");
-			no_aspm = true;
+			*no_aspm = 1;
 			flags = base_flags;
 		}
 	}
@@ -473,7 +419,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
 				 * We have ASPM control, but the FADT indicates
 				 * that it's unsupported. Clear it.
 				 */
-				clear_aspm = true;
+				*clear_aspm = 1;
 			}
 		} else {
 			dev_info(&device->dev,
@@ -489,13 +435,77 @@ static int acpi_pci_root_add(struct acpi_device *device,
 			 * flag here, to defer the action until after the ACPI
 			 * root scan.
 			 */
-			no_aspm = true;
+			*no_aspm = 1;
 		}
 	} else {
 		dev_info(&device->dev,
 			 "Unable to request _OSC control "
 			 "(_OSC support mask: 0x%02x)\n", flags);
 	}
+}
+
+static int acpi_pci_root_add(struct acpi_device *device,
+			     const struct acpi_device_id *not_used)
+{
+	unsigned long long segment, bus;
+	acpi_status status;
+	int result;
+	struct acpi_pci_root *root;
+	acpi_handle handle = device->handle;
+	int no_aspm = 0, clear_aspm = 0;
+
+	root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
+	if (!root)
+		return -ENOMEM;
+
+	segment = 0;
+	status = acpi_evaluate_integer(handle, METHOD_NAME__SEG, NULL,
+				       &segment);
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+		dev_err(&device->dev,  "can't evaluate _SEG\n");
+		result = -ENODEV;
+		goto end;
+	}
+
+	/* Check _CRS first, then _BBN.  If no _BBN, default to zero. */
+	root->secondary.flags = IORESOURCE_BUS;
+	status = try_get_root_bridge_busnr(handle, &root->secondary);
+	if (ACPI_FAILURE(status)) {
+		/*
+		 * We need both the start and end of the downstream bus range
+		 * to interpret _CBA (MMCONFIG base address), so it really is
+		 * supposed to be in _CRS.  If we don't find it there, all we
+		 * can do is assume [_BBN-0xFF] or [0-0xFF].
+		 */
+		root->secondary.end = 0xFF;
+		dev_warn(&device->dev,
+			 FW_BUG "no secondary bus range in _CRS\n");
+		status = acpi_evaluate_integer(handle, METHOD_NAME__BBN,
+					       NULL, &bus);
+		if (ACPI_SUCCESS(status))
+			root->secondary.start = bus;
+		else if (status == AE_NOT_FOUND)
+			root->secondary.start = 0;
+		else {
+			dev_err(&device->dev, "can't evaluate _BBN\n");
+			result = -ENODEV;
+			goto end;
+		}
+	}
+
+	root->device = device;
+	root->segment = segment & 0xFFFF;
+	strcpy(acpi_device_name(device), ACPI_PCI_ROOT_DEVICE_NAME);
+	strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
+	device->driver_data = root;
+
+	pr_info(PREFIX "%s [%s] (domain %04x %pR)\n",
+	       acpi_device_name(device), acpi_device_bid(device),
+	       root->segment, &root->secondary);
+
+	root->mcfg_addr = acpi_pci_root_get_mcfg_addr(handle);
+
+	negotiate_os_control(root, &no_aspm, &clear_aspm);
 
 	/*
 	 * TBD: Need PCI interface for enumeration/configuration of roots.

[PATCH 09/14] PCI/ACPI: Split _OSC "support" and "control" flags into separate variables

[Bjorn Helgaas]

Previously we used "flags" for both:

  - the bitmask of features we support (segments, ASPM, MSI, etc.), and
  - the bitmask of features we want to control (native hotplug, AER, etc.)

To reduce confusion, this patch splits this into two variables:
"support" is the bitmask of features we support, and "control" is the
bitmask of features we want to control.  No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/pci_root.c |   35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 3e57f10..3e06d4e 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -363,7 +363,7 @@ EXPORT_SYMBOL(acpi_pci_osc_control_set);
 static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 				 int *clear_aspm)
 {
-	u32 flags, base_flags;
+	u32 support, base_support, control;
 	acpi_status status;
 	struct acpi_device *device = root->device;
 	acpi_handle handle = device->handle;
@@ -372,29 +372,29 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 	 * All supported architectures that use ACPI have support for
 	 * PCI domains, so we indicate this in _OSC support capabilities.
 	 */
-	flags = base_flags = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
-	acpi_pci_osc_support(root, flags);
+	support = base_support = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
+	acpi_pci_osc_support(root, support);
 
 	if (pci_ext_cfg_avail())
-		flags |= OSC_PCI_EXT_CONFIG_SUPPORT;
+		support |= OSC_PCI_EXT_CONFIG_SUPPORT;
 	if (pcie_aspm_support_enabled()) {
-		flags |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
+		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
 	}
 	if (pci_msi_enabled())
-		flags |= OSC_PCI_MSI_SUPPORT;
-	if (flags != base_flags) {
-		status = acpi_pci_osc_support(root, flags);
+		support |= OSC_PCI_MSI_SUPPORT;
+	if (support != base_support) {
+		status = acpi_pci_osc_support(root, support);
 		if (ACPI_FAILURE(status)) {
 			dev_info(&device->dev, "ACPI _OSC support "
 				"notification failed, disabling PCIe ASPM\n");
 			*no_aspm = 1;
-			flags = base_flags;
+			support = base_support;
 		}
 	}
 
 	if (!pcie_ports_disabled
-	    && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
-		flags = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
+	    && (support & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
+		control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
 			| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
 			| OSC_PCI_EXPRESS_PME_CONTROL;
 
@@ -403,17 +403,18 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 				dev_dbg(&device->dev,
 					"PCIe errors handled by BIOS.\n");
 			else
-				flags |= OSC_PCI_EXPRESS_AER_CONTROL;
+				control |= OSC_PCI_EXPRESS_AER_CONTROL;
 		}
 
 		dev_info(&device->dev,
-			"Requesting ACPI _OSC control (0x%02x)\n", flags);
+			"Requesting ACPI _OSC control (0x%02x)\n", control);
 
-		status = acpi_pci_osc_control_set(handle, &flags,
+		status = acpi_pci_osc_control_set(handle, &control,
 				       OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
 		if (ACPI_SUCCESS(status)) {
 			dev_info(&device->dev,
-				"ACPI _OSC control (0x%02x) granted\n", flags);
+				"ACPI _OSC control (0x%02x) granted\n",
+				control);
 			if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
 				/*
 				 * We have ASPM control, but the FADT indicates
@@ -425,7 +426,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 			dev_info(&device->dev,
 				"ACPI _OSC request failed (%s), "
 				"returned control mask: 0x%02x\n",
-				acpi_format_exception(status), flags);
+				acpi_format_exception(status), control);
 			dev_info(&device->dev,
 				 "ACPI _OSC control for PCIe not granted, disabling ASPM\n");
 			/*
@@ -440,7 +441,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 	} else {
 		dev_info(&device->dev,
 			 "Unable to request _OSC control "
-			 "(_OSC support mask: 0x%02x)\n", flags);
+			 "(_OSC support mask: 0x%02x)\n", support);
 	}
 }
 

[PATCH 10/14] PCI/ACPI: Run _OSC only once for OSPM feature support

[Bjorn Helgaas]

Previously, we ran _OSC once to tell the platform that we support
PCI Segment Groups, then we ran it again if we supported any additional
features (ASPM, MSI, or extended config space).  I don't think it's
necessary to run it twice, since we can easily build the complete
mask of features we support before running _OSC the first time.

We run _OSC again later when requesting control of PCIe features;
that's unaffected by this change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/pci_root.c |   19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 3e06d4e..0e20041 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -373,23 +373,18 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 	 * PCI domains, so we indicate this in _OSC support capabilities.
 	 */
 	support = base_support = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
-	acpi_pci_osc_support(root, support);
-
 	if (pci_ext_cfg_avail())
 		support |= OSC_PCI_EXT_CONFIG_SUPPORT;
-	if (pcie_aspm_support_enabled()) {
+	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 (support != base_support) {
-		status = acpi_pci_osc_support(root, support);
-		if (ACPI_FAILURE(status)) {
-			dev_info(&device->dev, "ACPI _OSC support "
-				"notification failed, disabling PCIe ASPM\n");
-			*no_aspm = 1;
-			support = base_support;
-		}
+	status = acpi_pci_osc_support(root, support);
+	if (ACPI_FAILURE(status)) {
+		dev_info(&device->dev, "ACPI _OSC support "
+			"notification failed, disabling PCIe ASPM\n");
+		*no_aspm = 1;
+		support = base_support;
 	}
 
 	if (!pcie_ports_disabled

[PATCH 11/14] PCI/ACPI: Skip _OSC control tests if _OSC support call failed

[Bjorn Helgaas]

If the _OSC support notification fails, we will never request control
(because "support == OSC_PCI_SEGMENT_GROUPS_SUPPORT", which doesn't include
all the features in ACPI_PCIE_REQ_SUPPORT), so we can return early to
simplify the code.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/pci_root.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 0e20041..67cc43a 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -363,7 +363,7 @@ EXPORT_SYMBOL(acpi_pci_osc_control_set);
 static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 				 int *clear_aspm)
 {
-	u32 support, base_support, control;
+	u32 support, control;
 	acpi_status status;
 	struct acpi_device *device = root->device;
 	acpi_handle handle = device->handle;
@@ -372,7 +372,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 	 * All supported architectures that use ACPI have support for
 	 * PCI domains, so we indicate this in _OSC support capabilities.
 	 */
-	support = base_support = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
+	support = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
 	if (pci_ext_cfg_avail())
 		support |= OSC_PCI_EXT_CONFIG_SUPPORT;
 	if (pcie_aspm_support_enabled())
@@ -381,10 +381,10 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 		support |= OSC_PCI_MSI_SUPPORT;
 	status = acpi_pci_osc_support(root, support);
 	if (ACPI_FAILURE(status)) {
-		dev_info(&device->dev, "ACPI _OSC support "
-			"notification failed, disabling PCIe ASPM\n");
+		dev_info(&device->dev, "_OSC failed (%s); disabling ASPM\n",
+			 acpi_format_exception(status));
 		*no_aspm = 1;
-		support = base_support;
+		return;
 	}
 
 	if (!pcie_ports_disabled

[PATCH 12/14] PCI/ACPI: Separate out _OSC "PCIe port services disabled" path

[Bjorn Helgaas]

Test "pcie_ports_disabled" separately so we can give a better message.
Previously we said "Unable to request _OSC control..."; now we'll
say "PCIe port services disabled; not requesting _OSC control".

"pcie_ports_disabled" is true when CONFIG_PCIEPORTBUS=n or we boot
with "pcie_ports=compat".

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/pci_root.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 67cc43a..68e5a18 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -387,8 +387,12 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 		return;
 	}
 
-	if (!pcie_ports_disabled
-	    && (support & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
+	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) {
 		control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
 			| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
 			| OSC_PCI_EXPRESS_PME_CONTROL;

[PATCH 13/14] PCI/ACPI: Separate out _OSC "we don't support enough services" path

[Bjorn Helgaas]

Test the services we support (extended config space, ASPM, MSI) separately
so we can give a better message.  Previously we said "Unable to request
_OSC control..."; now we'll say "we support %#02x but %#02x are required".

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/pci_root.c |   84 ++++++++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 42 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 68e5a18..65aefcf 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -392,55 +392,55 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 		return;
 	}
 
-	if ((support & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
-		control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
-			| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
-			| OSC_PCI_EXPRESS_PME_CONTROL;
-
-		if (pci_aer_available()) {
-			if (aer_acpi_firmware_first())
-				dev_dbg(&device->dev,
-					"PCIe errors handled by BIOS.\n");
-			else
-				control |= OSC_PCI_EXPRESS_AER_CONTROL;
-		}
+	if ((support & ACPI_PCIE_REQ_SUPPORT) != ACPI_PCIE_REQ_SUPPORT) {
+		dev_info(&device->dev, "Not requesting _OSC control (we support %#02x but %#02x are required)\n",
+			 support, ACPI_PCIE_REQ_SUPPORT);
+		return;
+	}
+
+	control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
+		| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
+		| OSC_PCI_EXPRESS_PME_CONTROL;
+
+	if (pci_aer_available()) {
+		if (aer_acpi_firmware_first())
+			dev_dbg(&device->dev,
+				"PCIe errors handled by BIOS.\n");
+		else
+			control |= OSC_PCI_EXPRESS_AER_CONTROL;
+	}
 
+	dev_info(&device->dev,
+		 "Requesting ACPI _OSC control (0x%02x)\n", control);
+
+	status = acpi_pci_osc_control_set(handle, &control,
+					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
+	if (ACPI_SUCCESS(status)) {
 		dev_info(&device->dev,
-			"Requesting ACPI _OSC control (0x%02x)\n", control);
-
-		status = acpi_pci_osc_control_set(handle, &control,
-				       OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
-		if (ACPI_SUCCESS(status)) {
-			dev_info(&device->dev,
-				"ACPI _OSC control (0x%02x) granted\n",
-				control);
-			if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
-				/*
-				 * We have ASPM control, but the FADT indicates
-				 * that it's unsupported. Clear it.
-				 */
-				*clear_aspm = 1;
-			}
-		} else {
-			dev_info(&device->dev,
-				"ACPI _OSC request failed (%s), "
-				"returned control mask: 0x%02x\n",
-				acpi_format_exception(status), control);
-			dev_info(&device->dev,
-				 "ACPI _OSC control for PCIe not granted, disabling ASPM\n");
+			 "ACPI _OSC control (0x%02x) granted\n",
+			 control);
+		if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
 			/*
-			 * We want to disable ASPM here, but aspm_disabled
-			 * needs to remain in its state from boot so that we
-			 * properly handle PCIe 1.1 devices.  So we set this
-			 * flag here, to defer the action until after the ACPI
-			 * root scan.
+			 * We have ASPM control, but the FADT indicates
+			 * that it's unsupported. Clear it.
 			 */
-			*no_aspm = 1;
+			*clear_aspm = 1;
 		}
 	} else {
 		dev_info(&device->dev,
-			 "Unable to request _OSC control "
-			 "(_OSC support mask: 0x%02x)\n", support);
+			"ACPI _OSC request failed (%s), "
+			"returned control mask: 0x%02x\n",
+			acpi_format_exception(status), control);
+		dev_info(&device->dev,
+			 "ACPI _OSC control for PCIe not granted, disabling ASPM\n");
+		/*
+		 * We want to disable ASPM here, but aspm_disabled
+		 * needs to remain in its state from boot so that we
+		 * properly handle PCIe 1.1 devices.  So we set this
+		 * flag here, to defer the action until after the ACPI
+		 * root scan.
+		 */
+		*no_aspm = 1;
 	}
 }
 

[PATCH 14/14] PCI/ACPI: Decode _OSC bitmasks symbolically

[Bjorn Helgaas]

This updates _OSC-related messages to be more human-readable.  We now always
show the features we declare support for (this was previously invisible) as
well as the features we are granted control of.

Typical changes:

  -acpi PNP0A08:00: Requesting ACPI _OSC control (0x1d)
  -acpi PNP0A08:00: ACPI _OSC control (0x1d) granted
  +acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
  +acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME AER PCIeCapability]

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/pci_root.c |   84 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 65aefcf..cba966e 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -127,6 +127,55 @@ static acpi_status try_get_root_bridge_busnr(acpi_handle handle,
 	return AE_OK;
 }
 
+struct pci_osc_bit_struct {
+	u32 bit;
+	char *desc;
+};
+
+static struct pci_osc_bit_struct pci_osc_support_bit[] = {
+	{ OSC_PCI_EXT_CONFIG_SUPPORT, "ExtendedConfig" },
+	{ OSC_PCI_ASPM_SUPPORT, "ASPM" },
+	{ OSC_PCI_CLOCK_PM_SUPPORT, "ClockPM" },
+	{ OSC_PCI_SEGMENT_GROUPS_SUPPORT, "Segments" },
+	{ OSC_PCI_MSI_SUPPORT, "MSI" },
+};
+
+static struct pci_osc_bit_struct pci_osc_control_bit[] = {
+	{ OSC_PCI_EXPRESS_NATIVE_HP_CONTROL, "PCIeHotplug" },
+	{ OSC_PCI_SHPC_NATIVE_HP_CONTROL, "SHPCHotplug" },
+	{ OSC_PCI_EXPRESS_PME_CONTROL, "PME" },
+	{ OSC_PCI_EXPRESS_AER_CONTROL, "AER" },
+	{ OSC_PCI_EXPRESS_CAPABILITY_CONTROL, "PCIeCapability" },
+};
+
+static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word,
+			    struct pci_osc_bit_struct *table, int size)
+{
+	char buf[80];
+	int i, len = 0;
+	struct pci_osc_bit_struct *entry;
+
+	buf[0] = '\0';
+	for (i = 0, entry = table; i < size; i++, entry++)
+		if (word & entry->bit)
+			len += snprintf(buf + len, sizeof(buf) - len, "%s%s",
+				        len ? " " : "", entry->desc);
+
+	dev_info(&root->device->dev, "_OSC: %s [%s]\n", msg, buf);
+}
+
+static void decode_osc_support(struct acpi_pci_root *root, char *msg, u32 word)
+{
+	decode_osc_bits(root, msg, word, pci_osc_support_bit,
+			ARRAY_SIZE(pci_osc_support_bit));
+}
+
+static void decode_osc_control(struct acpi_pci_root *root, char *msg, u32 word)
+{
+	decode_osc_bits(root, msg, word, pci_osc_control_bit,
+			ARRAY_SIZE(pci_osc_control_bit));
+}
+
 static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766";
 
 static acpi_status acpi_pci_run_osc(acpi_handle handle,
@@ -340,10 +389,14 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
 			goto out;
 		if (ctrl == *mask)
 			break;
+		decode_osc_control(root, "platform does not support",
+				   ctrl & ~(*mask));
 		ctrl = *mask;
 	}
 
 	if ((ctrl & req) != req) {
+		decode_osc_control(root, "not requesting control; platform does not support",
+				   req & ~(ctrl));
 		status = AE_SUPPORT;
 		goto out;
 	}
@@ -363,7 +416,7 @@ EXPORT_SYMBOL(acpi_pci_osc_control_set);
 static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 				 int *clear_aspm)
 {
-	u32 support, control;
+	u32 support, control, requested;
 	acpi_status status;
 	struct acpi_device *device = root->device;
 	acpi_handle handle = device->handle;
@@ -379,6 +432,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
 	if (pci_msi_enabled())
 		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",
@@ -393,8 +448,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 	}
 
 	if ((support & ACPI_PCIE_REQ_SUPPORT) != ACPI_PCIE_REQ_SUPPORT) {
-		dev_info(&device->dev, "Not requesting _OSC control (we support %#02x but %#02x are required)\n",
-			 support, ACPI_PCIE_REQ_SUPPORT);
+		decode_osc_support(root, "not requesting OS control; OS requires",
+				   ACPI_PCIE_REQ_SUPPORT);
 		return;
 	}
 
@@ -404,21 +459,17 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 
 	if (pci_aer_available()) {
 		if (aer_acpi_firmware_first())
-			dev_dbg(&device->dev,
-				"PCIe errors handled by BIOS.\n");
+			dev_info(&device->dev,
+				 "PCIe AER handled by firmware\n");
 		else
 			control |= OSC_PCI_EXPRESS_AER_CONTROL;
 	}
 
-	dev_info(&device->dev,
-		 "Requesting ACPI _OSC control (0x%02x)\n", control);
-
+	requested = control;
 	status = acpi_pci_osc_control_set(handle, &control,
 					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
 	if (ACPI_SUCCESS(status)) {
-		dev_info(&device->dev,
-			 "ACPI _OSC control (0x%02x) granted\n",
-			 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
@@ -427,12 +478,11 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 			*clear_aspm = 1;
 		}
 	} else {
-		dev_info(&device->dev,
-			"ACPI _OSC request failed (%s), "
-			"returned control mask: 0x%02x\n",
-			acpi_format_exception(status), control);
-		dev_info(&device->dev,
-			 "ACPI _OSC control for PCIe not granted, disabling ASPM\n");
+		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));
+
 		/*
 		 * We want to disable ASPM here, but aspm_disabled
 		 * needs to remain in its state from boot so that we

Re: [PATCH 00/14] _OSC simplification

[Rafael J. Wysocki]

On Friday, September 06, 2013 11:13:24 AM Bjorn Helgaas wrote:
> This is a long series of mostly minor changes with the goal of
> simplifying the _OSC-related code and making the negotiation between
> Linux and the platform more understandable.
> 
> My intent is that this doesn't change any behavior except for the text
> in dmesg.
> 
> However, this restructuring does raise some questions in my mind, such
> as the fact that we do not call pcie_no_aspm() to disable ASPM in two
> cases where it seems like we might want to:
> 
>   1) When pcie_ports_disabled, e.g., when booting with
>      "pcie_ports=compat".
>
>   2) When Linux doesn't support all the required services, e.g., if
>      compiled with ASPM support but not MSI support.

We tried that (i.e. 2)), but then it caused energy usage to increase on some
systems with power-hungry graphics adapters and Phoronix made some fuss about
that. :-)

Also, please have a look at:

commit 3c076351c4027a56d5005a39a0b518a4ba393ce2
Author: Matthew Garrett <mjg@redhat.com>
Date:   Thu Nov 10 16:38:33 2011 -0500

    PCI: Rework ASPM disable code

> I haven't looked deeply, so maybe there are other constraints that
> keep us from using ASPM in these cases.  In any case, this series
> doesn't change any behavior in this regard.
> 
> Below are samples of the dmesg changes from this series.  Typical case
> where we successfully get control of PCIe services:
> 
>   -acpi PNP0A08:00: Requesting ACPI _OSC control (0x1d)
>   -acpi PNP0A08:00: ACPI _OSC control (0x1d) granted
>   +acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
>   +acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME AER PCIeCapability]
> 
> Failure case where the system has no _OSC method:
> 
>   -acpi PNP0A03:00: ACPI _OSC support notification failed, disabling PCIe ASPM
>   -acpi PNP0A03:00: Unable to request _OSC control (_OSC support mask: 0x08)
>   +acpi PNP0A03:00: _OSC: OS supports [ASPM ClockPM Segments MSI]
>   +acpi PNP0A03:00: _OSC failed (AE_NOT_FOUND); disabling ASPM
> 
> Linux only requests _OSC control of features as a group; if Linux
> doesn't support *all* of those features, we don't request control of
> any of them (per 415e12b237).  Here's an example booted with
> "pci=nomsi":
> 
>   -acpi PNP0A08:00: Unable to request _OSC control (_OSC support mask: 0x0f)
>   +acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments]
>   +acpi PNP0A08:00: _OSC: not requesting OS control; OS requires [ExtendedConfig ASPM ClockPM MSI]
> 
> Here's a system that doesn't grant OS control of the PCIe Capability.
> This is another case where we don't want control of anything unless we
> have control of the PCIe Capability:
> 
>   -acpi PNP0A08:00: Requesting ACPI _OSC control (0x1d)
>   -acpi PNP0A08:00: ACPI _OSC request failed (AE_SUPPORT), returned control mask: 0x0d
>   -acpi PNP0A08:00: ACPI _OSC control for PCIe not granted, disabling ASPM
>   +acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
>   +acpi PNP0A08:00: _OSC: platform does not support [PCIeCapability]
>   +acpi PNP0A08:00: _OSC: not requesting control; platform does not support [PCIeCapability]
>   +acpi PNP0A08:00: _OSC: OS requested [PCIeHotplug PME AER PCIeCapability]
>   +acpi PNP0A08:00: _OSC: platform willing to grant [PCIeHotplug PME AER]
>   +acpi PNP0A08:00: _OSC failed (AE_SUPPORT); disabling ASPM

That looks nice. :-)

Thanks,
Rafael

Re: [PATCH 00/14] _OSC simplification

[Rafael J. Wysocki]

On Friday, September 06, 2013 11:13:24 AM Bjorn Helgaas wrote:
> This is a long series of mostly minor changes with the goal of
> simplifying the _OSC-related code and making the negotiation between
> Linux and the platform more understandable.
> 
> My intent is that this doesn't change any behavior except for the text
> in dmesg.
> 
> However, this restructuring does raise some questions in my mind, such
> as the fact that we do not call pcie_no_aspm() to disable ASPM in two
> cases where it seems like we might want to:
> 
>   1) When pcie_ports_disabled, e.g., when booting with
>      "pcie_ports=compat".
> 
>   2) When Linux doesn't support all the required services, e.g., if
>      compiled with ASPM support but not MSI support.
> 
> I haven't looked deeply, so maybe there are other constraints that
> keep us from using ASPM in these cases.  In any case, this series
> doesn't change any behavior in this regard.
> 
> Below are samples of the dmesg changes from this series.  Typical case
> where we successfully get control of PCIe services:
> 
>   -acpi PNP0A08:00: Requesting ACPI _OSC control (0x1d)
>   -acpi PNP0A08:00: ACPI _OSC control (0x1d) granted
>   +acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
>   +acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME AER PCIeCapability]
> 
> Failure case where the system has no _OSC method:
> 
>   -acpi PNP0A03:00: ACPI _OSC support notification failed, disabling PCIe ASPM
>   -acpi PNP0A03:00: Unable to request _OSC control (_OSC support mask: 0x08)
>   +acpi PNP0A03:00: _OSC: OS supports [ASPM ClockPM Segments MSI]
>   +acpi PNP0A03:00: _OSC failed (AE_NOT_FOUND); disabling ASPM
> 
> Linux only requests _OSC control of features as a group; if Linux
> doesn't support *all* of those features, we don't request control of
> any of them (per 415e12b237).  Here's an example booted with
> "pci=nomsi":
> 
>   -acpi PNP0A08:00: Unable to request _OSC control (_OSC support mask: 0x0f)
>   +acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments]
>   +acpi PNP0A08:00: _OSC: not requesting OS control; OS requires [ExtendedConfig ASPM ClockPM MSI]
> 
> Here's a system that doesn't grant OS control of the PCIe Capability.
> This is another case where we don't want control of anything unless we
> have control of the PCIe Capability:
> 
>   -acpi PNP0A08:00: Requesting ACPI _OSC control (0x1d)
>   -acpi PNP0A08:00: ACPI _OSC request failed (AE_SUPPORT), returned control mask: 0x0d
>   -acpi PNP0A08:00: ACPI _OSC control for PCIe not granted, disabling ASPM
>   +acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
>   +acpi PNP0A08:00: _OSC: platform does not support [PCIeCapability]
>   +acpi PNP0A08:00: _OSC: not requesting control; platform does not support [PCIeCapability]
>   +acpi PNP0A08:00: _OSC: OS requested [PCIeHotplug PME AER PCIeCapability]
>   +acpi PNP0A08:00: _OSC: platform willing to grant [PCIeHotplug PME AER]
>   +acpi PNP0A08:00: _OSC failed (AE_SUPPORT); disabling ASPM
> 
> 
> This is based on a923874198 ("Merge tag 'pci-v3.12-changes' of
> git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci").
> 
> ---
> 
> Bjorn Helgaas (14):
>       ACPI: Write _OSC bit field definitions in hex
>       ACPI: Rename OSC_QUERY_TYPE to OSC_QUERY_DWORD
>       ACPI: Tidy acpi_run_osc() declarations
>       ACPI: Remove unused OSC_PCI_NATIVE_HOTPLUG
>       ACPI: Write OSC_PCI_CONTROL_MASKS like OSC_PCI_SUPPORT_MASKS
>       PCI/ACPI: Name _OSC #defines more consistently
>       PCI/ACPI: Drop unnecessary _OSC existence tests
>       PCI/ACPI: Move _OSC stuff from acpi_pci_root_add() to negotiate_os_control()
>       PCI/ACPI: Split _OSC "support" and "control" flags into separate variables
>       PCI/ACPI: Run _OSC only once for OSPM feature support
>       PCI/ACPI: Skip _OSC control tests if _OSC support call failed
>       PCI/ACPI: Separate out _OSC "PCIe port services disabled" path
>       PCI/ACPI: Separate out _OSC "we don't support enough services" path
>       PCI/ACPI: Decode _OSC bitmasks symbolically

Please feel free to add

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

to the entire series.

Thanks,
Rafael

Re: [PATCH 00/14] _OSC simplification

[Bjorn Helgaas]

On Fri, Sep 6, 2013 at 6:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, September 06, 2013 11:13:24 AM Bjorn Helgaas wrote:
>> This is a long series of mostly minor changes with the goal of
>> simplifying the _OSC-related code and making the negotiation between
>> Linux and the platform more understandable.
>>
>> My intent is that this doesn't change any behavior except for the text
>> in dmesg.
>>
>> However, this restructuring does raise some questions in my mind, such
>> as the fact that we do not call pcie_no_aspm() to disable ASPM in two
>> cases where it seems like we might want to:
>>
>>   1) When pcie_ports_disabled, e.g., when booting with
>>      "pcie_ports=compat".
>>
>>   2) When Linux doesn't support all the required services, e.g., if
>>      compiled with ASPM support but not MSI support.
>
> We tried that (i.e. 2)), but then it caused energy usage to increase on some
> systems with power-hungry graphics adapters and Phoronix made some fuss about
> that. :-)

The _OSC section of the spec is pretty obtuse, but my reading of Table
4-6 (PCI Firmware r3.0) is that the OS is prohibited from writing to
the PCIe Capability (for ASPM, VC, AER, or other control) unless it
has been granted "PCI Express Capability Structure control".  That's
why I'm dubious about the fact that we use ASPM in those two cases
where we don't even *ask* for that control.

For the Phoronix case, I assume they made the argument that Windows
does use ASPM without having PCIe Capability control?  Do you have any
pointers to that discussion?

I don't propose changing this now, but it does raise my eyebrows.
Maybe some sort of dmesg note would be appropriate.

Thanks for looking at all this stuff; I know from experience that it's
pretty hard to wade through :)

Bjorn

Re: [PATCH 00/14] _OSC simplification

[Rafael J. Wysocki]

On Saturday, September 07, 2013 07:59:39 AM Bjorn Helgaas wrote:
> On Fri, Sep 6, 2013 at 6:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Friday, September 06, 2013 11:13:24 AM Bjorn Helgaas wrote:
> >> This is a long series of mostly minor changes with the goal of
> >> simplifying the _OSC-related code and making the negotiation between
> >> Linux and the platform more understandable.
> >>
> >> My intent is that this doesn't change any behavior except for the text
> >> in dmesg.
> >>
> >> However, this restructuring does raise some questions in my mind, such
> >> as the fact that we do not call pcie_no_aspm() to disable ASPM in two
> >> cases where it seems like we might want to:
> >>
> >>   1) When pcie_ports_disabled, e.g., when booting with
> >>      "pcie_ports=compat".
> >>
> >>   2) When Linux doesn't support all the required services, e.g., if
> >>      compiled with ASPM support but not MSI support.
> >
> > We tried that (i.e. 2)), but then it caused energy usage to increase on some
> > systems with power-hungry graphics adapters and Phoronix made some fuss about
> > that. :-)
> 
> The _OSC section of the spec is pretty obtuse, but my reading of Table
> 4-6 (PCI Firmware r3.0) is that the OS is prohibited from writing to
> the PCIe Capability (for ASPM, VC, AER, or other control) unless it
> has been granted "PCI Express Capability Structure control".  That's
> why I'm dubious about the fact that we use ASPM in those two cases
> where we don't even *ask* for that control.
> 
> For the Phoronix case, I assume they made the argument that Windows
> does use ASPM without having PCIe Capability control?

No, this was regarded as a kernel regression that needed to be addressed.

> Do you have any pointers to that discussion?

This is the Phoronix link:

http://www.phoronix.com/scan.php?page=article&item=linux_aspm_solution&num=1

and I think you can get to some more info from there.

> I don't propose changing this now, but it does raise my eyebrows.
> Maybe some sort of dmesg note would be appropriate.
> 
> Thanks for looking at all this stuff; I know from experience that it's
> pretty hard to wade through :)

No problem. :-)

Thanks,
Rafael

Re: [PATCH 00/14] _OSC simplification

[Robert Hancock]

On 09/07/2013 07:59 AM, Bjorn Helgaas wrote:
> On Fri, Sep 6, 2013 at 6:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Friday, September 06, 2013 11:13:24 AM Bjorn Helgaas wrote:
>>> This is a long series of mostly minor changes with the goal of
>>> simplifying the _OSC-related code and making the negotiation between
>>> Linux and the platform more understandable.
>>>
>>> My intent is that this doesn't change any behavior except for the text
>>> in dmesg.
>>>
>>> However, this restructuring does raise some questions in my mind, such
>>> as the fact that we do not call pcie_no_aspm() to disable ASPM in two
>>> cases where it seems like we might want to:
>>>
>>>    1) When pcie_ports_disabled, e.g., when booting with
>>>       "pcie_ports=compat".
>>>
>>>    2) When Linux doesn't support all the required services, e.g., if
>>>       compiled with ASPM support but not MSI support.
>>
>> We tried that (i.e. 2)), but then it caused energy usage to increase on some
>> systems with power-hungry graphics adapters and Phoronix made some fuss about
>> that. :-)
>
> The _OSC section of the spec is pretty obtuse, but my reading of Table
> 4-6 (PCI Firmware r3.0) is that the OS is prohibited from writing to
> the PCIe Capability (for ASPM, VC, AER, or other control) unless it
> has been granted "PCI Express Capability Structure control".  That's
> why I'm dubious about the fact that we use ASPM in those two cases
> where we don't even *ask* for that control.

AFAIK, I believe the problem was that in cases where we decided that 
ASPM shouldn't be used (such as if the FADT flag for "don't enable ASPM" 
was set), the kernel was explicitly disabling ASPM for devices even if 
the BIOS had turned it on. This caused a power regression compared to 
Windows which left ASPM enabled on those devices. I believe this was 
later changed to treat this case as "don't touch ASPM settings" rather 
than "force ASPM off on everything".

I'm not sure that all kernel messages have been updated to reflect this, 
though - there seem to be some cases where we confusingly say "disabling 
ASPM" when what it should likely say "disabling ASPM control".

>
> For the Phoronix case, I assume they made the argument that Windows
> does use ASPM without having PCIe Capability control?  Do you have any
> pointers to that discussion?
>
> I don't propose changing this now, but it does raise my eyebrows.
> Maybe some sort of dmesg note would be appropriate.
>
> Thanks for looking at all this stuff; I know from experience that it's
> pretty hard to wade through :)