[PATCH v1] clocksource: Avoid creating dead devices

[PATCH v1] clocksource: Avoid creating dead devices

[Saravana Kannan]

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.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/clocksource/timer-probe.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
index ee9574da53c0..a10f28d750a9 100644
--- a/drivers/clocksource/timer-probe.c
+++ b/drivers/clocksource/timer-probe.c
@@ -27,8 +27,10 @@ void __init timer_probe(void)
 
 		init_func_ret = match->data;
 
+		of_node_set_flag(np, OF_POPULATED);
 		ret = init_func_ret(np);
 		if (ret) {
+			of_node_clear_flag(np, OF_POPULATED);
 			if (ret != -EPROBE_DEFER)
 				pr_err("Failed to initialize '%pOF': %d\n", np,
 				       ret);
-- 
2.25.0.rc1.283.g88dfdc4193-goog

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

[Saravana Kannan]

On Fri, Jan 10, 2020 at 9:21 PM Saravana Kannan <saravanak@google.com> 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.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/clocksource/timer-probe.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
> index ee9574da53c0..a10f28d750a9 100644
> --- a/drivers/clocksource/timer-probe.c
> +++ b/drivers/clocksource/timer-probe.c
> @@ -27,8 +27,10 @@ void __init timer_probe(void)
>
>                 init_func_ret = match->data;
>
> +               of_node_set_flag(np, OF_POPULATED);
>                 ret = init_func_ret(np);
>                 if (ret) {
> +                       of_node_clear_flag(np, OF_POPULATED);
>                         if (ret != -EPROBE_DEFER)
>                                 pr_err("Failed to initialize '%pOF': %d\n", np,
>                                        ret);
> --
> 2.25.0.rc1.283.g88dfdc4193-goog

Gentle reminder.

-Saravana

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

[Daniel Lezcano]

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.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/clocksource/timer-probe.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
> index ee9574da53c0..a10f28d750a9 100644
> --- a/drivers/clocksource/timer-probe.c
> +++ b/drivers/clocksource/timer-probe.c
> @@ -27,8 +27,10 @@ void __init timer_probe(void)
>  
>  		init_func_ret = match->data;
>  
> +		of_node_set_flag(np, OF_POPULATED);
>  		ret = init_func_ret(np);
>  		if (ret) {
> +			of_node_clear_flag(np, OF_POPULATED);

Isn't it in conflict with:

drivers/clocksource/ingenic-timer.c

?

>  			if (ret != -EPROBE_DEFER)
>  				pr_err("Failed to initialize '%pOF': %d\n", np,
>  				       ret);
> 


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

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

[Saravana Kannan]

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.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/clocksource/timer-probe.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
> > index ee9574da53c0..a10f28d750a9 100644
> > --- a/drivers/clocksource/timer-probe.c
> > +++ b/drivers/clocksource/timer-probe.c
> > @@ -27,8 +27,10 @@ void __init timer_probe(void)
> >
> >               init_func_ret = match->data;
> >
> > +             of_node_set_flag(np, OF_POPULATED);
> >               ret = init_func_ret(np);
> >               if (ret) {
> > +                     of_node_clear_flag(np, OF_POPULATED);
>
> Isn't it in conflict with:
>
> drivers/clocksource/ingenic-timer.c
>
> ?

No, it won't interfere with that driver because:
1. This flag is getting set only if the driver already registered a
timer init function using TIMER_OF_DECLARE.
2. And if the function fails, we clear the flag.

So in the case of ingenic-timer, the device will still be there and be
probed by the driver.

My next step was going to be sending patches that'll actually allow
compiling timer drivers as modules.

Thanks,
Saravana

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

[Saravana Kannan]

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.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/clocksource/timer-probe.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
> > > index ee9574da53c0..a10f28d750a9 100644
> > > --- a/drivers/clocksource/timer-probe.c
> > > +++ b/drivers/clocksource/timer-probe.c
> > > @@ -27,8 +27,10 @@ void __init timer_probe(void)
> > >
> > >               init_func_ret = match->data;
> > >
> > > +             of_node_set_flag(np, OF_POPULATED);
> > >               ret = init_func_ret(np);
> > >               if (ret) {
> > > +                     of_node_clear_flag(np, OF_POPULATED);
> >
> > Isn't it in conflict with:
> >
> > drivers/clocksource/ingenic-timer.c
> >
> > ?
>
> No, it won't interfere with that driver because:
> 1. This flag is getting set only if the driver already registered a
> timer init function using TIMER_OF_DECLARE.
> 2. And if the function fails, we clear the flag.
>
> So in the case of ingenic-timer, the device will still be there and be
> probed by the driver.

Daniel, friendly reminder. Or do you have a patchworks link too that I
can keep an eye on?

-Saravana

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

[Daniel Lezcano]

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?


>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>>> ---
>>>>  drivers/clocksource/timer-probe.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
>>>> index ee9574da53c0..a10f28d750a9 100644
>>>> --- a/drivers/clocksource/timer-probe.c
>>>> +++ b/drivers/clocksource/timer-probe.c
>>>> @@ -27,8 +27,10 @@ void __init timer_probe(void)
>>>>
>>>>               init_func_ret = match->data;
>>>>
>>>> +             of_node_set_flag(np, OF_POPULATED);
>>>>               ret = init_func_ret(np);
>>>>               if (ret) {
>>>> +                     of_node_clear_flag(np, OF_POPULATED);
>>>
>>> Isn't it in conflict with:
>>>
>>> drivers/clocksource/ingenic-timer.c
>>>
>>> ?
>>
>> No, it won't interfere with that driver because:
>> 1. This flag is getting set only if the driver already registered a
>> timer init function using TIMER_OF_DECLARE.
>> 2. And if the function fails, we clear the flag.
>>
>> So in the case of ingenic-timer, the device will still be there and be
>> probed by the driver.
> 
> Daniel, friendly reminder. Or do you have a patchworks link too that I
> can keep an eye on?



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

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

[Saravana Kannan]

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.

Thanks,
Saravana

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

[Daniel Lezcano]

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

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

[Saravana Kannan]

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

[1] - https://lore.kernel.org/lkml/1470752332-14185-1-git-send-email-p.zabel@pengutronix.de/#t

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

[Daniel Lezcano]

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

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

[Saravana Kannan]

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.

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
>

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

[Saravana Kannan]

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

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

[Daniel Lezcano]

On 17/03/2020 19:08, Saravana Kannan wrote:
> On Tue, Mar 17, 2020 at 4:36 AM Paul Cercueil <paul@crapouillou.net> wrote:

[ ... ]

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

Applied [1], thanks

 -- Daniel

[1]
https://git.linaro.org/people/daniel.lezcano/linux.git/commit/?h=timers/drivers/next&id=4f41fe386a94639cd9a1831298d4f85db5662f1e


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

[tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[tip-bot2 for Saravana Kannan]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     4f41fe386a94639cd9a1831298d4f85db5662f1e
Gitweb:        https://git.kernel.org/tip/4f41fe386a94639cd9a1831298d4f85db5662f1e
Author:        Saravana Kannan <saravanak@google.com>
AuthorDate:    Fri, 10 Jan 2020 21:21:25 -08:00
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Tue, 17 Mar 2020 13:10:07 +01:00

clocksource/drivers/timer-probe: Avoid creating dead devices

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.

Signed-off-by: Saravana Kannan <saravanak@google.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20200111052125.238212-1-saravanak@google.com
---
 drivers/clocksource/timer-probe.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
index ee9574d..a10f28d 100644
--- a/drivers/clocksource/timer-probe.c
+++ b/drivers/clocksource/timer-probe.c
@@ -27,8 +27,10 @@ void __init timer_probe(void)
 
 		init_func_ret = match->data;
 
+		of_node_set_flag(np, OF_POPULATED);
 		ret = init_func_ret(np);
 		if (ret) {
+			of_node_clear_flag(np, OF_POPULATED);
 			if (ret != -EPROBE_DEFER)
 				pr_err("Failed to initialize '%pOF': %d\n", np,
 				       ret);

Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[Ionela Voinescu]

Hi guys,

On Thursday 19 Mar 2020 at 08:47:46 (-0000), tip-bot2 for Saravana Kannan wrote:
> The following commit has been merged into the timers/core branch of tip:
> 
> Commit-ID:     4f41fe386a94639cd9a1831298d4f85db5662f1e
> Gitweb:        https://git.kernel.org/tip/4f41fe386a94639cd9a1831298d4f85db5662f1e
> Author:        Saravana Kannan <saravanak@google.com>
> AuthorDate:    Fri, 10 Jan 2020 21:21:25 -08:00
> Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
> CommitterDate: Tue, 17 Mar 2020 13:10:07 +01:00
> 
> clocksource/drivers/timer-probe: Avoid creating dead devices
> 
> 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.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Link: https://lore.kernel.org/r/20200111052125.238212-1-saravanak@google.com
> ---
>  drivers/clocksource/timer-probe.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
> index ee9574d..a10f28d 100644
> --- a/drivers/clocksource/timer-probe.c
> +++ b/drivers/clocksource/timer-probe.c
> @@ -27,8 +27,10 @@ void __init timer_probe(void)
>  
>  		init_func_ret = match->data;
>  
> +		of_node_set_flag(np, OF_POPULATED);
>  		ret = init_func_ret(np);
>  		if (ret) {
> +			of_node_clear_flag(np, OF_POPULATED);
>  			if (ret != -EPROBE_DEFER)
>  				pr_err("Failed to initialize '%pOF': %d\n", np,
>  				       ret);
> 

This patch is creating problems on some vexpress platforms - ones that
are using CLKSRC_VERSATILE (drivers/clocksource/timer-versatile.c).
I noticed issues on TC2 and FVPs (fixed virtual platforms) starting with
next-20200318 and still reproducible with next-20200323.

It seems the issue this patch causes on TC2 and FVP is related to the
vexpress-sysreg node being used early for sched_clock_init
(timer_versatile.c: versatile_sched_clock_init). At this point (at
time_init) the node will be marked as OF_POPULATED, which flags that a
device is already created for it, but it is not, in this case.

Later at sysreg_init (vexpress-sysreg.c) a device will fail to be created
for it, as one already exists. This will result in a failure to create a
bridge and a system controller for a bunch of devices (mostly clocks and
regulators).

I think on the FVP it does not cause many issues as clocks are fixed and
regulator settings are probably nops so it boots fine and throws only
some warnings. On TC2 on the other hand it fails to boot and it hangs at
starting the kernel.

In my opinion the idea of the patch is not bad, but I'm not an expert on
this so the most I can offer for now is the basic understanding of the
issue. I've Cc-ed a few folks to potentially suggest alternatives/fixes.

For now, reverting this patch solves the problems on both platforms.
I tested this on next-20200318 which introduced the problem.

Hope it helps,
Ionela.

Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[Saravana Kannan]

On Tue, Mar 24, 2020 at 11:00 AM Ionela Voinescu
<ionela.voinescu@arm.com> wrote:
>
> Hi guys,
>
> On Thursday 19 Mar 2020 at 08:47:46 (-0000), tip-bot2 for Saravana Kannan wrote:
> > The following commit has been merged into the timers/core branch of tip:
> >
> > Commit-ID:     4f41fe386a94639cd9a1831298d4f85db5662f1e
> > Gitweb:        https://git.kernel.org/tip/4f41fe386a94639cd9a1831298d4f85db5662f1e
> > Author:        Saravana Kannan <saravanak@google.com>
> > AuthorDate:    Fri, 10 Jan 2020 21:21:25 -08:00
> > Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
> > CommitterDate: Tue, 17 Mar 2020 13:10:07 +01:00
> >
> > clocksource/drivers/timer-probe: Avoid creating dead devices
> >
> > 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.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Link: https://lore.kernel.org/r/20200111052125.238212-1-saravanak@google.com
> > ---
> >  drivers/clocksource/timer-probe.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
> > index ee9574d..a10f28d 100644
> > --- a/drivers/clocksource/timer-probe.c
> > +++ b/drivers/clocksource/timer-probe.c
> > @@ -27,8 +27,10 @@ void __init timer_probe(void)
> >
> >               init_func_ret = match->data;
> >
> > +             of_node_set_flag(np, OF_POPULATED);
> >               ret = init_func_ret(np);
> >               if (ret) {
> > +                     of_node_clear_flag(np, OF_POPULATED);
> >                       if (ret != -EPROBE_DEFER)
> >                               pr_err("Failed to initialize '%pOF': %d\n", np,
> >                                      ret);
> >
>
> This patch is creating problems on some vexpress platforms - ones that
> are using CLKSRC_VERSATILE (drivers/clocksource/timer-versatile.c).
> I noticed issues on TC2 and FVPs (fixed virtual platforms) starting with
> next-20200318 and still reproducible with next-20200323.
>
> It seems the issue this patch causes on TC2 and FVP is related to the
> vexpress-sysreg node being used early for sched_clock_init
> (timer_versatile.c: versatile_sched_clock_init). At this point (at
> time_init) the node will be marked as OF_POPULATED, which flags that a
> device is already created for it, but it is not, in this case.
>
> Later at sysreg_init (vexpress-sysreg.c) a device will fail to be created
> for it, as one already exists. This will result in a failure to create a
> bridge and a system controller for a bunch of devices (mostly clocks and
> regulators).
>
> I think on the FVP it does not cause many issues as clocks are fixed and
> regulator settings are probably nops so it boots fine and throws only
> some warnings. On TC2 on the other hand it fails to boot and it hangs at
> starting the kernel.
>
> In my opinion the idea of the patch is not bad, but I'm not an expert on
> this so the most I can offer for now is the basic understanding of the
> issue. I've Cc-ed a few folks to potentially suggest alternatives/fixes.
>
> For now, reverting this patch solves the problems on both platforms.
> I tested this on next-20200318 which introduced the problem.

I'll reply later today after I take a closer look. But will something
like what timer-ingenic.c did work for you? You can clear the flag
inside your early init.

-Saravana


>
> Hope it helps,
> Ionela.

Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[Saravana Kannan]

On Tue, Mar 24, 2020 at 11:34 AM Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Mar 24, 2020 at 11:00 AM Ionela Voinescu
> <ionela.voinescu@arm.com> wrote:
> >
> > Hi guys,
> >
> > On Thursday 19 Mar 2020 at 08:47:46 (-0000), tip-bot2 for Saravana Kannan wrote:
> > > The following commit has been merged into the timers/core branch of tip:
> > >
> > > Commit-ID:     4f41fe386a94639cd9a1831298d4f85db5662f1e
> > > Gitweb:        https://git.kernel.org/tip/4f41fe386a94639cd9a1831298d4f85db5662f1e
> > > Author:        Saravana Kannan <saravanak@google.com>
> > > AuthorDate:    Fri, 10 Jan 2020 21:21:25 -08:00
> > > Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
> > > CommitterDate: Tue, 17 Mar 2020 13:10:07 +01:00
> > >
> > > clocksource/drivers/timer-probe: Avoid creating dead devices
> > >
> > > 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.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > Link: https://lore.kernel.org/r/20200111052125.238212-1-saravanak@google.com
> > > ---
> > >  drivers/clocksource/timer-probe.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
> > > index ee9574d..a10f28d 100644
> > > --- a/drivers/clocksource/timer-probe.c
> > > +++ b/drivers/clocksource/timer-probe.c
> > > @@ -27,8 +27,10 @@ void __init timer_probe(void)
> > >
> > >               init_func_ret = match->data;
> > >
> > > +             of_node_set_flag(np, OF_POPULATED);
> > >               ret = init_func_ret(np);
> > >               if (ret) {
> > > +                     of_node_clear_flag(np, OF_POPULATED);
> > >                       if (ret != -EPROBE_DEFER)
> > >                               pr_err("Failed to initialize '%pOF': %d\n", np,
> > >                                      ret);
> > >
> >
> > This patch is creating problems on some vexpress platforms - ones that
> > are using CLKSRC_VERSATILE (drivers/clocksource/timer-versatile.c).
> > I noticed issues on TC2 and FVPs (fixed virtual platforms) starting with
> > next-20200318 and still reproducible with next-20200323.
> >
> > It seems the issue this patch causes on TC2 and FVP is related to the
> > vexpress-sysreg node being used early for sched_clock_init
> > (timer_versatile.c: versatile_sched_clock_init). At this point (at
> > time_init) the node will be marked as OF_POPULATED, which flags that a
> > device is already created for it, but it is not, in this case.
> >
> > Later at sysreg_init (vexpress-sysreg.c) a device will fail to be created
> > for it, as one already exists. This will result in a failure to create a
> > bridge and a system controller for a bunch of devices (mostly clocks and
> > regulators).
> >
> > I think on the FVP it does not cause many issues as clocks are fixed and
> > regulator settings are probably nops so it boots fine and throws only
> > some warnings. On TC2 on the other hand it fails to boot and it hangs at
> > starting the kernel.
> >
> > In my opinion the idea of the patch is not bad, but I'm not an expert on
> > this so the most I can offer for now is the basic understanding of the
> > issue. I've Cc-ed a few folks to potentially suggest alternatives/fixes.
> >
> > For now, reverting this patch solves the problems on both platforms.
> > I tested this on next-20200318 which introduced the problem.
>
> I'll reply later today after I take a closer look. But will something
> like what timer-ingenic.c did work for you? You can clear the flag
> inside your early init.

Firstly, sorry my patch broke your platform.

I took a closer look. So two different drivers [1] [2] are saying they
know how to handle "arm,vexpress-sysreg" and are expecting to run at
the same time. That seems a bit unusual to me. I wonder if this is a
violation of the device-driver model because this expectation would
never be allowed if these device drivers were actual drivers
registered with driver-core. But that's a discussion for another time.

To fix this issue you are facing, this patch should work:
https://lore.kernel.org/lkml/20200324195302.203115-1-saravanak@google.com/T/#u

Can you please test it and give a Tested-by?

Thanks,
Saravana

[1] drivers/mfd/vexpress-sysreg.c:  { .compatible = "arm,vexpress-sysreg", },
[2] drivers/clocksource/timer-versatile.c:TIMER_OF_DECLARE(vexpress,
"arm,vexpress-sysreg"

Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[Jon Hunter]

On 24/03/2020 17:59, Ionela Voinescu wrote:
> Hi guys,
> 
> On Thursday 19 Mar 2020 at 08:47:46 (-0000), tip-bot2 for Saravana Kannan wrote:
>> The following commit has been merged into the timers/core branch of tip:
>>
>> Conommit-ID:     4f41fe386a94639cd9a1831298d4f85db5662f1e
>> Gitweb:        https://git.kernel.org/tip/4f41fe386a94639cd9a1831298d4f85db5662f1e
>> Author:        Saravana Kannan <saravanak@google.com>
>> AuthorDate:    Fri, 10 Jan 2020 21:21:25 -08:00
>> Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
>> CommitterDate: Tue, 17 Mar 2020 13:10:07 +01:00
>>
>> clocksource/drivers/timer-probe: Avoid creating dead devices
>>
>> 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.
>>
>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Link: https://lore.kernel.org/r/20200111052125.238212-1-saravanak@google.com
>> ---
>>  drivers/clocksource/timer-probe.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
>> index ee9574d..a10f28d 100644
>> --- a/drivers/clocksource/timer-probe.c
>> +++ b/drivers/clocksource/timer-probe.c
>> @@ -27,8 +27,10 @@ void __init timer_probe(void)
>>  
>>  		init_func_ret = match->data;
>>  
>> +		of_node_set_flag(np, OF_POPULATED);
>>  		ret = init_func_ret(np);
>>  		if (ret) {
>> +			of_node_clear_flag(np, OF_POPULATED);
>>  			if (ret != -EPROBE_DEFER)
>>  				pr_err("Failed to initialize '%pOF': %d\n", np,
>>  				       ret);
>>
> 
> This patch is creating problems on some vexpress platforms - ones that
> are using CLKSRC_VERSATILE (drivers/clocksource/timer-versatile.c).
> I noticed issues on TC2 and FVPs (fixed virtual platforms) starting with
> next-20200318 and still reproducible with next-20200323.

I am also seeing a regression on tegra30-cardhu-a04 when testing system
suspend on -next. Bisect is pointing to this commit and reverting on top
of -next fixes the problem. Unfortunately, there is no crash dump
observed, but the device hangs somewhere when testing suspend.

I have not looked into this any further but wanted to report the problem.

Cheers
Jon

-- 
nvpublic

Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[Thomas Gleixner]

Saravana Kannan <saravanak@google.com> writes:
> On Tue, Mar 24, 2020 at 11:34 AM Saravana Kannan <saravanak@google.com> wrote:
> I took a closer look. So two different drivers [1] [2] are saying they
> know how to handle "arm,vexpress-sysreg" and are expecting to run at
> the same time. That seems a bit unusual to me. I wonder if this is a
> violation of the device-driver model because this expectation would
> never be allowed if these device drivers were actual drivers
> registered with driver-core. But that's a discussion for another time.
>
> To fix this issue you are facing, this patch should work:
> https://lore.kernel.org/lkml/20200324195302.203115-1-saravanak@google.com/T/#u

Sorry, that's not a fix. That's a crude hack.

As this is also causing trouble on tegra30-cardhu-a04 the only sane
solution is to revert it and start over with a proper solution for the
vexpress problem and a root cause analysis for the tegra.

Thanks,

        tglx

Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[Saravana Kannan]

On Wed, Mar 25, 2020 at 2:47 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Saravana Kannan <saravanak@google.com> writes:
> > On Tue, Mar 24, 2020 at 11:34 AM Saravana Kannan <saravanak@google.com> wrote:
> > I took a closer look. So two different drivers [1] [2] are saying they
> > know how to handle "arm,vexpress-sysreg" and are expecting to run at
> > the same time. That seems a bit unusual to me. I wonder if this is a
> > violation of the device-driver model because this expectation would
> > never be allowed if these device drivers were actual drivers
> > registered with driver-core. But that's a discussion for another time.
> >
> > To fix this issue you are facing, this patch should work:
> > https://lore.kernel.org/lkml/20200324195302.203115-1-saravanak@google.com/T/#u
>
> Sorry, that's not a fix. That's a crude hack.

If device nodes are being handled by drivers without binding a driver
to struct devices, then not setting OF_POPULATED is wrong. So the
original patch sets it. There are also very valid reasons for allowing
OF_POPULATED to be cleared by a driver as discussed here [1].

The approach of the original patch (setting the flag and letting the
driver sometimes clear it) is also followed by many other frameworks
like irq, clk, i2c, etc. Even ingenic-timer.c already does it for the
exact same reason.

So, why is the vexpress fix a crude hack?

> As this is also causing trouble on tegra30-cardhu-a04 the only sane
> solution is to revert it and start over with a proper solution for the
> vexpress problem and a root cause analysis for the tegra.

If someone can tell me which of the timer drivers are relevant for
tegra30-cardhu-a04, I can help fix it.
If you want to revert the original patch first before waiting for a
tegra fix, that's okay by me.

However, for vexpress, what do you propose I do? The driver itself is
doing weird stuff with two drivers handling the exact same device. I
can't just go edit the DT files because technically I don't know their
hardware. Looks to me like they should have a separate and proper
device for the timer and not have two drivers handle the same device.
If you don't like my vexpress fix, then don't take it but ask the
vexpress maintainer to fix their DT and driver maybe? But that might
break the kernel compatibility with existing DT binaries on devices
(yes, vexpress seems like a virtual platform, so updating DT blobs
might not be hard). My vexpress fix doesn't break backwards
compatibility.

So, can you please accept my vexpress fix or tell us what you think is
a "proper solution"?

-Saravana

Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[Saravana Kannan]

On Wed, Mar 25, 2020 at 3:56 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Mar 25, 2020 at 2:47 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > Saravana Kannan <saravanak@google.com> writes:
> > > On Tue, Mar 24, 2020 at 11:34 AM Saravana Kannan <saravanak@google.com> wrote:
> > > I took a closer look. So two different drivers [1] [2] are saying they
> > > know how to handle "arm,vexpress-sysreg" and are expecting to run at
> > > the same time. That seems a bit unusual to me. I wonder if this is a
> > > violation of the device-driver model because this expectation would
> > > never be allowed if these device drivers were actual drivers
> > > registered with driver-core. But that's a discussion for another time.
> > >
> > > To fix this issue you are facing, this patch should work:
> > > https://lore.kernel.org/lkml/20200324195302.203115-1-saravanak@google.com/T/#u
> >
> > Sorry, that's not a fix. That's a crude hack.
>
> If device nodes are being handled by drivers without binding a driver
> to struct devices, then not setting OF_POPULATED is wrong. So the
> original patch sets it. There are also very valid reasons for allowing
> OF_POPULATED to be cleared by a driver as discussed here [1].

Forgot to include [1]
[1] - https://lore.kernel.org/lkml/20200111052125.238212-1-saravanak@google.com/T/#m7b043de4c75e6c1de309d3ca5146bb0c7b3dfc80
For some reason Paul's email is missing from lore.kernel.org.

-Saravana

Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[Dmitry Osipenko]

26.03.2020 00:29, Jon Hunter пишет:
> 
> On 24/03/2020 17:59, Ionela Voinescu wrote:
>> Hi guys,
>>
>> On Thursday 19 Mar 2020 at 08:47:46 (-0000), tip-bot2 for Saravana Kannan wrote:
>>> The following commit has been merged into the timers/core branch of tip:
>>>
>>> Conommit-ID:     4f41fe386a94639cd9a1831298d4f85db5662f1e
>>> Gitweb:        https://git.kernel.org/tip/4f41fe386a94639cd9a1831298d4f85db5662f1e
>>> Author:        Saravana Kannan <saravanak@google.com>
>>> AuthorDate:    Fri, 10 Jan 2020 21:21:25 -08:00
>>> Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
>>> CommitterDate: Tue, 17 Mar 2020 13:10:07 +01:00
>>>
>>> clocksource/drivers/timer-probe: Avoid creating dead devices
>>>
>>> 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.
>>>
>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Link: https://lore.kernel.org/r/20200111052125.238212-1-saravanak@google.com
>>> ---
>>>  drivers/clocksource/timer-probe.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
>>> index ee9574d..a10f28d 100644
>>> --- a/drivers/clocksource/timer-probe.c
>>> +++ b/drivers/clocksource/timer-probe.c
>>> @@ -27,8 +27,10 @@ void __init timer_probe(void)
>>>  
>>>  		init_func_ret = match->data;
>>>  
>>> +		of_node_set_flag(np, OF_POPULATED);
>>>  		ret = init_func_ret(np);
>>>  		if (ret) {
>>> +			of_node_clear_flag(np, OF_POPULATED);
>>>  			if (ret != -EPROBE_DEFER)
>>>  				pr_err("Failed to initialize '%pOF': %d\n", np,
>>>  				       ret);
>>>
>>
>> This patch is creating problems on some vexpress platforms - ones that
>> are using CLKSRC_VERSATILE (drivers/clocksource/timer-versatile.c).
>> I noticed issues on TC2 and FVPs (fixed virtual platforms) starting with
>> next-20200318 and still reproducible with next-20200323.
> 
> I am also seeing a regression on tegra30-cardhu-a04 when testing system
> suspend on -next. Bisect is pointing to this commit and reverting on top
> of -next fixes the problem. Unfortunately, there is no crash dump
> observed, but the device hangs somewhere when testing suspend.
> 
> I have not looked into this any further but wanted to report the problem.

IIUC, this should also break the watchdog driver on Tegra because the
device tree node is shared by both clocksource and watchdog.

Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[Daniel Lezcano]

Hi Saravana,

On 25/03/2020 23:56, Saravana Kannan wrote:
> On Wed, Mar 25, 2020 at 2:47 PM Thomas Gleixner 
> <tglx@linutronix.de> wrote:
>> 
>> Saravana Kannan <saravanak@google.com> writes:
>>> On Tue, Mar 24, 2020 at 11:34 AM Saravana Kannan 
>>> <saravanak@google.com> wrote: I took a closer look. So two 
>>> different drivers [1] [2] are saying they know how to handle 
>>> "arm,vexpress-sysreg" and are expecting to run at the same 
>>> time. That seems a bit unusual to me. I wonder if this is a 
>>> violation of the device-driver model because this expectation 
>>> would never be allowed if these device drivers were actual 
>>> drivers registered with driver-core. But that's a discussion 
>>> for another time.
>>> 
>>> To fix this issue you are facing, this patch should work: 
>>> https://lore.kernel.org/lkml/20200324195302.203115-1-saravanak@google.com/T/#u
>>
>>
>>>
>>>
>>> 
> Sorry, that's not a fix. That's a crude hack.
> 
> If device nodes are being handled by drivers without binding a 
> driver to struct devices, then not setting OF_POPULATED is wrong. 
> So the original patch sets it. There are also very valid reasons 
> for allowing OF_POPULATED to be cleared by a driver as discussed 
> here [1].
> 
> The approach of the original patch (setting the flag and letting 
> the driver sometimes clear it) is also followed by many other 
> frameworks like irq, clk, i2c, etc. Even ingenic-timer.c already 
> does it for the exact same reason.
> 
> So, why is the vexpress fix a crude hack?
> 
>> As this is also causing trouble on tegra30-cardhu-a04 the only 
>> sane solution is to revert it and start over with a proper 
>> solution for the vexpress problem and a root cause analysis for 
>> the tegra.
> 
> If someone can tell me which of the timer drivers are relevant for 
> tegra30-cardhu-a04, I can help fix it. If you want to revert the 
> original patch first before waiting for a tegra fix, that's okay by
> me.


It seems the OF_POPULATED flag change spotted something wrong in
different drivers and that is a good thing. Thanks for your patch for
that.

Without putting in question your analysis, we need to stabilize the
release, can you send a revert of your patch?

Let's try to figure out what is happening and fix the issues in the
other drivers for the next cycle.

Thanks

  -- Daniel



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

Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[Thomas Gleixner]

Saravana Kannan <saravanak@google.com> writes:
> On Wed, Mar 25, 2020 at 2:47 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
>> Saravana Kannan <saravanak@google.com> writes:
>> > On Tue, Mar 24, 2020 at 11:34 AM Saravana Kannan <saravanak@google.com> wrote:
>> > I took a closer look. So two different drivers [1] [2] are saying they
>> > know how to handle "arm,vexpress-sysreg" and are expecting to run at
>> > the same time. That seems a bit unusual to me. I wonder if this is a
>> > violation of the device-driver model because this expectation would
>> > never be allowed if these device drivers were actual drivers
>> > registered with driver-core. But that's a discussion for another time.
>> >
>> > To fix this issue you are facing, this patch should work:
>> > https://lore.kernel.org/lkml/20200324195302.203115-1-saravanak@google.com/T/#u
>>
>> Sorry, that's not a fix. That's a crude hack.
>
> If device nodes are being handled by drivers without binding a driver
> to struct devices, then not setting OF_POPULATED is wrong. So the
> original patch sets it. There are also very valid reasons for allowing
> OF_POPULATED to be cleared by a driver as discussed here [1].
>
> The approach of the original patch (setting the flag and letting the
> driver sometimes clear it) is also followed by many other frameworks
> like irq, clk, i2c, etc. Even ingenic-timer.c already does it for the
> exact same reason.
>
> So, why is the vexpress fix a crude hack?

If it's the right thing to do and accepted by the DT folks, then the
changelog should provide a proper explanation for it. The one you
provided just baffles me. Plus the clearing of the flag really needs a
big fat comment.

It still does not make any sense to me.

arm,vexpress-sysreg is a MFD device, so can the ARM people please
explain, why the sched clock part is not just another MFD sub-device or
simply has it's own DT match?

>> As this is also causing trouble on tegra30-cardhu-a04 the only sane
>> solution is to revert it and start over with a proper solution for the
>> vexpress problem and a root cause analysis for the tegra.
>
> If someone can tell me which of the timer drivers are relevant for
> tegra30-cardhu-a04, I can help fix it.

git grep perhaps? And that's pretty much the same problem:

drivers/clocksource/timer-tegra.c:TIMER_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
drivers/rtc/rtc-tegra.c:        { .compatible = "nvidia,tegra20-rtc", },

Without looking deeper I suspect that these two are not the only ones.

Can the DT folks pretty please comment on this and provide some guidance
how to fix that w/o sprinkling 

    of_node_clear_flag(node, OF_POPULATED);

all over the place?

Thanks,

        tglx

Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[Rob Herring]

On Thu, Mar 26, 2020 at 4:34 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Saravana Kannan <saravanak@google.com> writes:
> > On Wed, Mar 25, 2020 at 2:47 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> >> Saravana Kannan <saravanak@google.com> writes:
> >> > On Tue, Mar 24, 2020 at 11:34 AM Saravana Kannan <saravanak@google.com> wrote:
> >> > I took a closer look. So two different drivers [1] [2] are saying they
> >> > know how to handle "arm,vexpress-sysreg" and are expecting to run at
> >> > the same time. That seems a bit unusual to me. I wonder if this is a
> >> > violation of the device-driver model because this expectation would
> >> > never be allowed if these device drivers were actual drivers
> >> > registered with driver-core. But that's a discussion for another time.
> >> >
> >> > To fix this issue you are facing, this patch should work:
> >> > https://lore.kernel.org/lkml/20200324195302.203115-1-saravanak@google.com/T/#u
> >>
> >> Sorry, that's not a fix. That's a crude hack.
> >
> > If device nodes are being handled by drivers without binding a driver
> > to struct devices, then not setting OF_POPULATED is wrong. So the
> > original patch sets it. There are also very valid reasons for allowing
> > OF_POPULATED to be cleared by a driver as discussed here [1].
> >
> > The approach of the original patch (setting the flag and letting the
> > driver sometimes clear it) is also followed by many other frameworks
> > like irq, clk, i2c, etc. Even ingenic-timer.c already does it for the
> > exact same reason.
> >
> > So, why is the vexpress fix a crude hack?
>
> If it's the right thing to do and accepted by the DT folks, then the
> changelog should provide a proper explanation for it. The one you
> provided just baffles me. Plus the clearing of the flag really needs a
> big fat comment.

IMO, commit 4f41fe386a946 should be reverted and be done with it.
There's no way the timer core can know whether a specific node should
be scanned or not. If you really want to avoid a struct device, then
set OF_POPULATED in specific timer drivers. But I'd rather not see
more places mucking with OF_POPULATED. It's really only bus code that
should be touching it.

Is having a struct device really a problem? If we want to save memory
usage, I have some ideas that would save much more than 1 or 2 struct
devices.

> It still does not make any sense to me.
>
> arm,vexpress-sysreg is a MFD device, so can the ARM people please
> explain, why the sched clock part is not just another MFD sub-device or
> simply has it's own DT match?

The issue is DT nodes and Linux drivers aren't necessarily 1-1. That
would be nice, but hardware is messy and DT doesn't abstract that
away. If we tried to always make things 1-1, then if/when the Linux
driver structure changes we'd have to change the DT. If we decided to
add a node now, we'd still have to support the old DT for backwards
compatibility. We also have to consider the structure for another OS
may be different.

Generally, if I see a node with a compatible only it gets NAKed as
that's a sure sign of someone just trying to bind a driver and not
describing the h/w. We only do MFD sub-devices if those devices
provide or consume other DT resources.

Rob

Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[Saravana Kannan]

On Thu, Mar 26, 2020 at 3:18 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
>
> Hi Saravana,
>
> On 25/03/2020 23:56, Saravana Kannan wrote:
> > On Wed, Mar 25, 2020 at 2:47 PM Thomas Gleixner
> > <tglx@linutronix.de> wrote:
> >>
> >> Saravana Kannan <saravanak@google.com> writes:
> >>> On Tue, Mar 24, 2020 at 11:34 AM Saravana Kannan
> >>> <saravanak@google.com> wrote: I took a closer look. So two
> >>> different drivers [1] [2] are saying they know how to handle
> >>> "arm,vexpress-sysreg" and are expecting to run at the same
> >>> time. That seems a bit unusual to me. I wonder if this is a
> >>> violation of the device-driver model because this expectation
> >>> would never be allowed if these device drivers were actual
> >>> drivers registered with driver-core. But that's a discussion
> >>> for another time.
> >>>
> >>> To fix this issue you are facing, this patch should work:
> >>> https://lore.kernel.org/lkml/20200324195302.203115-1-saravanak@google.com/T/#u
> >>
> >>
> >>>
> >>>
> >>>
> > Sorry, that's not a fix. That's a crude hack.
> >
> > If device nodes are being handled by drivers without binding a
> > driver to struct devices, then not setting OF_POPULATED is wrong.
> > So the original patch sets it. There are also very valid reasons
> > for allowing OF_POPULATED to be cleared by a driver as discussed
> > here [1].
> >
> > The approach of the original patch (setting the flag and letting
> > the driver sometimes clear it) is also followed by many other
> > frameworks like irq, clk, i2c, etc. Even ingenic-timer.c already
> > does it for the exact same reason.
> >
> > So, why is the vexpress fix a crude hack?
> >
> >> As this is also causing trouble on tegra30-cardhu-a04 the only
> >> sane solution is to revert it and start over with a proper
> >> solution for the vexpress problem and a root cause analysis for
> >> the tegra.
> >
> > If someone can tell me which of the timer drivers are relevant for
> > tegra30-cardhu-a04, I can help fix it. If you want to revert the
> > original patch first before waiting for a tegra fix, that's okay by
> > me.
>
>
> It seems the OF_POPULATED flag change spotted something wrong in
> different drivers and that is a good thing. Thanks for your patch for
> that.
>
> Without putting in question your analysis, we need to stabilize the
> release, can you send a revert of your patch?
>
> Let's try to figure out what is happening and fix the issues in the
> other drivers for the next cycle.

Make sense. Will do soon.

-Saravana

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

Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[Saravana Kannan]

On Thu, Mar 26, 2020 at 8:02 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Mar 26, 2020 at 4:34 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > Saravana Kannan <saravanak@google.com> writes:
> > > On Wed, Mar 25, 2020 at 2:47 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > >
> > >> Saravana Kannan <saravanak@google.com> writes:
> > >> > On Tue, Mar 24, 2020 at 11:34 AM Saravana Kannan <saravanak@google.com> wrote:
> > >> > I took a closer look. So two different drivers [1] [2] are saying they
> > >> > know how to handle "arm,vexpress-sysreg" and are expecting to run at
> > >> > the same time. That seems a bit unusual to me. I wonder if this is a
> > >> > violation of the device-driver model because this expectation would
> > >> > never be allowed if these device drivers were actual drivers
> > >> > registered with driver-core. But that's a discussion for another time.
> > >> >
> > >> > To fix this issue you are facing, this patch should work:
> > >> > https://lore.kernel.org/lkml/20200324195302.203115-1-saravanak@google.com/T/#u
> > >>
> > >> Sorry, that's not a fix. That's a crude hack.
> > >
> > > If device nodes are being handled by drivers without binding a driver
> > > to struct devices, then not setting OF_POPULATED is wrong. So the
> > > original patch sets it. There are also very valid reasons for allowing
> > > OF_POPULATED to be cleared by a driver as discussed here [1].
> > >
> > > The approach of the original patch (setting the flag and letting the
> > > driver sometimes clear it) is also followed by many other frameworks
> > > like irq, clk, i2c, etc. Even ingenic-timer.c already does it for the
> > > exact same reason.
> > >
> > > So, why is the vexpress fix a crude hack?
> >
> > If it's the right thing to do and accepted by the DT folks, then the
> > changelog should provide a proper explanation for it. The one you
> > provided just baffles me. Plus the clearing of the flag really needs a
> > big fat comment.
>
> IMO, commit 4f41fe386a946 should be reverted and be done with it.
> There's no way the timer core can know whether a specific node should
> be scanned or not. If you really want to avoid a struct device, then
> set OF_POPULATED in specific timer drivers. But I'd rather not see
> more places mucking with OF_POPULATED. It's really only bus code that
> should be touching it.

Since most drivers don't need the struct device, my patch sets the
flag in the timer core. And for the few exception cases where the
device is needed, we can clear the flag in the driver. That'll reduce
the number of places mucking with OF_POPULATED.

Does this seem okay to you?

> Is having a struct device really a problem? If we want to save memory
> usage, I have some ideas that would save much more than 1 or 2 struct
> devices.

Keep in mind that struct devices cost more than one struct device of
memory. They also create a bunch of sysfs nodes, uevents, etc.

> > It still does not make any sense to me.
> >
> > arm,vexpress-sysreg is a MFD device, so can the ARM people please
> > explain, why the sched clock part is not just another MFD sub-device or
> > simply has it's own DT match?
>
> The issue is DT nodes and Linux drivers aren't necessarily 1-1. That
> would be nice, but hardware is messy and DT doesn't abstract that
> away. If we tried to always make things 1-1, then if/when the Linux
> driver structure changes we'd have to change the DT.

I agree with this. I'm definitely not asking to create a node just
because we want another struct device.

> If we decided to
> add a node now, we'd still have to support the old DT for backwards
> compatibility.

Right, I agree it's too late to fix this DT for vexpress-sysreg now.

> We also have to consider the structure for another OS
> may be different.
>
> Generally, if I see a node with a compatible only it gets NAKed as
> that's a sure sign of someone just trying to bind a driver and not
> describing the h/w. We only do MFD sub-devices if those devices
> provide or consume other DT resources.

If we have a timer MFD, it'd technically consume a fixed-clock and not
be empty. vexpress-sysreg just assumes the clock is 24 MHz right now.

My point about the vexpress DT nodes being weird is not about Linux
devices, rather it's that:
1. It's already a MFD
2. Most of the functions are separated clearly into their functional
device nodes
3. However, the timer functionality is combined with the parent MFD
device when the parent MFD device implements a completely separate
function (gpio?). Why?

If one wants to split out the functions, do it fully. If one wants it
all under one mega driver (ugh!) then combine it all. Going halfway
causes these weird issues. That's my complaint about how the DT layout
is for this device.

Having said all that, I'd rather manipulate the OF_POPULATED flag than
break DT backward compatibility at this point.

But in general, I think we should try to reject two separate drivers
claiming to be compatible with the same device and expecting to work
at the same time. That's just weird.

-Saravana

Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices

[Rob Herring]

On Thu, Mar 26, 2020 at 12:09 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Mar 26, 2020 at 8:02 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Thu, Mar 26, 2020 at 4:34 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> > >
> > > Saravana Kannan <saravanak@google.com> writes:
> > > > On Wed, Mar 25, 2020 at 2:47 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > > >
> > > >> Saravana Kannan <saravanak@google.com> writes:
> > > >> > On Tue, Mar 24, 2020 at 11:34 AM Saravana Kannan <saravanak@google.com> wrote:
> > > >> > I took a closer look. So two different drivers [1] [2] are saying they
> > > >> > know how to handle "arm,vexpress-sysreg" and are expecting to run at
> > > >> > the same time. That seems a bit unusual to me. I wonder if this is a
> > > >> > violation of the device-driver model because this expectation would
> > > >> > never be allowed if these device drivers were actual drivers
> > > >> > registered with driver-core. But that's a discussion for another time.
> > > >> >
> > > >> > To fix this issue you are facing, this patch should work:
> > > >> > https://lore.kernel.org/lkml/20200324195302.203115-1-saravanak@google.com/T/#u
> > > >>
> > > >> Sorry, that's not a fix. That's a crude hack.
> > > >
> > > > If device nodes are being handled by drivers without binding a driver
> > > > to struct devices, then not setting OF_POPULATED is wrong. So the
> > > > original patch sets it. There are also very valid reasons for allowing
> > > > OF_POPULATED to be cleared by a driver as discussed here [1].
> > > >
> > > > The approach of the original patch (setting the flag and letting the
> > > > driver sometimes clear it) is also followed by many other frameworks
> > > > like irq, clk, i2c, etc. Even ingenic-timer.c already does it for the
> > > > exact same reason.
> > > >
> > > > So, why is the vexpress fix a crude hack?
> > >
> > > If it's the right thing to do and accepted by the DT folks, then the
> > > changelog should provide a proper explanation for it. The one you
> > > provided just baffles me. Plus the clearing of the flag really needs a
> > > big fat comment.
> >
> > IMO, commit 4f41fe386a946 should be reverted and be done with it.
> > There's no way the timer core can know whether a specific node should
> > be scanned or not. If you really want to avoid a struct device, then
> > set OF_POPULATED in specific timer drivers. But I'd rather not see
> > more places mucking with OF_POPULATED. It's really only bus code that
> > should be touching it.
>
> Since most drivers don't need the struct device, my patch sets the
> flag in the timer core. And for the few exception cases where the
> device is needed, we can clear the flag in the driver. That'll reduce
> the number of places mucking with OF_POPULATED.
>
> Does this seem okay to you?

No. Like I said, I prefer fewer cases of these flags being mucked with.

There are some other ways to solve this. Add a new "ignore" flag. That
would be somewhat better as OF_POPULATED is not necessarily static
(overlays!). Another way would be changing 'status' property to
something other than 'okay'.

> > Is having a struct device really a problem? If we want to save memory
> > usage, I have some ideas that would save much more than 1 or 2 struct
> > devices.
>
> Keep in mind that struct devices cost more than one struct device of
> memory. They also create a bunch of sysfs nodes, uevents, etc.

Having the device in sysfs could be argued to be a feature.

How much memory would we be saving? Is this really the biggest problem
we have for something that's been this way for at least 10 years now.

> > > It still does not make any sense to me.
> > >
> > > arm,vexpress-sysreg is a MFD device, so can the ARM people please
> > > explain, why the sched clock part is not just another MFD sub-device or
> > > simply has it's own DT match?
> >
> > The issue is DT nodes and Linux drivers aren't necessarily 1-1. That
> > would be nice, but hardware is messy and DT doesn't abstract that
> > away. If we tried to always make things 1-1, then if/when the Linux
> > driver structure changes we'd have to change the DT.
>
> I agree with this. I'm definitely not asking to create a node just
> because we want another struct device.
>
> > If we decided to
> > add a node now, we'd still have to support the old DT for backwards
> > compatibility.
>
> Right, I agree it's too late to fix this DT for vexpress-sysreg now.
>
> > We also have to consider the structure for another OS
> > may be different.
> >
> > Generally, if I see a node with a compatible only it gets NAKed as
> > that's a sure sign of someone just trying to bind a driver and not
> > describing the h/w. We only do MFD sub-devices if those devices
> > provide or consume other DT resources.
>
> If we have a timer MFD, it'd technically consume a fixed-clock and not
> be empty. vexpress-sysreg just assumes the clock is 24 MHz right now.

Possibly. Depends if only the timer uses the clock or all the other
functions use the clock.

If only someone had said the clock should be in DT [1].

> My point about the vexpress DT nodes being weird is not about Linux
> devices, rather it's that:
> 1. It's already a MFD
> 2. Most of the functions are separated clearly into their functional
> device nodes
> 3. However, the timer functionality is combined with the parent MFD
> device when the parent MFD device implements a completely separate
> function (gpio?). Why?

Looking at the history, it doesn't look like this got much review and
it would look a bit different if we were starting over. In any case, a
node for just a counter register seems like overkill. I often get
these MFD or system controller bindings piecemeal, so it's hard to
make any intelligent design decisions.

Actually, the fact that sched clock support was added later on without
doing a DT change was probably the main thing this binding got right.

> If one wants to split out the functions, do it fully. If one wants it
> all under one mega driver (ugh!) then combine it all. Going halfway
> causes these weird issues. That's my complaint about how the DT layout
> is for this device.

It's not how the binding is split, but that the functions are split
between a non-driver thing and driver things.

> Having said all that, I'd rather manipulate the OF_POPULATED flag than
> break DT backward compatibility at this point.
>
> But in general, I think we should try to reject two separate drivers
> claiming to be compatible with the same device and expecting to work
> at the same time. That's just weird.

If we had 2 proper drivers, we wouldn't be having this conversation.
It's not a problem to have multiple child drivers for a single DT node
when everything is a proper driver. We do that all the time.

Rob

[1] https://lkml.org/lkml/2014/4/16/422

[tip: timers/core] Revert "clocksource/drivers/timer-probe: Avoid creating dead devices"

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     4479730e9263befbb9ce9563a09563db2acb8f7c
Gitweb:        https://git.kernel.org/tip/4479730e9263befbb9ce9563a09563db2acb8f7c
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Sat, 28 Mar 2020 11:20:36 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Sat, 28 Mar 2020 11:25:44 +01:00

Revert "clocksource/drivers/timer-probe: Avoid creating dead devices"

This reverts commit 4f41fe386a94639cd9a1831298d4f85db5662f1e.

The change breaks systems on which the DT node of a device is used by
multiple drivers. The proposed workaround to clear OF_POPULATED is just a
band aid and this needs to be cleaned up at the root of the problem.

Revert this for now.

Reported-by: Ionela Voinescu <ionela.voinescu@arm.com>
Reported-by: Jon Hunter <jonathanh@nvidia.com>
Requested-by: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Saravana Kannan <saravanak@google.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20200324175955.GA16972@arm.com
---
 drivers/clocksource/timer-probe.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clocksource/timer-probe.c b/drivers/clocksource/timer-probe.c
index a10f28d..ee9574d 100644
--- a/drivers/clocksource/timer-probe.c
+++ b/drivers/clocksource/timer-probe.c
@@ -27,10 +27,8 @@ void __init timer_probe(void)
 
 		init_func_ret = match->data;
 
-		of_node_set_flag(np, OF_POPULATED);
 		ret = init_func_ret(np);
 		if (ret) {
-			of_node_clear_flag(np, OF_POPULATED);
 			if (ret != -EPROBE_DEFER)
 				pr_err("Failed to initialize '%pOF': %d\n", np,
 				       ret);