ACPI fixes for PCIe AER

PATCH In review Andrew Patterson 4 participants 10 messages Jan 23, 2008 – Feb 12, 2008

Subsystems (2)

ACPI PCI

lore

Share & tools

lore

Andrew Patterson

author

2008-01-23 00:18 UTC #0 lore raw

cover ACPI fixes for PCIe AER

The following patch series fixes some bugs in how Linux determines whether PCIe Advance Error Reporting (AER) is supported on a platform. It is currently broken on at least HP IA-64 systems.

- PCI: Run ACPI _OSC method on root bridges only
- ACPI: Check for any matching CID when walking namespace.
- PCI ACPI: AER driver should only register PCIe devices with _OSC.
- PCI ACPI: Added a function to register _OSC with only PCIe devices.

These patches apply to gregkh's patch tree:

git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/patches

Signature · 1 line

Andrew Patterson

Andrew Patterson

author

2008-01-23 00:18 UTC #1 lore raw

PATCH 1/4 PCI ACPI: Added a function to register _OSC with only PCIe devices.

From: Andrew Patterson <[email protected]>

The function pci_osc_support_set() traverses every root bridge when checking for _OSC support for a capability. It quits as soon as it finds a device/bridge that doesn't support the requested capability. This won't work for systems that have mixed PCI and PCIe bridges when checking for PCIe features. I split this function into two -- pci_osc_support_set() and pcie_osc_support_set(). The latter is used when only PCIe devices should be traversed.

Signed-off-by Andrew Patterson <[email protected]>

---

 drivers/pci/pci-acpi.c   |    6 +++---
 include/linux/pci-acpi.h |   11 ++++++++++-
 2 files changed, 13 insertions(+), 4 deletions(-)

drivers/pci/pci-acpi.c +3 -3

index 02e4876..ec61428 100644
@@ -156,13 +156,13 @@ run_osc_out:
 }
 
 /**
- * pci_osc_support_set - register OS support to Firmware
+__pci_osc_support_set - register OS support to Firmware
  * @flags: OS support bits
  *
  * Update OS support fields and doing a _OSC Query to obtain an update
  * from Firmware on supported control bits.
  **/
