Message-ID: <CANDhNCr5n+HtHQEqCq0ZxbvX-nC3u9ewJ1_fj0h1gFQZ3nB8iA@mail.gmail.com>
Date: Wed, 16 Apr 2025 12:48:56 -0700
From: John Stultz <jstultz@...gle.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: Will McVicker <willmcvicker@...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>, 
	Saravana Kannan <saravanak@...gle.com>, 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 Wed, Apr 16, 2025 at 6:46 AM Daniel Lezcano
<daniel.lezcano@...aro.org> 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:
> > > 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.
>
> Yes, I agree.
>
> So the whole clockevent / clocksource drivers policy would have to be making
> impossible to unload a module once it is loaded.
>
> Do you have any ideas how to ensure that the converted drivers follow this
> rule without putting more burden on the maintainer?

Permanent modules just don't have a module_exit() hook, so that is
pretty easy to look for.
Obviously, I don't want to add more burden to the maintainership.

>From a given clockevent driver (or maybe a function pointer), we could
check on the registration by calling __module_address(addr) [thanks to
Sami Tolvanen for pointing that function out to me] on one of the
function pointers provided, and check that there isn't a module->exit
pointer.

For clocksources we already have an owner pointer in the clocksource
struct that the driver is supposed to set if it's a module, but
clocksources already handle the get/puts needed to prevent modules
unloading under us.

> > > * the timekeeping may do jump in the past [if and] when switching the
> > > clocksource
> >
> > ? It shouldn't. We've had tests in kselftest that switch between
> > clocksources checking for inconsistencies for awhile, so if such a
> > jump occurred it would be considered a bug.
>
> But in the context of modules, the current clocksource counter is running but
> what about the clocksource's counter related to the module which will be
> started when the driver is loaded and then switches to the new clocksource. Is
> it possible in this case there is a time drift between the clocksource which
> was started at boot time and the one started later when the module is loaded ?

The clocksource code already has support for modules, and will do the
module_get and call enable hooks before switching to the new
clocksource. So the clocksource from the module has to be functioning
and running before timekeeping switches to using it.

By drift, its true changing clocksources can change the underlying
crystal so NTP has to begin again to determine any long-term frequency
adjustment needed, but we signal that properly.  And there should be
no time inconsistency during the switch as we accumulate everything
from the current clocksource and read a new base value from the new
clocksource. If there is, it's a bug.

> > >  * the GKI approach is to have an update for the 'mainline' kernel and
> > > let the different SoC vendors deal with their drivers. I'm afraid this
> > > will prevent driver fixes to be carry on upstream because they will stay
> > > in the OoT kernels
> >
> > I'm not sure I understand this point?  Could you expand on it a bit?
> > While I very much can understand concerns and potential downsides of
> > the GKI approach, I'm not sure how that applies to the submission
> > here, as the benefit would apply to classic distro kernels as much as
> > GKI.
>
> Ok let's consider my comment as out of the technical aspects of the changes. I
> can clarify it but it does not enter into consideration for the module
> conversion. It is an overall feeling about the direction of all in modules for
> GKI policy. I'm a little worried about changes not carried on mainline because
> it is no longer an obstacle to keep OoT drivers. The core kernel is mainline
> but the drivers can be vendor provided as module. I understand it is already
> the case but the time framework is the base brick of the system, so there is
> the risk a platform is supported with less than the minimum functionality.

So separately from this patch submission, I agree that the GKI
approach does not enforce vendor participation upstream. But there is
no rule *anywhere* that makes folks participate, and with the old
vendor trees it was definitely worse.

The GKI does result in vendors having a common interest in the
*actual* common portions of the kernel to be working well, so we can
make sure things like bug fixes, etc are submitted upstream first.
That is a clear benefit over separate vendor trees, but it's not a
magic tool to get everyone submitting all of their code upstream.

Trying to cajole upstream participation via barriers (not supporting
modular drivers upstream to try to enforce vendors submit patches to
add built in drivers for support) won't really work because they will
just be enabled as modules anyway out of tree. And it's hard to argue
against, as there isn't really a technical benefit to the GKI
requiring lots of SoC specific hardware support be built in.  So it
only ends up being another reason to not bother with upstream.

<Sorry, I'm getting a bit soap-boxy here>
I personally think the best tool we have to improve participation and
collaboration with the community is to do what we can to make it a
positive/beneficial/worthwhile experience. Every time I've submitted
patches and had bugs pointed out or fixes suggested, is a huge value
to me, and I have tremendous appreciation for folks sharing their
knowledge and expertise. And every time a patch I send gets merged or
a bug I reported ends up being fixed, there is a real sense of pride
in the contribution made to such an important project. Then, to get to
a point of a maintainership, and to be able to consider these amazing
influential developers as (relative)peers feels like such a career
accomplishment!  As an individual, those moments feel awesome and
motivate me over and over to want to reach out and share patches or
issues or thoughts.

And yes, there are organizations that focus more on how to exploit the
community for their benefit without contributing, and I get the
protective reaction that maintainers have to that. But I also know
there is *a lot* of amazing expertise inside the heads of
*individuals* who don't participate because they don't feel the "us vs
them" combative interactions are worth it. I think we/the community
are missing out, and those folks are the ones we should be trying to
welcome and include in order to build up our community.

Maintainers and the community need to keep high technical standards
and make the right long term choices, and developers won't agree all
the time on what those are, and I think that's all fine! But I think
if we want to grow the community and have more participation (as well
as growing folks into maintainers), I think we'll have more success
focusing on the honey than the vinegar.

thanks
-john

Powered by blists - more mailing lists