| Message ID | 1347013172-12465-4-git-send-email-daniel.lezcano@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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; > >
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; >> >> >
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
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;
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(-)