Re: [PATCH 09/12] ACPI: add clarifying comment about processor throttling in HW reduced mode

[Al Stone <al.stone@linaro.org>]

On 11/20/2013 05:14 PM, Rafael J. Wysocki wrote:
> On Wednesday, November 20, 2013 02:54:32 PM Al Stone wrote:
>> On 11/17/2013 03:20 PM, Rafael J. Wysocki wrote:
>>> On Saturday, November 09, 2013 06:36:19 PM al.stone@linaro.org wrote:
>>>> From: Al Stone <ahs3@redhat.com>
>>>>
>>>> Signed-off-by: Al Stone <al.stone@linaro.org>
>>>> ---
>>>>    drivers/acpi/processor_throttling.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/processor_throttling.c b/drivers/acpi/processor_throttling.c
>>>> index e7dd2c1..200738e 100644
>>>> --- a/drivers/acpi/processor_throttling.c
>>>> +++ b/drivers/acpi/processor_throttling.c
>>>> @@ -942,6 +942,10 @@ static int acpi_processor_get_fadt_info(struct acpi_processor *pr)
>>>>    		return -EINVAL;
>>>>    	}
>>>>
>>>> +	/*
>>>> +	 * NB: in HW reduced mode, duty_width is always zero
>>>> +	 * so this count may not be what is wanted.
>>>> +	 */
>>>>    	pr->throttling.state_count = 1 << acpi_gbl_FADT.duty_width;
>>>>
>>>>    	/*
>>>> @@ -991,6 +995,10 @@ static int acpi_processor_set_throttling_fadt(struct acpi_processor *pr,
>>>>    		/* Used to clear all duty_value bits */
>>>>    		duty_mask = pr->throttling.state_count - 1;
>>>>
>>>> +		/*
>>>> +		 * NB: in HW reduced mode, duty_offset is always zero
>>>> +		 * so this mask may not be what is wanted.
>>>> +		 */
>>>>    		duty_mask <<= acpi_gbl_FADT.duty_offset;
>>>>    		duty_mask = ~duty_mask;
>>>>    	}
>>>
>>> I'm not sure how these comments help to be honest.  It looks like
>>> pr->throttling.state_count should be 0 in HW reduced mode, shouldn't it?
>>>
>>
>> It should.  The comments clarified things for me but perhaps
>> they should just note that these values are always zero in
>> reduced HW mode.  The other option would be to not add any
>> comments, of course.  Hopefully someone working with reduced
>> HW mode would be aware of these changes to the FADT values.
>>
>> I can go either way; what's the preference?
>
> I would just avoid making changes until we figure out what to do ultimately
> here.  That depends on what pr->throttling.state_count is used for and that
> part should be hardened against pr->throttling.state_count == 0.  Having
> done that, we can simply set pr->throttling.state_count = 0 in the HW reduced
> mode without adding any comments at all.
>
> Thanks!
>

I'll remove this patch for now.  I think what makes sense is a
separate patch to harden the use of pr->throttling.state_count
in processor_throttling.c and processor_thermal.c.  I see a
couple of places where the values may not make sense when it
is zero (regardless of reduced HW mode) but I think I'd like
to treat that as a separate issue and think it through some
more.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------