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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

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?


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