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

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

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@jp.fujitsu.com>
> > ---
> >   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