| Message ID | 1426704372-20406-8-git-send-email-daniel.lezcano@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 03/19/2015 12:08 PM, Lorenzo Pieralisi wrote: > On Wed, Mar 18, 2015 at 06:46:11PM +0000, Daniel Lezcano wrote: >> Some architectures have some cpus which does not support idle states. >> >> Let the underlying low level code to return -ENOSYS when it is not >> possible to set an idle state. > > http://www.spinics.net/lists/linux-acpi/msg55550.html > > -ENXIO ? Ack, it makes more sense. >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> drivers/cpuidle/cpuidle-arm.c | 45 +++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 43 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c >> index 1c94b88..0682ed0 100644 >> --- a/drivers/cpuidle/cpuidle-arm.c >> +++ b/drivers/cpuidle/cpuidle-arm.c >> @@ -17,11 +17,14 @@ >> #include <linux/kernel.h> >> #include <linux/module.h> >> #include <linux/of.h> >> +#include <linux/slab.h> >> >> #include <asm/cpuidle.h> >> >> #include "dt_idle_states.h" >> >> +static DEFINE_PER_CPU(struct cpuidle_device, *cpuidle_arm_dev); >> + >> /* >> * arm_enter_idle_state - Programs CPU to enter the specified state >> * >> @@ -94,6 +97,7 @@ static int __init arm_idle_init(void) >> { >> int cpu, ret; >> struct cpuidle_driver *drv = &arm_idle_driver; >> + struct cpuidle_device *dev; >> >> /* >> * Initialize idle states data, starting at index 1. >> @@ -105,18 +109,55 @@ static int __init arm_idle_init(void) >> if (ret <= 0) >> return ret ? : -ENODEV; >> >> + ret = cpuidle_register_driver(drv); >> + if (ret) { >> + pr_err("Failed to register cpuidle driver\n"); >> + return ret; >> + } >> + >> /* >> * Call arch CPU operations in order to initialize >> * idle states suspend back-end specific data >> */ >> for_each_possible_cpu(cpu) { >> ret = arm_cpuidle_init(cpu); >> + >> + /* This cpu does not support any idle states */ >> + if (ret == -ENOSYS) >> + continue; >> + >> if (ret) { >> pr_err("CPU %d failed to init idle CPU ops\n", cpu); >> - return ret; >> + goto out_fail; >> + } >> + >> + dev = kzalloc(sizeof(*dev), GFP_KERNEL); >> + if (!dev) { >> + pr_err("Failed to allocate cpuidle device\n"); >> + goto out_fail; >> + } >> + >> + dev->cpu = cpu; >> + per_cpu(cpuidle_arm_dev, cpu) = dev; >> + >> + ret = cpuidle_register_device(dev); >> + if (ret) { >> + pr_err("Failed to register cpuidle device for CPU %d\n", >> + cpu); >> + kfree(dev); >> + goto out_fail; >> } >> } >> +out: >> + return ret; > > return 0; > >> >> - return cpuidle_register(drv, NULL); >> +out_fail: >> + for (cpu--; cpu <= 0; cpu--) { > > This loop is wrong. Why is it wrong ? We have to initialize at cpu - 1 to unregister the previous registered cpu, not the current one, no ? >> + dev = per_cpu(cpuidle_arm_dev, cpu); >> + cpuidle_unregister_device(dev); >> + kfree(dev); >> + } >> + cpuidle_unregister_driver(drv); >> + goto out; > > You just: > > return ret; > > little point in jumping around to just return. Ok.
On 03/19/2015 12:38 PM, Lorenzo Pieralisi wrote: > On Thu, Mar 19, 2015 at 11:27:20AM +0000, Daniel Lezcano wrote: > > [...] > >>>> for_each_possible_cpu(cpu) { >>>> ret = arm_cpuidle_init(cpu); >>>> + >>>> + /* This cpu does not support any idle states */ >>>> + if (ret == -ENOSYS) >>>> + continue; >>>> + >>>> if (ret) { >>>> pr_err("CPU %d failed to init idle CPU ops\n", cpu); >>>> - return ret; >>>> + goto out_fail; >>>> + } >>>> + >>>> + dev = kzalloc(sizeof(*dev), GFP_KERNEL); >>>> + if (!dev) { >>>> + pr_err("Failed to allocate cpuidle device\n"); >>>> + goto out_fail; >>>> + } >>>> + >>>> + dev->cpu = cpu; >>>> + per_cpu(cpuidle_arm_dev, cpu) = dev; >>>> + >>>> + ret = cpuidle_register_device(dev); >>>> + if (ret) { >>>> + pr_err("Failed to register cpuidle device for CPU %d\n", >>>> + cpu); >>>> + kfree(dev); >>>> + goto out_fail; >>>> } >>>> } >>>> +out: >>>> + return ret; >>> >>> return 0; >>> >>>> >>>> - return cpuidle_register(drv, NULL); >>>> +out_fail: >>>> + for (cpu--; cpu <= 0; cpu--) { >>> >>> This loop is wrong. >> >> Why is it wrong ? We have to initialize at cpu - 1 to unregister the >> previous registered cpu, not the current one, no ? > > Yes, but on cpu>=0 not cpu<=0 > > while (--cpu >= 0) Ah, yes. Thanks for spotting this. -- Daniel
On Wed, Mar 18 2015 at 12:46 -0600, Daniel Lezcano wrote: >Some architectures have some cpus which does not support idle states. > >Let the underlying low level code to return -ENOSYS when it is not >possible to set an idle state. > >Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >--- > drivers/cpuidle/cpuidle-arm.c | 45 +++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 43 insertions(+), 2 deletions(-) > >diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c >index 1c94b88..0682ed0 100644 >--- a/drivers/cpuidle/cpuidle-arm.c >+++ b/drivers/cpuidle/cpuidle-arm.c >@@ -17,11 +17,14 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of.h> >+#include <linux/slab.h> > > #include <asm/cpuidle.h> > > #include "dt_idle_states.h" > >+static DEFINE_PER_CPU(struct cpuidle_device, *cpuidle_arm_dev); Is a per-cpu variable needed? There seems to be no per-cpu access, other than at init. >+ > /* > * arm_enter_idle_state - Programs CPU to enter the specified state > * >@@ -94,6 +97,7 @@ static int __init arm_idle_init(void) > { > int cpu, ret; > struct cpuidle_driver *drv = &arm_idle_driver; >+ struct cpuidle_device *dev; > > /* > * Initialize idle states data, starting at index 1. >@@ -105,18 +109,55 @@ static int __init arm_idle_init(void) > if (ret <= 0) > return ret ? : -ENODEV; > >+ ret = cpuidle_register_driver(drv); >+ if (ret) { >+ pr_err("Failed to register cpuidle driver\n"); >+ return ret; >+ } >+ > /* > * Call arch CPU operations in order to initialize > * idle states suspend back-end specific data > */ > for_each_possible_cpu(cpu) { > ret = arm_cpuidle_init(cpu); >+ >+ /* This cpu does not support any idle states */ >+ if (ret == -ENOSYS) >+ continue; >+ > if (ret) { > pr_err("CPU %d failed to init idle CPU ops\n", cpu); >- return ret; >+ goto out_fail; >+ } >+ >+ dev = kzalloc(sizeof(*dev), GFP_KERNEL); >+ if (!dev) { >+ pr_err("Failed to allocate cpuidle device\n"); >+ goto out_fail; >+ } >+ >+ dev->cpu = cpu; >+ per_cpu(cpuidle_arm_dev, cpu) = dev; >+ >+ ret = cpuidle_register_device(dev); >+ if (ret) { >+ pr_err("Failed to register cpuidle device for CPU %d\n", >+ cpu); >+ kfree(dev); >+ goto out_fail; > } > } >+out: >+ return ret; > >- return cpuidle_register(drv, NULL); >+out_fail: >+ for (cpu--; cpu <= 0; cpu--) { >+ dev = per_cpu(cpuidle_arm_dev, cpu); >+ cpuidle_unregister_device(dev); >+ kfree(dev); >+ } >+ cpuidle_unregister_driver(drv); >+ goto out; > } > device_initcall(arm_idle_init); >-- >1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 03/19/2015 04:31 PM, Lina Iyer wrote: > On Wed, Mar 18 2015 at 12:46 -0600, Daniel Lezcano wrote: >> Some architectures have some cpus which does not support idle states. >> >> Let the underlying low level code to return -ENOSYS when it is not >> possible to set an idle state. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> drivers/cpuidle/cpuidle-arm.c | 45 >> +++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 43 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cpuidle/cpuidle-arm.c >> b/drivers/cpuidle/cpuidle-arm.c >> index 1c94b88..0682ed0 100644 >> --- a/drivers/cpuidle/cpuidle-arm.c >> +++ b/drivers/cpuidle/cpuidle-arm.c >> @@ -17,11 +17,14 @@ >> #include <linux/kernel.h> >> #include <linux/module.h> >> #include <linux/of.h> >> +#include <linux/slab.h> >> >> #include <asm/cpuidle.h> >> >> #include "dt_idle_states.h" >> >> +static DEFINE_PER_CPU(struct cpuidle_device, *cpuidle_arm_dev); > > Is a per-cpu variable needed? There seems to be no per-cpu access, other > than at init. :) Actually it is the cpuidle framework which is using the per cpu variable. >> + >> /* >> * arm_enter_idle_state - Programs CPU to enter the specified state >> * >> @@ -94,6 +97,7 @@ static int __init arm_idle_init(void) >> { >> int cpu, ret; >> struct cpuidle_driver *drv = &arm_idle_driver; >> + struct cpuidle_device *dev; >> >> /* >> * Initialize idle states data, starting at index 1. >> @@ -105,18 +109,55 @@ static int __init arm_idle_init(void) >> if (ret <= 0) >> return ret ? : -ENODEV; >> >> + ret = cpuidle_register_driver(drv); >> + if (ret) { >> + pr_err("Failed to register cpuidle driver\n"); >> + return ret; >> + } >> + >> /* >> * Call arch CPU operations in order to initialize >> * idle states suspend back-end specific data >> */ >> for_each_possible_cpu(cpu) { >> ret = arm_cpuidle_init(cpu); >> + >> + /* This cpu does not support any idle states */ >> + if (ret == -ENOSYS) >> + continue; >> + >> if (ret) { >> pr_err("CPU %d failed to init idle CPU ops\n", cpu); >> - return ret; >> + goto out_fail; >> + } >> + >> + dev = kzalloc(sizeof(*dev), GFP_KERNEL); >> + if (!dev) { >> + pr_err("Failed to allocate cpuidle device\n"); >> + goto out_fail; >> + } >> + >> + dev->cpu = cpu; >> + per_cpu(cpuidle_arm_dev, cpu) = dev; >> + >> + ret = cpuidle_register_device(dev); >> + if (ret) { >> + pr_err("Failed to register cpuidle device for CPU %d\n", >> + cpu); >> + kfree(dev); >> + goto out_fail; >> } >> } >> +out: >> + return ret; >> >> - return cpuidle_register(drv, NULL); >> +out_fail: >> + for (cpu--; cpu <= 0; cpu--) { >> + dev = per_cpu(cpuidle_arm_dev, cpu); >> + cpuidle_unregister_device(dev); >> + kfree(dev); >> + } >> + cpuidle_unregister_driver(drv); >> + goto out; >> } >> device_initcall(arm_idle_init); >> -- >> 1.9.1 >>
On Thu, Mar 19 2015 at 09:33 -0600, Daniel Lezcano wrote: >On 03/19/2015 04:31 PM, Lina Iyer wrote: >>On Wed, Mar 18 2015 at 12:46 -0600, Daniel Lezcano wrote: >>>Some architectures have some cpus which does not support idle states. >>> >>>Let the underlying low level code to return -ENOSYS when it is not >>>possible to set an idle state. >>> >>>Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>--- >>>drivers/cpuidle/cpuidle-arm.c | 45 >>>+++++++++++++++++++++++++++++++++++++++++-- >>>1 file changed, 43 insertions(+), 2 deletions(-) >>> >>>diff --git a/drivers/cpuidle/cpuidle-arm.c >>>b/drivers/cpuidle/cpuidle-arm.c >>>index 1c94b88..0682ed0 100644 >>>--- a/drivers/cpuidle/cpuidle-arm.c >>>+++ b/drivers/cpuidle/cpuidle-arm.c >>>@@ -17,11 +17,14 @@ >>>#include <linux/kernel.h> >>>#include <linux/module.h> >>>#include <linux/of.h> >>>+#include <linux/slab.h> >>> >>>#include <asm/cpuidle.h> >>> >>>#include "dt_idle_states.h" >>> >>>+static DEFINE_PER_CPU(struct cpuidle_device, *cpuidle_arm_dev); >> >>Is a per-cpu variable needed? There seems to be no per-cpu access, other >>than at init. > >:) > >Actually it is the cpuidle framework which is using the per cpu variable. > But this is static pointer, I can see that struct cpuidle_device * being used by cpuidle framework. It gets stored here? DEFINE_PER_CPU(struct cpuidle_device *, cpuidle_devices); But we dont need to save it here, for in this file, there is no per-cpu access, other than when the registration fails. >>>+ >>>/* >>> * arm_enter_idle_state - Programs CPU to enter the specified state >>> * >>>@@ -94,6 +97,7 @@ static int __init arm_idle_init(void) >>>{ >>> int cpu, ret; >>> struct cpuidle_driver *drv = &arm_idle_driver; >>>+ struct cpuidle_device *dev; >>> >>> /* >>> * Initialize idle states data, starting at index 1. >>>@@ -105,18 +109,55 @@ static int __init arm_idle_init(void) >>> if (ret <= 0) >>> return ret ? : -ENODEV; >>> >>>+ ret = cpuidle_register_driver(drv); >>>+ if (ret) { >>>+ pr_err("Failed to register cpuidle driver\n"); >>>+ return ret; >>>+ } >>>+ >>> /* >>> * Call arch CPU operations in order to initialize >>> * idle states suspend back-end specific data >>> */ >>> for_each_possible_cpu(cpu) { >>> ret = arm_cpuidle_init(cpu); >>>+ >>>+ /* This cpu does not support any idle states */ >>>+ if (ret == -ENOSYS) >>>+ continue; >>>+ >>> if (ret) { >>> pr_err("CPU %d failed to init idle CPU ops\n", cpu); >>>- return ret; >>>+ goto out_fail; >>>+ } >>>+ >>>+ dev = kzalloc(sizeof(*dev), GFP_KERNEL); >>>+ if (!dev) { >>>+ pr_err("Failed to allocate cpuidle device\n"); >>>+ goto out_fail; >>>+ } >>>+ >>>+ dev->cpu = cpu; >>>+ per_cpu(cpuidle_arm_dev, cpu) = dev; >>>+ >>>+ ret = cpuidle_register_device(dev); >>>+ if (ret) { >>>+ pr_err("Failed to register cpuidle device for CPU %d\n", >>>+ cpu); >>>+ kfree(dev); >>>+ goto out_fail; >>> } >>> } >>>+out: >>>+ return ret; >>> >>>- return cpuidle_register(drv, NULL); >>>+out_fail: >>>+ for (cpu--; cpu <= 0; cpu--) { >>>+ dev = per_cpu(cpuidle_arm_dev, cpu); >>>+ cpuidle_unregister_device(dev); >>>+ kfree(dev); >>>+ } >>>+ cpuidle_unregister_driver(drv); >>>+ goto out; >>>} >>>device_initcall(arm_idle_init); >>>-- >>>1.9.1 >>> > > >-- > <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-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/19/2015 04:42 PM, Lina Iyer wrote: > On Thu, Mar 19 2015 at 09:33 -0600, Daniel Lezcano wrote: >> On 03/19/2015 04:31 PM, Lina Iyer wrote: >>> On Wed, Mar 18 2015 at 12:46 -0600, Daniel Lezcano wrote: >>>> Some architectures have some cpus which does not support idle states. >>>> >>>> Let the underlying low level code to return -ENOSYS when it is not >>>> possible to set an idle state. >>>> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> --- >>>> drivers/cpuidle/cpuidle-arm.c | 45 >>>> +++++++++++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 43 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/cpuidle/cpuidle-arm.c >>>> b/drivers/cpuidle/cpuidle-arm.c >>>> index 1c94b88..0682ed0 100644 >>>> --- a/drivers/cpuidle/cpuidle-arm.c >>>> +++ b/drivers/cpuidle/cpuidle-arm.c >>>> @@ -17,11 +17,14 @@ >>>> #include <linux/kernel.h> >>>> #include <linux/module.h> >>>> #include <linux/of.h> >>>> +#include <linux/slab.h> >>>> >>>> #include <asm/cpuidle.h> >>>> >>>> #include "dt_idle_states.h" >>>> >>>> +static DEFINE_PER_CPU(struct cpuidle_device, *cpuidle_arm_dev); >>> >>> Is a per-cpu variable needed? There seems to be no per-cpu access, other >>> than at init. >> >> :) >> >> Actually it is the cpuidle framework which is using the per cpu variable. >> > > But this is static pointer, I can see that struct cpuidle_device * being > used by cpuidle framework. > > It gets stored here? > > DEFINE_PER_CPU(struct cpuidle_device *, cpuidle_devices); > > But we dont need to save it here, for in this file, there is no per-cpu > access, other than when the registration fails. Indeed, that is true. There is no need of these cpuidle variables. Furthermore, we can access the cpuidle_devices directly as they are exported in cpuidle.h. I will fix it. Thanks -- Daniel >>>> + >>>> /* >>>> * arm_enter_idle_state - Programs CPU to enter the specified state >>>> * >>>> @@ -94,6 +97,7 @@ static int __init arm_idle_init(void) >>>> { >>>> int cpu, ret; >>>> struct cpuidle_driver *drv = &arm_idle_driver; >>>> + struct cpuidle_device *dev; >>>> >>>> /* >>>> * Initialize idle states data, starting at index 1. >>>> @@ -105,18 +109,55 @@ static int __init arm_idle_init(void) >>>> if (ret <= 0) >>>> return ret ? : -ENODEV; >>>> >>>> + ret = cpuidle_register_driver(drv); >>>> + if (ret) { >>>> + pr_err("Failed to register cpuidle driver\n"); >>>> + return ret; >>>> + } >>>> + >>>> /* >>>> * Call arch CPU operations in order to initialize >>>> * idle states suspend back-end specific data >>>> */ >>>> for_each_possible_cpu(cpu) { >>>> ret = arm_cpuidle_init(cpu); >>>> + >>>> + /* This cpu does not support any idle states */ >>>> + if (ret == -ENOSYS) >>>> + continue; >>>> + >>>> if (ret) { >>>> pr_err("CPU %d failed to init idle CPU ops\n", cpu); >>>> - return ret; >>>> + goto out_fail; >>>> + } >>>> + >>>> + dev = kzalloc(sizeof(*dev), GFP_KERNEL); >>>> + if (!dev) { >>>> + pr_err("Failed to allocate cpuidle device\n"); >>>> + goto out_fail; >>>> + } >>>> + >>>> + dev->cpu = cpu; >>>> + per_cpu(cpuidle_arm_dev, cpu) = dev; >>>> + >>>> + ret = cpuidle_register_device(dev); >>>> + if (ret) { >>>> + pr_err("Failed to register cpuidle device for CPU %d\n", >>>> + cpu); >>>> + kfree(dev); >>>> + goto out_fail; >>>> } >>>> } >>>> +out: >>>> + return ret; >>>> >>>> - return cpuidle_register(drv, NULL); >>>> +out_fail: >>>> + for (cpu--; cpu <= 0; cpu--) { >>>> + dev = per_cpu(cpuidle_arm_dev, cpu); >>>> + cpuidle_unregister_device(dev); >>>> + kfree(dev); >>>> + } >>>> + cpuidle_unregister_driver(drv); >>>> + goto out; >>>> } >>>> device_initcall(arm_idle_init); >>>> -- >>>> 1.9.1 >>>> >> >> >> -- >> <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 >>
diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c index 1c94b88..0682ed0 100644 --- a/drivers/cpuidle/cpuidle-arm.c +++ b/drivers/cpuidle/cpuidle-arm.c @@ -17,11 +17,14 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/slab.h> #include <asm/cpuidle.h> #include "dt_idle_states.h" +static DEFINE_PER_CPU(struct cpuidle_device, *cpuidle_arm_dev); + /* * arm_enter_idle_state - Programs CPU to enter the specified state * @@ -94,6 +97,7 @@ static int __init arm_idle_init(void) { int cpu, ret; struct cpuidle_driver *drv = &arm_idle_driver; + struct cpuidle_device *dev; /* * Initialize idle states data, starting at index 1. @@ -105,18 +109,55 @@ static int __init arm_idle_init(void) if (ret <= 0) return ret ? : -ENODEV; + ret = cpuidle_register_driver(drv); + if (ret) { + pr_err("Failed to register cpuidle driver\n"); + return ret; + } + /* * Call arch CPU operations in order to initialize * idle states suspend back-end specific data */ for_each_possible_cpu(cpu) { ret = arm_cpuidle_init(cpu); + + /* This cpu does not support any idle states */ + if (ret == -ENOSYS) + continue; + if (ret) { pr_err("CPU %d failed to init idle CPU ops\n", cpu); - return ret; + goto out_fail; + } + + dev = kzalloc(sizeof(*dev), GFP_KERNEL); + if (!dev) { + pr_err("Failed to allocate cpuidle device\n"); + goto out_fail; + } + + dev->cpu = cpu; + per_cpu(cpuidle_arm_dev, cpu) = dev; + + ret = cpuidle_register_device(dev); + if (ret) { + pr_err("Failed to register cpuidle device for CPU %d\n", + cpu); + kfree(dev); + goto out_fail; } } +out: + return ret; - return cpuidle_register(drv, NULL); +out_fail: + for (cpu--; cpu <= 0; cpu--) { + dev = per_cpu(cpuidle_arm_dev, cpu); + cpuidle_unregister_device(dev); + kfree(dev); + } + cpuidle_unregister_driver(drv); + goto out; } device_initcall(arm_idle_init);
Some architectures have some cpus which does not support idle states. Let the underlying low level code to return -ENOSYS when it is not possible to set an idle state. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/cpuidle/cpuidle-arm.c | 45 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-)