Re: [PATCH 7/8] ACPI / PCI: Do not preserve _OSC control bits returned by a query (v2)

On Tuesday, August 03, 2010, Rafael J. Wysocki wrote:

On Tuesday, August 03, 2010, Hidetoshi Seto wrote:

quoted

(2010/08/03 13:52), Kenji Kaneshige wrote:

quoted

(2010/08/03 6:59), Rafael J. Wysocki wrote:

quoted

@@ -434,19 +432,6 @@ acpi_status acpi_pci_osc_control_set(acp
      if ((root->osc_control_set&  control_req) == control_req)
          goto out;

-    /* Need to query controls first before requesting them */
-    if (!root->osc_queried) {
-        status = acpi_pci_query_osc(root, root->osc_support_set, NULL);
-        if (ACPI_FAILURE(status))
-            goto out;
-    }
-    if ((root->osc_control_qry&  control_req) != control_req) {
-        printk(KERN_DEBUG
-               "Firmware did not grant requested _OSC control\n");
-        status = AE_SUPPORT;
-        goto out;
-    }

I think acpi_pci_osc_control_set() still need to query before commit
to ensure all the requested controls are granted to OS.

So the code needs to be

    status = acpi_pci_query_osc(root, root->osc_support_set, &control_req);
    if (ACPI_FAILURE(status))
        goto out;

Hum, since acpi_status acpi_pci_osc_control_set() is an exported
function, we cannot be too careful here. 

OTOH, I think this second query should be done in caller too,
i.e. pcie_port_acpi_setup() in patch [4/8].

Now:
  pcie_port_acpi_setup()
  {
    flags = A|B|C|D;
    acpi_pci_osc_control_query(handle, &flags);
    /* note: flags might be changed after query */
    acpi_pci_osc_control_set(handle, flags);
  }

Strictly, it could be like:

New:
  pcie_port_acpi_setup()
  {
    flags = A|B|C|D;
    do {
      pre_flags = flags;
      acpi_pci_osc_control_query(handle, &flags);
    } while (flags && pre_flags != flags);
    if (flags)
      acpi_pci_osc_control_set(handle, flags);
  }

IMHO these checks are kind of preventive guard for corner cases,
and I suppose it can be implemented by an incremental patch later.

OK

The assumption here is that the BIOS would only mask the bits it's not
going to grant control of and will do it in a consistent way.  So, I guess the
purpose of the change above would be to protect us from buggy BIOSes.

What about the appended patch (on top of 4/8)?

Rafael


---
 drivers/pci/pcie/portdrv_acpi.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Index: linux-2.6/drivers/pci/pcie/portdrv_acpi.c
===================================================================

--- linux-2.6.orig/drivers/pci/pcie/portdrv_acpi.c
+++ linux-2.6/drivers/pci/pcie/portdrv_acpi.c

@@ -35,7 +35,7 @@ int pcie_port_acpi_setup(struct pci_dev 
 {
 	acpi_status status;
 	acpi_handle handle;
-	u32 flags;
+	u32 flags, prev_flags = 0;
 
 	if (acpi_pci_disabled)
 		return 0;

@@ -55,11 +55,14 @@ int pcie_port_acpi_setup(struct pci_dev 
 			flags |= OSC_PCI_EXPRESS_AER_CONTROL;
 	}
 
-	status = acpi_pci_osc_control_query(handle, &flags);
-	if (ACPI_FAILURE(status)) {
-		dev_dbg(&port->dev, "ACPI _OSC query failed (code %d)\n",
-			status);
-		return -ENODEV;
+	while (flags != prev_flags) {
+		prev_flags = flags;
+		status = acpi_pci_osc_control_query(handle, &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)) {