linux-kernel - Re: [PATCH RFC] timer: of: Create a platform_device before the framework is initialized

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aGzp5esx1SpR9aL5@mai.linaro.org>
Date: Tue, 8 Jul 2025 11:50:29 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Arnd Bergmann <arnd@...db.de>
Cc: William McVicker <willmcvicker@...gle.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org,
	Lorenzo Pieralisi <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>,
	John Stultz <jstultz@...gle.com>, Stephen Boyd <sboyd@...nel.org>,
	Saravana Kannan <saravanak@...gle.com>,
	Linux-Arch <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

On Tue, Jul 01, 2025 at 09:52:45AM +0200, Arnd Bergmann wrote:
> On Tue, Jul 1, 2025, at 01:53, William McVicker wrote:
> >> @@ -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.
> 
> The patch doesn't actually have a module_platform_driver_probe()
> yet either, so loading the module wouldn't actually do anything.
> 
> I feel that this RFC by itself a good step in the direction we want, 
> so Daniel should go ahead with prototyping the next two steps:
> adding the platform_driver registration into OF_DECLARE_PDEV,
> and converting a driver so it can be used either with the _OF_DECLARE()
> or the platform_driver case.

I'm questioning the relevance of adding the macro when the driver is
not compiled as a module.

The first step of this macro is to allow the existing init functions
to be converted to the same signature as the module probe functions in
order to share the same code and take benefit of the devm_ variants
function which will considerably reduce the code size of the drivers.

Then we have the following situations:

 1. The driver has to be loaded very early TIMER_OF_DECLARE_PDEV
 (MODULE=no) the function timer-probe() is used

 2. The driver is a module_platform_driver() and MODULE=no, then it is
 built as a builtin_platform_driver(), we do not care about having it
 loaded by timer-probe()

 3. The driver is a module_platform_driver() and MODULE=yes

If we do the change to have the TIMER_OF_DECLARE_PDEV() adding the
platform_driver registration when MODULE=yes but using timer-probe
when MODULE=no, we change the initialization and we will have issues
with timers needing resources like SCMI clocks and where the
mechanisms rely on EPROBE_DEFER.

IMO, module_platform_driver and timer_probe must be separated.

Let's assume the one possible future use case with the VF PIT. This
timer is only used on ARM but now it is also supported for the ARM64
s32g2. For the first platform we need it very early and in the second
case, we need it later because the architected timers are there.

We should endup with:

static const struct of_device_id pit_timer_of_match[] = {
	{ .compatible = "nxp,s32g2-pit", .data = &s32g2_data },
	{ .compatible = "nxp,s32g3-pit", .data = &s32g3_data },
	{ }
};
MODULE_DEVICE_TABLE(of, pit_timer_of_match);

static struct platform_driver nxp_pit_driver = {
	.driver = {
		.name = "nxp-pit",
		.of_match_table = pit_timer_of_match,
	},
	.probe = pit_timer_probe,
};
module_platform_driver(nxp_pit_driver);

TIMER_OF_DECLARE_PDEV(vf610, "fsl,vf610-pit", pit_timer_probe);

If we change the TIMER_OF_DECLARE_PDEV to a macro which relies on
timer_probe when MODULE=no, then the "nxp-pit" on the s32gX will fail
to initialize because of the SCMI clocks not ready and the routine
won't reprobe the function. This issue won't happen with
builtin_platform_driver

What about something like:

TIMER_OF_DECLARE_PLATFORM_DRIVER(__name, __driver) \
  TIMER_OF_DECLARE_PDEV(__name, __driver->probe); \
#ifdef MODULE
  module_platform_driver(__driver);
#endif

Then in the timer_probe() we browse the of_match_table compatibles and
if the probe function succeed then we do of_node_set_flag(np,
OF_POPULATED) which is supposed to prevent calling the probe function
later.

Thoughts ?

--

 <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