| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aGMjfxIvbCkyR5rw@google.com>
Date: Mon, 30 Jun 2025 16:53:35 -0700
From: William McVicker <willmcvicker@...gle.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
lorenzo.pieralisi@...aro.org, Hans de Goede <hansg@...nel.org>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
Rob Herring <robh@...nel.org>, Thomas Gleixner <tglx@...utronix.de>,
Arnd Bergmann <arnd@...db.de>, John Stultz <jstultz@...gle.com>,
Stephen Boyd <sboyd@...nel.org>,
Saravana Kannan <saravanak@...gle.com>,
"open list:GENERIC INCLUDE/ASM HEADER FILES" <linux-arch@...r.kernel.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE" <devicetree@...r.kernel.org>
Subject: Re: [PATCH RFC] timer: of: Create a platform_device before the
framework is initialized
Hi Daniel,
On 06/25/2025, Daniel Lezcano wrote:
> In the context of the time keeping and the timers, some platforms have
> timers which need to be initialized very early. It is the case of the
> ARM platform which do not have the architected timers.
>
> The macro TIMER_OF_DECLARE adds an entry in the timer init functions
> array at compile time and the function timer_probe is called from the
> timer_init() function in kernel/time.c
>
> This array contains a t-uple with the init function and the compatible
> string.
>
> The init function has a device node pointer parameter.
>
> The timer_probe() function browses the of nodes and find the ones
> matching the compatible string given when using the TIMER_OF_DECLARE
> macro. It then calls the init function with the device node as a
> pointer.
>
> But there are some platforms where there are multiple timers like the
> ARM64 with the architected timers. Those are always initialized very
> early and the other timers can be initialized later.
>
> For this reason we find timer drivers with the platform_driver
> incarnation. Consequently their init functions are different, they
> have a platform_device pointer parameter and rely on the devm_
> function for rollbacking.
>
> To summarize, we have:
> - TIMER_OF_DECLARE with init function prototype:
> int (*init)(struct device_node *np);
>
> - module_platform_driver (and variant) with the probe function
> prototype:
> int (*init)(struct platform_device *pdev);
>
> The current situation with the timers is the following:
>
> - Two platforms can have the same timer hardware, hence the same
> driver but one without alternate timers and the other with multiple
> timers. For example, the Exynos platform has only the Exynos MCT on
> ARM but has the architeched timers in addition on the ARM64.
>
> - The timer drivers can be modules now which was not the case until
> recently. TIMER_OF_DECLARE do not allow the build as a module.
>
> It results in duplicate init functions (one with rollback and one with
> devm_) and different way to declare the driver (TIMER_OF_DECLARE and
> module_platform_driver).
>
> This proposed change is to unify the prototyping of the init functions
> to receive a platform_device pointer as parameter. Consequently, it
> will allow a smoother and nicer module conversion and a huge cleanup
> of the init functions by removing all the rollback code from all the
> timer drivers. It introduces a TIMER_OF_DECLARE_PDEV macro.
>
> If the macro is used a platform_device is manually allocated and
> initialized with the needed information for the probe
> function. Otherwise module_platform_driver can be use instead with the
> same probe function without the timer_probe() initialization.
>
> I don't have an expert knowledge of the platform_device internal
> subtilitie so I'm not sure if this approach is valid. However, it has
> been tested on a Rockchip board with the "rockchip,rk3288-timer" and
> verified the macro and the devm_ rollback work correctly.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
> Cc: Hans de Goede <hansg@...nel.org>
> Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> Cc: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
> Cc: Rob Herring <robh@...nel.org>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> ---
> drivers/clocksource/timer-probe.c | 61 ++++++++++++++++++++++++++++++-
> include/asm-generic/vmlinux.lds.h | 2 +
> include/linux/clocksource.h | 3 ++
> include/linux/of.h | 5 +++
> 4 files changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
> index b7860bc0db4b..6b2b341b8c95 100644
> --- a/drivers/clocksource/timer-probe.c
> +++ b/drivers/clocksource/timer-probe.c
> @@ -7,13 +7,18 @@
> #include <linux/init.h>
> #include <linux/of.h>
> #include <linux/clocksource.h>
> +#include <linux/platform_device.h>
>
> extern struct of_device_id __timer_of_table[];
> +extern struct of_device_id __timer_pdev_of_table[];
>
> static const struct of_device_id __timer_of_table_sentinel
> __used __section("__timer_of_table_end");
>
> -void __init timer_probe(void)
> +static const struct of_device_id __timer_pdev_of_table_sentinel
> + __used __section("__timer_pdev_of_table_end");
> +
> +static int __init timer_of_probe(void)
> {
> struct device_node *np;
> const struct of_device_id *match;
> @@ -38,6 +43,60 @@ void __init timer_probe(void)
> timers++;
> }
>
> + return timers;
> +}
> +
> +static int __init timer_pdev_of_probe(void)
> +{
> + struct device_node *np;
> + struct platform_device *pdev;
> + const struct of_device_id *match;
> + of_init_fn_pdev init_func;
> + unsigned int timers = 0;
> + int ret;
> +
> + for_each_matching_node_and_match(np, __timer_pdev_of_table, &match) {
> + if (!of_device_is_available(np))
> + continue;
> +
> + init_func = match->data;
> +
> + pdev = platform_device_alloc(of_node_full_name(np), -1);
> + if (!pdev)
> + continue;
> +
> + ret = device_add_of_node(&pdev->dev, np);
> + if (ret) {
> + platform_device_put(pdev);
> + continue;
> + }
> +
> + dev_set_name(&pdev->dev, pdev->name);
> +
> + ret = init_func(pdev);
> + if (!ret) {
> + timers++;
> + continue;
> + }
> +
> + if (ret != -EPROBE_DEFER)
> + pr_err("Failed to initialize '%pOF': %d\n", np,
> + ret);
> +
> + device_remove_of_node(&pdev->dev);
> +
> + platform_device_put(pdev);
> + }
> +
> + return timers;
> +}
> +
> +void __init timer_probe(void)
> +{
> + unsigned timers = 0;
> +
> + timers += timer_of_probe();
> + timers += timer_pdev_of_probe();
> timers += acpi_probe_device_table(timer);
>
> if (!timers)
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index fa5f19b8d53a..97606499c8d7 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -318,6 +318,7 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
> KEEP(*(__##name##_of_table_end))
>
> #define TIMER_OF_TABLES() OF_TABLE(CONFIG_TIMER_OF, timer)
> +#define TIMER_PDEV_OF_TABLES() OF_TABLE(CONFIG_TIMER_OF, timer_pdev)
> #define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
> #define CLK_OF_TABLES() OF_TABLE(CONFIG_COMMON_CLK, clk)
> #define RESERVEDMEM_OF_TABLES() OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
> @@ -714,6 +715,7 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
> CLK_OF_TABLES() \
> RESERVEDMEM_OF_TABLES() \
> TIMER_OF_TABLES() \
> + TIMER_PDEV_OF_TABLES() \
> CPU_METHOD_OF_TABLES() \
> CPUIDLE_METHOD_OF_TABLES() \
> KERNEL_DTB() \
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index 65b7c41471c3..0eeabd207040 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -289,6 +289,9 @@ extern int clocksource_i8253_init(void);
> #define TIMER_OF_DECLARE(name, compat, fn) \
> OF_DECLARE_1_RET(timer, name, compat, fn)
>
> +#define TIMER_OF_DECLARE_PDEV(name, compat, fn) \
> + OF_DECLARE_PDEV(timer_pdev, name, compat, fn)
> +
> #ifdef CONFIG_TIMER_PROBE
> extern void timer_probe(void);
> #else
> diff --git a/include/linux/of.h b/include/linux/of.h
> index a62154aeda1b..a312a6f5ecc1 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1540,9 +1540,12 @@ static inline int of_get_available_child_count(const struct device_node *np)
> _OF_DECLARE_STUB(table, name, compat, fn, fn_type)
> #endif
>
> +struct platform_device;
> +
> typedef int (*of_init_fn_2)(struct device_node *, struct device_node *);
> typedef int (*of_init_fn_1_ret)(struct device_node *);
> typedef void (*of_init_fn_1)(struct device_node *);
> +typedef int (*of_init_fn_pdev)(struct platform_device *);
>
> #define OF_DECLARE_1(table, name, compat, fn) \
> _OF_DECLARE(table, name, compat, fn, of_init_fn_1)
> @@ -1550,6 +1553,8 @@ typedef void (*of_init_fn_1)(struct device_node *);
> _OF_DECLARE(table, name, compat, fn, of_init_fn_1_ret)
> #define OF_DECLARE_2(table, name, compat, fn) \
> _OF_DECLARE(table, name, compat, fn, of_init_fn_2)
> +#define OF_DECLARE_PDEV(table, name, compat, fn) \
> + _OF_DECLARE(table, name, compat, fn, of_init_fn_pdev)
To support auto-module loading you'll need to also define the
MODULE_DEVICE_TABLE() as part of TIMER_OF_DECLARE_PDEV().
I haven't tested the patch yet, but aside from my comment above it LGTM.
Thanks,
Will
>
> /**
> * struct of_changeset_entry - Holds a changeset entry
> --
> 2.43.0
>
Powered by blists - more mailing lists