Re: [PATCH 0/2] cpu-features.h rename

[Ralf Baechle <ralf@linux-mips.org>]

On Wed, Mar 15, 2017 at 08:24:11AM +0100, Marcin Nowakowski wrote:
> Date:   Wed, 15 Mar 2017 08:24:11 +0100
> From: Marcin Nowakowski <marcin.nowakowski@imgtec.com>
> To: Florian Fainelli <f.fainelli@gmail.com>, Ralf Baechle
>  <ralf@linux-mips.org>
> CC: linux-mips@linux-mips.org
> Subject: Re: [PATCH 0/2] cpu-features.h rename
> Content-Type: text/plain; charset="windows-1252"; format=flowed
> 
> Hi Florian,
> 
> On 15.03.2017 03:04, Florian Fainelli wrote:
> > 
> > 
> > On 03/14/2017 01:45 AM, Marcin Nowakowski wrote:
> > > Hi Ralf,
> > > 
> > > On 14.03.2017 09:29, Ralf Baechle wrote:
> > > > On Tue, Mar 14, 2017 at 08:40:21AM +0100, Marcin Nowakowski wrote:
> > > > 
> > > > > On 13.03.2017 18:08, Florian Fainelli wrote:
> > > > > > On 03/13/2017 06:33 AM, Marcin Nowakowski wrote:
> > > > > > > Since the introduction of GENERIC_CPU_AUTOPROBE
> > > > > > > (https://patchwork.linux-mips.org/patch/15395/) we've got 2 very
> > > > > > > similarily
> > > > > > > named headers: cpu-features.h and cpufeature.h.
> > > > > > > Since the latter is used by all platforms that implement
> > > > > > > GENERIC_CPU_AUTOPROBE functionality, it's better to rename the
> > > > > > > MIPS-specific
> > > > > > > cpu-features.h.
> > > > > > > 
> > > > > > > Marcin Nowakowski (2):
> > > > > > >   MIPS: mach-rm: Remove recursive include of cpu-feature-overrides.h
> > > > > > >   MIPS: rename cpu-features.h -> cpucaps.h
> > > > > > 
> > > > > > That's a lot of churn that could cause some good headaches in
> > > > > > backporting stable changes affecting cpu-feature-overrides.h.
> > > > > > 
> > > > > > Can we just do the cpu-features.h -> cpucaps.h rename and keep
> > > > > > cpu-feature-overrides.h around?
> > > > > 
> > > > > That's of course possible, but I think it would make the naming quite
> > > > > confusing as well, as it would be very unclear for any reader as to
> > > > > why a
> > > > > 'cpu-feature-overrides' overrides 'cpucaps'.
> > > > > 
> > > > > I've looked at the change history of these files and most receive very
> > > > > little updates (which is hardly surprising given the changes are done
> > > > > mostly
> > > > > during initial integration of a new cpu or soon after), and none of the
> > > > > changes in those files were marked for stable. I think it's safe to
> > > > > assume
> > > > > that this pattern is not likely to change, would you agree?
> > > > 
> > > > I've noticed the same pattern - and it's a little concerning.  Not adding
> > > > values for later features means the'll probably be runtime detected
> > > > resulting in a bigger, slower kernel.
> > > 
> > > But that is a type of optimisation that may (should?) be done when new
> > > features are added, which in most cases doesn't make it a candidate for
> > > backporting to stable.
> > 
> > You may be fixing actual bugs by patching this file, e.g: selecting the
> > correct value for e.g: cpu_has_dc_aliases, cpu_has_ic_fills_ic,
> > cpu_dcache_line_size() and so on. Ideally every feature in there has
> > been properly set/cleared in arch/mips/kernel/cpu-probe.c but there
> > could be platform relying exclusively on cpu-feature-overrides.h to
> > provide the correct value.
> 
> Yes, of course that is possible and I'm not dismissing that fact.
> I've only stated that looking at the git history of these files (which dates
> back to 2008 when they were moved from a different location), there have
> been only a few changes to them and most of the changes were not bugfixes
> for specific cores but general code changes applied throughout the tree.
> So in an unlikely case that a bug is discovered that will be fixed by
> updating a specific cpu(caps|feature)-override.h, there would be a slightly
> increased effort required to backport the patch due to a filename
> difference, but IMO that's hardly a reason to prevent any changes and to
> keep the filenames inconsistent?
> It's not like I'm changing the whole logic behind cpu_has functionality ...
> 
> 
> > If not about the backport argument, just changing that many files at
> > once (have they actually been build tested at least?)
> 
> I have build-tested this with some defconfigs affected.
> 
> > just does not seem
> > practical nor worth it to me.
> 
> I think having a sensible file naming scheme is worth the change and you
> seem to see this change as a much bigger one than I do. From my perspective
> this change is a really trivial one.

As a general rule, design for the future, not the past.

  Ralf