lkml.org 
[lkml]   [2020]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v1] clocksource: Avoid creating dead devices
On Tue, Mar 17, 2020 at 4:36 AM Paul Cercueil <paul@crapouillou.net> wrote:
>
> Hi Saravana, Daniel,
>
>
> Le lun. 16 mars 2020 à 11:15, Saravana Kannan <saravanak@google.com> a
> écrit :
> > On Mon, Mar 16, 2020 at 11:07 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 16/03/2020 18:49, Saravana Kannan wrote:
> >> > On Mon, Mar 16, 2020 at 7:57 AM Daniel Lezcano
> >> > <daniel.lezcano@linaro.org> wrote:
> >> >>
> >> >> On 08/03/2020 06:53, Saravana Kannan wrote:
> >> >>> On Wed, Mar 4, 2020 at 11:56 AM Daniel Lezcano
> >> >>> <daniel.lezcano@linaro.org> wrote:
> >> >>>>
> >> >>>> On 04/03/2020 20:30, Saravana Kannan wrote:
> >> >>>>> On Thu, Feb 27, 2020 at 1:22 PM Saravana Kannan
> >> <saravanak@google.com> wrote:
> >> >>>>>>
> >> >>>>>> On Thu, Feb 27, 2020 at 1:06 AM Daniel Lezcano
> >> >>>>>> <daniel.lezcano@linaro.org> 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
> >>
>
>

\
 
 \ /
  Last update: 2020-03-17 19:09    [W:0.072 / U:1.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site