-acpi_status pci_osc_support_set(u32 flags)
+acpi_status __pci_osc_support_set(u32 flags, const char *hid)
 {
 	u32 temp;
 	acpi_status retval;

@@ -176,7 +176,7 @@ acpi_status pci_osc_support_set(u32 flags)
 	temp = ctrlset_buf[OSC_CONTROL_TYPE];
 	ctrlset_buf[OSC_QUERY_TYPE] = OSC_QUERY_ENABLE;
 	ctrlset_buf[OSC_CONTROL_TYPE] = OSC_CONTROL_MASKS;
-	acpi_get_devices ( PCI_ROOT_HID_STRING,
+	acpi_get_devices(hid,
 			acpi_query_osc,
 			ctrlset_buf,
 			(void **) &retval );

include/linux/pci-acpi.h +10 -1

index 936ef82..3ba2506 100644
@@ -48,7 +48,15 @@
 
 #ifdef CONFIG_ACPI
 extern acpi_status pci_osc_control_set(acpi_handle handle, u32 flags);
-extern acpi_status pci_osc_support_set(u32 flags);
+extern acpi_status __pci_osc_support_set(u32 flags, const char *hid);
+static inline acpi_status pci_osc_support_set(u32 flags)
+{
+	return __pci_osc_support_set(flags, PCI_ROOT_HID_STRING);
+}
+static inline acpi_status pcie_osc_support_set(u32 flags)
+{
+	return __pci_osc_support_set(flags, PCI_EXPRESS_ROOT_HID_STRING);
+}
 #else
 #if !defined(AE_ERROR)
 typedef u32 		acpi_status;

@@ -57,6 +65,7 @@ typedef u32 		acpi_status;
 static inline acpi_status pci_osc_control_set(acpi_handle handle, u32 flags)
 {return AE_ERROR;}
 static inline acpi_status pci_osc_support_set(u32 flags) {return AE_ERROR;} 
+static inline acpi_status pcie_osc_support_set(u32 flags) {return AE_ERROR;}
 #endif
 
 #endif	/* _PCI_ACPI_H_ */

Andrew Patterson

author

2008-01-23 00:18 UTC #2 lore raw

PATCH 2/4 PCI ACPI: AER driver should only register PCIe devices with _OSC.

In reply to Andrew Patterson

From: Andrew Patterson <[email protected]>

AER is only used with PCIe devices so we should only check PCIe devices for _OSC support.

Signed-off-by Andrew Patterson <[email protected]>

---

 drivers/pci/pcie/aer/aerdrv_acpi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/pci/pcie/aer/aerdrv_acpi.c +1 -1

index 1a1eb45..f685bf5 100644
@@ -50,7 +50,7 @@ int aer_osc_setup(struct pcie_device *pciedev)
 	}
 
 	if (handle) {
-		pci_osc_support_set(OSC_EXT_PCI_CONFIG_SUPPORT);
+		pcie_osc_support_set(OSC_EXT_PCI_CONFIG_SUPPORT);
 		status = pci_osc_control_set(handle,
 					OSC_PCI_EXPRESS_AER_CONTROL |
 					OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL);

Andrew Patterson

author

2008-01-23 00:18 UTC #3 lore raw

PATCH 3/4 ACPI: Check for any matching CID when walking namespace.

In reply to Andrew Patterson

From: Andrew Patterson <[email protected]>

The callback function acpi_ns_get_device_callback called from acpi_get_devices() will check CID's if the HID does not match. This code has a bug where it requires that all CIDs match the HID. Changed the code so that any CID match will do.

Signed-off-by Andrew Patterson <[email protected]>

---

 drivers/acpi/namespace/nsxfeval.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

drivers/acpi/namespace/nsxfeval.c +8 -3

index f39fbc6..e562b24 100644
@@ -443,6 +443,7 @@ acpi_ns_get_device_callback(acpi_handle obj_handle,
 	struct acpica_device_id hid;
 	struct acpi_compatible_id_list *cid;
 	acpi_native_uint i;
+	int found;
 
 	status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
 	if (ACPI_FAILURE(status)) {

@@ -496,16 +497,20 @@ acpi_ns_get_device_callback(acpi_handle obj_handle,
 
 			/* Walk the CID list */
 
+			found = 0;
 			for (i = 0; i < cid->count; i++) {
 				if (ACPI_STRNCMP(cid->id[i].value, info->hid,
 						 sizeof(struct
-							acpi_compatible_id)) !=
+							acpi_compatible_id)) ==
 				    0) {
-					ACPI_FREE(cid);
-					return (AE_OK);
+					found = 1;
+					break;
 				}
 			}
 			ACPI_FREE(cid);
+			if (!found) {
+				return (AE_OK);
+			}
 		}
 	}

Greg KH

2008-01-23 17:51 UTC #4 lore raw

Re: [PATCH 3/4] ACPI: Check for any matching CID when walking namespace.

Quoted 7 lines · Andrew Patterson

From: Andrew Patterson <[email protected]>
The callback function acpi_ns_get_device_callback called from acpi_get_devices() will check CID's if the HID does not match. This code has a bug where it requires that all CIDs match the HID. Changed the code so that any CID match will do.

Quoted 5 lines

Signed-off-by Andrew Patterson <[email protected]>
---
 drivers/acpi/namespace/nsxfeval.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

This should probably go through the ACPI tree, not the PCI tree, right?

thanks,

greg k-h

Andrew Patterson

author

2008-01-23 19:35 UTC #5 lore raw

Re: [PATCH 3/4] ACPI: Check for any matching CID when walking namespace.

Quoted 8 lines · Greg KH

Quoted 7 lines · Andrew Patterson
From: Andrew Patterson <[email protected]>
The callback function acpi_ns_get_device_callback called from acpi_get_devices() will check CID's if the HID does not match. This code has a bug where it requires that all CIDs match the HID. Changed the code so that any CID match will do.

Quoted 8 lines

Quoted 5 lines
Signed-off-by Andrew Patterson <[email protected]>
---
 drivers/acpi/namespace/nsxfeval.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)
This should probably go through the ACPI tree, not the PCI tree, right?

Yes, but it is needed to get PCIe AER working (at least on the systems that I have seen). I suspect we will have the same issue with PCIe hotplug. Len Brown is looking at it.

Quoted 3 lines

thanks,
greg k-h

Len Brown

reviewer

2008-01-23 19:40 UTC #6 lore raw

Re: [PATCH 3/4] ACPI: Check for any matching CID when walking namespace.

In reply to Andrew Patterson

Right Greg, this is not only an ACPI patch,
but a patch that touches the ACPICA core.
Lets get Bob to see if it applies to the latest upstream ACPICA code.

Andrew,
Do you give Intel permission to accept this patch under both licenses on the top of the file, such that we can re-distribute it in ACPICA to all the OSs that use ACPICA?

