Re: [PATCH v2 1/2] ARM: cpuidle: Correct driver unregistration if init fails

[Rafael J. Wysocki]

On Tue, Oct 10, 2017 at 7:47 AM, Leo Yan <leo.yan@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> 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
>