Re: [PATCH 3/6] acpi : remove pointless cpuidle device state_count init

From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: lenb@kernel.org, linux-pm@vger.kernel.org,
	linux-acpi@vger.kernel.org, patches@linaro.org,
	linaro-dev@lists.linaro.org, pdeschrijver@nvidia.com,
	lorenzo.pieralisi@arm.com
Subject: Re: [PATCH 3/6] acpi : remove pointless cpuidle device state_count init
Date: Sun, 16 Sep 2012 22:38:49 +0200	[thread overview]
Message-ID: <505638D9.80302@linaro.org> (raw)
In-Reply-To: <201209072350.50389.rjw@sisk.pl>

On 09/07/2012 11:50 PM, Rafael J. Wysocki wrote:
> On Friday, September 07, 2012, Daniel Lezcano wrote:
>> The cpuidle core takes care of filling this field from drv->state_count.
> 
> I'm not quite sure this is always valid.  If dev has already been
> initialized and dev->state_count is different from 0,
> cpuidle_enable_device() doesn't actually change it.
> 
> Have you considered all of the possible scenarios?

Ok, there is the scenario where the ACPI supports _CST.
At runtime, ACPI may notify the OS a C-state changed for a specific cpu
and this is done through 'acpi_processor_cst_has_changed' followed by
'acpi_processor_setup_cpuidle_cx'.

So at the end, we could have different cpus with one cpu with less
C-states than the other, if I understood correctly ACPIspec30.pdf =>
page 262 :)

In conclusion, we should keep this variable filled from there and keep
this in mind in cpuidle.c in order to not break acpi in the future.
Maybe a comment in cpuidle.c would help ... especially with a single
C-state registration.

>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/acpi/processor_idle.c |    2 --
>>  1 files changed, 0 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index 084b1d2..fc4757e 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -1035,8 +1035,6 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr)
>>  			break;
>>  	}
>>  
>> -	dev->state_count = count;
>> -
>>  	if (!count)
>>  		return -EINVAL;
>>  
>>
> 

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html