Re: [PATCH V4 1/2] cpuidle: simplify multiple driver support

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

On 09/18/2013 07:21 AM, Viresh Kumar wrote:
> On 8 June 2013 03:23, Daniel Lezcano <daniel.lezcano@linaro.org> 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 multiple driver support and also the cpuidle_[un]register functions,
>> added recently in the cpuidle framework.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Sorry for jumping onto this thread so late :(
> The current code in cpuidle/driver.c contains two definitions of these routines:
> __cpuidle_get_cpu_driver() and __cpuidle_{set|unset}_driver(), enclosed
> withing #if/else..
> 
> These duplicate routines exist only to save some space where we don't
> have multiple drivers for a platform, right? So, that we don't waste up
> space for per-cpu variables for holding cpuidle_driver..

That's right.

> But what about multi platform kernels? This config option would be enabled
> then and we would finally run into the same problem again..
> 
> The worst side of this is: We will run separate code paths for a platform
> which doesn't have support for multiple drivers in the multiplatform kernel..
> Even if it works, it looks a bit wrong design wise..

Where is it wrong in design ? If the multiple driver support is enabled
in the kernel but the driver handles all the cpu, it works.

> Either we can have a runtime variable in cpuidle_driver or somewhere else
> that will let us know if we want multiple drivers for our platform or not
> 
> OR
> 
> do the per-cpu stuff for everybody..

I don't think it is worth to add more code complexity to save an extra
small chunk of memory on ARM. If we don't want to support the multiple
driver, the option is disabled and we use a single variable. That is the
case for x86. If we want to enable it for multiple platforms support on
ARM, then we need per-cpu and we lose a bit of per-cpu memory.

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