(hey, and check out the spiffy new acpica home page: http://acpica.org/ )

thanks,
-Len

Quoted 50 lines · Andrew Patterson

From: Andrew Patterson <[email protected]>

Signed-off-by Andrew Patterson <[email protected]>

---

 drivers/acpi/namespace/nsxfeval.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

drivers/acpi/namespace/nsxfeval.c +8 -3

index f39fbc6..e562b24 100644
@@ -443,6 +443,7 @@ acpi_ns_get_device_callback(acpi_handle obj_handle,
 	struct acpica_device_id hid;
 	struct acpi_compatible_id_list *cid;
 	acpi_native_uint i;
+	int found;
 
 	status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
 	if (ACPI_FAILURE(status)) {

@@ -496,16 +497,20 @@ acpi_ns_get_device_callback(acpi_handle obj_handle,
 
 			/* Walk the CID list */
 
+			found = 0;
 			for (i = 0; i < cid->count; i++) {
 				if (ACPI_STRNCMP(cid->id[i].value, info->hid,
 						 sizeof(struct
-							acpi_compatible_id)) !=
+							acpi_compatible_id)) ==
 				    0) {
-					ACPI_FREE(cid);
-					return (AE_OK);
+					found = 1;
+					break;
 				}
 			}
 			ACPI_FREE(cid);
+			if (!found) {
+				return (AE_OK);
+			}
 		}
 	}

Andrew Patterson

author

2008-01-23 19:48 UTC #7 lore raw

Re: [PATCH 3/4] ACPI: Check for any matching CID when walking namespace.

Quoted 9 lines · Len Brown

Right Greg, this is not only an ACPI patch,
but a patch that touches the ACPICA core.
Lets get Bob to see if it applies to the latest upstream ACPICA code.
Andrew,
Do you give Intel permission to accept this patch under both licenses on the top of the file, such that we can re-distribute it in ACPICA to all the OSs that use ACPICA?

Yes, by all means.

Andrew Patterson
Hewlett-Packard

Quoted 56 lines

(hey, and check out the spiffy new acpica home page: http://acpica.org/ )

thanks,
-Len

Quoted 50 lines · Andrew Patterson

From: Andrew Patterson <[email protected]>

Signed-off-by Andrew Patterson <[email protected]>

---

 drivers/acpi/namespace/nsxfeval.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

drivers/acpi/namespace/nsxfeval.c +8 -3

index f39fbc6..e562b24 100644
@@ -443,6 +443,7 @@ acpi_ns_get_device_callback(acpi_handle obj_handle,
 	struct acpica_device_id hid;
 	struct acpi_compatible_id_list *cid;
 	acpi_native_uint i;
+	int found;
 
 	status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
 	if (ACPI_FAILURE(status)) {

@@ -496,16 +497,20 @@ acpi_ns_get_device_callback(acpi_handle obj_handle,
 
 			/* Walk the CID list */
 
+			found = 0;
 			for (i = 0; i < cid->count; i++) {
 				if (ACPI_STRNCMP(cid->id[i].value, info->hid,
 						 sizeof(struct
-							acpi_compatible_id)) !=
+							acpi_compatible_id)) ==
 				    0) {
-					ACPI_FREE(cid);
-					return (AE_OK);
+					found = 1;
+					break;
 				}
 			}
 			ACPI_FREE(cid);
+			if (!found) {
+				return (AE_OK);
+			}
 		}
 	}

Moore, Robert

2008-02-12 23:42 UTC #8 lore raw

RE: [PATCH 3/4] ACPI: Check for any matching CID when walking namespace.

In reply to Len Brown

Ok, I've rolled this in:

			/* Walk the CID list */

			found = FALSE;
			for (i = 0; i < cid->count; i++) {
				if (ACPI_STRNCMP (cid->id[i].value,

info->hid,
sizeof (struct
acpi_compatible_id)) == 0) {

					/* Found a matching CID */

					found = TRUE;
					break;
				}
			}

			ACPI_FREE (cid);
			if (!found) {
				return (AE_OK);
			}
		}
	}

	/* We have a valid device, invoke the user function */

	status = info->user_function (obj_handle, nesting_level,
info->context,
			 return_value);
	return (status);
}

Thanks,
Bob

Quoted 6 lines

-----Original Message-----
From: Len Brown [mailto:[email protected]]
Sent: Wednesday, January 23, 2008 11:40 AM
To: Andrew Patterson; Moore, Robert
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Nguyen, Tom L;

[email protected]

Quoted 7 lines

Subject: Re: [PATCH 3/4] ACPI: Check for any matching CID when walking namespace.
Right Greg, this is not only an ACPI patch,
but a patch that touches the ACPICA core.
Lets get Bob to see if it applies to the latest upstream ACPICA code.

Quoted 6 lines

Andrew,
Do you give Intel permission to accept this patch under both licenses on the top of the file, such that we can re-distribute it in ACPICA to all the OSs that use ACPICA?
(hey, and check out the spiffy new acpica home page: http://acpica.org/

)

Quoted 9 lines

thanks,
-Len
Quoted 4 lines · Andrew Patterson
From: Andrew Patterson <[email protected]>
The callback function acpi_ns_get_device_callback called from acpi_get_devices() will check CID's if the HID does not match. This

