Messages in this thread | | | Subject | Re: [PATCH v1] clocksource: Avoid creating dead devices | From | Daniel Lezcano <> | Date | Mon, 16 Mar 2020 19:07:30 +0100 |
| |
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 :/
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
| |