Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 23/05/2025 20:06, Saravana Kannan wrote:
> On Fri, May 23, 2025 at 10:06 AM William McVicker
> <willmcvicker@google.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@linaro.org> wrote:
>>>>>>> On Wed, Apr 02, 2025 at 04:33:57PM -0700, Will McVicker wrote:
>>>>>>>> From: Donghoon Yu <hoony.yu@samsung.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 -- 

Why do you want to unload ?

That is another aspect and the time framework is not totally ready for 
that. So I would consider for the moment to load only.

> I tried to find the answer to my
> question above and it wasn't clear (showing that it's not obvious).

Globally the idea would be to take a ref to the module when the 
clockevent or the clocksource is in use and release the ref when it is 
unused. That needs an extra function unregister_clockevent_device() and 
a verification of the current time core code to check if the ref is 
get/put correctly which is, after investigating a bit, not correct at 
the first glance.

>>> 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?

TIMER_OF_DECLARE relies on a mechanism building an array at compile 
time. It is called very early in the boot process.

What would be nice is to introduce something like 
TIMER_OF_MODULE_DECLARE() where builtin means TIMER_OF_DECLARE and 
module means module_platform_driver()



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog