PCI / PCIe: Ask BIOS for control of all native services at once (v4)

From: Rafael J. Wysocki <[email protected]>

PCIe port service drivers ask the BIOS, through _OSC, for control of the services they handle. Unfortunately, each of them individually asks for control of the PCIe capability structure and if that is granted, some BIOSes expect that the other PCIe port services will be configured and handled by the kernel as well. If that is not the case (eg. one of the PCIe port service drivers is not loaded), the BIOS may be confused and may cause the system as a whole to misbehave (eg. on one of such systems enabling the native PCIe PME service without loading the native PCIe hot-plug service driver causes a storm of ACPI notify requests to appear).

For this reason rework the PCIe port driver so that (1) it checks which native PCIe port services can be enabled, according to the BIOS, and (2) it requests control of all these services simultaneously. In particular, this causes pcie_portdrv_probe() to fail if the BIOS refuses to grant control of the PCIe capability structure, which means that no native PCIe port services can be enabled for the PCIe root complex the given port belongs to.

Make it possible to override this behavior using a new command line switch pcie_ports= that can be set to 'auto' (ask the BIOS, the default), 'native' (use the PCIe native services regardless of the BIOS response to the control request), or 'compat' (do not use the PCIe native services at all).

Accordingly, rework the existing PCIe port service drivers so that they don't request control of the services directly.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

From: Rafael J. Wysocki <[email protected]>

PCIe port service drivers ask the BIOS, through _OSC, for control of the services they handle. Unfortunately, each of them individually asks for control of the PCIe capability structure and if that is granted, some BIOSes expect that the other PCIe port services will be configured and handled by the kernel as well. If that is not the case (eg. one of the PCIe port service drivers is not loaded), the BIOS may be confused and may cause the system as a whole to misbehave (eg. on one of such systems enabling the native PCIe PME service without loading the native PCIe hot-plug service driver causes a storm of ACPI notify requests to appear).

For this reason rework the PCIe port driver so that (1) it checks which native PCIe port services can be enabled, according to the BIOS, and (2) it requests control of all these services simultaneously. In particular, this causes pcie_portdrv_probe() to fail if the BIOS refuses to grant control of the PCIe capability structure, which means that no native PCIe port services can be enabled for the PCIe root complex the given port belongs to.

Make it possible to override this behavior using a new command line switch pcie_ports= that can be set to 'auto' (ask the BIOS, the default), 'native' (use the PCIe native services regardless of the BIOS response to the control request), or 'compat' (do not use the PCIe native services at all).

Accordingly, rework the existing PCIe port service drivers so that they don't request control of the services directly.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Quoted 30 lines · On Wed, 28 Jul 2010 23:23:56 +0200 "Rafael J. Wysocki"<[email protected]> wrote:

From: Rafael J. Wysocki<[email protected]>

PCIe port service drivers ask the BIOS, through _OSC, for control of the services they handle. Unfortunately, each of them individually asks for control of the PCIe capability structure and if that is granted, some BIOSes expect that the other PCIe port services will be configured and handled by the kernel as well. If that is not the case (eg. one of the PCIe port service drivers is not loaded), the BIOS may be confused and may cause the system as a whole to misbehave (eg. on one of such systems enabling the native PCIe PME service without loading the native PCIe hot-plug service driver causes a storm of ACPI notify requests to appear).

For this reason rework the PCIe port driver so that (1) it checks which native PCIe port services can be enabled, according to the BIOS, and (2) it requests control of all these services simultaneously. In particular, this causes pcie_portdrv_probe() to fail if the BIOS refuses to grant control of the PCIe capability structure, which means that no native PCIe port services can be enabled for the PCIe root complex the given port belongs to.

Make it possible to override this behavior using a new command line switch pcie_ports= that can be set to 'auto' (ask the BIOS, the default), 'native' (use the PCIe native services regardless of the BIOS response to the control request), or 'compat' (do not use the PCIe native services at all).

Accordingly, rework the existing PCIe port service drivers so that they don't request control of the services directly.

Quoted 2 lines

Signed-off-by Rafael J. Wysocki<[email protected]>

---

Kenji-san, are you ok with this version? I would like to get your ack (and ideally your tested-by) for this one since it affects functionality you need.

Quoted 33 lines · Jesse Barnes

Quoted 30 lines · On Wed, 28 Jul 2010 23:23:56 +0200 "Rafael J. Wysocki"<[email protected]> wrote:

From: Rafael J. Wysocki<[email protected]>

PCIe port service drivers ask the BIOS, through _OSC, for control of the services they handle. Unfortunately, each of them individually asks for control of the PCIe capability structure and if that is granted, some BIOSes expect that the other PCIe port services will be configured and handled by the kernel as well. If that is not the case (eg. one of the PCIe port service drivers is not loaded), the BIOS may be confused and may cause the system as a whole to misbehave (eg. on one of such systems enabling the native PCIe PME service without loading the native PCIe hot-plug service driver causes a storm of ACPI notify requests to appear).

For this reason rework the PCIe port driver so that (1) it checks which native PCIe port services can be enabled, according to the BIOS, and (2) it requests control of all these services simultaneously. In particular, this causes pcie_portdrv_probe() to fail if the BIOS refuses to grant control of the PCIe capability structure, which means that no native PCIe port services can be enabled for the PCIe root complex the given port belongs to.

Make it possible to override this behavior using a new command line switch pcie_ports= that can be set to 'auto' (ask the BIOS, the default), 'native' (use the PCIe native services regardless of the BIOS response to the control request), or 'compat' (do not use the PCIe native services at all).

Accordingly, rework the existing PCIe port service drivers so that they don't request control of the services directly.

Quoted 7 lines

Quoted 2 lines

Signed-off-by Rafael J. Wysocki<[email protected]>

---

Kenji-san, are you ok with this version? I would like to get your ack (and ideally your tested-by) for this one since it affects functionality you need.

Hi Jesse, Rafael,

I've just started reviewing the latest version of the patch. I have good impression about this version so far. Please give me a few days for deeper review. And I've booked the test machine for this patch in next week. I'll try to test the patch and send you the result as soon as I can.

Here are two comments I have so far.

Though it is not directly related to Rafael's patch, I found one problem in _OSC query handling while reviewing this patch. Currently, all the _OSC controls are queried at the same time in acpi_pci_osc_support() and the result is preserved for later acpi_pci_osc_control_set() call. But query result can vary depending on the combination of requested controls.

So I think query result must not be preserved. Since Rafael's patch uses query result, this problem should be fixed at the same time. I'll try to make a patch for this and update Rafael's patch if needed.

I think the following changes should be done in the separated patch. I guess this change is for the BIOS which enables hotplug interrupt at when _OSC is evaluated with native hot-plug control. Right?

Anyway, it should be separated patch with appropriate description.

Quoted 49 lines · Kenji Kaneshige

Quoted 33 lines · Jesse Barnes

Quoted 30 lines · On Wed, 28 Jul 2010 23:23:56 +0200 "Rafael J. Wysocki"<[email protected]> wrote:

From: Rafael J. Wysocki<[email protected]>

PCIe port service drivers ask the BIOS, through _OSC, for control of the services they handle. Unfortunately, each of them individually asks for control of the PCIe capability structure and if that is granted, some BIOSes expect that the other PCIe port services will be configured and handled by the kernel as well. If that is not the case (eg. one of the PCIe port service drivers is not loaded), the BIOS may be confused and may cause the system as a whole to misbehave (eg. on one of such systems enabling the native PCIe PME service without loading the native PCIe hot-plug service driver causes a storm of ACPI notify requests to appear).

For this reason rework the PCIe port driver so that (1) it checks which native PCIe port services can be enabled, according to the BIOS, and (2) it requests control of all these services simultaneously. In particular, this causes pcie_portdrv_probe() to fail if the BIOS refuses to grant control of the PCIe capability structure, which means that no native PCIe port services can be enabled for the PCIe root complex the given port belongs to.

Make it possible to override this behavior using a new command line switch pcie_ports= that can be set to 'auto' (ask the BIOS, the default), 'native' (use the PCIe native services regardless of the BIOS response to the control request), or 'compat' (do not use the PCIe native services at all).

Accordingly, rework the existing PCIe port service drivers so that they don't request control of the services directly.

Quoted 7 lines

Quoted 2 lines

Signed-off-by Rafael J. Wysocki<[email protected]>

---

Kenji-san, are you ok with this version? I would like to get your ack (and ideally your tested-by) for this one since it affects functionality you need.

Hi Jesse, Rafael,

I've just started reviewing the latest version of the patch. I have good impression about this version so far. Please give me a few days for deeper review. And I've booked the test machine for this patch in next week. I'll try to test the patch and send you the result as soon as I can.

Quoted 7 lines

Here are two comments I have so far.

Though it is not directly related to Rafael's patch, I found one problem in _OSC query handling while reviewing this patch. Currently, all the _OSC controls are queried at the same time in acpi_pci_osc_support() and the result is preserved for later acpi_pci_osc_control_set() call. But query result can vary depending on the combination of requested controls.

In principle it can, but acpi_pci_query_osc() asks for all aplicable control bits, in which case the formware can only clear those bits in the response if the corresponding features are unsupported. Doing otherwise would be in violation of Section 6.2.9.1. of ACPI spec 3.0b, the following in particular:

"If any bits in the Control Field are returned cleared (masked to zero) by the _OSC control method, the respective feature is designated unsupported by the platform and must not be enabled by the OS."

Quoted 3 lines

So I think query result must not be preserved. Since Rafael's patch uses query result, this problem should be fixed at the same time. I'll try to make a patch for this and update Rafael's patch if needed.

It would be sufficient to change acpi_pci_osc_control_get(), introduced by my patch, to call acpi_pci_query_osc() unconditionally, but as I said above, I don't think it's necessary.

Quoted 4 lines

I think the following changes should be done in the separated patch. I guess this change is for the BIOS which enables hotplug interrupt at when _OSC is evaluated with native hot-plug control. Right?

Yes, it is.

Quoted 1 line

Anyway, it should be separated patch with appropriate description.

But the $subject patch does analogous things for AER and PCIe PME. Do you think they also should be introduced by separate patches?

Quoted 69 lines · Rafael J. Wysocki

Quoted 51 lines · Kenji Kaneshige

Quoted 33 lines · Jesse Barnes

Quoted 30 lines · On Wed, 28 Jul 2010 23:23:56 +0200 "Rafael J. Wysocki"<[email protected]> wrote:

From: Rafael J. Wysocki<[email protected]>

PCIe port service drivers ask the BIOS, through _OSC, for control of the services they handle. Unfortunately, each of them individually asks for control of the PCIe capability structure and if that is granted, some BIOSes expect that the other PCIe port services will be configured and handled by the kernel as well. If that is not the case (eg. one of the PCIe port service drivers is not loaded), the BIOS may be confused and may cause the system as a whole to misbehave (eg. on one of such systems enabling the native PCIe PME service without loading the native PCIe hot-plug service driver causes a storm of ACPI notify requests to appear).

For this reason rework the PCIe port driver so that (1) it checks which native PCIe port services can be enabled, according to the BIOS, and (2) it requests control of all these services simultaneously. In particular, this causes pcie_portdrv_probe() to fail if the BIOS refuses to grant control of the PCIe capability structure, which means that no native PCIe port services can be enabled for the PCIe root complex the given port belongs to.

Make it possible to override this behavior using a new command line switch pcie_ports= that can be set to 'auto' (ask the BIOS, the default), 'native' (use the PCIe native services regardless of the BIOS response to the control request), or 'compat' (do not use the PCIe native services at all).

Accordingly, rework the existing PCIe port service drivers so that they don't request control of the services directly.

Quoted 7 lines

Quoted 2 lines

Signed-off-by Rafael J. Wysocki<[email protected]>

---

Kenji-san, are you ok with this version? I would like to get your ack (and ideally your tested-by) for this one since it affects functionality you need.

Hi Jesse, Rafael,

I've just started reviewing the latest version of the patch. I have good impression about this version so far. Please give me a few days for deeper
review. And I've booked the test machine for this patch in next week. I'll
try to test the patch and send you the result as soon as I can.

Quoted 8 lines

Here are two comments I have so far.

Though it is not directly related to Rafael's patch, I found one problem in _OSC query handling while reviewing this patch. Currently, all the _OSC controls are queried at the same time in acpi_pci_osc_support() and the result is preserved for later acpi_pci_osc_control_set() call. But query result can vary depending on the combination of requested controls.

In principle it can, but acpi_pci_query_osc() asks for all aplicable control
bits, in which case the formware can only clear those bits in the response if
the corresponding features are unsupported. Doing otherwise would be in violation of Section 6.2.9.1. of ACPI spec 3.0b, the following in particular:

Quoted 5 lines

"If any bits in the Control Field are returned cleared (masked to zero) by the _OSC control method,
the respective feature is designated unsupported by the platform and must not be enabled by the OS."

My concern is as follows.

For easy, assume as follows:
- all the _OSC controls are A, B, C
-B depends on C.
- When requesting A, B and C, all A, B and C are granted to OS.
- When requesting A, B, only A is granted to OS.

Current linux queries all A, B and C at the boot time and preserve the result (all A, B and C can be granted to OS). Here, if a service driver requests A and B using acpi_pci_osc_control_set(), acpi_pci_osc_control_set()
checks if both of those controls can be granted to OS. The expected result here is only A can be granted to OS, but current linux judges both of them can be granted because linux does this check by seeing preserved result. As a result, acpi_pci_osc_control_set() evaluates _OSC with query flag cleared to request A and B. Since the control B returned cleared, acpi_pci_osc_control() returns error to the service driver. As a result, control A is granted to OS even though there is no driver to handle the control A.

Quoted 8 lines

Quoted 3 lines

So I think query result must not be preserved. Since Rafael's patch uses query result, this problem should be fixed at the same time. I'll try to make a patch for this and update Rafael's patch if needed.

It would be sufficient to change acpi_pci_osc_control_get(), introduced by my patch, to call acpi_pci_query_osc() unconditionally, but as I said above,
I don't think it's necessary.

I made a patch for this and updated your patch slightly. I'll send them soon.

Quoted 6 lines

Quoted 5 lines

I think the following changes should be done in the separated patch. I guess this change is for the BIOS which enables hotplug interrupt at when
_OSC is evaluated with native hot-plug control. Right?

Quoted 6 lines

Yes, it is.

Quoted 1 line

Anyway, it should be separated patch with appropriate description.

But the $subject patch does analogous things for AER and PCIe PME. Do you think they also should be introduced by separate patches?

I think the following changes in your patch should be introduced by separate patches.

