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.
Signed-off-by: Donghoon Yu <hoony.yu@samsung.com>
Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
[original commit from https://android.googlesource.com/kernel/gs/+/8a52a8288ec7d88ff78f0b37480dbb0e9c65bbfd]
Signed-off-by: Will McVicker <willmcvicker@google.com>
---
drivers/clocksource/Kconfig | 3 +-
drivers/clocksource/exynos_mct.c | 49 +++++++++++++++++++++++++++-----
2 files changed, 44 insertions(+), 8 deletions(-)
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 487c85259967..e89373827c3a 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -443,7 +443,8 @@ config ATMEL_TCB_CLKSRC
Support for Timer Counter Blocks on Atmel SoCs.
config CLKSRC_EXYNOS_MCT
- bool "Exynos multi core timer driver" if COMPILE_TEST
+ tristate "Exynos multi core timer driver" if ARM64
+ default y if ARCH_EXYNOS || COMPILE_TEST
depends on ARM || ARM64
depends on ARCH_ARTPEC || ARCH_EXYNOS || COMPILE_TEST
help
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 62febeb4e1de..8943274378be 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -15,9 +15,11 @@
#include <linux/cpu.h>
#include <linux/delay.h>
#include <linux/percpu.h>
+#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_irq.h>
#include <linux/of_address.h>
+#include <linux/platform_device.h>
#include <linux/clocksource.h>
#include <linux/sched_clock.h>
@@ -241,7 +243,7 @@ static cycles_t exynos4_read_current_timer(void)
}
#endif
-static int __init exynos4_clocksource_init(bool frc_shared)
+static int exynos4_clocksource_init(bool frc_shared)
{
/*
* When the frc is shared, the main processor should have already
@@ -511,7 +513,7 @@ static int exynos4_mct_dying_cpu(unsigned int cpu)
return 0;
}
-static int __init exynos4_timer_resources(struct device_node *np)
+static int exynos4_timer_resources(struct device_node *np)
{
struct clk *mct_clk, *tick_clk;
@@ -539,7 +541,7 @@ static int __init exynos4_timer_resources(struct device_node *np)
* @local_idx: array mapping CPU numbers to local timer indices
* @nr_local: size of @local_idx array
*/
-static int __init exynos4_timer_interrupts(struct device_node *np,
+static int exynos4_timer_interrupts(struct device_node *np,
unsigned int int_type,
const u32 *local_idx,
size_t nr_local)
@@ -652,7 +654,7 @@ static int __init exynos4_timer_interrupts(struct device_node *np,
return err;
}
-static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
+static int mct_init_dt(struct device_node *np, unsigned int int_type)
{
bool frc_shared = of_property_read_bool(np, "samsung,frc-shared");
u32 local_idx[MCT_NR_LOCAL] = {0};
@@ -700,15 +702,48 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
return exynos4_clockevent_init();
}
-
-static int __init mct_init_spi(struct device_node *np)
+static int mct_init_spi(struct device_node *np)
{
return mct_init_dt(np, MCT_INT_SPI);
}
-static int __init mct_init_ppi(struct device_node *np)
+static int mct_init_ppi(struct device_node *np)
{
return mct_init_dt(np, MCT_INT_PPI);
}
+
+#ifdef MODULE
+static int exynos4_mct_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ int (*mct_init)(struct device_node *np);
+
+ mct_init = of_device_get_match_data(dev);
+ if (!mct_init)
+ return -EINVAL;
+
+ return mct_init(dev->of_node);
+}
+
+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
+
+MODULE_DESCRIPTION("Exynos Multi Core Timer Driver");
+MODULE_LICENSE("GPL");
--
2.49.0.472.ge94155a9ec-googHi Will,
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.
Before this could be accepted, I really need a strong acked-by from Thomas
Thanks
-- Daniel
> Signed-off-by: Donghoon Yu <hoony.yu@samsung.com>
> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> [original commit from https://android.googlesource.com/kernel/gs/+/8a52a8288ec7d88ff78f0b37480dbb0e9c65bbfd]
> Signed-off-by: Will McVicker <willmcvicker@google.com>
> ---
> drivers/clocksource/Kconfig | 3 +-
> drivers/clocksource/exynos_mct.c | 49 +++++++++++++++++++++++++++-----
> 2 files changed, 44 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 487c85259967..e89373827c3a 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -443,7 +443,8 @@ config ATMEL_TCB_CLKSRC
> Support for Timer Counter Blocks on Atmel SoCs.
>
> config CLKSRC_EXYNOS_MCT
> - bool "Exynos multi core timer driver" if COMPILE_TEST
> + tristate "Exynos multi core timer driver" if ARM64
> + default y if ARCH_EXYNOS || COMPILE_TEST
> depends on ARM || ARM64
> depends on ARCH_ARTPEC || ARCH_EXYNOS || COMPILE_TEST
> help
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index 62febeb4e1de..8943274378be 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -15,9 +15,11 @@
> #include <linux/cpu.h>
> #include <linux/delay.h>
> #include <linux/percpu.h>
> +#include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_irq.h>
> #include <linux/of_address.h>
> +#include <linux/platform_device.h>
> #include <linux/clocksource.h>
> #include <linux/sched_clock.h>
>
> @@ -241,7 +243,7 @@ static cycles_t exynos4_read_current_timer(void)
> }
> #endif
>
> -static int __init exynos4_clocksource_init(bool frc_shared)
> +static int exynos4_clocksource_init(bool frc_shared)
> {
> /*
> * When the frc is shared, the main processor should have already
> @@ -511,7 +513,7 @@ static int exynos4_mct_dying_cpu(unsigned int cpu)
> return 0;
> }
>
> -static int __init exynos4_timer_resources(struct device_node *np)
> +static int exynos4_timer_resources(struct device_node *np)
> {
> struct clk *mct_clk, *tick_clk;
>
> @@ -539,7 +541,7 @@ static int __init exynos4_timer_resources(struct device_node *np)
> * @local_idx: array mapping CPU numbers to local timer indices
> * @nr_local: size of @local_idx array
> */
> -static int __init exynos4_timer_interrupts(struct device_node *np,
> +static int exynos4_timer_interrupts(struct device_node *np,
> unsigned int int_type,
> const u32 *local_idx,
> size_t nr_local)
> @@ -652,7 +654,7 @@ static int __init exynos4_timer_interrupts(struct device_node *np,
> return err;
> }
>
> -static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
> +static int mct_init_dt(struct device_node *np, unsigned int int_type)
> {
> bool frc_shared = of_property_read_bool(np, "samsung,frc-shared");
> u32 local_idx[MCT_NR_LOCAL] = {0};
> @@ -700,15 +702,48 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
> return exynos4_clockevent_init();
> }
>
> -
> -static int __init mct_init_spi(struct device_node *np)
> +static int mct_init_spi(struct device_node *np)
> {
> return mct_init_dt(np, MCT_INT_SPI);
> }
>
> -static int __init mct_init_ppi(struct device_node *np)
> +static int mct_init_ppi(struct device_node *np)
> {
> return mct_init_dt(np, MCT_INT_PPI);
> }
> +
> +#ifdef MODULE
> +static int exynos4_mct_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + int (*mct_init)(struct device_node *np);
> +
> + mct_init = of_device_get_match_data(dev);
> + if (!mct_init)
> + return -EINVAL;
> +
> + return mct_init(dev->of_node);
> +}
> +
> +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
> +
> +MODULE_DESCRIPTION("Exynos Multi Core Timer Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.49.0.472.ge94155a9ec-goog
>
--
<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
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. > * it may end up with some interactions with cpuidle at boot time and > the broadcast timer Do you have more details as to your concerns here? I know there can be cases of issues if the built in clockevent drivers are problematic and the working ones don't load until later, you can have races where if the system goes into idle before the module loads it could stall out (there was a recent issue with an older iMac TSC halting in idle and it not reliably getting disqualified before it got stuck in idle). In those cases I could imagine folks reasonably arguing for including the working clock as a built in, but I'm not sure I'd say forcing everything to be built in is the better approach. > * 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. > * 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. I realize in the time since I started this reply, Will has already covered much of the above! So apologies for being redundant. That said, there are some non-modularization changes in this series that should be considered even if the modularization logic is a continued sticking point. -john
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. [ ... ] -- <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
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/ Regards, Will [...]
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.
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. Furthermore, we should not assume if the modules are enabled in
the kernel that implies the driver is compiled as a module.
--
<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
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.
>
> 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()?
> 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
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 -- 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
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
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. 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? > > * it may end up with some interactions with cpuidle at boot time and > > the broadcast timer > > Do you have more details as to your concerns here? I know there can be > cases of issues if the built in clockevent drivers are problematic and > the working ones don't load until later, you can have races where if > the system goes into idle before the module loads it could stall out > (there was a recent issue with an older iMac TSC halting in idle and > it not reliably getting disqualified before it got stuck in idle). Yes, that is that kind of issue I suspect. > In > those cases I could imagine folks reasonably arguing for including the > working clock as a built in, but I'm not sure I'd say forcing > everything to be built in is the better approach. When the first driver converted as a module will be accepted, I'm pretty sure there will be a wave of patches to convert more drivers into modules. What tool / use cases / tests can we put in place to ensure it is not breaking the existing platforms for the different configurations? > > * 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 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. -- <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
On Wed, Apr 16, 2025 at 6:46 AM Daniel Lezcano <daniel.lezcano@linaro.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@linaro.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
On Wed, Apr 16, 2025 at 12:48 PM John Stultz <jstultz@google.com> wrote: > On Wed, Apr 16, 2025 at 6:46 AM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: > > 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. Saravana also pointed out to me another approach that the irqchip code uses: macros to populate an owner field with THIS_MODULE so that one can easily get to the module struct https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/irqchip.h#n41 thanks -john
On 04/15/2025, Daniel Lezcano wrote: > Hi Will, Hi Daniel, > > 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. > > Before this could be accepted, I really need a strong acked-by from Thomas Thanks for the response! I'll copy-and-paste your replies from that previous thread and try to address your concerns. > * 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 can't speak for that specific thread or their intent, but I can speak to this thread and our intent. This whole patch series is about upstreaming the downstream changes. So saying this will prevent others from upstreaming changes is punishing the folks who are actually trying to upstream changes. I don't think that's a fair way to handle this. Also, rejecting this series will not prevent people from upstreaming their changes, it'll just make it more unlikely because they now have to deal with upstreaming more changes that were rejected in the past. That's daunting for someone who doesn't do upstreaming often. I'm telling this from experience dealing with SoC vendors and asking them to upstream stuff. With that said, let me try to address some of your technical concerns. > * the core code may not be prepared for that, so loading / unloading > the modules with active timers may result into some issues We had the same concern for irqchip drivers. We can easily disable unloading for these clocksource modules just like we did for irqchip by making them permanent modules. > * it may end up with some interactions with cpuidle at boot time and > the broadcast timer If I'm understanding this correctly, no driver is guaranteed to probe at initialization time regardless of whether it is built-in or a module. Taking a look at the other clocksource drivers, I found that the following drivers are all calling `clocksource_register_hz()` and `clockevents_config_and_register()` at probe time. timer-sun5i.c sh_tmu.c sh_cmt.c timer-tegra186.c timer-stm32-lp.c (only calls clockevents_config_and_register()) So this concern is unrelated to building these drivers are modules. Please let me know if I'm missing something here. > * the timekeeping may do jump in the past [if and] when switching the > clocksource Can you clarify how this relates to modules? IIUC, the clocksource can be changed anytime by writing to: /sys/devices/system/clocksource/clocksource0/current_clocksource If there's a bug related to timekeeping and changing the clocksource, then that should be handled separately from the modularization code. For ARM64 in general, the recommendation is to use the ARM architected timer which is not a module and is used for scheduling and timekeeping. While the Exynos MCT driver can functionally be used as the primary clocksource, it's not recommended due to performance issues. So building the MCT driver as a kernel module really shouldn't be an issue and has been thoroughly testing on several generations of Pixel devices which is why we are trying to upstream our downstream technical debt (so we can directly using the upstream version of the Exynos MCT driver). Thanks, Will [...]
On Tue, Apr 15, 2025 at 02:25:43PM -0700, William McVicker wrote: > On 04/15/2025, Daniel Lezcano wrote: > > Hi Will, > > Hi Daniel, > > > > > 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. > > > > Before this could be accepted, I really need a strong acked-by from Thomas > > Thanks for the response! I'll copy-and-paste your replies from that previous > thread and try to address your concerns. > > > * 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 can't speak for that specific thread or their intent, but I can speak to this > thread and our intent. > > This whole patch series is about upstreaming the downstream changes. So saying > this will prevent others from upstreaming changes is punishing the folks who > are actually trying to upstream changes. I don't think that's a fair way to > handle this. > > Also, rejecting this series will not prevent people from upstreaming their > changes, it'll just make it more unlikely because they now have to deal with > upstreaming more changes that were rejected in the past. That's daunting for > someone who doesn't do upstreaming often. I'm telling this from experience > dealing with SoC vendors and asking them to upstream stuff. > > With that said, let me try to address some of your technical concerns. I won't reject the series based on my opinion. Answering the technical concerns will prevail. Why is it needed to convert the timer into a module ? > > * the core code may not be prepared for that, so loading / unloading > > the modules with active timers may result into some issues > > We had the same concern for irqchip drivers. We can easily disable unloading > for these clocksource modules just like we did for irqchip by making them > permanent modules. In the clockevent / clocksource initialization process, depending on the platform, some are needed very early and other can be loaded later. For example, the usual configuration is the architected timers are initialized very early, then the external timer is loaded a bit later. And when this one is loaded it does not take over the architected timers. It acts as a "broadcast" timer to program the next timer event when the current CPU is going to an idle state where the local timer is stopped. Other cases are the architected timers are not desired and the 'external' timer is used in place when it is loaded with a higher rating. Some configuration can mimic local timers by settting a per CPU timer. Some platforms could be without the architected timers, so the 'external' timer is used. Let's imagine the system started, the timers are running and then we load a module with a timer replacing the current ones. Does it work well ? Are we sure, the timer modularization is compatible with all the timer use cases ? > > * it may end up with some interactions with cpuidle at boot time and > > the broadcast timer > > If I'm understanding this correctly, no driver is guaranteed to probe at > initialization time regardless of whether it is built-in or a module. Taking > a look at the other clocksource drivers, I found that the following drivers are > all calling `clocksource_register_hz()` and `clockevents_config_and_register()` > at probe time. > > timer-sun5i.c > sh_tmu.c > sh_cmt.c > timer-tegra186.c > timer-stm32-lp.c (only calls clockevents_config_and_register()) > > So this concern is unrelated to building these drivers are modules. Please let > me know if I'm missing something here. We would have to check each platform individually to answer this question. The interaction between cpuidle and the timer module is about not having a broadcast timer when cpuidle initializes and then having it later when the module is loaded. Did you check the deep idle states are used after loading the module ? > > * the timekeeping may do jump in the past [if and] when switching the > > clocksource > > Can you clarify how this relates to modules? IIUC, the clocksource can be > changed anytime by writing to: > > /sys/devices/system/clocksource/clocksource0/current_clocksource The clocksource counter is stopped when it is not the current one. So when you switch it, the new clocksource counter could be different and can make you jump back in time. > If there's a bug related to timekeeping and changing the clocksource, then that > should be handled separately from the modularization code. I disagree :) The whole point is the time framework may not be totally immune against the timer modularization. It is about identifying the corner cases where the timer driver modularization can have an impact and set the scene to support it. > For ARM64 in general, the recommendation is to use the ARM architected timer > which is not a module and is used for scheduling and timekeeping. While the > Exynos MCT driver can functionally be used as the primary clocksource, it's not > recommended due to performance issues. So building the MCT driver as a kernel > module really shouldn't be an issue and has been thoroughly testing on several > generations of Pixel devices which is why we are trying to upstream our > downstream technical debt (so we can directly using the upstream version of the > Exynos MCT driver). The discussion is not about only the Exynos MCT but as you are not the first one asking to convert the timer driver to a module, we should check what could be the impact on the time framework and the system in general. Others proposed to convert to module and I asked to investigate the impact. Nobody came back with a clear answer and there is no feedback from Thomas. > [...] -- <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
Hi Daniel, [..] > > > 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: Aside from the timer modularization part here which is proving a bit contentious are you OK with queueing the other parts of this MCT series? As I need some of the MCT driver changes to enable CPUIdle on Pixel 6 upstream. Exiting from c2 idle state is broken for gs101 without some of these MCT changes. [..] > > > > > > 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. > > > > > > Before this could be accepted, I really need a strong acked-by from Thomas > > > > Thanks for the response! I'll copy-and-paste your replies from that previous > > thread and try to address your concerns. > > > > > * 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 can't speak for that specific thread or their intent, but I can speak to this > > thread and our intent. > > > > This whole patch series is about upstreaming the downstream changes. So saying > > this will prevent others from upstreaming changes is punishing the folks who > > are actually trying to upstream changes. I don't think that's a fair way to > > handle this. > > > > Also, rejecting this series will not prevent people from upstreaming their > > changes, it'll just make it more unlikely because they now have to deal with > > upstreaming more changes that were rejected in the past. That's daunting for > > someone who doesn't do upstreaming often. I'm telling this from experience > > dealing with SoC vendors and asking them to upstream stuff. > > > > With that said, let me try to address some of your technical concerns. > > I won't reject the series based on my opinion. Answering the technical concerns > will prevail. > > Why is it needed to convert the timer into a module ? MCT is hardware very specific to Exynos based SoCs. Forcing this built-in is just bloat for many other systems that will never use it. The aim of this series is to upstream the downstream OOT code with the goal of then switching over to the upstream version of the driver. I agree with your points though that we should be sure the timer framework can handle this, and we aren't introducing subtle bugs. > > > > * the core code may not be prepared for that, so loading / unloading > > > the modules with active timers may result into some issues > > > > We had the same concern for irqchip drivers. We can easily disable unloading > > for these clocksource modules just like we did for irqchip by making them > > permanent modules. > > In the clockevent / clocksource initialization process, depending on the > platform, some are needed very early and other can be loaded later. > > For example, the usual configuration is the architected timers are initialized > very early, then the external timer is loaded a bit later. And when this one is > loaded it does not take over the architected timers. It acts as a "broadcast" > timer to program the next timer event when the current CPU is going to an idle > state where the local timer is stopped. > > Other cases are the architected timers are not desired and the 'external' timer > is used in place when it is loaded with a higher rating. Some configuration can > mimic local timers by settting a per CPU timer. > > Some platforms could be without the architected timers, so the 'external' timer > is used. > > Let's imagine the system started, the timers are running and then we load a > module with a timer replacing the current ones. Does it work well ? > > Are we sure, the timer modularization is compatible with all the timer use cases ? It's a good question. We can say it's been used as a module on multiple generations of Pixel devices in production without issues. Whether that covers all timer use cases I can't say. > > > > * it may end up with some interactions with cpuidle at boot time and > > > the broadcast timer > > > > If I'm understanding this correctly, no driver is guaranteed to probe at > > initialization time regardless of whether it is built-in or a module. Taking > > a look at the other clocksource drivers, I found that the following drivers are > > all calling `clocksource_register_hz()` and `clockevents_config_and_register()` > > at probe time. > > > > timer-sun5i.c > > sh_tmu.c > > sh_cmt.c > > timer-tegra186.c > > timer-stm32-lp.c (only calls clockevents_config_and_register()) > > > > So this concern is unrelated to building these drivers are modules. Please let > > me know if I'm missing something here. > > We would have to check each platform individually to answer this question. > > The interaction between cpuidle and the timer module is about not having a > broadcast timer when cpuidle initializes and then having it later when the > module is loaded. Did you check the deep idle states are used after loading the > module ? For gs101 / Oriole deep idle states are used after loading the module as if the MCT timer *isn't* used then the CPU can't wake up from c2 and eventually there are no CPUs left leading to a hang. > > The discussion is not about only the Exynos MCT but as you are not the first > one asking to convert the timer driver to a module, we should check what could > be the impact on the time framework and the system in general. > > Others proposed to convert to module and I asked to investigate the impact. > Nobody came back with a clear answer and there is no feedback from Thomas. Hopefully the above answers go a little bit towards giving a degree of confidence that this is a safe change :) regards, Peter.
© 2016 - 2026 Red Hat, Inc.