Date:   Mon, 16 Mar 2020 19:07:30 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Saravana Kannan <saravanak@...gle.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Android Kernel Team <kernel-team@...roid.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Paul Cercueil <paul@...pouillou.net>
Subject: Re: [PATCH v1] clocksource: Avoid creating dead devices

On 16/03/2020 18:49, Saravana Kannan wrote:
> On Mon, Mar 16, 2020 at 7:57 AM Daniel Lezcano
> <daniel.lezcano@...aro.org> wrote:
>>
>> On 08/03/2020 06:53, Saravana Kannan wrote:
>>> On Wed, Mar 4, 2020 at 11:56 AM Daniel Lezcano
>>> <daniel.lezcano@...aro.org> wrote:
>>>>
>>>> On 04/03/2020 20:30, Saravana Kannan wrote:
>>>>> On Thu, Feb 27, 2020 at 1:22 PM Saravana Kannan <saravanak@...gle.com> wrote:
>>>>>>
>>>>>> On Thu, Feb 27, 2020 at 1:06 AM Daniel Lezcano
>>>>>> <daniel.lezcano@...aro.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

Powered by blists - more mailing lists