Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1402497 > unrolled thread

Re: [PATCH v5 2/5] ACPI / processor_idle: Add support for Low Power Idle(LPI) states

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 2/5] ACPI / processor_idle: Add support for Low Power  Idle(LPI) states "Prakash, Prashanth" <pprakash@codeaurora.org> - 2016-05-17 19:50 +0200
    Re: [PATCH v5 2/5] ACPI / processor_idle: Add support for Low Power  Idle(LPI) states Sudeep Holla <sudeep.holla@arm.com> - 2016-05-18 19:40 +0200
      Re: [PATCH v5 2/5] ACPI / processor_idle: Add support for Low Power  Idle(LPI) states "Prakash, Prashanth" <pprakash@codeaurora.org> - 2016-05-18 21:20 +0200
        Re: [PATCH v5 2/5] ACPI / processor_idle: Add support for Low Power  Idle(LPI) states Sudeep Holla <sudeep.holla@arm.com> - 2016-05-19 15:30 +0200

#1402497 — Re: [PATCH v5 2/5] ACPI / processor_idle: Add support for Low Power Idle(LPI) states

Hi Sudeep,

On 5/11/2016 9:37 AM, Sudeep Holla wrote:
> +
> +static int acpi_processor_get_lpi_info(struct acpi_processor *pr)
> +{
> +	int ret, i;
> +	struct acpi_lpi_states_array *info;
> +	struct acpi_device *d = NULL;
> +	acpi_handle handle = pr->handle, pr_ahandle;
> +	acpi_status status;
> +
> +	if (!osc_pc_lpi_support_confirmed)
> +		return -EOPNOTSUPP;
> +
> +	max_leaf_depth = 0;
> +	if (!acpi_has_method(handle, "_LPI"))
> +		return -EINVAL;
> +	flat_state_cnt = 0;
> +
> +	while (ACPI_SUCCESS(status = acpi_get_parent(handle, &pr_ahandle))) {
> +		if (!acpi_has_method(handle, "_LPI"))
> +			continue;
> +
> +		acpi_bus_get_device(handle, &d);
> +		if (!strcmp(acpi_device_hid(d), ACPI_PROCESSOR_CONTAINER_HID))
> +			break;
> +
> +		max_leaf_depth++;
> +		handle = pr_ahandle;
> +	}
> +
In the above loop, we break when we find a device with HID == ACPI_PROCESSOR_CONTAINER_HID.
Shouldn't we continue to parse as long as the parent HID == ACPI_PROCESSOR_CONTAINER_HID?
This is required to make sure we parse states in levels higher than cluster level
in processor hierarchy.

Also, I think it might be safe to break out of the loop if we didn't find _LPI
package, instead of continuing. Given  the presence of LPI entry: "Enabled Parent
State", I can't think of a non-ambiguous scenario where we might find LPI packages
in state N and N+2, but not in N+1, as we will not be able to figure out which
state in N enables which states in N+2. Thoughts?

Thanks,
Prashanth

[toc] | [next] | [standalone]

#1403192

On 17/05/16 18:46, Prakash, Prashanth wrote:
> Hi Sudeep,
>
> On 5/11/2016 9:37 AM, Sudeep Holla wrote:
>> +
>> +static int acpi_processor_get_lpi_info(struct acpi_processor *pr)
>> +{
>> +	int ret, i;
>> +	struct acpi_lpi_states_array *info;
>> +	struct acpi_device *d = NULL;
>> +	acpi_handle handle = pr->handle, pr_ahandle;
>> +	acpi_status status;
>> +
>> +	if (!osc_pc_lpi_support_confirmed)
>> +		return -EOPNOTSUPP;
>> +
>> +	max_leaf_depth = 0;
>> +	if (!acpi_has_method(handle, "_LPI"))
>> +		return -EINVAL;
>> +	flat_state_cnt = 0;
>> +
>> +	while (ACPI_SUCCESS(status = acpi_get_parent(handle, &pr_ahandle))) {
>> +		if (!acpi_has_method(handle, "_LPI"))
>> +			continue;
>> +
>> +		acpi_bus_get_device(handle, &d);
>> +		if (!strcmp(acpi_device_hid(d), ACPI_PROCESSOR_CONTAINER_HID))
>> +			break;
>> +
>> +		max_leaf_depth++;
>> +		handle = pr_ahandle;
>> +	}
>> +
> In the above loop, we break when we find a device with HID ==
> ACPI_PROCESSOR_CONTAINER_HID. Shouldn't we continue to parse as long as the
> parent HID == ACPI_PROCESSOR_CONTAINER_HID? This is required to make sure we
> parse states in levels higher than cluster level in processor hierarchy.
>

Yes, thanks for pointing that out. With just clusters in _LPI on my dev
board, I missed it.

> Also, I think it might be safe to break out of the loop if we didn't find
> _LPI package, instead of continuing. Given  the presence of LPI entry:
> "Enabled Parent State", I can't think of a non-ambiguous scenario where we
> might find LPI packages in state N and N+2, but not in N+1, as we will not
> be able to figure out which state in N enables which states in N+2.
> Thoughts?

Though I admit I haven't thought in detail on how to deal with the
asymmetric topology, but that was the reason why I continue instead of
breaking.

Excerpts from the spec: "... This example is symmetric but that is not a
requirement. For example, a system may contain a different number of
processors in different containers or an asymmetric hierarchy where one
side of the topology tree is deeper than another...."

-- 
Regards,
Sudeep

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1403234

On 5/18/2016 11:37 AM, Sudeep Holla wrote:
>
>
> On 17/05/16 18:46, Prakash, Prashanth wrote:
>> Hi Sudeep,
>>
>> On 5/11/2016 9:37 AM, Sudeep Holla wrote:
>>> +
>>> +static int acpi_processor_get_lpi_info(struct acpi_processor *pr)
>>> +{
>>> +    int ret, i;
>>> +    struct acpi_lpi_states_array *info;
>>> +    struct acpi_device *d = NULL;
>>> +    acpi_handle handle = pr->handle, pr_ahandle;
>>> +    acpi_status status;
>>> +
>>> +    if (!osc_pc_lpi_support_confirmed)
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    max_leaf_depth = 0;
>>> +    if (!acpi_has_method(handle, "_LPI"))
>>> +        return -EINVAL;
>>> +    flat_state_cnt = 0;
>>> +
>>> +    while (ACPI_SUCCESS(status = acpi_get_parent(handle, &pr_ahandle))) {
>>> +        if (!acpi_has_method(handle, "_LPI"))
>>> +            continue;
>>> +
>>> +        acpi_bus_get_device(handle, &d);
>>> +        if (!strcmp(acpi_device_hid(d), ACPI_PROCESSOR_CONTAINER_HID))
>>> +            break;
>>> +
>>> +        max_leaf_depth++;
>>> +        handle = pr_ahandle;
>>> +    }
>>> +
>> In the above loop, we break when we find a device with HID ==
>> ACPI_PROCESSOR_CONTAINER_HID. Shouldn't we continue to parse as long as the
>> parent HID == ACPI_PROCESSOR_CONTAINER_HID? This is required to make sure we
>> parse states in levels higher than cluster level in processor hierarchy.
>>
>
> Yes, thanks for pointing that out. With just clusters in _LPI on my dev
> board, I missed it.
>
Same reason, I failed to notice it all this time :)
>> Also, I think it might be safe to break out of the loop if we didn't find
>> _LPI package, instead of continuing. Given  the presence of LPI entry:
>> "Enabled Parent State", I can't think of a non-ambiguous scenario where we
>> might find LPI packages in state N and N+2, but not in N+1, as we will not
>> be able to figure out which state in N enables which states in N+2.
>> Thoughts?
>
> Though I admit I haven't thought in detail on how to deal with the
> asymmetric topology, but that was the reason why I continue instead of
> breaking.
>
> Excerpts from the spec: "... This example is symmetric but that is not a
> requirement. For example, a system may contain a different number of
> processors in different containers or an asymmetric hierarchy where one
> side of the topology tree is deeper than another...."
>
If it addresses asymmetric topology, sure we can keep as it doesn't impact other
scenarios. Also, we need to set handle=pr_ahandle prior to the continue statement.


Thanks,
Prashanth

[toc] | [prev] | [next] | [standalone]

#1403737

On 18/05/16 20:13, Prakash, Prashanth wrote:
>
>
> On 5/18/2016 11:37 AM, Sudeep Holla wrote:
>>
>>
>> On 17/05/16 18:46, Prakash, Prashanth wrote:
>>> Hi Sudeep,
>>>
>>> On 5/11/2016 9:37 AM, Sudeep Holla wrote:
>>>> +
>>>> +static int acpi_processor_get_lpi_info(struct acpi_processor *pr)
>>>> +{
>>>> +    int ret, i;
>>>> +    struct acpi_lpi_states_array *info;
>>>> +    struct acpi_device *d = NULL;
>>>> +    acpi_handle handle = pr->handle, pr_ahandle;
>>>> +    acpi_status status;
>>>> +
>>>> +    if (!osc_pc_lpi_support_confirmed)
>>>> +        return -EOPNOTSUPP;
>>>> +
>>>> +    max_leaf_depth = 0;
>>>> +    if (!acpi_has_method(handle, "_LPI"))
>>>> +        return -EINVAL;
>>>> +    flat_state_cnt = 0;
>>>> +
>>>> +    while (ACPI_SUCCESS(status = acpi_get_parent(handle, &pr_ahandle))) {
>>>> +        if (!acpi_has_method(handle, "_LPI"))
>>>> +            continue;
>>>> +
>>>> +        acpi_bus_get_device(handle, &d);
>>>> +        if (!strcmp(acpi_device_hid(d), ACPI_PROCESSOR_CONTAINER_HID))
>>>> +            break;
>>>> +
>>>> +        max_leaf_depth++;
>>>> +        handle = pr_ahandle;
>>>> +    }
>>>> +
>>> In the above loop, we break when we find a device with HID ==
>>> ACPI_PROCESSOR_CONTAINER_HID. Shouldn't we continue to parse as long as the
>>> parent HID == ACPI_PROCESSOR_CONTAINER_HID? This is required to make sure we
>>> parse states in levels higher than cluster level in processor hierarchy.
>>>
>>
>> Yes, thanks for pointing that out. With just clusters in _LPI on my dev
>> board, I missed it.
>>
> Same reason, I failed to notice it all this time :)

No worries.

>>> Also, I think it might be safe to break out of the loop if we didn't find
>>> _LPI package, instead of continuing. Given  the presence of LPI entry:
>>> "Enabled Parent State", I can't think of a non-ambiguous scenario where we
>>> might find LPI packages in state N and N+2, but not in N+1, as we will not
>>> be able to figure out which state in N enables which states in N+2.
>>> Thoughts?
>>
>> Though I admit I haven't thought in detail on how to deal with the
>> asymmetric topology, but that was the reason why I continue instead of
>> breaking.
>>
>> Excerpts from the spec: "... This example is symmetric but that is not a
>> requirement. For example, a system may contain a different number of
>> processors in different containers or an asymmetric hierarchy where one
>> side of the topology tree is deeper than another...."
>>
> If it addresses asymmetric topology, sure we can keep as it doesn't impact other
> scenarios. Also, we need to set handle=pr_ahandle prior to the continue statement.
>

Yes I noticed it yesterday, the more I think, I feel we can break out of
the loop. At any level, we need to have container nodes and that must
contain _LPI irrespective of asymmetricity. So you were right. I have
fixed accordingly and have pushed out on my branch.

-- 
Regards,
Sudeep

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web