Re: [PATCH v1] clocksource: Avoid creating dead devices

[Saravana Kannan]

On Mon, Mar 16, 2020 at 11:07 AM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:
>
> On 16/03/2020 18:49, Saravana Kannan wrote:
> > On Mon, Mar 16, 2020 at 7:57 AM Daniel Lezcano
> > <daniel.lezcano@xxxxxxxxxx> wrote:
> >>
> >> On 08/03/2020 06:53, Saravana Kannan wrote:
> >>> On Wed, Mar 4, 2020 at 11:56 AM Daniel Lezcano
> >>> <daniel.lezcano@xxxxxxxxxx> wrote:
> >>>>
> >>>> On 04/03/2020 20:30, Saravana Kannan wrote:
> >>>>> On Thu, Feb 27, 2020 at 1:22 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> >>>>>>
> >>>>>> On Thu, Feb 27, 2020 at 1:06 AM Daniel Lezcano
> >>>>>> <daniel.lezcano@xxxxxxxxxx> wrote:
> >>>>>>>
> >>>>>>> On 11/01/2020 06:21, Saravana Kannan wrote:
> >>>>>>>> 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.
> >>>>
> >>>> TBH, I'm missing the rational with the explanation and the code. Can you
> >>>> elaborate or rephrase it?
> >>>
> >>> Ok, let me start from the top.
> >>>
> >>> When the kernel boots, timer_probe() is called (via time_init()) way
> >>> before any of the initcalls are called in do_initcalls().
> >>>
> >>> In systems with CONFIG_OF, of_platform_default_populate_init() gets
> >>> called at arch_initcall_sync() level.
> >>> of_platform_default_populate_init() is what kicks off creating
> >>> platform devices from device nodes in DT. However, if the struct
> >>> device_node that corresponds to a device node in DT has OF_POPULATED
> >>> flag set, a platform device is NOT created for it (because it's
> >>> considered already "populated"/taken care of).
> >>>
> >>> When a timer driver registers using TIMER_OF_DECLARE(), the driver's
> >>> init code is called from timer_probe() on the struct device_node that
> >>> corresponds to the timer device node. At this point the timer is
> >>> already "probed". If you don't mark this device node with
> >>> OF_POPULATED, at arch_initcall_sync() it's going to have a pointless
> >>> struct platform_device created that's just using up memory and
> >>> pointless.
> >>>
> >>> So my patch sets the OF_POPULATED flag for all timer device_node's
> >>> that are successfully probed from timer_probe().
> >>>
> >>> If a timer driver doesn't use TIMER_OF_DECLARE() and just registers as
> >>> a platform device, the driver init function won't be called from
> >>> timer_probe() and it's corresponding devices won't have OF_POPULATED
> >>> set in their device_node. So platform_devices will be created for them
> >>> and they'll probe as normal platform devices. This is why my change
> >>> doesn't break drivers/clocksource/ingenic-timer.c.
> >>>
> >>> Btw, this is no different from what irqchip does with IRQCHIP_DECLARE.
> >>>
> >>> Hope that clears it up.
> >>
> >> Yes, thanks for the explanation.
> >>
> >> Why not just set the OF_POPULATED if the probe succeeds?
> >>
> >> Like:
> >>
> >> diff --git a/drivers/clocksource/timer-probe.c
> >> b/drivers/clocksource/timer-probe.c
> >> index ee9574da53c0..f290639ff824 100644
> >> --- a/drivers/clocksource/timer-probe.c
> >> +++ b/drivers/clocksource/timer-probe.c
> >> @@ -35,6 +35,7 @@ void __init timer_probe(void)
> >>                         continue;
> >>                 }
> >>
> >> +               of_node_set_flag(np, OF_POPULATED);
> >>                 timers++;
> >>         }
> >>
> >> instead of setting the flag and clearing it in case of failure?
> >
> > Looking at IRQ framework which first did it the way you suggested and
> > then changed it to the way I did it, it looks like it allows for
> > drivers that need to split the initialization between early init (not
> > just error out, but init partly) and later driver probe. See [1].
> >
> > Also, most of the other frameworks that set OF_POPULATED, set it
> > before calling the initialization function for the device. Maybe it's
> > to make sure the device node data "looks the same" whether a device is
> > initialized during early init or during normal device probe (since the
> > OF_POPULATED is set before the probe is called) -- i.e. have
> > OF_POPULATED  set before the device initialization code is actually
> > run?
> >
> > Honestly I don't have a strong opinion either way, but I lean towards
> > following what IRQ does.
>
> Thanks for the pointer. Indeed it is to catch situation where the driver
> is clearing the flag like:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/ingenic-timer.c#n245
>
> But I'm not able to figure out why it is cleared here :/

I think I know what's going on. He wants to implement PM support for
this timer. But PM support is tied to devices. So, clearing out the
flag allows creating the device which then hooks into PM ops.

Although, it looks like the driver assumes the timer framework was
setting the OF_POPULATED flag.

-Saravana

>
> Paul?
>
>
> --
>  <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
>