Re: [linux-pm] [PATCH 1/8] ACPI / PCI: Introduce acpi_pci_osc_control_query()

[Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>]

(2010/08/04 5:58), Rafael J. Wysocki wrote:
> On Tuesday, August 03, 2010, Rafael J. Wysocki wrote:
>> On Tuesday, August 03, 2010, Kenji Kaneshige wrote:
>>> (2010/08/03 6:53), Rafael J. Wysocki wrote:
>>>> From: Rafael J. Wysocki<rjw@sisk.pl>
>>>
>>> <snip.>
>>>
>>>> +	mutex_lock(&osc_lock);
>>>> +	status = acpi_pci_query_osc(root, root->osc_support_set, ctrl_mask);
>>>> +	mutex_unlock(&osc_lock);
>>>> +
>>>
>>> One more comment here.
>>>
>>> I think we can skip acpi_pci_query_osc() if all of queried controls are
>>> already granted to OS. Please see below
>>>
>>> 	mutex_lock(&osc_lock);
>>> 	if ((root->osc_control_set&  *ctrl_mask) == *ctrl_mask) {
>>> 		*ctrl_mask = root->osc_control_set;
>>> 		goto out;
>>> 	}
>>> 	status = acpi_pci_query_osc(root, root->osc_support_set, ctrl_mask);
>>> 	mutex_unlock(&osc_lock);
>>> out:
>>
>> Well I guess you mean:
>>
>>   	mutex_lock(&osc_lock);
>>   	if ((root->osc_control_set&  *ctrl_mask) != *ctrl_mask)
>>   		status = acpi_pci_query_osc(root, root->osc_support_set, ctrl_mask);
>>   	mutex_unlock(&osc_lock);
>>
>> Otherwise we would return with the mutex held. :-)
>

Oops... sorry...

> Updated patch is appended, please tell me what you think.

Looks good to me. The below (in your updated patch) was what I wanted to mean

> +	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);

Thanks,
Kenji Kaneshige