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

Message ID 1347013172-12465-4-git-send-email-daniel.lezcano@linaro.org
State New
Headers show

Commit Message

Daniel Lezcano Sept. 7, 2012, 10:19 a.m.
The cpuidle core takes care of filling this field from drv->state_count.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/acpi/processor_idle.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

Comments

Amit Kucheria Sept. 7, 2012, 11:01 a.m. | #1
Patch 1 and 3 are independent cleanups. Perhaps you can separate them
out from the per-cpu latency series in case you have to repost.

On Fri, Sep 7, 2012 at 3:49 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> The cpuidle core takes care of filling this field from drv->state_count.
>
> 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;
>
> --
> 1.7.5.4
>
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
Rafael J. Wysocki Sept. 7, 2012, 9:50 p.m. | #2
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?

Rafael


> 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;
>  
>
Daniel Lezcano Sept. 16, 2012, 8:38 p.m. | #3
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;
>>  
>>
>
Rafael J. Wysocki Sept. 16, 2012, 9:02 p.m. | #4
On Sunday, September 16, 2012, Daniel Lezcano wrote:
> 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.

Sure, adding a comment in there sounds like a good idea.

Thanks,
Rafael

Patch

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;