Messages in this thread | | | From | Saravana Kannan <> | Date | Tue, 17 Mar 2020 11:08:04 -0700 | Subject | Re: [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 > >> > >
| |