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

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Tuesday, August 03, 2010, Hidetoshi Seto wrote:
> (2010/08/03 13:52), Kenji Kaneshige wrote:
> > (2010/08/03 6:59), Rafael J. Wysocki wrote:
> >> @@ -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.

Thanks,
Rafael