Re: [PATCH v2 00/12] arm64: Kconfig: Update ARCH_EXYNOS select configs

[Arnd Bergmann]

On Thu, Sep 30, 2021 at 8:15 AM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxxxxx> wrote:
> On 29/09/2021 21:48, Will McVicker wrote:
> > On Wed, Sep 29, 2021 at 6:02 AM Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxxxxx> wrote:
> >> What is more, it seems you entirely ignored Geert's comments. I pointed
> >> attention to it last time and you just said you will send v2 instead of
> >> joining discussion.
> >>
> >> It's a NAK for this reason - ignoring what Geert brought: you just broke
> >> distro configs for Exynos.
> >
> > First off I did want to chime into the discussion from the previous
> > patchset, but I felt that Lee and Saravana addressed all your concerns
> > regarding the intent and feasibility. You also made it clear what the
> > next steps were that I needed to take.
>
> One of the steps was problem with distros using everything as modules.
> They should not receive these drivers as modules.
> Reminder: these are essential drivers and all Exynos platforms must have
> them as built-in (at least till someone really tests this on multiple
> setups).

Agreed. I absolutely love the work of the GKI developers to turn more
drivers into loadable modules, but the "correctness-first" principle is
not up for negotiation. If you are uncomfortable with the code or the
amount of testing because you think it breaks something, you should
reject the patches. Moving core platform functionality is fundamentally
hard and it can go wrong in all possible ways where it used to work
by accident because the init order was fixed.

> >> Please also explain why Exynos is so special that we deviate from the
> >> policy for all SoC that critical SoC-related drivers have to be enabled
> >> (built-in or as module).
> >
> > I am not actually changing ANY default build configurations here and
> > I'm not removing any existing configuration.
>
> You are changing not default, but selectability which is part of the
> enforced configuration to make platforms working. The distros do not
> always choose defaults but rather all as modules. Kernel configuration
> is huge and complex, so by mistake they could now even disable
> potentially essential driver. There is no need to disable for example
> essential clock driver on a supported Exynos platform.

I'm not overly worried about the defaults. If the drivers work as loadable
modules, I'm happy with them being loadable modules in distros.
If they don't work this way, then the patches are broken and should
not get merged.

I don't even mind having essential drivers that can be turned off,
since we already have a ton of those (e.g. serial ports on most platforms).
It's up to distros to know which drivers to enable, though having
either reasonable defaults or fail-safe Kconfig dependencies (e.g.
making it impossible to turn off but allowing modules) is clearly
best.

> > I tried to make it pretty
> > clear in my original patch series commit messages that none of my
> > changes modify the default behavior. The .config is the same with and
> > without my patches. All of these drivers remain enabled as built-in.
> > So if there is a distro that requires all of these drivers to be
> > built-in, then they can continue as is without noticing any
> > difference. IOW, all of these changes are/should be backwards
> > compatible.
>
> I was not referring to default neither to backwards compatibility.
> Please explain why Exynos is special that it does not require essential
> drivers to be selected as built-in. For example why aren't same changes
> done for Renesas?
>
> Is that now a new global approach that all SoC drivers should be allowed
> to be disabled for ARCH_XXX?

I wouldn't enforce it either way across platforms. I would prefer drivers
to be loadable modules where possible (and tested), rather than
selected by the platform Kconfig. If you want to ensure the exynos
drivers are impossible to turn into a nonworking state, that's up to you.

> > You said that upstream supports a generic
> > kernel, but I argue that the upstream "generic" arm64 kernel can't be
> > considered generic if it builds in SoC specific drivers that can be
> > modules.
>
> Good point, but since having them as modules was not tested, I consider
> it as theoretical topic.

I actually disagree strongly with labelling the kernel as "non-generic"
just because it requires platform specific support to be built-in rather than
a loadable module. This has never been possible on any platform
I'm aware of, and likely never will, except for minor variations of
an existing platform.

Look at x86 as an example: there are less than a dozen SoC platforms
supported and they are incredibly similar hardware-wise, but the kernel
is anything but "generic" in the sense that was mentioned above.
Most of the platform specific drivers in arch/x86/platform and the
corresponding bits in drivers/{irqchip,clocksource,iommu} are always
built-in, and a lot more is hardwired in architecture code as PCI
quirks or conditional on cpuid or dmi firmware checks.

> >> Even if there was, I think it is good to have dependencies like
> >> ARCH_EXYNOS, as they let us partition the (19000, as Arnd said recently)
> >> Kconfig symbols into better manageable groups.  Without these, we cannot
> >> do better than "depends on ARM || ARM64 || COMPILE_TEST".
> >
> > My patch series still keeps the dependencies on ARCH_EXYNOS. I am
> > totally fine with "depends on ARCH_EXYNOS" and totally fine with
> > "default ARCH_EXYNOS". The problem we have is that ARCH_EXYNOS
> > forcefully selects SoC specific drivers to be built-in because it just
> > adds more and more SoC-specific drivers to a generic kernel.
>
> The selected drivers are essential for supported platforms. We don't
> even know what are these unsupported, downstream platforms you want
> customize kernel for. They cannot be audited, cannot be compared.
>
> Therefore I don't agree with calling it a "problem" that we select
> *necessary* drivers for supported platforms. It's by design - supported
> platforms should receive them without ability to remove.
>
> If you want to change it, let me paste from previous discussion:
>
> Affecting upstream platforms just because vendor/downstream does not
> want to mainline some code is unacceptable. Please upstream your drivers
> and DTS.

Agreed. I understand that it would be convenient for SoC vendors to
never have to upstream their platform code again, and that Android
would benefit from this in the short run.

>From my upstream perspective, this is absolutely a non-goal. If it becomes
easier as a side-effect of making the kernel more modular, that's fine.
The actual goal should be to get more people to contribute upstream so
devices run code that has been reviewed and integrated into new kernels.

> > I know you are asking for me to only push changes that have proven to
> > work.
>
> Yep, tested.

I'm generally fine with "obviously correct" ones as well, but it's up to
you to categorize them ;-)

         Arnd