Re: [PATCH V9] powercap/drivers/idle_injection: Add an idle injection framework

[Rafael J. Wysocki]

On Tue, Jun 19, 2018 at 3:23 PM, Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:

[cut]

One more thing.

> +/**
> + * idle_injection_register - idle injection init routine
> + * @cpumask: the list of CPUs managed by the idle injection device
> + *
> + * This is the initialization function in charge of creating the
> + * initializing of the timer and allocate the structures. It does not
> + * starts the idle injection cycles.
> + *
> + * Return: NULL if an allocation fails.
> + */
> +struct idle_injection_device *idle_injection_register(struct cpumask *cpumask)
> +{
> +       struct idle_injection_device *ii_dev;
> +       int cpu, cpu_rb;
> +
> +       ii_dev = kzalloc(sizeof(*ii_dev) + cpumask_size(), GFP_KERNEL);
> +       if (!ii_dev)
> +               return NULL;
> +
> +       cpumask_copy(to_cpumask(ii_dev->cpumask), cpumask);
> +       hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +       ii_dev->timer.function = idle_injection_wakeup_fn;
> +
> +       for_each_cpu(cpu, to_cpumask(ii_dev->cpumask)) {
> +
> +               if (per_cpu(idle_injection_device, cpu)) {
> +                       pr_err("cpu%d is already registered\n", cpu);

If you print something like this, it should be clear what it is about
and what piece of code it comes from as there will be no context
around it it the log.

> +                       goto out_rollback_per_cpu;

Shorten the label perhaps?

> +               }
> +
> +               per_cpu(idle_injection_device, cpu) = ii_dev;
> +       }
> +
> +       return ii_dev;
> +
> +out_rollback_per_cpu:
> +       for_each_cpu(cpu_rb, to_cpumask(ii_dev->cpumask)) {
> +               if (cpu == cpu_rb)
> +                       break;
> +               per_cpu(idle_injection_device, cpu_rb) = NULL;
> +       }
> +
> +       kfree(ii_dev);
> +
> +       return NULL;
> +}
> +