Date:   Thu, 16 May 2019 10:39:08 -0700
From:   Olof Johansson <olof@...om.net>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Patrick Venture <venture@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        ARM SoC <arm@...nel.org>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        "linux-alpha@...r.kernel.org" <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [GIT PULL 3/4] ARM: SoC-related driver updates

On Thu, May 16, 2019 at 9:27 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Wed, May 15, 2019 at 11:43 PM Olof Johansson <olof@...om.net> wrote:
> >
> > Various driver updates for platforms and a couple of the small driver
> > subsystems we merge through our tree:
>
> Hmm. This moved the aspeed drivers from drivers/misc to
> drivers/soc/aspeed (in commit 524feb799408 "soc: add aspeed folder and
> misc drivers"), but in the meantime we also had a new aspeed soc
> driver added (in commit 01c60dcea9f7 "drivers/misc: Add Aspeed P2A
> control driver").
>
> I ended up resolving that "conflict" by moving the new aspeed P2A
> control driver to be with the other aspeed drivers too. That seemed to
> be the cleanest model.

Yeah, that's the approach we're heading towards with aspeed.

Part of the reason for why I wasn't 100% sure we wanted to move all
drivers over, is that most of drivers/soc has been for "soc glue
logic" code, not for the little SoC-specific drivers where we've
pushed hard to get out into their best-matching driver directories
instead.

Aspeed is an unusually "messy" SoC in that it has a handful of little
widgets used to communicate with the host (in its role as BMC), and
either we'd squint and put all of them in drivers/misc, or we could
pick them up in drivers/soc as we're now doing. Either way the code
will be in the kernel, and keeping it together might not be a bad
idea.

We might get more of a kitchen sink in drivers/soc over time with this
slight change in approach, but we've dealt with messes before and if
it happens, we'll clean it up when it gets too bad. Sometimes letting
it happen is the best way of seeing the bigger picture and not
over-engineer something upfront.

> I'm used to doing these kinds of fixups in a merge, but I have to
> admit that maybe I should have made it a separate commit, because now
> it's kind of non-obvious, and it's sometimes harder to see changes
> that are in a merge commit than in a separate commit.
>
> In particular, it looks like "git log --follow" is not smart enough to
> follow a rename through a merge. But I think that is a git problem,
> and not a very serious one at that ("git blame" has no such problem).
>
> And it means that now the merge has
>
>  drivers/{misc => soc/aspeed}/aspeed-lpc-ctrl.c                   |   0
>  drivers/{misc => soc/aspeed}/aspeed-lpc-snoop.c                  |   0
>  drivers/{misc => soc/aspeed}/aspeed-p2a-ctrl.c                   |   0
>
> when you do "git show --stat" on it, which looks correct, and it feels
> like conceptually the right merge resolution to me.
>
> Sending out this explanatory email to everybody involved, just so that
> this doesn't take you by surprise. But it looks like Patrick Venture
> is not just the author of that moved driver, he was also involved in
> the move of the two other drivers, so I'm guessing there's not going
> to be a lot of confusion here.

Yeah. I think that's fine in this case.

I've got some horror stories from botched rebases where merges ended
up containing actual code changes and that caused immense confusion,
but that's not the case here.

> HOWEVER. More subtly, as part of my *testing* for this, I also
> realized that commit 524feb799408 is buggy. In my tests, the config
> worked fine, but the aspeed drivers were never actually *built*. The
> reason is that commit 524feb799408 ends up doing
>
>    obj-$(CONFIG_ARCH_ASPEED)      += aspeed/
>
> which is completely wrong, because the Kconfig fules are
>
>         depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
>
> so those drivers can be configured even if ARCH_ASPEED *isn't* set.
> The Kconfig part works fine, because the soc/aspeed/Kconfig file is
> included unconditionally, but the actual build process then never
> builds anything in the drivers/soc/aspeed/ subdirectory.
>
> I solved _that_ problem by adding a new config option:
>
>   config SOC_ASPEED
>       def_bool y
>       depends on ARCH_ASPEED || COMPILE_TEST
>
> and using that instead of ARCH_ASPEED.

Yep, looks good -- thanks!

> End result: this was a somewhat messy merge, and the most subtle mess
> was because of that buggy 524feb799408 "soc: add aspeed folder and
> misc drivers").
>
> I *think* I sorted it all out correctly, and now I see the aspeed
> drivers being built (and cleanly at that) but I really *really* want
> people to double-check this all.
>
> Also, I think that the same "we don't actually build-test the end
> result" problem exists else-where for the same reasons.
>
> At the very least, drivers/soc/{atmel,rockchip,zte} seem to have the
> exact same pattern: the Kconfig files enable the drivers, but the
> Makefile in drivers/soc doesn't actually traverse into the
> subdirectories.
>
> End result: CONFIG_COMPILE_TEST doesn't actually do any compile
> testing for those drivers.
>
> I did not try to fix all of those things up, because I didn't do the
> driver movements there.

We'll follow up with patches for that, thanks for pointing it out.

I have to admit that most of my focus tends to be to check for new
errors, not whether everything still is built. Easy to automate, so
I'll poke around with it a bit.

-Olof

Powered by blists - more mailing lists