Re: [PATCH 4/8] thermal/drivers/Kconfig: Convert the CPU cooling device to a choice

[Daniel Thompson]

On Fri, Jan 26, 2018 at 02:25:58PM +0100, Daniel Lezcano wrote:
> > Perhaps I should have distinguished more between runtime-meaning-boot-time
> > and runtime-meaning-full-system-operation. To be clear none of my
> > comments are about being able to enable/disable idle injection on a
> > fully running system. It is the impact on multi-platform binaries that
> > attracted my attention.
> > 
> > 
> >> I prefer to keep simple self-encapsulated feature code and make it
> >> evolve to something better instead of sending a blowing patch series
> >> taking into account all possible combinations. Choosing the strategy at
> >> compile time may be look restrictive but we can live with that without
> >> problem and iteratively do the change until the choice becomes the
> >> default strategy selection option.
> > 
> > You won't find me arguing against iterative improvement. However I
> > did observe that the idle injection code consists almost exclusively 
> > of new lines of code. Thus I don't understand why this new code 
> > interferes with cpufreq cooling to the point that the existing cooling
> > options must compiled out of the kernel before we can exploit it.
> > 
> > I can see the combo code does have tentacles in more places but even so
> > I have the same question. What prevents the existing cooling strategy
> > from being compiled at the same time.
> > 
> > You appear to be saying that there's not yet enough infrastructure to
> > decide which type of cooler to instantiate[1] so its OK to hack around
> > the decision by forcing it to be made it at compile time. Even if we
> > want to force a decision by some means, is compile time really the best
> > point to do that?
> 
> For the moment, yes. We are backward compatible, there is no change with
> the current feature.
> 
> I don't see really the point of being so afraid with this compilation
> option, it is not the first time we have compile time code which
> migrates to runtime code. The important thing is we don't break the
> existing.

I guess its because the code that implements the cooling devices doesn't
look like there is any conflict preventing them being enabled at the
same time except. There isn't even that much problem choosing between
cpufreq-only and cpuidle-only during instatiation (preferring cpufreq
for the benefit of existing platforms) although I can see that we cannot
choose between cpufreq-only and combo without extending the DT bindings.


> Having the runtime selection is the objective after improving the CPU
> cooling devices. But this is far from being trivial and will need a
> rework of the cooling device / framework (and the changes grouped in a
> separate patch series).

I guess if I'm in a minority of one then I can agree to disagree at
this point. I don't like build-time only features (for all the distro
centric reasons I've already mentioned) but I'm not that heavily
invested... and look forward to seeing runtime selection implemented.


Daniel.