Re: [RFC PATCH v15 01/11] ARM: cpuidle: Register per cpuidle device

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

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.