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

[Saravana Kannan]

On Tue, Mar 24, 2020 at 11:34 AM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> On Tue, Mar 24, 2020 at 11:00 AM Ionela Voinescu
> <ionela.voinescu@xxxxxxx> wrote:
> >
> > 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.
>
> I'll reply later today after I take a closer look. But will something
> like what timer-ingenic.c did work for you? You can clear the flag
> inside your early init.

Firstly, sorry my patch broke your platform.

I took a closer look. So two different drivers [1] [2] are saying they
know how to handle "arm,vexpress-sysreg" and are expecting to run at
the same time. That seems a bit unusual to me. I wonder if this is a
violation of the device-driver model because this expectation would
never be allowed if these device drivers were actual drivers
registered with driver-core. But that's a discussion for another time.

To fix this issue you are facing, this patch should work:
https://lore.kernel.org/lkml/20200324195302.203115-1-saravanak@xxxxxxxxxx/T/#u

Can you please test it and give a Tested-by?

Thanks,
Saravana

[1] drivers/mfd/vexpress-sysreg.c:  { .compatible = "arm,vexpress-sysreg", },
[2] drivers/clocksource/timer-versatile.c:TIMER_OF_DECLARE(vexpress,
"arm,vexpress-sysreg"