Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[Ionela Voinescu]

Hi guys,

On Thursday 19 Mar 2020 at 08:47:46 (-0000), tip-bot2 for Saravana Kannan wrote:
> The following commit has been merged into the timers/core branch of tip:
> 
> Commit-ID:     4f41fe386a94639cd9a1831298d4f85db5662f1e
> Gitweb:        https://git.kernel.org/tip/4f41fe386a94639cd9a1831298d4f85db5662f1e
> Author:        Saravana Kannan <saravanak@xxxxxxxxxx>
> AuthorDate:    Fri, 10 Jan 2020 21:21:25 -08:00
> Committer:     Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> CommitterDate: Tue, 17 Mar 2020 13:10:07 +01:00
> 
> clocksource/drivers/timer-probe: Avoid creating dead devices
> 
> Timer initialization is done during early boot way before the driver
> core starts processing devices and drivers. Timers initialized during
> this early boot period don't really need or use a struct device.
> 
> However, for timers represented as device tree nodes, the struct devices
> are still created and sit around unused and wasting memory. This change
> avoid this by marking the device tree nodes as "populated" if the
> corresponding timer is successfully initialized.
> 
> Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> Link: https://lore.kernel.org/r/20200111052125.238212-1-saravanak@xxxxxxxxxx
> ---
>  drivers/clocksource/timer-probe.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
> index ee9574d..a10f28d 100644
> --- a/drivers/clocksource/timer-probe.c
> +++ b/drivers/clocksource/timer-probe.c
> @@ -27,8 +27,10 @@ void __init timer_probe(void)
>  
>  		init_func_ret = match->data;
>  
> +		of_node_set_flag(np, OF_POPULATED);
>  		ret = init_func_ret(np);
>  		if (ret) {
> +			of_node_clear_flag(np, OF_POPULATED);
>  			if (ret != -EPROBE_DEFER)
>  				pr_err("Failed to initialize '%pOF': %d\n", np,
>  				       ret);
> 

This patch is creating problems on some vexpress platforms - ones that
are using CLKSRC_VERSATILE (drivers/clocksource/timer-versatile.c).
I noticed issues on TC2 and FVPs (fixed virtual platforms) starting with
next-20200318 and still reproducible with next-20200323.

It seems the issue this patch causes on TC2 and FVP is related to the
vexpress-sysreg node being used early for sched_clock_init
(timer_versatile.c: versatile_sched_clock_init). At this point (at
time_init) the node will be marked as OF_POPULATED, which flags that a
device is already created for it, but it is not, in this case.

Later at sysreg_init (vexpress-sysreg.c) a device will fail to be created
for it, as one already exists. This will result in a failure to create a
bridge and a system controller for a bunch of devices (mostly clocks and
regulators).

I think on the FVP it does not cause many issues as clocks are fixed and
regulator settings are probably nops so it boots fine and throws only
some warnings. On TC2 on the other hand it fails to boot and it hangs at
starting the kernel.

In my opinion the idea of the patch is not bad, but I'm not an expert on
this so the most I can offer for now is the basic understanding of the
issue. I've Cc-ed a few folks to potentially suggest alternatives/fixes.

For now, reverting this patch solves the problems on both platforms.
I tested this on next-20200318 which introduced the problem.

Hope it helps,
Ionela.