* [PATCH 2/2] ARM: cpuidle: replace cpuidle_get_driver with cpuidle_get_cpu_driver
2017-09-04 6:52 [PATCH 1/2] ARM: cpuidle: refine failure handling in init flow Leo Yan
@ 2017-09-04 6:52 ` Leo Yan
2017-10-07 12:12 ` Leo Yan
2017-09-04 7:00 ` [PATCH 1/2] ARM: cpuidle: refine failure handling in init flow Leo Yan
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Leo Yan @ 2017-09-04 6:52 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, linux-pm, linux-kernel
Cc: Leo Yan, Stefan Wahren
commit d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
supports multiple CPU idle driver so every CPU has its own driver. When
the initialization fails, the failure handling releases the resources
for every previous CPU; so it needs to retrieve every CPU device and
driver handler and unregister them. But the function
cpuidle_get_driver() can only return current CPU driver handler but not
the iterated CPU driver handler, so it cannot release resource properly.
This patch is to replace cpuidle_get_driver() with
cpuidle_get_cpu_driver(), every CPU has its own device handler so this
function can get back correct driver handler for the CPU according to
the CPU device handler. By using this CPU driver handler we can release
resource properly.
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
---
drivers/cpuidle/cpuidle-arm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index f419f6a..ef34780 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -161,9 +161,9 @@ static int __init arm_idle_init(void)
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(drv);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] ARM: cpuidle: replace cpuidle_get_driver with cpuidle_get_cpu_driver
2017-09-04 6:52 ` [PATCH 2/2] ARM: cpuidle: replace cpuidle_get_driver with cpuidle_get_cpu_driver Leo Yan
@ 2017-10-07 12:12 ` Leo Yan
0 siblings, 0 replies; 7+ messages in thread
From: Leo Yan @ 2017-10-07 12:12 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, linux-pm, linux-kernel; +Cc: Stefan Wahren
On Mon, Sep 04, 2017 at 02:52:14PM +0800, Leo Yan wrote:
> commit d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
> supports multiple CPU idle driver so every CPU has its own driver. When
> the initialization fails, the failure handling releases the resources
> for every previous CPU; so it needs to retrieve every CPU device and
> driver handler and unregister them. But the function
> cpuidle_get_driver() can only return current CPU driver handler but not
> the iterated CPU driver handler, so it cannot release resource properly.
>
> This patch is to replace cpuidle_get_driver() with
> cpuidle_get_cpu_driver(), every CPU has its own device handler so this
> function can get back correct driver handler for the CPU according to
> the CPU device handler. By using this CPU driver handler we can release
> resource properly.
>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
Gentle ping.
> ---
> drivers/cpuidle/cpuidle-arm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> index f419f6a..ef34780 100644
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -161,9 +161,9 @@ static int __init arm_idle_init(void)
>
> 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(drv);
> }
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] ARM: cpuidle: refine failure handling in init flow
2017-09-04 6:52 [PATCH 1/2] ARM: cpuidle: refine failure handling in init flow Leo Yan
2017-09-04 6:52 ` [PATCH 2/2] ARM: cpuidle: replace cpuidle_get_driver with cpuidle_get_cpu_driver Leo Yan
@ 2017-09-04 7:00 ` Leo Yan
2017-10-07 12:12 ` Leo Yan
2017-10-09 12:04 ` Daniel Lezcano
3 siblings, 0 replies; 7+ messages in thread
From: Leo Yan @ 2017-09-04 7:00 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, linux-pm, linux-kernel; +Cc: Stefan Wahren
Hi Stefan, Daniel,
On Mon, Sep 04, 2017 at 02:52:13PM +0800, Leo Yan wrote:
> After applied Stefan Wahren patch ("ARM: cpuidle: Avoid memleak if init
> fail") there have no memleak issue, but the code is not consistent to
> handle initialization failure between driver registration and device
> registration. And when device registration fails, it misses to
> unregister the driver.
>
> So this patch is to refine failure handling in init flow, it adds two
> 'goto' tags: when register device fails, it goto 'init_dev_fail' tag and
> free 'dev' structure and unregister driver; when register driver fails,
> it goto 'init_drv_fail' tag and free 'drv' structure.
>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
Just want to reminding, this patch is based on Stefan's patch:
https://patchwork.kernel.org/patch/9932833/
First thing is Stefan patch is not merged so I just want to note here
have some dependency with Stefan patch. BTW, if you think we should
merge this patch with Stefan patch, it's fine for me to merge into
Stefan patch.
Thanks for suggetion.
> ---
> drivers/cpuidle/cpuidle-arm.c | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> index 52a7505..f419f6a 100644
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -86,10 +86,13 @@ static int __init arm_idle_init(void)
>
> for_each_possible_cpu(cpu) {
>
> + drv = NULL;
> + dev = NULL;
> +
> drv = kmemdup(&arm_idle_driver, sizeof(*drv), GFP_KERNEL);
> if (!drv) {
> ret = -ENOMEM;
> - goto out_fail;
> + goto init_drv_fail;
> }
>
> drv->cpumask = (struct cpumask *)cpumask_of(cpu);
> @@ -104,13 +107,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 init_drv_fail;
> }
>
> ret = cpuidle_register_driver(drv);
> if (ret) {
> pr_err("Failed to register cpuidle driver\n");
> - goto init_fail;
> + goto init_drv_fail;
> }
>
> /*
> @@ -128,14 +131,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 init_dev_fail;
> }
>
> dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> if (!dev) {
> pr_err("Failed to allocate cpuidle device\n");
> ret = -ENOMEM;
> - goto out_fail;
> + goto init_dev_fail;
> }
> dev->cpu = cpu;
>
> @@ -143,15 +146,19 @@ 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 init_dev_fail;
> }
> }
>
> return 0;
> -init_fail:
> +
> +init_dev_fail:
> + kfree(dev);
> + cpuidle_unregister_driver(drv);
> +
> +init_drv_fail:
> kfree(drv);
> -out_fail:
> +
> while (--cpu >= 0) {
> dev = per_cpu(cpuidle_devices, cpu);
> cpuidle_unregister_device(dev);
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] ARM: cpuidle: refine failure handling in init flow
2017-09-04 6:52 [PATCH 1/2] ARM: cpuidle: refine failure handling in init flow Leo Yan
2017-09-04 6:52 ` [PATCH 2/2] ARM: cpuidle: replace cpuidle_get_driver with cpuidle_get_cpu_driver Leo Yan
2017-09-04 7:00 ` [PATCH 1/2] ARM: cpuidle: refine failure handling in init flow Leo Yan
@ 2017-10-07 12:12 ` Leo Yan
2017-10-09 12:04 ` Daniel Lezcano
3 siblings, 0 replies; 7+ messages in thread
From: Leo Yan @ 2017-10-07 12:12 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, linux-pm, linux-kernel; +Cc: Stefan Wahren
On Mon, Sep 04, 2017 at 02:52:13PM +0800, Leo Yan wrote:
> After applied Stefan Wahren patch ("ARM: cpuidle: Avoid memleak if init
> fail") there have no memleak issue, but the code is not consistent to
> handle initialization failure between driver registration and device
> registration. And when device registration fails, it misses to
> unregister the driver.
>
> So this patch is to refine failure handling in init flow, it adds two
> 'goto' tags: when register device fails, it goto 'init_dev_fail' tag and
> free 'dev' structure and unregister driver; when register driver fails,
> it goto 'init_drv_fail' tag and free 'drv' structure.
>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
Gentle ping...
> ---
> drivers/cpuidle/cpuidle-arm.c | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> index 52a7505..f419f6a 100644
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -86,10 +86,13 @@ static int __init arm_idle_init(void)
>
> for_each_possible_cpu(cpu) {
>
> + drv = NULL;
> + dev = NULL;
> +
> drv = kmemdup(&arm_idle_driver, sizeof(*drv), GFP_KERNEL);
> if (!drv) {
> ret = -ENOMEM;
> - goto out_fail;
> + goto init_drv_fail;
> }
>
> drv->cpumask = (struct cpumask *)cpumask_of(cpu);
> @@ -104,13 +107,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 init_drv_fail;
> }
>
> ret = cpuidle_register_driver(drv);
> if (ret) {
> pr_err("Failed to register cpuidle driver\n");
> - goto init_fail;
> + goto init_drv_fail;
> }
>
> /*
> @@ -128,14 +131,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 init_dev_fail;
> }
>
> dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> if (!dev) {
> pr_err("Failed to allocate cpuidle device\n");
> ret = -ENOMEM;
> - goto out_fail;
> + goto init_dev_fail;
> }
> dev->cpu = cpu;
>
> @@ -143,15 +146,19 @@ 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 init_dev_fail;
> }
> }
>
> return 0;
> -init_fail:
> +
> +init_dev_fail:
> + kfree(dev);
> + cpuidle_unregister_driver(drv);
> +
> +init_drv_fail:
> kfree(drv);
> -out_fail:
> +
> while (--cpu >= 0) {
> dev = per_cpu(cpuidle_devices, cpu);
> cpuidle_unregister_device(dev);
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] ARM: cpuidle: refine failure handling in init flow
2017-09-04 6:52 [PATCH 1/2] ARM: cpuidle: refine failure handling in init flow Leo Yan
` (2 preceding siblings ...)
2017-10-07 12:12 ` Leo Yan
@ 2017-10-09 12:04 ` Daniel Lezcano
2017-10-09 14:24 ` Leo Yan
3 siblings, 1 reply; 7+ messages in thread
From: Daniel Lezcano @ 2017-10-09 12:04 UTC (permalink / raw)
To: Leo Yan, Rafael J. Wysocki, linux-pm, linux-kernel; +Cc: Stefan Wahren
On 04/09/2017 08:52, Leo Yan wrote:
> After applied Stefan Wahren patch ("ARM: cpuidle: Avoid memleak if init
> fail") there have no memleak issue, but the code is not consistent to
> handle initialization failure between driver registration and device
> registration. And when device registration fails, it misses to
> unregister the driver.
>
> So this patch is to refine failure handling in init flow, it adds two
> 'goto' tags: when register device fails, it goto 'init_dev_fail' tag and
> free 'dev' structure and unregister driver; when register driver fails,
> it goto 'init_drv_fail' tag and free 'drv' structure.
>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> drivers/cpuidle/cpuidle-arm.c | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> index 52a7505..f419f6a 100644
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -86,10 +86,13 @@ static int __init arm_idle_init(void)
>
> for_each_possible_cpu(cpu) {
>
> + drv = NULL;
^^^^^
This initialization is not needed.
> + dev = NULL;
> +
> drv = kmemdup(&arm_idle_driver, sizeof(*drv), GFP_KERNEL);
> if (!drv) {
> ret = -ENOMEM;
> - goto out_fail;
> + goto init_drv_fail;
Here we can jump directly to out_fail, no ?
> }
>
> drv->cpumask = (struct cpumask *)cpumask_of(cpu);
> @@ -104,13 +107,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 init_drv_fail;
goto out_kfree_drv;
> }
>
> ret = cpuidle_register_driver(drv);
> if (ret) {
> pr_err("Failed to register cpuidle driver\n");
> - goto init_fail;
> + goto init_drv_fail;
goto out_unregister_drv;
etc ...
> return 0;
> -init_fail:
> +
> +init_dev_fail:
> + kfree(dev);
> + cpuidle_unregister_driver(drv);
> +
> +init_drv_fail:
> kfree(drv);
> -out_fail:
> +
So, the code should end up with:
out_kfree_dev:
kfree(dev);
out_unregister_drv:
cpuidle_unregister_drv(drv);
out_kfree_drv:
kfree(drv);
> while (--cpu >= 0) {
> dev = per_cpu(cpuidle_devices, cpu);
> cpuidle_unregister_device(dev);
>
Perhaps it could nicer to create a function with the rollback embedded:
for_each_possible_cpu(cpu) {
ret = arm_idle_init_cpu(cpu)
if (ret)
goto out_fail;
}
return 0;
out_fail:
while (--cpu >= 0) {
cpuidle_unregister_device(per_cpu(cpuidle_devices,cpu));
cpuidle_unregister_driver(cpuidle_get_cpu_driver(dev));
kfree(dev);
kfree(drv);
}
return ret;
And arm_idle_init_cpu(int cpu) does what is currently in the loop content.
--
<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] 7+ messages in thread* Re: [PATCH 1/2] ARM: cpuidle: refine failure handling in init flow
2017-10-09 12:04 ` Daniel Lezcano
@ 2017-10-09 14:24 ` Leo Yan
0 siblings, 0 replies; 7+ messages in thread
From: Leo Yan @ 2017-10-09 14:24 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: Rafael J. Wysocki, linux-pm, linux-kernel, Stefan Wahren
Hi Daniel,
On Mon, Oct 09, 2017 at 02:04:40PM +0200, Daniel Lezcano wrote:
> On 04/09/2017 08:52, Leo Yan wrote:
> > After applied Stefan Wahren patch ("ARM: cpuidle: Avoid memleak if init
> > fail") there have no memleak issue, but the code is not consistent to
> > handle initialization failure between driver registration and device
> > registration. And when device registration fails, it misses to
> > unregister the driver.
> >
> > So this patch is to refine failure handling in init flow, it adds two
> > 'goto' tags: when register device fails, it goto 'init_dev_fail' tag and
> > free 'dev' structure and unregister driver; when register driver fails,
> > it goto 'init_drv_fail' tag and free 'drv' structure.
> >
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: Stefan Wahren <stefan.wahren@i2se.com>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> > drivers/cpuidle/cpuidle-arm.c | 25 ++++++++++++++++---------
> > 1 file changed, 16 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> > index 52a7505..f419f6a 100644
> > --- a/drivers/cpuidle/cpuidle-arm.c
> > +++ b/drivers/cpuidle/cpuidle-arm.c
> > @@ -86,10 +86,13 @@ static int __init arm_idle_init(void)
> >
> > for_each_possible_cpu(cpu) {
> >
> > + drv = NULL;
>
> ^^^^^
>
> This initialization is not needed.
Yeah.
> > + dev = NULL;
> > +
> > drv = kmemdup(&arm_idle_driver, sizeof(*drv), GFP_KERNEL);
> > if (!drv) {
> > ret = -ENOMEM;
> > - goto out_fail;
> > + goto init_drv_fail;
>
> Here we can jump directly to out_fail, no ?
Yes, can directly jump to out_fail.
> > }
> >
> > drv->cpumask = (struct cpumask *)cpumask_of(cpu);
> > @@ -104,13 +107,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 init_drv_fail;
>
> goto out_kfree_drv;
>
> > }
> >
> > ret = cpuidle_register_driver(drv);
> > if (ret) {
> > pr_err("Failed to register cpuidle driver\n");
> > - goto init_fail;
> > + goto init_drv_fail;
>
> goto out_unregister_drv;
Just want to check again, here should be "goto out_kfree_drv"?
> etc ...
>
> > return 0;
> > -init_fail:
> > +
> > +init_dev_fail:
> > + kfree(dev);
> > + cpuidle_unregister_driver(drv);
> > +
> > +init_drv_fail:
> > kfree(drv);
> > -out_fail:
> > +
>
> So, the code should end up with:
>
> out_kfree_dev:
> kfree(dev);
> out_unregister_drv:
> cpuidle_unregister_drv(drv);
> out_kfree_drv:
> kfree(drv);
Yeah, this is clearer than my patch :)
> > while (--cpu >= 0) {
> > dev = per_cpu(cpuidle_devices, cpu);
> > cpuidle_unregister_device(dev);
> >
>
> Perhaps it could nicer to create a function with the rollback embedded:
>
> for_each_possible_cpu(cpu) {
> ret = arm_idle_init_cpu(cpu)
> if (ret)
> goto out_fail;
> }
>
> return 0;
>
> out_fail:
>
> while (--cpu >= 0) {
> cpuidle_unregister_device(per_cpu(cpuidle_devices,cpu));
> cpuidle_unregister_driver(cpuidle_get_cpu_driver(dev));
> kfree(dev);
> kfree(drv);
> }
>
> return ret;
>
> And arm_idle_init_cpu(int cpu) does what is currently in the loop content.
Understood. I will split into two patches, one patch is to fix
resource releasing issue, the second patch is refactoring patch with a
'new function with the rollback embedded'.
Thanks a lot for the reviewing and suggestion.
Thanks,
Leo Yan
^ permalink raw reply [flat|nested] 7+ messages in thread