[PATCH] ARM: multi_v7: Rebuild default configuration on v4.3-rc1

[Thierry Reding]

On Tue, Sep 15, 2015 at 03:23:21PM +0100, Russell King - ARM Linux wrote:
> [Added Linus]
> 
> On Tue, Sep 15, 2015 at 03:46:40PM +0200, Thierry Reding wrote:
> > On Tue, Sep 15, 2015 at 12:12:57PM +0100, Russell King - ARM Linux wrote:
> > > On Tue, Sep 15, 2015 at 12:51:40PM +0200, Thierry Reding wrote:
> > > > From: Thierry Reding <treding at nvidia.com>
> > > > 
> > > > It's becoming more and more difficult to update the multi_v7 default
> > > > configuration because it hasn't been kept in proper order. Typically the
> > > > workflow to update it would be to do the following:
> > > 
> > > If we start regularly updating the defconfigs, it'll annoy Linus,
> > > because it will create a lot of useless churn, bloating his diffstats
> > > needlessly.
> > 
> > We are already regularly updating defconfigs. It's a natural thing to do
> > as new features are implemented.
> > 
> > > Since the defconfig files are not order specific, it would have been
> > > a good idea if savedefconfig had sorted the options into alphanumeric
> > > order propr to outputting them.  That would cut down on the useless
> > > churn, some of which is in your patch.
> > 
> > That would be one solution. But it would also mean that people need to
> > actually go and check that things are properly sorted. Most of the churn
> > here doesn't come from the fact that these options have moved in Kconfig
> > but because people have inserted them in the wrong places. The same
> > could happen even if savedefconfig sorted options alphanumerically,
> > because evidently people aren't using the tools properly.
> 
> Hardly surprising - if people are going around adding an option to
> all architectures, they're not going to run 'make savedefconfig' on
> each and every configuration they find to add that option.  That's
> far too much work.  It's not that "people aren't using the tools
> properly" it's that the tools are too much effort when you've got a
> global change to do.

I don't expect people to do this for every option they add. What I was
expecting people to do is when they add a new feature to a default
configuration that they add it in the right place. I'm also not
advocating for doing this for every default configuration, but the
multi_v7_defconfig is where pretty much everyone in the ARM world adds
options. It is also one which /should/ be updated through the ARM SoC
tree only, so there's technically a way to enforce this.

> However, there's not really anything that can be done about it,
> because nothing is simpler than concatenating an option to all
> configs, or adding it using an editor.

It's not that easy. As a platform maintainer I often need to deal with
the situation where the default configuration needs to be updated with
the latest options so that "everything works". But because the default
configuration is all messed up there is no easy way for me to do that,
because once I have a .config that has all the new options enabled the
corresponding defconfig has a huge diff compared to the default and it
takes a ridiculous amount of work to sort out the actual changes.

I doubt that I'm the only one in that position and I think this has the
potential to make things a little easier for everyone contributing to
this default configuration.

> > > In fact, most of this patch is pure churn - it's merely moving options
> > > about.  It provides very little in the way of useful benefit for its
> > > size.  We need to do better than this.
> > 
> > Yes, I know. In fact I stated that in the commit message. But this
> > really should be a one-time thing.
> 
> The fact is, this won't be a one-time thing.  People will continue to
> add options in random places in these files, and we'll need to "fix"
> it with useless churn yet again, and again, and again.

Like I said, this file should only ever get modified via the ARM SoC
tree and hence we should be able to enforce proper updates.

> > If we make it easy for people to
> > properly update the default configuration there will be less churn
> > further down the road. The changes that end up in multi_v7 default
> > configuration patches should be what's generated from the defconfig
> > output. So unless options get removed from Kconfig there should not
> > be any unnecessary churn subsequently.
> 
> I think you're living in an ideal world...
> 
> > As for changes that move options in Kconfig, I think those should be
> > discouraged. The sane thing to do is order them alphabetically, which
> > will remove any need to move things around.
> 
> ... which is why you re-ordered drivers/gpu/drm/Kconfig, moving
> bridge/Kconfig after the panel Kconfig, which now resulted in this
> churn in _this_ churn filled patch:
> 
>  CONFIG_DRM=y
>  # CONFIG_DRM_I2C_CH7006 is not set
>  # CONFIG_DRM_I2C_SIL164 is not set
> -CONFIG_DRM_NXP_PTN3460=m
> -CONFIG_DRM_PARADE_PS8622=m
>  CONFIG_DRM_NOUVEAU=m
>  CONFIG_DRM_EXYNOS=m
> ...
>  CONFIG_DRM_PANEL_SIMPLE=y
> +CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0=m
> +CONFIG_DRM_NXP_PTN3460=m
> +CONFIG_DRM_PARADE_PS8622=m
>  CONFIG_FB_ARMCLCD=y

The reason for those changes were that bridge drivers were completely
randomly added in Kconfig. That should've never happened in the first
place and the changes you mentioned are actually supposed to prevent
this kind of random additions in the future and keep things properly
sorted.

> > > > diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
> > > > index 03deb7fb35e8..56775eb9b9cc 100644
> > > > --- a/arch/arm/configs/multi_v7_defconfig
> > > > +++ b/arch/arm/configs/multi_v7_defconfig
> > > > @@ -12,7 +12,6 @@ CONFIG_MODULE_UNLOAD=y
> > > >  CONFIG_PARTITION_ADVANCED=y
> > > >  CONFIG_CMDLINE_PARTITION=y
> > > >  CONFIG_ARCH_VIRT=y
> > > > -CONFIG_ARCH_ALPINE=y
> > > >  CONFIG_ARCH_MVEBU=y
> > > >  CONFIG_MACH_ARMADA_370=y
> > > >  CONFIG_MACH_ARMADA_375=y
> > > > @@ -20,14 +19,15 @@ CONFIG_MACH_ARMADA_38X=y
> > > >  CONFIG_MACH_ARMADA_39X=y
> > > >  CONFIG_MACH_ARMADA_XP=y
> > > >  CONFIG_MACH_DOVE=y
> > > > +CONFIG_ARCH_ALPINE=y
> > > 
> > > This option has moved, and the above two changes are therefore nothing
> > > but churn.
> > [...]
> > 
> > This and all the below I know already. I went through each and every one
> > of these options and checked what happened to them. Those listed in the
> > commit message are the ones that have actually changed. Those not listed
> > are the ones that have only moved around.
> 
> Right, so from me this patch gets a NAK, because it's mostly churn
> with very little benefit.
> 
> Linus has threatened to remove the defconfigs in the past for this
> kind of stupid churn.  For me to say anything else about this would
> be re-inviting their removal.

Like I said, I consider the churn a mostly one-time thing. But keeping
this file properly up to date is going to help prevent it from becoming
stale and crufty.

But if everyone else feels really strongly about this, consider the
patch dropped. I'll go and do something else instead.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150915/da1af186/attachment.sig>