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

[Saravana Kannan]

On Tue, Mar 17, 2020 at 4:36 AM Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:
>
> Hi Saravana, Daniel,
>
>
> Le lun. 16 mars 2020 Ã 11:15, Saravana Kannan <saravanak@xxxxxxxxxx> a
> Ãcrit :
> > 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.
>
> That's correct - the OF_POPULATED flag is cleared so that the driver
> will probe as a platform_device. When I did write the driver this was
> required or the platform_device would not probe.

Interesting. I went and looked at the kernel when your patch merged.
As far as I can tell, you shouldn't have needed to clear OF_POPULATED
because the timer framework never set OF_POPULATED even back then.

If this driver was based in drivers/irqchip/irq-ingenic-tcu.c and you
were initially just trying to get it to create a device, then you'd
have needed to clear OF_POPULATED because IRQ chip framework does set
the flag.

In any case, it's good that you cleared it -- it'll continue to work
with my patch.

Daniel,

Looks like this answers all the concerns you had. I also checked every
driver in drivers/clocksource that had the word "probe" in it to make
sure it won't need any updates to ingenic-timer.c. Can we merge this?

Thanks,
Saravana

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