Message-ID: <CAGETcx84OfLNRjMNGh4jS54_DgRuXx+gF5DhfiGrgckoyOfTMQ@mail.gmail.com>
Date: Fri, 23 May 2025 11:06:24 -0700
From: Saravana Kannan <saravanak@...gle.com>
To: William McVicker <willmcvicker@...gle.com>
Cc: Daniel Lezcano <daniel.lezcano@...aro.org>, John Stultz <jstultz@...gle.com>, 
	Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>, 
	Peter Griffin <peter.griffin@...aro.org>, André Draszik <andre.draszik@...aro.org>, 
	Tudor Ambarus <tudor.ambarus@...aro.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Alim Akhtar <alim.akhtar@...sung.com>, Thomas Gleixner <tglx@...utronix.de>, 
	Krzysztof Kozlowski <krzk@...nel.org>, Donghoon Yu <hoony.yu@...sung.com>, 
	Hosung Kim <hosung0.kim@...sung.com>, kernel-team@...roid.com, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	Youngmin Nam <youngmin.nam@...sung.com>, linux-samsung-soc@...r.kernel.org, 
	devicetree@...r.kernel.org
Subject: Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

On Fri, May 23, 2025 at 10:06 AM William McVicker
<willmcvicker@...gle.com> wrote:
>
> On 05/23/2025, Daniel Lezcano wrote:
> >
> > Hi William,
> >
> > On 15/05/2025 01:16, William McVicker wrote:
> > > On 05/13/2025, Daniel Lezcano wrote:
> > > > On Tue, Apr 15, 2025 at 05:48:41PM -0700, John Stultz wrote:
> > > > > On Tue, Apr 15, 2025 at 9:50 AM Daniel Lezcano
> > > > > <daniel.lezcano@...aro.org> wrote:
> > > > > > On Wed, Apr 02, 2025 at 04:33:57PM -0700, Will McVicker wrote:
> > > > > > > From: Donghoon Yu <hoony.yu@...sung.com>
> > > > > > >
> > > > > > > On Arm64 platforms the Exynos MCT driver can be built as a module. On
> > > > > > > boot (and even after boot) the arch_timer is used as the clocksource and
> > > > > > > tick timer. Once the MCT driver is loaded, it can be used as the wakeup
> > > > > > > source for the arch_timer.
> > > > > >
> > > > > >  From a previous thread where there is no answer:
> > > > > >
> > > > > > https://lore.kernel.org/all/c1e8abec-680c-451d-b5df-f687291aa413@linaro.org/
> > > > > >
> > > > > > I don't feel comfortable with changing the clocksource / clockevent drivers to
> > > > > > a module for the reasons explained in the aforementionned thread.
> > > > >
> > > > > I wasn't CC'ed on that, but to address a few of your points:
> > > > >
> > > > > > I have some concerns about this kind of changes:
> > > > > >
> > > > > >    * the core code may not be prepared for that, so loading / unloading
> > > > > > the modules with active timers may result into some issues
> > > > >
> > > > > That's a fair concern, but permanent modules (which are loaded but not
> > > > > unloaded) shouldn't suffer this issue. I recognize having modules be
> > > > > fully unloadable is generally cleaner and preferred, but I also see
> > > > > the benefit of allowing permanent modules to be one-way loaded so a
> > > > > generic/distro kernel shared between lots of different platforms
> > > > > doesn't need to be bloated with drivers that aren't used everywhere.
> > > > > Obviously any single driver doesn't make a huge difference, but all
> > > > > the small drivers together does add up.
> > > >
> > > > Perhaps using module_platform_driver_probe() should do the trick with
> > > > some scripts updated for my git hooks to check
> > > > module_platform_driver() is not used.
> > >
> > > Using `module_platform_driver_probe()` won't work as that still defines
> > > a `module_exit()` hook. If you want to automatically handle this in code, then
> > > the best approach is to follow what Saravana did in [1] for irqchip drivers.
> > > Basically by using `builtin_platform_driver(drv_name##_driver)`, you will only
> > > define the `module_init()` hook when the driver is compiled as a module which
> > > ensures you always get a permanent module.
> > >
> > > [1] https://lore.kernel.org/linux-arm-kernel/20200718000637.3632841-1-saravanak@google.com/
> >
> > Thanks for the pointer and the heads up regarding the module_exit() problem
> > with module_platform_driver_probe().
> >
> > After digging into the timekeeping framework it appears if the owner of the
> > clockevent device is set to THIS_MODULE, then the framework automatically
> > grabs a reference preventing unloading the module when this one is
> > registered.
> >
> > IMO it was not heavily tested but for me it is enough to go forward with the
> > module direction regarding the drivers.
>
> Great! Thanks for looking into that. I'll add that in the next revision and
> verify we can't unload the module.

Daniel, is the module_get() done when someone uses the clock source or
during registration? Also, we either want to support modules that can
be unloaded or we don't. In that case, it's better to make it explicit
in the macros too. It's clear and it's set where it matters. Not
hidden deep inside the code -- I tried to find the answer to my
question above and it wasn't clear (showing that it's not obvious).

>
> >
> > One point though, the condition:
> >
> > +#ifdef MODULE
> > [ ... ]
> > +static const struct of_device_id exynos4_mct_match_table[] = {
> > +     { .compatible = "samsung,exynos4210-mct", .data = &mct_init_spi, },
> > +     { .compatible = "samsung,exynos4412-mct", .data = &mct_init_ppi, },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, exynos4_mct_match_table);
> > +
> > +static struct platform_driver exynos4_mct_driver = {
> > +     .probe          = exynos4_mct_probe,
> > +     .driver         = {
> > +             .name   = "exynos-mct",
> > +             .of_match_table = exynos4_mct_match_table,
> > +     },
> > +module_platform_driver(exynos4_mct_driver);
> > +#else
> >  TIMER_OF_DECLARE(exynos4210, "samsung,exynos4210-mct", mct_init_spi);
> >  TIMER_OF_DECLARE(exynos4412, "samsung,exynos4412-mct", mct_init_ppi);
> > +#endif
> >
> >  is not acceptable as is. We don't want to do the same in all the drivers.
>
> Are you suggesting we create a new timer macro to handle if we want to use
> TIMER_OF_DECLARE() or builtin_platform_driver()?

One you convert a driver to tristate, there's no reason to continue
using TIMER_OF_DECLARE. Just always do the "module" approach. If it
gets built in, it'll just initialize early?

What am I missing?

Thanks,
Saravana

>
> > Furthermore, we should not assume if the modules are enabled in the kernel
> > that implies the driver is compiled as a module.
>
> Ah yes, that's right. The ifdef should be checking
> CONFIG_CLKSRC_EXYNOS_MCT_MODULE.
>
> Thanks,
> Will

Powered by blists - more mailing lists