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

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

(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.


Thanks,
H.Seto