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

[Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>]

(2010/08/05 8:51), Rafael J. Wysocki wrote:
> On Wednesday, August 04, 2010, Rafael J. Wysocki wrote:
>> On Wednesday, August 04, 2010, Hidetoshi Seto wrote:
>>> (2010/08/04 14:46), Kenji Kaneshige wrote:
>>>> (2010/08/04 6:02), Rafael J. Wysocki wrote:
>>>>> On Tuesday, August 03, 2010, Rafael J. Wysocki wrote:
>>>>>> 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;
>>>>>>>
>>>>
>>>> Sorry, that should have been
>>>>
>>>>     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;
>>>>         goto out;
>>>>     }
>>>>
>>>> I know current pcie_port_acpi_setup() queries the requesting controls
>>>> before acpi_pci_osc_control_set() and only one control is requested
>>>> in the other code. However, I think acpi_pci_osc_control_set() still
>>>> need to query the requested controls to ensure all the requested
>>>> controls, in case someone calls this function without querying the
>>>> requesting controls. In other words, I think it must be ensured that
>>>> any controls are never granted to OS when acpi_pci_osc_control_set()
>>>> returns error.
>>>
>>> I think the following patch is what you mean.
>>>
>>> And... (Continue to next post)
>>>
>>> Thanks,
>>> H.Seto
>>
>> OK, I'll repalce my 7/8 with the patch below.
> 
> Actually, having reconsidered that, I don't think this approach is valid.
> 
> First, it has the problem that if acpi_pci_osc_control_set() returns error
> code, the caller doesn't really know whether the query failed, or the final
> request failed.  Arguably, it won't matter for the majority of callers, but
> some of them might be interested in knowing that in principle.

Ugh... there are only 2 callers now and both of them are in the majority.
I don't think it is a time to take care of an invisible minority who might
require acpi_pci_osc_raw() to complete its work.

> 
> Second, the callers that call acpi_pci_osc_control_query() before
> acpi_pci_osc_control_set() don't need the additional query inside
> of acpi_pci_osc_control_set().

So we can recommend all of callers not to call acpi_pci_osc_control_query()
before acpi_pci_osc_control_set().

I suppose that almost all of "the majority" just want to set fixed set of
controls and they will just return error when fails anyway.

> 
> Therefore I'd prefer to have two separate functions, one for querying and the
> other for requesting control.  Then, we can provide a helper that calls the
> both of them for the callers of acpi_pci_osc_control_set() that don't need
> to call acpi_pci_osc_control_query() directly by themselves.

I'm afraid the "two" is not enough for the minority.

Therefore I don't think it is a time to prepare for such an inexistent
minor usage.

> 
> Given that acpi_pci_osc_control_query() is introduced by
> https://patchwork.kernel.org/patch/117176/ , the helper may be implemented
> like in the appended patch.
> 
> After that patch, the $subject patch can be applied without any modifications
> to remove the no-longer-used fields in struct acpi_pci_root.
> 
> Thanks,
> Rafael
> 
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: ACPI / PCI: Introduce function for requesting _OSC controls safely
> 
> Calling raw acpi_pci_osc_control_set() is generally unsafe, because
> it may return error code even if control of some requested features
> have been granted by the BIOS.  For this reason the callers of
> acpi_pci_osc_control_set() should call acpi_pci_osc_control_query()
> before it to make sure that the BIOS is willing to grant control of
> the requested features.
> 
> Introduce helper function acpi_pci_osc_control_set_safe() allowing
> a caller of acpi_pci_osc_control_set() who is not interested in
> the control bits returned by acpi_pci_osc_control_query() to
> request control of _OSC features in a safe way.
> 
> Make acpi_get_hp_hw_control_from_firmware() use the new function.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/acpi/pci_root.c          |   24 ++++++++++++++++++++++++
>  drivers/pci/hotplug/acpi_pcihp.c |    2 +-
>  include/linux/acpi.h             |    1 +
>  3 files changed, 26 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/acpi/pci_root.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/pci_root.c
> +++ linux-2.6/drivers/acpi/pci_root.c
> @@ -472,6 +472,30 @@ out:
>  }
>  EXPORT_SYMBOL(acpi_pci_osc_control_set);
>  
> +/**
> + * acpi_pci_osc_control_set_safe - Query and set _OSC control bit mask.
> + * @handle: ACPI handle of a PCI root bridge (or PCIe Root Complex).
> + * @flags: Mask of _OSC bits to query and set.
> + *
> + * Check if the BIOS is willing to grant control of the features represented
> + * by @flags and request control of these features from it.
> + **/
> +acpi_status acpi_pci_osc_control_set_safe(acpi_handle handle, u32 flags)
> +{
> +	acpi_status status;
> +	u32 ctrl = flags;
> +
> +	status = acpi_pci_osc_control_query(handle, &flags);
> +	if (ACPI_FAILURE(status))
> +		return status;
> +	if ((ctrl & flags) != ctrl)
> +		return AE_SUPPORT;
> +
> +	status = acpi_pci_osc_control_set(handle, flags);
> +	return status;
> +}
> +EXPORT_SYMBOL(acpi_pci_osc_control_set_safe);
> +
>  static int __devinit acpi_pci_root_add(struct acpi_device *device)
>  {
>  	unsigned long long segment, bus;
> Index: linux-2.6/drivers/pci/hotplug/acpi_pcihp.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/hotplug/acpi_pcihp.c
> +++ linux-2.6/drivers/pci/hotplug/acpi_pcihp.c
> @@ -358,7 +358,7 @@ int acpi_get_hp_hw_control_from_firmware
>  		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_safe(handle, flags);
>  		if (ACPI_SUCCESS(status))
>  			goto got_one;
>  		if (status == AE_SUPPORT)
> Index: linux-2.6/include/linux/acpi.h
> ===================================================================
> --- linux-2.6.orig/include/linux/acpi.h
> +++ linux-2.6/include/linux/acpi.h
> @@ -307,6 +307,7 @@ acpi_status acpi_run_osc(acpi_handle han
>  
>  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_safe(acpi_handle handle, u32 flags);
>  extern void acpi_early_init(void);
>  
>  #else	/* !CONFIG_ACPI */
> 
> 

So I'd like to say NAK against this patch, sorry.


Thanks,
H.Seto