Re: [PATCH] cpuidle: simplify multiple driver support

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 05/18/2013 01:15 PM, Francesco Lavra wrote:
> On 05/14/2013 03:21 PM, Daniel Lezcano wrote:
>> Commit bf4d1b5ddb78f86078ac6ae0415802d5f0c68f92 brought the multiple driver
>> support. The code added a couple of new API to register the driver per cpu.
>> That led to some code complexity to handle the kernel config options when
>> the multiple driver support is enabled or not, which is not really necessary.
>> The code has to be compatible when the multiple driver support is not enabled,
>> and the multiple driver support has to be compatible with the old api.
>>
>> This patch removes this API, which is not yet used by any driver but needed
>> for the HMP cpuidle drivers which will come soon, and replaces its usage
>> by a cpumask pointer in the cpuidle driver structure telling what cpus are
>> handled by the driver. That let the API cpuidle_[un]register_driver to be used
>> for the multipled driver support and also the cpuidle_[un]register functions,
>> added recently in the cpuidle framework.
>>
>> The current code, a bit poor in comments, has been commented and simplified.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> [...]

[ ... ]

Hi Francesco,

thanks for reviewing the patch.

>> -EXPORT_SYMBOL_GPL(cpuidle_unregister_driver);
>> -
>> -#else
>> -
>> -static struct cpuidle_driver *cpuidle_curr_driver;
>> +	if (!WARN_ON(drv->refcnt > 0))
>> +		return;
> 
> Shouldn't it be
> if (WARN_ON(drv->refcnt > 0))
> 	return;
> ?

Yes, good catch !

>> -static inline void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu)
>> -{
>> -	cpuidle_curr_driver = drv;
>> -}
>> +	__cpuidle_unset_driver(drv);
>>  
>> -static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
>> -{
>> -	return cpuidle_curr_driver;
>> +	if (drv->bctimer) {
>> +		drv->bctimer = 0;
>> +		on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer,
>> +				 (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1);
>> +	}
>>  }
> 
> Also, shouldn't things be undone in reverse order as they were done in
> __cpuidle_register_driver()? I mean disabling the timer broadcast
> notification before calling __cpuidle_unset_driver().

Yes, that makes sense.

Thanks
  -- Daniel

-- 
 <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