Re: [linux-pm] [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, 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>

It looks good to me.

Thanks,
Rafael


> ---
>  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(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 061e6f4..9a288df 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -372,38 +372,6 @@ out:
>  }
>  EXPORT_SYMBOL_GPL(acpi_get_pci_dev);
>  
> - /**
> - * acpi_pci_osc_control_query - Get the _OSC bits the kernel can control.
> - * @handle: ACPI handle of a PCI root bridge (or PCIe Root Complex).
> - * @mask: Mask of _OSC bits to query and the place to put the result into.
> - **/
> -acpi_status acpi_pci_osc_control_query(acpi_handle handle, u32 *mask)
> -{
> -	struct acpi_pci_root *root;
> -	acpi_handle tmp;
> -	acpi_status status = AE_OK;
> -
> -	if (!mask || !(*mask & OSC_PCI_CONTROL_MASKS))
> -		return AE_BAD_PARAMETER;
> -
> -	root = acpi_pci_find_root(handle);
> -	if (!root)
> -		return AE_NOT_EXIST;
> -
> -	status = acpi_get_handle(handle, "_OSC", &tmp);
> -	if (ACPI_FAILURE(status))
> -		return status;
> -
> -	mutex_lock(&osc_lock);
> -	if ((*mask & root->osc_control_set) == *mask)
> -		*mask = root->osc_control_set;
> -	else
> -		status = acpi_pci_query_osc(root, root->osc_support_set, mask);
> -	mutex_unlock(&osc_lock);
> -
> -	return status;
> -}
> -
>  /**
>   * 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.
>   **/
> -acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 flags)
> +acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *flags)
>  {
>  	acpi_status status;
>  	u32 control_req, result, capbuf[3];
>  	acpi_handle tmp;
>  	struct acpi_pci_root *root;
>  
> -	control_req = (flags & OSC_PCI_CONTROL_MASKS);
> +	if (!flags)
> +		return AE_BAD_PARAMETER;
> +
> +	control_req = (*flags & OSC_PCI_CONTROL_MASKS);
>  	if (!control_req)
>  		return AE_TYPE;
>  
> @@ -432,8 +403,10 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 flags)
>  
>  	mutex_lock(&osc_lock);
>  	/* No need to evaluate _OSC if the control was already granted. */
> -	if ((root->osc_control_set & control_req) == control_req)
> +	if ((root->osc_control_set & control_req) == control_req) {
> +		*flags = root->osc_control_set;
>  		goto out;
> +	}
>  
>  	/* Need to query controls first before requesting them */
>  	result = control_req;
> @@ -441,8 +414,10 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 flags)
>  	if (ACPI_FAILURE(status))
>  		goto out;
>  	if ((result & control_req) != control_req) {
> -		printk(KERN_DEBUG
> -		       "Firmware did not grant requested _OSC control\n");
> +		printk(KERN_DEBUG PREFIX
> +		       "Firmware did not grant requested _OSC control: %x/%x\n",
> +		       control_req, result);
> +		*flags = result;
>  		status = AE_SUPPORT;
>  		goto out;
>  	}
> @@ -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;
> +	else
> +		printk(KERN_WARNING FW_BUG PREFIX
> +		       "_OSC did not grant controls that passed query\n");
>  out:
>  	mutex_unlock(&osc_lock);
>  	return status;
> diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
> index 12ea415..675181d 100644
> --- a/drivers/pci/hotplug/acpi_pcihp.c
> +++ b/drivers/pci/hotplug/acpi_pcihp.c
> @@ -358,7 +358,7 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev, u32 flags)
>  		acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
>  		dbg("Trying to get hotplug control for %s\n",
>  				(char *)string.pointer);
> -		status = acpi_pci_osc_control_set(handle, flags);
> +		status = acpi_pci_osc_control_set(handle, &flags);
>  		if (ACPI_SUCCESS(status))
>  			goto got_one;
>  		if (status == AE_SUPPORT)
> diff --git a/drivers/pci/pcie/portdrv_acpi.c b/drivers/pci/pcie/portdrv_acpi.c
> index a07a70e..764d0b3 100644
> --- a/drivers/pci/pcie/portdrv_acpi.c
> +++ b/drivers/pci/pcie/portdrv_acpi.c
> @@ -55,20 +55,17 @@ int pcie_port_acpi_setup(struct pci_dev *port, int *srv_mask)
>  			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;
> +retry:
> +	status = acpi_pci_osc_control_set(handle, &flags);
> +	if (status == AE_SUPPORT) {
> +		if (!(flags & OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL)) {
> +			dev_dbg(&port->dev, "BIOS refuses to grant control of "
> +				"PCIe Capability Structure\n");
> +			return -EACCES;
> +		}
> +		if (flags & ~OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL)
> +			goto retry;
>  	}
> -
> -	if (!(flags & OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL)) {
> -		dev_dbg(&port->dev, "BIOS refuses to grant control of PCIe "
> -			"Capability Structure\n");
> -		return -EACCES;
> -	}
> -
> -	status = acpi_pci_osc_control_set(handle, flags);
>  	if (ACPI_FAILURE(status)) {
>  		dev_dbg(&port->dev, "ACPI _OSC request failed (code %d)\n",
>  			status);
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index a9afe9c..1b5aac6 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -305,8 +305,7 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);
>  				OSC_PCI_EXPRESS_AER_CONTROL |		\
>  				OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL)
>  
> -extern acpi_status acpi_pci_osc_control_query(acpi_handle handle, u32 *mask);
> -extern acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 flags);
> +extern acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *flags);
>  extern void acpi_early_init(void);
>  
>  #else	/* !CONFIG_ACPI */
>