MIME-Version: 1.0
References: <20230214105412.5856-1-walter.chang@mediatek.com>
 <20230214105412.5856-5-walter.chang@mediatek.com> <1726f1d2-7411-2b49-79cd-f5aecff2d19e@linaro.org>
 <CANDhNCrYaepM3d-xHybqc9n5j0q0iMfPiTGPZKskEWjqjD6nRA@mail.gmail.com> <12f2d6a8-e16f-5bac-10ee-035c1b4b0356@linaro.org>
In-Reply-To: <12f2d6a8-e16f-5bac-10ee-035c1b4b0356@linaro.org>
From:   John Stultz <jstultz@google.com>
Date:   Wed, 15 Feb 2023 12:59:49 -0800
Message-ID: <CANDhNCrae9=8-w9-Qaq527-j8qMYzHGiAXAmKpxsoxD4=UP5zg@mail.gmail.com>
Subject: Re: [PATCH v2 4/4] clocksource/drivers/timer-mediatek: Make
 timer-mediatek become loadable module
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     walter.chang@mediatek.com,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Feb 15, 2023 at 10:35 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/02/2023 00:20, John Stultz wrote:
> > On Tue, Feb 14, 2023 at 3:09 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >> On 14/02/2023 11:53, walter.chang@mediatek.com wrote:
> >>> From: Chun-Hung Wu <chun-hung.wu@mediatek.com>
> >>>
> >>> Make the timer-mediatek driver which can register
> >>> an always-on timer as tick_broadcast_device on
> >>> MediaTek SoCs become loadable module in GKI.
> >>
> >> Other questions are unanswered. Please do not ignore feedback and
> >> respond to it.
> >>
> >
> > Apologies, I know it can be a pain to repeat yourself, but would you
> > clarify which questions were unanswered?
> > My initial skim made it seem like the items you raised were addressed
> > in some form (though maybe not sufficiently?).
>
> Questions were:
>
> 1. IOW, does the system boot fine? What's the impact of this being a module?

I believe this was answered in the cover letter.
" If the system does not load this module at startup, system will also
boot normally by using built-in architecture timer (in this case is
Arm Generic Timer) instead."

> 2. It is not the first time there is a proposal to convert the timers to
> modules. After asking, nobody came with a real study regarding the
> impact of the modularization of these drivers vs the time core framework
> and the benefit.

Maybe it would be helpful to be more specific in the criteria you're
looking for in such a study?

At least with the GKI, there is a need to support a wide array of
hardware, while minimizing the memory overhead of unnecessary code on
each device. As I mentioned in an earlier reply, this is kernel wide,
so while moving a single driver out to being a module isn't going to
be very substantial, the cumulative effect of not having to build in
every driver needed adds up.

So I think asking to see how much the kernel size changes by
modularizing this one initial driver is a reasonable request, though
I'd not expect it to be a huge gain on its own, but a reduction is a
reduction, and I'm not sure there are many clear downsides.

Again, it's not expected that every driver can be moved to a module,
as there are a number of cases where we need the functionality to be
present early in boot, and that's fine.

> 3. We need to tests that involve loading and unloading of such
> modules to see if the transition between this timer as broadcast and one
> CPU itself as broadcast happens correctly and system survives across such
> loading and unloading of the modules.

Modules may be permanent and not unloadable (this patch doesn't
provide a remove hook). While unloadable modules are abstractly nicer,
for supporting a wide array of hardware with minimal memory impact,
permanent modules are equally beneficial (only load them on hardware
that needs them, from that point there is no need to unload them). So
I'm not sure there's much practical value in unloading them.

As for testing the loading side, that sounds fair, though I'd expect
that to be done as part of developing the patch.

So maybe Walter can confirm the device works appropriately across many
boot ups where the module is loaded in their testing? If there is a
specific test criteria you would like to see, please clarify.

And, many thanks for re-outlining your concerns here! I appreciate it!

thanks
-john