On Wednesday, August 04, 2010, Kenji Kaneshige wrote:
> (2010/08/04 17:43), Hidetoshi Seto wrote:
> > (2010/08/04 14:46), Kenji Kaneshige wrote:
> >> By the way, I think it is getting confusing regarding who query the
> >> controls. IMO, querying controls to ensure all the requested controls
> >> are granted to OS should be done in acpi_pci_osc_control_set(), as
> >> I said above. On the other hand, PCIe port driver need to query
> >> controls for other reason... Now I think it might be better to change
> >> acpi_pci_osc_control_set() like below instead of introducing
> >> acpi_pci_osc_control_query().
> >>
> >> acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *flags)
> >> {
> >> ...
> >> query = control_req;
> >> status = acpi_pci_query_osc(root, root->osc_support_set,&query);
> >> if (ACPI_FAILURE(status))
> >> goto out;
> >> if ((query& control_req) != control_req) {
> >> printk_(KERN_DEBUG
> >> "Firmware did not grant requested _OSC control\n");
> >> status = AE_SUPPORT;
> >> *flags = (query& control_req);
> >> goto out;
> >> }
> >> ...
> >> }
> >>
> >> And do as follows in pcie_port_acpi_setup()
> >>
> >> status = acpi_pci_osc_control_set(handle,&flags);
> >> if (status == AE_SUPPORT) {
> >> /* 2nd try */
> >> status = acpi_pci_osc_control_set(handle,&flags);
> >> }
> >> if (ACPI_FAILURE(status)) {
> >> ...
> >>
> >> What do you think about this?
> >
> > I think it makes sense, though some minor cares are required.
> >
> > Does this incremental patch (apply after [8/8]) looks good?
> > If it is OK, I'll test these 8+1 patches within the next 2days.
> >
> > Thanks,
> > H.Seto
> >
> > =====
> > Subject: ACPI/PCI: Unify acpi_pci_osc_control_*()
> >
> > Now AE_SUPPORT of acpi_pci_osc_control_set() tells not only
> > that query fails with the requested control bits but also that
> > the result of query is stored into the pointed place.
> >
> > This allow user to retry acpi_pci_osc_control_set() with the
> > result of query.
> >
> > Signed-off-by: Hidetoshi Seto<seto.hidetoshi@xxxxxxxxxxxxxx>
> > ---
> > drivers/acpi/pci_root.c | 54 +++++++++++--------------------------
> > drivers/pci/hotplug/acpi_pcihp.c | 2 +-
> > drivers/pci/pcie/portdrv_acpi.c | 23 +++++++---------
> > include/linux/acpi.h | 3 +-
> > 4 files changed, 28 insertions(+), 54 deletions(-)
>
> <snip.>
>
> > /**
> > * acpi_pci_osc_control_set - commit requested control to Firmware
> > * @handle: acpi_handle for the target ACPI object
> > @@ -411,14 +379,17 @@ acpi_status acpi_pci_osc_control_query(acpi_handle handle, u32 *mask)
> > *
> > * Attempt to take control from Firmware on requested control bits.
> > **/
>
> Updating description of this function would be appreciated.
>
> <snip.>
>
> > @@ -452,7 +427,10 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 flags)
> > capbuf[OSC_CONTROL_TYPE] = root->osc_control_set | control_req;
> > status = acpi_pci_run_osc(handle, capbuf,&result);
> > if (ACPI_SUCCESS(status))
> > - root->osc_control_set = result;
> > + root->osc_control_set = *flags = result;
>
> I don't think we need to update *flags here, though it depends on the design
> of this function interface.
> IMHO, updating *flags only when AE_SUPPORT is returned is easy to understand.
I think the patch is correct. We should update *flags in any case so that the
caller can check what's the full mask of granted controls even if it asked for
fewer control bits.
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