- optimizing the checks in acpi_pci_osc_control_set()
- Disabling hot-plug interrupt
- Removing module_exit()

Thanks,
Kenji Kaneshige

Make capabilitis buffer in acpi_pci_run_osc() instead of caller of this function. This makes the code a little cleaner. This has no functional changes.

Signed-off-by Kenji Kaneshige <[email protected]>

---
 drivers/acpi/pci_root.c |   34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

Index: linux-2.6.35-rc6/drivers/acpi/pci_root.c
===================================================================

drivers/acpi/pci_root.c +8 -7

@@ -206,9 +206,10 @@ static void acpi_pci_bridge_scan(struct 
 
 static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766";
 
-static acpi_status acpi_pci_run_osc(acpi_handle handle,
-				    const u32 *capbuf, u32 *retval)
+static acpi_status acpi_pci_run_osc(acpi_handle handle, u32 support,
+				    u32 control, bool query, u32 *retval)
 {
+	u32 capbuf[3];
 	struct acpi_osc_context context = {
 		.uuid_str = pci_osc_uuid_str,
 		.rev = 1,

@@ -217,6 +218,10 @@ static acpi_status acpi_pci_run_osc(acpi
 	};
 	acpi_status status;
 
+	capbuf[OSC_QUERY_TYPE] = (query == true) ? OSC_QUERY_ENABLE : 0;
+	capbuf[OSC_SUPPORT_TYPE] = support;
+	capbuf[OSC_CONTROL_TYPE] = control;
+
 	status = acpi_run_osc(handle, &context);
 	if (ACPI_SUCCESS(status)) {
 		*retval = *((u32 *)(context.ret.pointer + 8));

@@ -228,17 +233,16 @@ static acpi_status acpi_pci_run_osc(acpi
 static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, u32 flags)
 {
 	acpi_status status;
-	u32 support_set, result, capbuf[3];
+	u32 result;
 
 	/* do _OSC query for all possible controls */
-	support_set = root->osc_support_set | (flags & OSC_PCI_SUPPORT_MASKS);
-	capbuf[OSC_QUERY_TYPE] = OSC_QUERY_ENABLE;
-	capbuf[OSC_SUPPORT_TYPE] = support_set;
-	capbuf[OSC_CONTROL_TYPE] = OSC_PCI_CONTROL_MASKS;

drivers/acpi/pci_root.c +1 -2

 		root->osc_control_qry = result;
 		root->osc_queried = 1;
 	}

@@ -373,7 +377,7 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev);
 acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 flags)
 {
 	acpi_status status;
-	u32 control_req, result, capbuf[3];
+	u32 control_req, result;
 	acpi_handle tmp;
 	struct acpi_pci_root *root;
 

@@ -407,10 +411,10 @@ acpi_status acpi_pci_osc_control_set(acp
 		goto out;
 	}
 
-	capbuf[OSC_QUERY_TYPE] = 0;

Make capabilitis buffer in acpi_pci_run_osc() instead of caller of this function. This makes the code a little cleaner. This has no functional changes.

Signed-off-by Kenji Kaneshige <[email protected]>

---
 drivers/acpi/pci_root.c |   34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

Index: linux-2.6.35-rc6/drivers/acpi/pci_root.c
===================================================================

drivers/acpi/pci_root.c +8 -7

@@ -206,9 +206,10 @@ static void acpi_pci_bridge_scan(struct 
 
 static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766";
 
-static acpi_status acpi_pci_run_osc(acpi_handle handle,
-				    const u32 *capbuf, u32 *retval)
+static acpi_status acpi_pci_run_osc(acpi_handle handle, u32 support,
+				    u32 control, bool query, u32 *retval)
 {
+	u32 capbuf[3];
 	struct acpi_osc_context context = {
 		.uuid_str = pci_osc_uuid_str,
 		.rev = 1,

@@ -217,6 +218,10 @@ static acpi_status acpi_pci_run_osc(acpi
 	};
 	acpi_status status;
 
+	capbuf[OSC_QUERY_TYPE] = (query == true) ? OSC_QUERY_ENABLE : 0;
+	capbuf[OSC_SUPPORT_TYPE] = support;
+	capbuf[OSC_CONTROL_TYPE] = control;
+
 	status = acpi_run_osc(handle, &context);
 	if (ACPI_SUCCESS(status)) {
 		*retval = *((u32 *)(context.ret.pointer + 8));

@@ -228,17 +233,16 @@ static acpi_status acpi_pci_run_osc(acpi
 static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, u32 flags)
 {
 	acpi_status status;
-	u32 support_set, result, capbuf[3];
+	u32 result;
 
 	/* do _OSC query for all possible controls */
-	support_set = root->osc_support_set | (flags & OSC_PCI_SUPPORT_MASKS);
-	capbuf[OSC_QUERY_TYPE] = OSC_QUERY_ENABLE;
-	capbuf[OSC_SUPPORT_TYPE] = support_set;
-	capbuf[OSC_CONTROL_TYPE] = OSC_PCI_CONTROL_MASKS;

drivers/acpi/pci_root.c +1 -2

 		root->osc_control_qry = result;
 		root->osc_queried = 1;
 	}

@@ -373,7 +377,7 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev);
 acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 flags)
 {
 	acpi_status status;
-	u32 control_req, result, capbuf[3];
+	u32 control_req, result;
 	acpi_handle tmp;
 	struct acpi_pci_root *root;
 

@@ -407,10 +411,10 @@ acpi_status acpi_pci_osc_control_set(acp
 		goto out;
 	}
 
-	capbuf[OSC_QUERY_TYPE] = 0;

Currently, all the _OSC controls are queried at the same time in acpi_pci_osc_support() and the result is preserved for later acpi_pci_osc_control_set() call. But query result can vary depending on the combination of requested controls. Therefore, query result must not be preserved.

Signed-off-by Kenji Kaneshige <[email protected]>

---
 drivers/acpi/pci_root.c |   49 ++++++++++++++++++++++++++----------------------
 include/acpi/acpi_bus.h |    3 --
 2 files changed, 27 insertions(+), 25 deletions(-)

Index: linux-2.6.35-rc6/drivers/acpi/pci_root.c
===================================================================

drivers/acpi/pci_root.c +27 -22

@@ -230,35 +230,42 @@ static acpi_status acpi_pci_run_osc(acpi
 	return status;
 }
 
-static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, u32 flags)
+static acpi_status acpi_pci_osc_control_query(struct acpi_pci_root *root,
+					      u32 flags, u32 *result)
 {
-	acpi_status status;
-	u32 result;
-
-	/* do _OSC query for all possible controls */
-	flags &= OSC_PCI_SUPPORT_MASKS;
-	status = acpi_pci_run_osc(root->device->handle,
-				  root->osc_support_set | flags,
-				  OSC_PCI_CONTROL_MASKS,
-				  true, &result);
-	if (ACPI_SUCCESS(status)) {
-		root->osc_support_set |= flags;
-		root->osc_control_qry = result;
-		root->osc_queried = 1;
+	/* No need to run _OSC if requested control bits are already granted */
+	flags &= OSC_PCI_CONTROL_MASKS;
+	if ((root->osc_control_set & flags) == flags) {
+		*result = root->osc_control_set;
+		return AE_OK;
 	}
-	return status;
+	return acpi_pci_run_osc(root->device->handle,
+				root->osc_support_set,
+				root->osc_control_set | flags,
+				true, result);
 }
 
 static acpi_status acpi_pci_osc_support(struct acpi_pci_root *root, u32 flags)
 {
 	acpi_status status;
 	acpi_handle tmp;
+	u32 result;
 
 	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);
+	/* No need to run _OSC if requested support bits are already set */
+	flags &= OSC_PCI_SUPPORT_MASKS;
+	if ((root->osc_support_set & flags) == flags)
+		goto out;
+	status = acpi_pci_run_osc(root->device->handle,
+				  root->osc_support_set | flags,
+				  root->osc_control_set,
+				  true, &result);
+	if (ACPI_SUCCESS(status))
+		root->osc_support_set |= flags;
+out:
 	mutex_unlock(&osc_lock);
 	return status;
 }

@@ -399,12 +406,10 @@ acpi_status acpi_pci_osc_control_set(acp
 		goto out;
 
 	/* Need to query controls first before requesting them */
-	if (!root->osc_queried) {
-		status = acpi_pci_query_osc(root, root->osc_support_set);
-		if (ACPI_FAILURE(status))
-			goto out;
-	}
-	if ((root->osc_control_qry & control_req) != control_req) {
+	status = acpi_pci_osc_control_query(root, control_req, &result);
+	if (ACPI_FAILURE(status))
+		goto out;
+	if ((result & control_req) != control_req) {
 		printk(KERN_DEBUG
 		       "Firmware did not grant requested _OSC control\n");
 		status = AE_SUPPORT;

Index: linux-2.6.35-rc6/include/acpi/acpi_bus.h
===================================================================

include/acpi/acpi_bus.h +0 -3

@@ -377,9 +377,6 @@ struct acpi_pci_root {
 
 	u32 osc_support_set;	/* _OSC state of support bits */
 	u32 osc_control_set;	/* _OSC state of control bits */
-	u32 osc_control_qry;	/* the latest _OSC query result */
-
-	u32 osc_queried:1;	/* has _OSC control been queried? */
 };
 
 /* helper */

Currently, all the _OSC controls are queried at the same time in acpi_pci_osc_support() and the result is preserved for later acpi_pci_osc_control_set() call. But query result can vary depending on the combination of requested controls. Therefore, query result must not be preserved.

Signed-off-by Kenji Kaneshige <[email protected]>

---
 drivers/acpi/pci_root.c |   49 ++++++++++++++++++++++++++----------------------
 include/acpi/acpi_bus.h |    3 --
 2 files changed, 27 insertions(+), 25 deletions(-)

Index: linux-2.6.35-rc6/drivers/acpi/pci_root.c
===================================================================

drivers/acpi/pci_root.c +27 -22

@@ -230,35 +230,42 @@ static acpi_status acpi_pci_run_osc(acpi
 	return status;
 }
 
-static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, u32 flags)
+static acpi_status acpi_pci_osc_control_query(struct acpi_pci_root *root,
+					      u32 flags, u32 *result)
 {
-	acpi_status status;
-	u32 result;
-
-	/* do _OSC query for all possible controls */
-	flags &= OSC_PCI_SUPPORT_MASKS;
-	status = acpi_pci_run_osc(root->device->handle,
-				  root->osc_support_set | flags,
-				  OSC_PCI_CONTROL_MASKS,
-				  true, &result);
-	if (ACPI_SUCCESS(status)) {
-		root->osc_support_set |= flags;
-		root->osc_control_qry = result;
-		root->osc_queried = 1;
+	/* No need to run _OSC if requested control bits are already granted */
+	flags &= OSC_PCI_CONTROL_MASKS;
+	if ((root->osc_control_set & flags) == flags) {
+		*result = root->osc_control_set;
+		return AE_OK;
 	}
-	return status;
+	return acpi_pci_run_osc(root->device->handle,
+				root->osc_support_set,
+				root->osc_control_set | flags,
+				true, result);
 }
 
 static acpi_status acpi_pci_osc_support(struct acpi_pci_root *root, u32 flags)
 {
 	acpi_status status;
 	acpi_handle tmp;
+	u32 result;
 
 	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);
+	/* No need to run _OSC if requested support bits are already set */
+	flags &= OSC_PCI_SUPPORT_MASKS;
+	if ((root->osc_support_set & flags) == flags)
+		goto out;
+	status = acpi_pci_run_osc(root->device->handle,
+				  root->osc_support_set | flags,
+				  root->osc_control_set,
+				  true, &result);
+	if (ACPI_SUCCESS(status))
+		root->osc_support_set |= flags;
+out:
 	mutex_unlock(&osc_lock);
 	return status;
 }

@@ -399,12 +406,10 @@ acpi_status acpi_pci_osc_control_set(acp
 		goto out;
 
 	/* Need to query controls first before requesting them */
-	if (!root->osc_queried) {
-		status = acpi_pci_query_osc(root, root->osc_support_set);
-		if (ACPI_FAILURE(status))
-			goto out;
-	}
-	if ((root->osc_control_qry & control_req) != control_req) {
+	status = acpi_pci_osc_control_query(root, control_req, &result);
+	if (ACPI_FAILURE(status))
+		goto out;
+	if ((result & control_req) != control_req) {
 		printk(KERN_DEBUG
 		       "Firmware did not grant requested _OSC control\n");
 		status = AE_SUPPORT;

Index: linux-2.6.35-rc6/include/acpi/acpi_bus.h
===================================================================

include/acpi/acpi_bus.h +0 -3

@@ -377,9 +377,6 @@ struct acpi_pci_root {
 
 	u32 osc_support_set;	/* _OSC state of support bits */
 	u32 osc_control_set;	/* _OSC state of control bits */
-	u32 osc_control_qry;	/* the latest _OSC query result */
-
-	u32 osc_queried:1;	/* has _OSC control been queried? */
 };
 
 /* helper */

Check for ACPI _OSC object is heavier than other checks in acpi_pci_osc_control_set(). So move it after the other checks.

Signed-off-by Rafael J. Wysocki <[email protected]>

Signed-off-by Kenji Kaneshige <[email protected]>

---
 drivers/acpi/pci_root.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux-2.6.35-rc6/drivers/acpi/pci_root.c
===================================================================

drivers/acpi/pci_root.c +4 -4

@@ -388,10 +388,6 @@ acpi_status acpi_pci_osc_control_set(acp
 	acpi_handle tmp;
 	struct acpi_pci_root *root;
 
-	status = acpi_get_handle(handle, "_OSC", &tmp);
-	if (ACPI_FAILURE(status))
-		return status;
-
 	control_req = (flags & OSC_PCI_CONTROL_MASKS);
 	if (!control_req)
 		return AE_TYPE;

@@ -400,6 +396,10 @@ acpi_status acpi_pci_osc_control_set(acp
 	if (!root)
 		return AE_NOT_EXIST;
 
+	status = acpi_get_handle(handle, "_OSC", &tmp);
+	if (ACPI_FAILURE(status))
+		return status;
+
 	mutex_lock(&osc_lock);
 	/* No need to evaluate _OSC if the control was already granted. */
 	if ((root->osc_control_set & control_req) == control_req)

Check for ACPI _OSC object is heavier than other checks in acpi_pci_osc_control_set(). So move it after the other checks.

Signed-off-by Rafael J. Wysocki <[email protected]>

Signed-off-by Kenji Kaneshige <[email protected]>

---
 drivers/acpi/pci_root.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux-2.6.35-rc6/drivers/acpi/pci_root.c
===================================================================

drivers/acpi/pci_root.c +4 -4

@@ -388,10 +388,6 @@ acpi_status acpi_pci_osc_control_set(acp
 	acpi_handle tmp;
 	struct acpi_pci_root *root;
 
-	status = acpi_get_handle(handle, "_OSC", &tmp);
-	if (ACPI_FAILURE(status))
-		return status;
-
 	control_req = (flags & OSC_PCI_CONTROL_MASKS);
 	if (!control_req)
 		return AE_TYPE;

@@ -400,6 +396,10 @@ acpi_status acpi_pci_osc_control_set(acp
 	if (!root)
 		return AE_NOT_EXIST;
 
+	status = acpi_get_handle(handle, "_OSC", &tmp);
+	if (ACPI_FAILURE(status))
+		return status;
+
 	mutex_lock(&osc_lock);
 	/* No need to evaluate _OSC if the control was already granted. */
 	if ((root->osc_control_set & control_req) == control_req)

drivers/pci/pcie/portdrv_acpi.c new file +25 -0

+/**
+ * pcie_port_acpi_setup - Request the BIOS to release control of PCIe services.
+ * @port: PCIe Port service for a root port or event collector.
+ * @srv_mask: Bit mask of services that can be enabled for @port.
+ *
+ * Invoked when @port is identified as a PCIe port device.  To avoid conflicts
+ * with the BIOS PCIe port native services support requires the BIOS to yield
+ * control of these services to the kernel.  The mask of services that the BIOS
+ * allows to be enabled for @port is written to @srv_mask.
+ *
+ * NOTE: It turns out that we cannot do that for individual port services
+ * separately, because that would make some systems work incorrectly.
+ */
+int pcie_port_acpi_setup(struct pci_dev *port, int *srv_mask)
+{
+	acpi_status status;
+	acpi_handle handle;
+	u32 flags, request;
+
+	if (acpi_pci_disabled)
+		return 0;
+
+	handle = acpi_find_root_bridge_handle(port);
+	if (!handle)
+		return -EINVAL;

drivers/pci/pcie/portdrv_acpi.c new file +24 -0

+
+	status = acpi_pci_osc_control_query(handle, request, &flags);
+	if (ACPI_FAILURE(status)) {
+		dev_dbg(&port->dev, "ACPI _OSC query failed (code %d)\n",
+			status);
+		return -ENODEV;
+	}
+
+	if (!(flags & OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL)) {
+		dev_dbg(&port->dev, "BIOS refuses to grant control of PCIe "
+			"Capability Structure\n");
+		return -EACCES;
+	}
+
+	request &= flags;
+	*srv_mask = PCIE_PORT_SERVICE_VC;
+	if (request & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)
+		*srv_mask |= PCIE_PORT_SERVICE_HP;
+	if (request & OSC_PCI_EXPRESS_PME_CONTROL)
+		*srv_mask |= PCIE_PORT_SERVICE_PME;
+	if (request & OSC_PCI_EXPRESS_AER_CONTROL)
+		*srv_mask |= PCIE_PORT_SERVICE_AER;
+
+	status = acpi_pci_osc_control_set(handle, request);

drivers/pci/pcie/portdrv_acpi.c new file +25 -0

+/**
+ * pcie_port_acpi_setup - Request the BIOS to release control of PCIe services.
+ * @port: PCIe Port service for a root port or event collector.
+ * @srv_mask: Bit mask of services that can be enabled for @port.
+ *
+ * Invoked when @port is identified as a PCIe port device.  To avoid conflicts
+ * with the BIOS PCIe port native services support requires the BIOS to yield
+ * control of these services to the kernel.  The mask of services that the BIOS
+ * allows to be enabled for @port is written to @srv_mask.
+ *
+ * NOTE: It turns out that we cannot do that for individual port services
+ * separately, because that would make some systems work incorrectly.
+ */
+int pcie_port_acpi_setup(struct pci_dev *port, int *srv_mask)
+{
+	acpi_status status;
+	acpi_handle handle;
+	u32 flags, request;
+
+	if (acpi_pci_disabled)
+		return 0;
+
+	handle = acpi_find_root_bridge_handle(port);
+	if (!handle)
+		return -EINVAL;

drivers/pci/pcie/portdrv_acpi.c new file +24 -0

+
+	status = acpi_pci_osc_control_query(handle, request, &flags);
+	if (ACPI_FAILURE(status)) {
+		dev_dbg(&port->dev, "ACPI _OSC query failed (code %d)\n",
+			status);
+		return -ENODEV;
+	}
+
+	if (!(flags & OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL)) {
+		dev_dbg(&port->dev, "BIOS refuses to grant control of PCIe "
+			"Capability Structure\n");
+		return -EACCES;
+	}
+
+	request &= flags;
+	*srv_mask = PCIE_PORT_SERVICE_VC;
+	if (request & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)
+		*srv_mask |= PCIE_PORT_SERVICE_HP;
+	if (request & OSC_PCI_EXPRESS_PME_CONTROL)
+		*srv_mask |= PCIE_PORT_SERVICE_PME;
+	if (request & OSC_PCI_EXPRESS_AER_CONTROL)
+		*srv_mask |= PCIE_PORT_SERVICE_AER;
+
+	status = acpi_pci_osc_control_set(handle, request);

PCIe port service drivers ask the BIOS, through _OSC, for control of the services they handle. Unfortunately, each of them individually asks for control of the PCIe capability structure and if that is granted, some BIOSes expect that the other PCIe port services will be configured and handled by the kernel as well. If that is not the case (eg. one of the PCIe port service drivers is not loaded), the BIOS may be confused and may cause the system as a whole to misbehave (eg. on one of such systems enabling the native PCIe PME service without loading the native PCIe hot-plug service driver causes a storm of ACPI notify requests to appear).

For this reason rework the PCIe port driver so that (1) it checks which native PCIe port services can be enabled, according to the BIOS, and (2) it requests control of all these services simultaneously. In particular, this causes pcie_portdrv_probe() to fail if the BIOS refuses to grant control of the PCIe capability structure, which means that no native PCIe port services can be enabled for the PCIe root complex the given port belongs to.

Make it possible to override this behavior using a new command line switch pcie_ports= that can be set to 'auto' (ask the BIOS, the default), 'native' (use the PCIe native services regardless of the BIOS response to the control request), or 'compat' (do not use the PCIe native services at all).

Accordingly, rework the existing PCIe port service drivers so that they don't request control of the services directly.

Signed-off-by Rafael J. Wysocki <[email protected]>

Signed-off-by Kenji Kaneshige <[email protected]>

Tested-by Kenji Kaneshige <[email protected]>

PCIe port service drivers ask the BIOS, through _OSC, for control of the services they handle. Unfortunately, each of them individually asks for control of the PCIe capability structure and if that is granted, some BIOSes expect that the other PCIe port services will be configured and handled by the kernel as well. If that is not the case (eg. one of the PCIe port service drivers is not loaded), the BIOS may be confused and may cause the system as a whole to misbehave (eg. on one of such systems enabling the native PCIe PME service without loading the native PCIe hot-plug service driver causes a storm of ACPI notify requests to appear).

For this reason rework the PCIe port driver so that (1) it checks which native PCIe port services can be enabled, according to the BIOS, and (2) it requests control of all these services simultaneously. In particular, this causes pcie_portdrv_probe() to fail if the BIOS refuses to grant control of the PCIe capability structure, which means that no native PCIe port services can be enabled for the PCIe root complex the given port belongs to.

Make it possible to override this behavior using a new command line switch pcie_ports= that can be set to 'auto' (ask the BIOS, the default), 'native' (use the PCIe native services regardless of the BIOS response to the control request), or 'compat' (do not use the PCIe native services at all).

Accordingly, rework the existing PCIe port service drivers so that they don't request control of the services directly.

Signed-off-by Rafael J. Wysocki <[email protected]>

Signed-off-by Kenji Kaneshige <[email protected]>

Tested-by Kenji Kaneshige <[email protected]>

Quoted 69 lines · Rafael J. Wysocki

Quoted 51 lines · Kenji Kaneshige

Quoted 33 lines · Jesse Barnes

Quoted 30 lines · On Wed, 28 Jul 2010 23:23:56 +0200 "Rafael J. Wysocki"<[email protected]> wrote:

From: Rafael J. Wysocki<[email protected]>

PCIe port service drivers ask the BIOS, through _OSC, for control of the services they handle. Unfortunately, each of them individually asks for control of the PCIe capability structure and if that is granted, some BIOSes expect that the other PCIe port services will be configured and handled by the kernel as well. If that is not the case (eg. one of the PCIe port service drivers is not loaded), the BIOS may be confused and may cause the system as a whole to misbehave (eg. on one of such systems enabling the native PCIe PME service without loading the native PCIe hot-plug service driver causes a storm of ACPI notify requests to appear).

For this reason rework the PCIe port driver so that (1) it checks which native PCIe port services can be enabled, according to the BIOS, and (2) it requests control of all these services simultaneously. In particular, this causes pcie_portdrv_probe() to fail if the BIOS refuses to grant control of the PCIe capability structure, which means that no native PCIe port services can be enabled for the PCIe root complex the given port belongs to.

Make it possible to override this behavior using a new command line switch pcie_ports= that can be set to 'auto' (ask the BIOS, the default), 'native' (use the PCIe native services regardless of the BIOS response to the control request), or 'compat' (do not use the PCIe native services at all).

Accordingly, rework the existing PCIe port service drivers so that they don't request control of the services directly.

Quoted 7 lines

Quoted 2 lines

Signed-off-by Rafael J. Wysocki<[email protected]>

---

Kenji-san, are you ok with this version? I would like to get your ack (and ideally your tested-by) for this one since it affects functionality you need.

Hi Jesse, Rafael,

I've just started reviewing the latest version of the patch. I have good impression about this version so far. Please give me a few days for deeper
review. And I've booked the test machine for this patch in next week. I'll
try to test the patch and send you the result as soon as I can.

Quoted 8 lines

Here are two comments I have so far.

Though it is not directly related to Rafael's patch, I found one problem in _OSC query handling while reviewing this patch. Currently, all the _OSC controls are queried at the same time in acpi_pci_osc_support() and the result is preserved for later acpi_pci_osc_control_set() call. But query result can vary depending on the combination of requested controls.

In principle it can, but acpi_pci_query_osc() asks for all aplicable control
bits, in which case the formware can only clear those bits in the response if
the corresponding features are unsupported. Doing otherwise would be in violation of Section 6.2.9.1. of ACPI spec 3.0b, the following in particular:

Quoted 5 lines

"If any bits in the Control Field are returned cleared (masked to zero) by the _OSC control method,
the respective feature is designated unsupported by the platform and must not be enabled by the OS."

My concern is as follows.

For easy, assume as follows:
- all the _OSC controls are A, B, C
-B depends on C.
- When requesting A, B and C, all A, B and C are granted to OS.
- When requesting A, B, only A is granted to OS.

Current linux queries all A, B and C at the boot time and preserve the result (all A, B and C can be granted to OS). Here, if a service driver requests A and B using acpi_pci_osc_control_set(), acpi_pci_osc_control_set()
checks if both of those controls can be granted to OS. The expected result here is only A can be granted to OS, but current linux judges both of them can be granted because linux does this check by seeing preserved result. As a result, acpi_pci_osc_control_set() evaluates _OSC with query flag cleared to request A and B. Since the control B returned cleared, acpi_pci_osc_control() returns error to the service driver. As a result, control A is granted to OS even though there is no driver to handle the control A.

Quoted 8 lines

Quoted 3 lines

So I think query result must not be preserved. Since Rafael's patch uses query result, this problem should be fixed at the same time. I'll try to make a patch for this and update Rafael's patch if needed.

It would be sufficient to change acpi_pci_osc_control_get(), introduced by my patch, to call acpi_pci_query_osc() unconditionally, but as I said above,
I don't think it's necessary.

I made a patch for this and updated your patch slightly. I'll send them soon.

Quoted 6 lines

Quoted 5 lines

I think the following changes should be done in the separated patch. I guess this change is for the BIOS which enables hotplug interrupt at when
_OSC is evaluated with native hot-plug control. Right?

Quoted 6 lines

Yes, it is.

Quoted 1 line

Anyway, it should be separated patch with appropriate description.

But the $subject patch does analogous things for AER and PCIe PME. Do you think they also should be introduced by separate patches?

I think the following changes in your patch should be introduced by separate patches.

- optimizing the checks in acpi_pci_osc_control_set()
- Disabling hot-plug interrupt
- Removing module_exit()

Thanks,
Kenji Kaneshige

Quoted 58 lines · Rafael J. Wysocki

Quoted 49 lines · Kenji Kaneshige

Quoted 33 lines · Jesse Barnes

Quoted 30 lines · On Wed, 28 Jul 2010 23:23:56 +0200 "Rafael J. Wysocki"<[email protected]> wrote:

From: Rafael J. Wysocki<[email protected]>

PCIe port service drivers ask the BIOS, through _OSC, for control of the services they handle. Unfortunately, each of them individually asks for control of the PCIe capability structure and if that is granted, some BIOSes expect that the other PCIe port services will be configured and handled by the kernel as well. If that is not the case (eg. one of the PCIe port service drivers is not loaded), the BIOS may be confused and may cause the system as a whole to misbehave (eg. on one of such systems enabling the native PCIe PME service without loading the native PCIe hot-plug service driver causes a storm of ACPI notify requests to appear).

For this reason rework the PCIe port driver so that (1) it checks which native PCIe port services can be enabled, according to the BIOS, and (2) it requests control of all these services simultaneously. In particular, this causes pcie_portdrv_probe() to fail if the BIOS refuses to grant control of the PCIe capability structure, which means that no native PCIe port services can be enabled for the PCIe root complex the given port belongs to.

Make it possible to override this behavior using a new command line switch pcie_ports= that can be set to 'auto' (ask the BIOS, the default), 'native' (use the PCIe native services regardless of the BIOS response to the control request), or 'compat' (do not use the PCIe native services at all).

Accordingly, rework the existing PCIe port service drivers so that they don't request control of the services directly.

Quoted 7 lines

Quoted 2 lines

Signed-off-by Rafael J. Wysocki<[email protected]>

---

Kenji-san, are you ok with this version? I would like to get your ack (and ideally your tested-by) for this one since it affects functionality you need.

Hi Jesse, Rafael,

I've just started reviewing the latest version of the patch. I have good impression about this version so far. Please give me a few days for deeper review. And I've booked the test machine for this patch in next week. I'll try to test the patch and send you the result as soon as I can.

Quoted 7 lines

Here are two comments I have so far.

Though it is not directly related to Rafael's patch, I found one problem in _OSC query handling while reviewing this patch. Currently, all the _OSC controls are queried at the same time in acpi_pci_osc_support() and the result is preserved for later acpi_pci_osc_control_set() call. But query result can vary depending on the combination of requested controls.

Quoted 8 lines

In principle it can, but acpi_pci_query_osc() asks for all aplicable control bits, in which case the formware can only clear those bits in the response if the corresponding features are unsupported. Doing otherwise would be in violation of Section 6.2.9.1. of ACPI spec 3.0b, the following in particular:

"If any bits in the Control Field are returned cleared (masked to zero) by the _OSC control method, the respective feature is designated unsupported by the platform and must not be enabled by the OS."

My concern is as follows.

For easy, assume as follows:
- all the _OSC controls are A, B, C
-B depends on C.
- When requesting A, B and C, all A, B and C are granted to OS.
- When requesting A, B, only A is granted to OS.

Current linux queries all A, B and C at the boot time and preserve the result (all A, B and C can be granted to OS). Here, if a service driver requests A and B using acpi_pci_osc_control_set(),

acpi_pci_osc_control_set() checks if both of those controls can be granted to OS. The expected result here is only A can be granted to OS, but current linux judges both of them can be granted because linux does this check by seeing preserved result.
As a result, acpi_pci_osc_control_set() evaluates _OSC with query flag cleared to request A and B. Since the control B returned cleared, acpi_pci_osc_control() returns error to the service driver. As a result, control A is granted to OS even though there is no driver to handle the control A.

Quoted 7 lines

Quoted 3 lines

So I think query result must not be preserved. Since Rafael's patch uses query result, this problem should be fixed at the same time. I'll try to make a patch for this and update Rafael's patch if needed.

It would be sufficient to change acpi_pci_osc_control_get(), introduced by my patch, to call acpi_pci_query_osc() unconditionally, but as I said above, I don't think it's necessary.

I made a patch for this and updated your patch slightly. I'll send them soon.

Quoted 9 lines

Quoted 4 lines

I think the following changes should be done in the separated patch. I guess this change is for the BIOS which enables hotplug interrupt at when _OSC is evaluated with native hot-plug control. Right?

Yes, it is.

Quoted 1 line

Anyway, it should be separated patch with appropriate description.

Quoted 2 lines

But the $subject patch does analogous things for AER and PCIe PME. Do you think they also should be introduced by separate patches?

I think the following changes in your patch should be introduced by separate patches.

- optimizing the checks in acpi_pci_osc_control_set()

- Disabling hot-plug interrupt

- Removing module_exit()

Quoted 58 lines · Rafael J. Wysocki

Quoted 49 lines · Kenji Kaneshige

Quoted 33 lines · Jesse Barnes

Quoted 30 lines · On Wed, 28 Jul 2010 23:23:56 +0200 "Rafael J. Wysocki"<[email protected]> wrote:

From: Rafael J. Wysocki<[email protected]>

PCIe port service drivers ask the BIOS, through _OSC, for control of the services they handle. Unfortunately, each of them individually asks for control of the PCIe capability structure and if that is granted, some BIOSes expect that the other PCIe port services will be configured and handled by the kernel as well. If that is not the case (eg. one of the PCIe port service drivers is not loaded), the BIOS may be confused and may cause the system as a whole to misbehave (eg. on one of such systems enabling the native PCIe PME service without loading the native PCIe hot-plug service driver causes a storm of ACPI notify requests to appear).

For this reason rework the PCIe port driver so that (1) it checks which native PCIe port services can be enabled, according to the BIOS, and (2) it requests control of all these services simultaneously. In particular, this causes pcie_portdrv_probe() to fail if the BIOS refuses to grant control of the PCIe capability structure, which means that no native PCIe port services can be enabled for the PCIe root complex the given port belongs to.

Make it possible to override this behavior using a new command line switch pcie_ports= that can be set to 'auto' (ask the BIOS, the default), 'native' (use the PCIe native services regardless of the BIOS response to the control request), or 'compat' (do not use the PCIe native services at all).

Accordingly, rework the existing PCIe port service drivers so that they don't request control of the services directly.

Quoted 7 lines

Quoted 2 lines

Signed-off-by Rafael J. Wysocki<[email protected]>

---

Kenji-san, are you ok with this version? I would like to get your ack (and ideally your tested-by) for this one since it affects functionality you need.

Hi Jesse, Rafael,

I've just started reviewing the latest version of the patch. I have good impression about this version so far. Please give me a few days for deeper review. And I've booked the test machine for this patch in next week. I'll try to test the patch and send you the result as soon as I can.

Quoted 7 lines

Here are two comments I have so far.

Though it is not directly related to Rafael's patch, I found one problem in _OSC query handling while reviewing this patch. Currently, all the _OSC controls are queried at the same time in acpi_pci_osc_support() and the result is preserved for later acpi_pci_osc_control_set() call. But query result can vary depending on the combination of requested controls.

Quoted 8 lines

In principle it can, but acpi_pci_query_osc() asks for all aplicable control bits, in which case the formware can only clear those bits in the response if the corresponding features are unsupported. Doing otherwise would be in violation of Section 6.2.9.1. of ACPI spec 3.0b, the following in particular:

"If any bits in the Control Field are returned cleared (masked to zero) by the _OSC control method, the respective feature is designated unsupported by the platform and must not be enabled by the OS."

My concern is as follows.

For easy, assume as follows:
- all the _OSC controls are A, B, C
-B depends on C.
- When requesting A, B and C, all A, B and C are granted to OS.
- When requesting A, B, only A is granted to OS.

Current linux queries all A, B and C at the boot time and preserve the result (all A, B and C can be granted to OS). Here, if a service driver requests A and B using acpi_pci_osc_control_set(),

acpi_pci_osc_control_set() checks if both of those controls can be granted to OS. The expected result here is only A can be granted to OS, but current linux judges both of them can be granted because linux does this check by seeing preserved result.
As a result, acpi_pci_osc_control_set() evaluates _OSC with query flag cleared to request A and B. Since the control B returned cleared, acpi_pci_osc_control() returns error to the service driver. As a result, control A is granted to OS even though there is no driver to handle the control A.

Quoted 7 lines

Quoted 3 lines

So I think query result must not be preserved. Since Rafael's patch uses query result, this problem should be fixed at the same time. I'll try to make a patch for this and update Rafael's patch if needed.

It would be sufficient to change acpi_pci_osc_control_get(), introduced by my patch, to call acpi_pci_query_osc() unconditionally, but as I said above, I don't think it's necessary.

I made a patch for this and updated your patch slightly. I'll send them soon.

Quoted 9 lines

Quoted 4 lines

I think the following changes should be done in the separated patch. I guess this change is for the BIOS which enables hotplug interrupt at when _OSC is evaluated with native hot-plug control. Right?

Yes, it is.

Quoted 1 line

Anyway, it should be separated patch with appropriate description.

Quoted 2 lines

But the $subject patch does analogous things for AER and PCIe PME. Do you think they also should be introduced by separate patches?

I think the following changes in your patch should be introduced by separate patches.

- optimizing the checks in acpi_pci_osc_control_set()

- Disabling hot-plug interrupt

- Removing module_exit()

Quoted 49 lines · Kenji Kaneshige

Quoted 33 lines · Jesse Barnes

Quoted 30 lines · On Wed, 28 Jul 2010 23:23:56 +0200 "Rafael J. Wysocki"<[email protected]> wrote:

From: Rafael J. Wysocki<[email protected]>

PCIe port service drivers ask the BIOS, through _OSC, for control of the services they handle. Unfortunately, each of them individually asks for control of the PCIe capability structure and if that is granted, some BIOSes expect that the other PCIe port services will be configured and handled by the kernel as well. If that is not the case (eg. one of the PCIe port service drivers is not loaded), the BIOS may be confused and may cause the system as a whole to misbehave (eg. on one of such systems enabling the native PCIe PME service without loading the native PCIe hot-plug service driver causes a storm of ACPI notify requests to appear).

For this reason rework the PCIe port driver so that (1) it checks which native PCIe port services can be enabled, according to the BIOS, and (2) it requests control of all these services simultaneously. In particular, this causes pcie_portdrv_probe() to fail if the BIOS refuses to grant control of the PCIe capability structure, which means that no native PCIe port services can be enabled for the PCIe root complex the given port belongs to.

Make it possible to override this behavior using a new command line switch pcie_ports= that can be set to 'auto' (ask the BIOS, the default), 'native' (use the PCIe native services regardless of the BIOS response to the control request), or 'compat' (do not use the PCIe native services at all).

Accordingly, rework the existing PCIe port service drivers so that they don't request control of the services directly.

Quoted 7 lines

Quoted 2 lines

Signed-off-by Rafael J. Wysocki<[email protected]>

---

Kenji-san, are you ok with this version? I would like to get your ack (and ideally your tested-by) for this one since it affects functionality you need.

Hi Jesse, Rafael,

I've just started reviewing the latest version of the patch. I have good impression about this version so far. Please give me a few days for deeper review. And I've booked the test machine for this patch in next week. I'll try to test the patch and send you the result as soon as I can.

Quoted 7 lines

Here are two comments I have so far.

Though it is not directly related to Rafael's patch, I found one problem in _OSC query handling while reviewing this patch. Currently, all the _OSC controls are queried at the same time in acpi_pci_osc_support() and the result is preserved for later acpi_pci_osc_control_set() call. But query result can vary depending on the combination of requested controls.

In principle it can, but acpi_pci_query_osc() asks for all aplicable control bits, in which case the formware can only clear those bits in the response if the corresponding features are unsupported. Doing otherwise would be in violation of Section 6.2.9.1. of ACPI spec 3.0b, the following in particular:

"If any bits in the Control Field are returned cleared (masked to zero) by the _OSC control method, the respective feature is designated unsupported by the platform and must not be enabled by the OS."

Quoted 3 lines

So I think query result must not be preserved. Since Rafael's patch uses query result, this problem should be fixed at the same time. I'll try to make a patch for this and update Rafael's patch if needed.

It would be sufficient to change acpi_pci_osc_control_get(), introduced by my patch, to call acpi_pci_query_osc() unconditionally, but as I said above, I don't think it's necessary.

Quoted 4 lines

I think the following changes should be done in the separated patch. I guess this change is for the BIOS which enables hotplug interrupt at when _OSC is evaluated with native hot-plug control. Right?

Yes, it is.

Quoted 1 line

Anyway, it should be separated patch with appropriate description.

But the $subject patch does analogous things for AER and PCIe PME. Do you think they also should be introduced by separate patches?

Quoted 33 lines · Jesse Barnes

Quoted 30 lines · On Wed, 28 Jul 2010 23:23:56 +0200 "Rafael J. Wysocki"<[email protected]> wrote:

From: Rafael J. Wysocki<[email protected]>

PCIe port service drivers ask the BIOS, through _OSC, for control of the services they handle. Unfortunately, each of them individually asks for control of the PCIe capability structure and if that is granted, some BIOSes expect that the other PCIe port services will be configured and handled by the kernel as well. If that is not the case (eg. one of the PCIe port service drivers is not loaded), the BIOS may be confused and may cause the system as a whole to misbehave (eg. on one of such systems enabling the native PCIe PME service without loading the native PCIe hot-plug service driver causes a storm of ACPI notify requests to appear).

For this reason rework the PCIe port driver so that (1) it checks which native PCIe port services can be enabled, according to the BIOS, and (2) it requests control of all these services simultaneously. In particular, this causes pcie_portdrv_probe() to fail if the BIOS refuses to grant control of the PCIe capability structure, which means that no native PCIe port services can be enabled for the PCIe root complex the given port belongs to.

Make it possible to override this behavior using a new command line switch pcie_ports= that can be set to 'auto' (ask the BIOS, the default), 'native' (use the PCIe native services regardless of the BIOS response to the control request), or 'compat' (do not use the PCIe native services at all).

Accordingly, rework the existing PCIe port service drivers so that they don't request control of the services directly.

Quoted 7 lines

Quoted 2 lines

Signed-off-by Rafael J. Wysocki<[email protected]>

---

Kenji-san, are you ok with this version? I would like to get your ack (and ideally your tested-by) for this one since it affects functionality you need.

Hi Jesse, Rafael,

I've just started reviewing the latest version of the patch. I have good impression about this version so far. Please give me a few days for deeper review. And I've booked the test machine for this patch in next week. I'll try to test the patch and send you the result as soon as I can.

Here are two comments I have so far.

Though it is not directly related to Rafael's patch, I found one problem in _OSC query handling while reviewing this patch. Currently, all the _OSC controls are queried at the same time in acpi_pci_osc_support() and the result is preserved for later acpi_pci_osc_control_set() call. But query result can vary depending on the combination of requested controls.

So I think query result must not be preserved. Since Rafael's patch uses query result, this problem should be fixed at the same time. I'll try to make a patch for this and update Rafael's patch if needed.

I think the following changes should be done in the separated patch. I guess this change is for the BIOS which enables hotplug interrupt at when _OSC is evaluated with native hot-plug control. Right?

Anyway, it should be separated patch with appropriate description.

Quoted 30 lines · On Wed, 28 Jul 2010 23:23:56 +0200 "Rafael J. Wysocki"<[email protected]> wrote:

From: Rafael J. Wysocki<[email protected]>

PCIe port service drivers ask the BIOS, through _OSC, for control of the services they handle. Unfortunately, each of them individually asks for control of the PCIe capability structure and if that is granted, some BIOSes expect that the other PCIe port services will be configured and handled by the kernel as well. If that is not the case (eg. one of the PCIe port service drivers is not loaded), the BIOS may be confused and may cause the system as a whole to misbehave (eg. on one of such systems enabling the native PCIe PME service without loading the native PCIe hot-plug service driver causes a storm of ACPI notify requests to appear).

For this reason rework the PCIe port driver so that (1) it checks which native PCIe port services can be enabled, according to the BIOS, and (2) it requests control of all these services simultaneously. In particular, this causes pcie_portdrv_probe() to fail if the BIOS refuses to grant control of the PCIe capability structure, which means that no native PCIe port services can be enabled for the PCIe root complex the given port belongs to.

Make it possible to override this behavior using a new command line switch pcie_ports= that can be set to 'auto' (ask the BIOS, the default), 'native' (use the PCIe native services regardless of the BIOS response to the control request), or 'compat' (do not use the PCIe native services at all).

Accordingly, rework the existing PCIe port service drivers so that they don't request control of the services directly.

Quoted 2 lines

Signed-off-by Rafael J. Wysocki<[email protected]>

---

Kenji-san, are you ok with this version? I would like to get your ack (and ideally your tested-by) for this one since it affects functionality you need.