* [PATCH v2 1/2] ARM: cpuidle: Correct driver unregistration if init fails
@ 2017-10-10 5:47 Leo Yan
2017-10-10 5:47 ` [PATCH v2 2/2] ARM: cpuidle: Refactor rollback operations " Leo Yan
2017-10-10 22:00 ` [PATCH v2 1/2] ARM: cpuidle: Correct driver unregistration " Rafael J. Wysocki
0 siblings, 2 replies; 5+ messages in thread
From: Leo Yan @ 2017-10-10 5:47 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, linux-pm, linux-kernel; +Cc: Leo Yan
If cpuidle init fails, the code misses to unregister the driver for
current CPU. Furthermore, we also need to rollback to cancel all
previous CPUs registration; but the code retrieves driver handler by
using function cpuidle_get_driver(), this function returns back
current CPU driver handler but not previous CPU's handler, which leads
to the failure handling code cannot unregister previous CPUs driver.
This commit fixes two mentioned issues, it adds error handling path
'goto out_unregister_drv' for current CPU driver unregistration; and
it is to replace cpuidle_get_driver() with cpuidle_get_cpu_driver(),
the later function can retrieve driver handler for previous CPUs
according to the CPU device handler so can unregister the driver
properly.
This patch also adds extra error handling paths 'goto out_kfree_dev'
and 'goto out_kfree_drv' and adjusts the freeing sentences for previous
CPUs; so make the code more readable for freeing 'dev' and 'drv'
structures.
Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
---
drivers/cpuidle/cpuidle-arm.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index 52a7505..f47c545 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -104,13 +104,13 @@ static int __init arm_idle_init(void)
ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
if (ret <= 0) {
ret = ret ? : -ENODEV;
- goto init_fail;
+ goto out_kfree_drv;
}
ret = cpuidle_register_driver(drv);
if (ret) {
pr_err("Failed to register cpuidle driver\n");
- goto init_fail;
+ goto out_kfree_drv;
}
/*
@@ -128,14 +128,14 @@ static int __init arm_idle_init(void)
if (ret) {
pr_err("CPU %d failed to init idle CPU ops\n", cpu);
- goto out_fail;
+ goto out_unregister_drv;
}
dev = kzalloc(sizeof(*dev), GFP_KERNEL);
if (!dev) {
pr_err("Failed to allocate cpuidle device\n");
ret = -ENOMEM;
- goto out_fail;
+ goto out_unregister_drv;
}
dev->cpu = cpu;
@@ -143,21 +143,25 @@ static int __init arm_idle_init(void)
if (ret) {
pr_err("Failed to register cpuidle device for CPU %d\n",
cpu);
- kfree(dev);
- goto out_fail;
+ goto out_kfree_dev;
}
}
return 0;
-init_fail:
+
+out_kfree_dev:
+ kfree(dev);
+out_unregister_drv:
+ cpuidle_unregister_driver(drv);
+out_kfree_drv:
kfree(drv);
out_fail:
while (--cpu >= 0) {
dev = per_cpu(cpuidle_devices, cpu);
+ drv = cpuidle_get_cpu_driver(dev);
cpuidle_unregister_device(dev);
- kfree(dev);
- drv = cpuidle_get_driver();
cpuidle_unregister_driver(drv);
+ kfree(dev);
kfree(drv);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v2 2/2] ARM: cpuidle: Refactor rollback operations if init fails
2017-10-10 5:47 [PATCH v2 1/2] ARM: cpuidle: Correct driver unregistration if init fails Leo Yan
@ 2017-10-10 5:47 ` Leo Yan
2017-10-11 9:49 ` Daniel Lezcano
2017-10-10 22:00 ` [PATCH v2 1/2] ARM: cpuidle: Correct driver unregistration " Rafael J. Wysocki
1 sibling, 1 reply; 5+ messages in thread
From: Leo Yan @ 2017-10-10 5:47 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, linux-pm, linux-kernel; +Cc: Leo Yan
If init fails, we need execute two levels rollback operations: the first
level is for the failed CPU rollback operations, the second level is to
iterate all succeeded CPUs to cancel their registration; currently the
code uses one function to finish these two levels rollback operations.
This commit is to refactor rollback operations, so it adds a new
function arm_idle_init_cpu() to encapsulate one specified CPU driver
registration and rollback the first level operations; and use function
arm_idle_init() to iterate all CPUs and finish the second level's
rollback operations.
Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
drivers/cpuidle/cpuidle-arm.c | 145 ++++++++++++++++++++++++------------------
1 file changed, 82 insertions(+), 63 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index f47c545..ddee1b6 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -72,79 +72,74 @@ static const struct of_device_id arm_idle_state_match[] __initconst = {
};
/*
- * arm_idle_init
+ * arm_idle_init_cpu
*
* Registers the arm specific cpuidle driver with the cpuidle
* framework. It relies on core code to parse the idle states
* and initialize them using driver data structures accordingly.
*/
-static int __init arm_idle_init(void)
+static int __init arm_idle_init_cpu(int cpu)
{
- int cpu, ret;
+ int ret;
struct cpuidle_driver *drv;
struct cpuidle_device *dev;
- for_each_possible_cpu(cpu) {
+ drv = kmemdup(&arm_idle_driver, sizeof(*drv), GFP_KERNEL);
+ if (!drv)
+ return -ENOMEM;
- drv = kmemdup(&arm_idle_driver, sizeof(*drv), GFP_KERNEL);
- if (!drv) {
- ret = -ENOMEM;
- goto out_fail;
- }
-
- drv->cpumask = (struct cpumask *)cpumask_of(cpu);
-
- /*
- * Initialize idle states data, starting at index 1. This
- * driver is DT only, if no DT idle states are detected (ret
- * == 0) let the driver initialization fail accordingly since
- * there is no reason to initialize the idle driver if only
- * wfi is supported.
- */
- ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
- if (ret <= 0) {
- ret = ret ? : -ENODEV;
- goto out_kfree_drv;
- }
-
- ret = cpuidle_register_driver(drv);
- if (ret) {
- pr_err("Failed to register cpuidle driver\n");
- goto out_kfree_drv;
- }
-
- /*
- * Call arch CPU operations in order to initialize
- * idle states suspend back-end specific data
- */
- ret = arm_cpuidle_init(cpu);
-
- /*
- * Skip the cpuidle device initialization if the reported
- * failure is a HW misconfiguration/breakage (-ENXIO).
- */
- if (ret == -ENXIO)
- continue;
-
- if (ret) {
- pr_err("CPU %d failed to init idle CPU ops\n", cpu);
- goto out_unregister_drv;
- }
-
- dev = kzalloc(sizeof(*dev), GFP_KERNEL);
- if (!dev) {
- pr_err("Failed to allocate cpuidle device\n");
- ret = -ENOMEM;
- goto out_unregister_drv;
- }
- dev->cpu = cpu;
-
- ret = cpuidle_register_device(dev);
- if (ret) {
- pr_err("Failed to register cpuidle device for CPU %d\n",
- cpu);
- goto out_kfree_dev;
- }
+ drv->cpumask = (struct cpumask *)cpumask_of(cpu);
+
+ /*
+ * Initialize idle states data, starting at index 1. This
+ * driver is DT only, if no DT idle states are detected (ret
+ * == 0) let the driver initialization fail accordingly since
+ * there is no reason to initialize the idle driver if only
+ * wfi is supported.
+ */
+ ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
+ if (ret <= 0) {
+ ret = ret ? : -ENODEV;
+ goto out_kfree_drv;
+ }
+
+ ret = cpuidle_register_driver(drv);
+ if (ret) {
+ pr_err("Failed to register cpuidle driver\n");
+ goto out_kfree_drv;
+ }
+
+ /*
+ * Call arch CPU operations in order to initialize
+ * idle states suspend back-end specific data
+ */
+ ret = arm_cpuidle_init(cpu);
+
+ /*
+ * Skip the cpuidle device initialization if the reported
+ * failure is a HW misconfiguration/breakage (-ENXIO).
+ */
+ if (ret == -ENXIO)
+ return 0;
+
+ if (ret) {
+ pr_err("CPU %d failed to init idle CPU ops\n", cpu);
+ goto out_unregister_drv;
+ }
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ pr_err("Failed to allocate cpuidle device\n");
+ ret = -ENOMEM;
+ goto out_unregister_drv;
+ }
+ dev->cpu = cpu;
+
+ ret = cpuidle_register_device(dev);
+ if (ret) {
+ pr_err("Failed to register cpuidle device for CPU %d\n",
+ cpu);
+ goto out_kfree_dev;
}
return 0;
@@ -155,6 +150,30 @@ static int __init arm_idle_init(void)
cpuidle_unregister_driver(drv);
out_kfree_drv:
kfree(drv);
+ return ret;
+}
+
+/*
+ * arm_idle_init - Initializes arm cpuidle driver
+ *
+ * Initializes arm cpuidle driver for all CPUs, if any CPU fails
+ * to register cpuidle driver then rollback to cancel all CPUs
+ * registeration.
+ */
+static int __init arm_idle_init(void)
+{
+ int cpu, ret;
+ struct cpuidle_driver *drv;
+ struct cpuidle_device *dev;
+
+ for_each_possible_cpu(cpu) {
+ ret = arm_idle_init_cpu(cpu);
+ if (ret)
+ goto out_fail;
+ }
+
+ return 0;
+
out_fail:
while (--cpu >= 0) {
dev = per_cpu(cpuidle_devices, cpu);
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2 1/2] ARM: cpuidle: Correct driver unregistration if init fails
2017-10-10 5:47 [PATCH v2 1/2] ARM: cpuidle: Correct driver unregistration if init fails Leo Yan
2017-10-10 5:47 ` [PATCH v2 2/2] ARM: cpuidle: Refactor rollback operations " Leo Yan
@ 2017-10-10 22:00 ` Rafael J. Wysocki
2017-10-11 9:48 ` Daniel Lezcano
1 sibling, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2017-10-10 22:00 UTC (permalink / raw)
To: Leo Yan, Daniel Lezcano
Cc: Rafael J. Wysocki, Linux PM, Linux Kernel Mailing List
On Tue, Oct 10, 2017 at 7:47 AM, Leo Yan <leo.yan@linaro.org> wrote:
> If cpuidle init fails, the code misses to unregister the driver for
> current CPU. Furthermore, we also need to rollback to cancel all
> previous CPUs registration; but the code retrieves driver handler by
> using function cpuidle_get_driver(), this function returns back
> current CPU driver handler but not previous CPU's handler, which leads
> to the failure handling code cannot unregister previous CPUs driver.
>
> This commit fixes two mentioned issues, it adds error handling path
> 'goto out_unregister_drv' for current CPU driver unregistration; and
> it is to replace cpuidle_get_driver() with cpuidle_get_cpu_driver(),
> the later function can retrieve driver handler for previous CPUs
> according to the CPU device handler so can unregister the driver
> properly.
>
> This patch also adds extra error handling paths 'goto out_kfree_dev'
> and 'goto out_kfree_drv' and adjusts the freeing sentences for previous
> CPUs; so make the code more readable for freeing 'dev' and 'drv'
> structures.
>
> Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
Daniel, any comments here and on the [2/2]?
> ---
> drivers/cpuidle/cpuidle-arm.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> index 52a7505..f47c545 100644
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -104,13 +104,13 @@ static int __init arm_idle_init(void)
> ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
> if (ret <= 0) {
> ret = ret ? : -ENODEV;
> - goto init_fail;
> + goto out_kfree_drv;
> }
>
> ret = cpuidle_register_driver(drv);
> if (ret) {
> pr_err("Failed to register cpuidle driver\n");
> - goto init_fail;
> + goto out_kfree_drv;
> }
>
> /*
> @@ -128,14 +128,14 @@ static int __init arm_idle_init(void)
>
> if (ret) {
> pr_err("CPU %d failed to init idle CPU ops\n", cpu);
> - goto out_fail;
> + goto out_unregister_drv;
> }
>
> dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> if (!dev) {
> pr_err("Failed to allocate cpuidle device\n");
> ret = -ENOMEM;
> - goto out_fail;
> + goto out_unregister_drv;
> }
> dev->cpu = cpu;
>
> @@ -143,21 +143,25 @@ static int __init arm_idle_init(void)
> if (ret) {
> pr_err("Failed to register cpuidle device for CPU %d\n",
> cpu);
> - kfree(dev);
> - goto out_fail;
> + goto out_kfree_dev;
> }
> }
>
> return 0;
> -init_fail:
> +
> +out_kfree_dev:
> + kfree(dev);
> +out_unregister_drv:
> + cpuidle_unregister_driver(drv);
> +out_kfree_drv:
> kfree(drv);
> out_fail:
> while (--cpu >= 0) {
> dev = per_cpu(cpuidle_devices, cpu);
> + drv = cpuidle_get_cpu_driver(dev);
> cpuidle_unregister_device(dev);
> - kfree(dev);
> - drv = cpuidle_get_driver();
> cpuidle_unregister_driver(drv);
> + kfree(dev);
> kfree(drv);
> }
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2 1/2] ARM: cpuidle: Correct driver unregistration if init fails
2017-10-10 22:00 ` [PATCH v2 1/2] ARM: cpuidle: Correct driver unregistration " Rafael J. Wysocki
@ 2017-10-11 9:48 ` Daniel Lezcano
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Lezcano @ 2017-10-11 9:48 UTC (permalink / raw)
To: Rafael J. Wysocki, Leo Yan
Cc: Rafael J. Wysocki, Linux PM, Linux Kernel Mailing List
On 11/10/2017 00:00, Rafael J. Wysocki wrote:
> On Tue, Oct 10, 2017 at 7:47 AM, Leo Yan <leo.yan@linaro.org> wrote:
>> If cpuidle init fails, the code misses to unregister the driver for
>> current CPU. Furthermore, we also need to rollback to cancel all
>> previous CPUs registration; but the code retrieves driver handler by
>> using function cpuidle_get_driver(), this function returns back
>> current CPU driver handler but not previous CPU's handler, which leads
>> to the failure handling code cannot unregister previous CPUs driver.
>>
>> This commit fixes two mentioned issues, it adds error handling path
>> 'goto out_unregister_drv' for current CPU driver unregistration; and
>> it is to replace cpuidle_get_driver() with cpuidle_get_cpu_driver(),
>> the later function can retrieve driver handler for previous CPUs
>> according to the CPU device handler so can unregister the driver
>> properly.
>>
>> This patch also adds extra error handling paths 'goto out_kfree_dev'
>> and 'goto out_kfree_drv' and adjusts the freeing sentences for previous
>> CPUs; so make the code more readable for freeing 'dev' and 'drv'
>> structures.
>>
>> Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
>
> Daniel, any comments here and on the [2/2]?
Hi Rafael,
I'm ok with both.
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
--
<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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-10-11 9:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-10 5:47 [PATCH v2 1/2] ARM: cpuidle: Correct driver unregistration if init fails Leo Yan
2017-10-10 5:47 ` [PATCH v2 2/2] ARM: cpuidle: Refactor rollback operations " Leo Yan
2017-10-11 9:49 ` Daniel Lezcano
2017-10-10 22:00 ` [PATCH v2 1/2] ARM: cpuidle: Correct driver unregistration " Rafael J. Wysocki
2017-10-11 9:48 ` Daniel Lezcano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).