Re: [RFC PATCH v15 01/11] ARM: cpuidle: Register per cpuidle device
On Tue, Mar 10 2015 at 04:37 -0600, Russell King - ARM Linux wrote:
On Mon, Mar 09, 2015 at 09:16:36AM -0600, Lina Iyer wrote:
@@ -105,18 +109,46 @@ static int __init arm_idle_init(void)
if (ret <= 0)
return ret ? : -ENODEV;
+
A bit better formatting would be nice - you don't need the extra blank
line here.
+ 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) {
+
This blank line is not necessary either.
ret = arm_cpuidle_init(cpu);
However, a blank line here would be a good thing.
Sure.
+ /*
+* This cpu does not support any idle states
+*/
Also, formatting this as /* This cpu does not support any idle states */ is
acceptable too, and doesn't waste as many lines.
Will fix.
+ if (ret == -ENOSYS)
+ continue;
+
if (ret) {
pr_err("CPU %d failed to init idle CPU ops\n", cpu);
return ret;
}
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev)
+ return -ENOMEM;
+
+ dev->cpu = cpu;
+ ret = cpuidle_register_device(dev);
+ if (ret) {
+ pr_err("Failed to register cpuidle device for CPU %d\n",
+ cpu);
+ return ret;
It looks like we leak the 'dev' allocation here.
True, I will amend that.
Also, other error paths, it looks like we leave the previously registered
cpuidle devices in place. That may be acceptable if the intention is to
initialise as many CPUs as possible - but we then miss the
cpuidle_register() call below, which seems to make the registered devices
useless. It's a little inconsistent.
Also, it's useful to report why something fails - printing the error code
can help debugging if it isn't already printed elsewhere.
Fair point.
+ }
+
+ per_cpu(cpuidle_arm_dev, cpu) = dev;
}
- return cpuidle_register(drv, NULL);
+ return 0;
}
device_initcall(arm_idle_init);
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v15 01/11] ARM: cpuidle: Register per cpuidle device
On Mon, Mar 09, 2015 at 09:16:36AM -0600, Lina Iyer wrote:
> @@ -105,18 +109,46 @@ static int __init arm_idle_init(void)
> if (ret <= 0)
> return ret ? : -ENODEV;
>
> +
A bit better formatting would be nice - you don't need the extra blank
line here.
> + 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) {
> +
This blank line is not necessary either.
> ret = arm_cpuidle_init(cpu);
However, a blank line here would be a good thing.
> + /*
> + * This cpu does not support any idle states
> + */
Also, formatting this as /* This cpu does not support any idle states */ is
acceptable too, and doesn't waste as many lines.
> + if (ret == -ENOSYS)
> + continue;
> +
> if (ret) {
> pr_err("CPU %d failed to init idle CPU ops\n", cpu);
> return ret;
> }
> +
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return -ENOMEM;
> +
> + dev->cpu = cpu;
> + ret = cpuidle_register_device(dev);
> + if (ret) {
> + pr_err("Failed to register cpuidle device for CPU %d\n",
> +cpu);
> + return ret;
It looks like we leak the 'dev' allocation here.
Also, other error paths, it looks like we leave the previously registered
cpuidle devices in place. That may be acceptable if the intention is to
initialise as many CPUs as possible - but we then miss the
cpuidle_register() call below, which seems to make the registered devices
useless. It's a little inconsistent.
Also, it's useful to report why something fails - printing the error code
can help debugging if it isn't already printed elsewhere.
> + }
> +
> + per_cpu(cpuidle_arm_dev, cpu) = dev;
> }
>
> - return cpuidle_register(drv, NULL);
> + return 0;
> }
> device_initcall(arm_idle_init);
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v15 01/11] ARM: cpuidle: Register per cpuidle device
On Mon, Mar 09, 2015 at 03:40:06PM -0600, Lina Iyer 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;
> >>}
> >>+
> >>+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> >
> >devm_kzalloc? Otherwise, failures could lead to lost memory.
> >
> I dont have a local device to work with for allocation. May be I can get
> the cpu device node and the device therefore for allocation. Thoughts?
Ah, well the cpu device node could be used. My only concern is that dev cannot
be referenced after the cpu is removed.
> >>+ if (!dev)
> >>+ return -ENOMEM;
> >>+
> >>+ dev->cpu = cpu;
> >>+ ret = cpuidle_register_device(dev);
> >>+ if (ret) {
> >>+ pr_err("Failed to register cpuidle device for CPU %d\n",
> >>+ cpu);
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v15 01/11] ARM: cpuidle: Register per cpuidle device
On Mon, Mar 09 2015 at 15:29 -0600, Andy Gross wrote:
On Mon, Mar 09, 2015 at 09:16:36AM -0600, Lina Iyer wrote:
From: Daniel Lezcano
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
Signed-off-by: Lina Iyer
[Minor clean ups and fixes of the per-cpu variable]
---
drivers/cpuidle/cpuidle-arm.c | 34 +-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index 1c94b88..f7cdb73 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -17,11 +17,14 @@
#include
#include
#include
+#include
#include
#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
*
@@ -93,6 +96,7 @@ static const struct of_device_id arm_idle_state_match[]
__initconst = {
static int __init arm_idle_init(void)
{
int cpu, ret;
+ struct cpuidle_device *dev;
struct cpuidle_driver *drv = &arm_idle_driver;
/*
@@ -105,18 +109,46 @@ 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;
}
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
devm_kzalloc? Otherwise, failures could lead to lost memory.
I dont have a local device to work with for allocation. May be I can get
the cpu device node and the device therefore for allocation. Thoughts?
+ if (!dev)
+ return -ENOMEM;
+
+ dev->cpu = cpu;
+ ret = cpuidle_register_device(dev);
+ if (ret) {
+ pr_err("Failed to register cpuidle device for CPU %d\n",
+ cpu);
+ return ret;
+ }
+
+ per_cpu(cpuidle_arm_dev, cpu) = dev;
}
- return cpuidle_register(drv, NULL);
+ return 0;
}
device_initcall(arm_idle_init);
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v15 01/11] ARM: cpuidle: Register per cpuidle device
On Mon, Mar 09, 2015 at 09:16:36AM -0600, Lina Iyer wrote:
> From: Daniel Lezcano
>
> 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
> Signed-off-by: Lina Iyer
> [Minor clean ups and fixes of the per-cpu variable]
> ---
> drivers/cpuidle/cpuidle-arm.c | 34 +-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> index 1c94b88..f7cdb73 100644
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -17,11 +17,14 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> #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
> *
> @@ -93,6 +96,7 @@ static const struct of_device_id arm_idle_state_match[]
> __initconst = {
> static int __init arm_idle_init(void)
> {
> int cpu, ret;
> + struct cpuidle_device *dev;
> struct cpuidle_driver *drv = &arm_idle_driver;
>
> /*
> @@ -105,18 +109,46 @@ 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;
> }
> +
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
devm_kzalloc? Otherwise, failures could lead to lost memory.
> + if (!dev)
> + return -ENOMEM;
> +
> + dev->cpu = cpu;
> + ret = cpuidle_register_device(dev);
> + if (ret) {
> + pr_err("Failed to register cpuidle device for CPU %d\n",
> +cpu);
> + return ret;
> + }
> +
> + per_cpu(cpuidle_arm_dev, cpu) = dev;
> }
>
> - return cpuidle_register(drv, NULL);
> + return 0;
> }
> device_initcall(arm_idle_init);
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH v15 01/11] ARM: cpuidle: Register per cpuidle device
From: Daniel Lezcano
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
Signed-off-by: Lina Iyer
[Minor clean ups and fixes of the per-cpu variable]
---
drivers/cpuidle/cpuidle-arm.c | 34 +-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index 1c94b88..f7cdb73 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -17,11 +17,14 @@
#include
#include
#include
+#include
#include
#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
*
@@ -93,6 +96,7 @@ static const struct of_device_id arm_idle_state_match[]
__initconst = {
static int __init arm_idle_init(void)
{
int cpu, ret;
+ struct cpuidle_device *dev;
struct cpuidle_driver *drv = &arm_idle_driver;
/*
@@ -105,18 +109,46 @@ 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;
}
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev)
+ return -ENOMEM;
+
+ dev->cpu = cpu;
+ ret = cpuidle_register_device(dev);
+ if (ret) {
+ pr_err("Failed to register cpuidle device for CPU %d\n",
+ cpu);
+ return ret;
+ }
+
+ per_cpu(cpuidle_arm_dev, cpu) = dev;
}
- return cpuidle_register(drv, NULL);
+ return 0;
}
device_initcall(arm_idle_init);
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