code

Quoted 1 line

Quoted 1 line
has a bug where it requires that all CIDs match the HID. Changed the

code

Quoted 8 lines

Quoted 8 lines
so that any CID match will do.
Signed-off-by Andrew Patterson <[email protected]>
---
 drivers/acpi/namespace/nsxfeval.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

Quoted 6 lines

Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/acpi/namespace/nsxfeval.c
drivers/acpi/namespace/nsxfeval.c acpi_ns_get_device_callback(acpi_handle
Quoted 4 lines
index f39fbc6..e562b24 100644
drivers/acpi/namespace/nsxfeval.c +0 -0
@@ -443,6 +443,7 @@ acpi_ns_get_device_callback(acpi_handle

obj_handle,

Quoted 8 lines

drivers/acpi/namespace/nsxfeval.c acpi_ns_get_device_callback(acpi_handle

Quoted 8 lines

drivers/acpi/namespace/nsxfeval.c +1 -0

 	struct acpica_device_id hid;
 	struct acpi_compatible_id_list *cid;
 	acpi_native_uint i;
+	int found;
 
 	status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
 	if (ACPI_FAILURE(status)) {

@@ -496,16 +497,20 @@ acpi_ns_get_device_callback(acpi_handle

obj_handle,

Quoted 6 lines

drivers/acpi/namespace/nsxfeval.c acpi_ns_get_device_callback(acpi_handle obj_handle,
Quoted 6 lines
drivers/acpi/namespace/nsxfeval.c +1 -0
 			/* Walk the CID list */
 
+			found = 0;
 			for (i = 0; i < cid->count; i++) {
 				if (ACPI_STRNCMP(cid->id[i].value,

info->hid,

Quoted 2 lines

drivers/acpi/namespace/nsxfeval.c acpi_ns_get_device_callback(acpi_handle · line 501
Quoted 2 lines
drivers/acpi/namespace/nsxfeval.c +0 -1
 						 sizeof(struct
-

acpi_compatible_id)) !=

Quoted 1 line

drivers/acpi/namespace/nsxfeval.c acpi_ns_get_device_callback(acpi_handle · line 503
Quoted 1 line
drivers/acpi/namespace/nsxfeval.c +1 -0
+

acpi_compatible_id)) ==

Quoted 15 lines

drivers/acpi/namespace/nsxfeval.c +5 -2

 				    0) {
-					ACPI_FREE(cid);
-					return (AE_OK);
+					found = 1;
+					break;
 				}
 			}
 			ACPI_FREE(cid);
+			if (!found) {
+				return (AE_OK);
+			}
 		}
 	}

Andrew Patterson

author

2008-01-23 00:18 UTC #9 lore raw

PATCH 4/4 PCI: Run ACPI _OSC method on root bridges only

In reply to Andrew Patterson

From: Andrew Patterson <[email protected]>

According to the PCI Firmware Specification Revision 3.0 section 4.5, _OSC should only be called on a root brdige. Here is the relevant passage: "The _OSC interface defined in this section applies only to Host Bridge ACPI devices that originate PCI, PCI-X, or PCI Express hierarchies". Changed the code to find the parent root bridge of the device and call _OSC on that.

Signed-off-by Andrew Patterson <[email protected]>

---

 drivers/pci/pcie/aer/aerdrv_acpi.c |   22 ++++++----------------
 1 files changed, 6 insertions(+), 16 deletions(-)

drivers/pci/pcie/aer/aerdrv_acpi.c +6 ~~-16~~

index f685bf5..8c199ae 100644
@@ -31,23 +31,13 @@ int aer_osc_setup(struct pcie_device *pciedev)
 {
 	acpi_status status = AE_NOT_FOUND;
 	struct pci_dev *pdev = pciedev->port;
-	acpi_handle handle = DEVICE_ACPI_HANDLE(&pdev->dev);
-	struct pci_bus *parent;
+	acpi_handle handle = 0;
 
-	while (!handle) {
-		if (!pdev || !pdev->bus->parent)
-			break;
-		parent = pdev->bus->parent;
-		if (!parent->self)
-			/* Parent must be a host bridge */
-			handle = acpi_get_pci_rootbridge_handle(
-					pci_domain_nr(parent),
-					parent->number);
-		else
-			handle = DEVICE_ACPI_HANDLE(
-					&(parent->self->dev));
-		pdev = parent->self;
-	}
+	/* Find root host bridge */
+	while (pdev->bus && pdev->bus->self)
+		pdev = pdev->bus->self;
+	handle = acpi_get_pci_rootbridge_handle(
+		pci_domain_nr(pdev->bus), pdev->bus->number);
 
 	if (handle) {
 		pcie_osc_support_set(OSC_EXT_PCI_CONFIG_SUPPORT);