MIME-Version: 1.0
References: <20230208094813.20874-1-walter.chang@mediatek.com>
 <fdcea75c-1ddf-c4f4-5d46-f32a6619b66d@linaro.org> <CANDhNCp6esdor4kDDTDowkKRfRN9QW8Au7uJMhsxyw0eAG4i8A@mail.gmail.com>
 <20230210085157.GA175687@linaro.org>
In-Reply-To: <20230210085157.GA175687@linaro.org>
From:   John Stultz <jstultz@google.com>
Date:   Fri, 10 Feb 2023 11:58:16 -0800
Message-ID: <CANDhNComgUF+7UhG-MDjjm97AZRQb__4-1u++45UKfirdAMp+Q@mail.gmail.com>
Subject: Re: [PATCH 0/3] Support timer drivers as loadable modules
To:     Daniel Lezcano <daniel.lezcano@linaro.org>
Cc:     walter.chang@mediatek.com, Thomas Gleixner <tglx@linutronix.de>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        "Maciej W . Rozycki" <macro@orcam.me.uk>,
        wsd_upstream@mediatek.com, stanley.chu@mediatek.com,
        Chun-hung.Wu@mediatek.com, Freddy.Hsin@mediatek.com,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Todd Kjos <tkjos@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Feb 10, 2023 at 12:52 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On Thu, Feb 09, 2023 at 11:50:49AM -0800, John Stultz wrote:
> > On Thu, Feb 9, 2023 at 7:36 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> > > What for ?
> >
> > In general, it's the same reason why modules exist: We want to be able
> > to support a wide array of devices with a single kernel, but we don't
> > want all devices to pay the memory cost of code that will never be
> > used there. So being able to support loading device-specific bits like
> > clocksources (along with other device specific logic) helps.
>
> Agree, that is why modules are for.
>
> > Obviously it still has to make sense, and others have raised concerns
> > of stability issues if the hardware support is needed before we can
> > get to module loading, but I think if this allows drivers (such as
> > timer-mediatek) to be loadable safely, I see it as beneficial.
>
> From a technical point of view, it is arguable.
>
> But my main concern is the real reason of changing this to the module
> format. I see that as a way to overcome the effort to upstream the
> drivers. And the GKI is an alibi to justify the module conversion.

[Putting on my Android Antennae for a moment]

I can promise the GKI is no alibi - it is a real thing. Part of
convincing vendors to ship the same kernel is that we have to be able
to bring the security benefits of being able to update the unified
kernel without major impact to memory. Utilizing modules (all over -
as a lot of small cuts add up) is crucial for that.

Some vendors haven't historically been great about upstreaming device
support, and I understand the concern that allowing modules might
enable vendors to keep modules out of tree. But vendors inclined to do
that will find a way regardless (and because at a practical level,
because the need to keep the GKI size down the android tree will have
to carry a similar change to the one submitted here), so I don't think
rejecting such patches is a real disincentive.

Instead it just creates further needless fragmentation between
upstream and android kernels, which makes it further difficult to
justify and motivate upstream-hesitant vendors to submit their code to
lkml.

And again, there *has* to be upstream users, as we should not have any
maintenance burden for out of tree code! But in this case the upstream
timer-mediatek driver was named as a candidate module (obviously those
patches are needed when this series is re-sent).

Additionally, while I understand the concern and skepticism, for folks
who are working on upstreaming (Mediatek along with folks at Collabora
have done some great work!), having to deal with this meta-issue of
questioning of one's purity-of-intent, when one is actually submitting
patches I think makes the process of dealing with the community seem
even more difficult (making some folks question why bother).

The upstream kernel community is an amazing thing! And I understand
why we want to be protective of it. But I also worry that if we get
too wrapped up in suspicions of ill intent, we aren't going to be able
to bring folks into the fold and grow the community.  The license
doesn't require one to work with the community, so we should probably
be using more carrots and fewer sticks.

> Given the timers is a base brick of the core subsystems, without
> proper support of the timer (eg. bug fixes), the platform support will
> be wobbly.

As for bug-fixes, it should be noted that with the GKI, not all
modules are vendor modules!  There are GKI-modules, which are common
drivers/subsystem-infrastructure used by many devices (such as NFC,
Bluetooth, etc), and these in-tree drivers are updated with the GKI
kernel as modules. So there is motivation to ensure bug fixes to those
upstream drivers land upstream so they can be included when the GKI
and GKI modules are updated.

And to your point about it being a base-brick - Yes, obviously not
everything can be loaded from a module safely, and that's fine. But in
cases where devices can boot with a built in architected timer, then
are able to switch to more device specific clocksources, we'd really
like those device specific clocksources to be modules.

thanks
-john