* [PATCH v1] clocksource: Avoid creating dead devices @ 2020-01-11 5:21 Saravana Kannan 2020-02-19 2:20 ` Saravana Kannan ` (2 more replies) 0 siblings, 3 replies; 31+ messages in thread From: Saravana Kannan @ 2020-01-11 5:21 UTC (permalink / raw) To: Daniel Lezcano, Thomas Gleixner Cc: Saravana Kannan, kernel-team, linux-kernel 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 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v1] clocksource: Avoid creating dead devices 2020-01-11 5:21 [PATCH v1] clocksource: Avoid creating dead devices Saravana Kannan @ 2020-02-19 2:20 ` Saravana Kannan 2020-02-27 9:06 ` Daniel Lezcano 2020-03-19 8:47 ` [tip: timers/core] clocksource/drivers/timer-probe: " tip-bot2 for Saravana Kannan 2 siblings, 0 replies; 31+ messages in thread From: Saravana Kannan @ 2020-02-19 2:20 UTC (permalink / raw) To: Daniel Lezcano, Thomas Gleixner; +Cc: Android Kernel Team, LKML 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v1] clocksource: Avoid creating dead devices 2020-01-11 5:21 [PATCH v1] clocksource: Avoid creating dead devices Saravana Kannan 2020-02-19 2:20 ` Saravana Kannan @ 2020-02-27 9:06 ` Daniel Lezcano 2020-02-27 21:22 ` Saravana Kannan 2020-03-19 8:47 ` [tip: timers/core] clocksource/drivers/timer-probe: " tip-bot2 for Saravana Kannan 2 siblings, 1 reply; 31+ messages in thread From: Daniel Lezcano @ 2020-02-27 9:06 UTC (permalink / raw) To: Saravana Kannan, Thomas Gleixner; +Cc: kernel-team, linux-kernel 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v1] clocksource: Avoid creating dead devices 2020-02-27 9:06 ` Daniel Lezcano @ 2020-02-27 21:22 ` Saravana Kannan 2020-03-04 19:30 ` Saravana Kannan 0 siblings, 1 reply; 31+ messages in thread From: Saravana Kannan @ 2020-02-27 21:22 UTC (permalink / raw) To: Daniel Lezcano; +Cc: Thomas Gleixner, Android Kernel Team, LKML 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v1] clocksource: Avoid creating dead devices 2020-02-27 21:22 ` Saravana Kannan @ 2020-03-04 19:30 ` Saravana Kannan 2020-03-04 19:56 ` Daniel Lezcano 0 siblings, 1 reply; 31+ messages in thread From: Saravana Kannan @ 2020-03-04 19:30 UTC (permalink / raw) To: Daniel Lezcano; +Cc: Thomas Gleixner, Android Kernel Team, LKML 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v1] clocksource: Avoid creating dead devices 2020-03-04 19:30 ` Saravana Kannan @ 2020-03-04 19:56 ` Daniel Lezcano 2020-03-08 5:53 ` Saravana Kannan 0 siblings, 1 reply; 31+ messages in thread From: Daniel Lezcano @ 2020-03-04 19:56 UTC (permalink / raw) To: Saravana Kannan; +Cc: Thomas Gleixner, Android Kernel Team, LKML 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v1] clocksource: Avoid creating dead devices 2020-03-04 19:56 ` Daniel Lezcano @ 2020-03-08 5:53 ` Saravana Kannan 2020-03-16 14:57 ` Daniel Lezcano 0 siblings, 1 reply; 31+ messages in thread From: Saravana Kannan @ 2020-03-08 5:53 UTC (permalink / raw) To: Daniel Lezcano; +Cc: Thomas Gleixner, Android Kernel Team, LKML 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v1] clocksource: Avoid creating dead devices 2020-03-08 5:53 ` Saravana Kannan @ 2020-03-16 14:57 ` Daniel Lezcano 2020-03-16 17:49 ` Saravana Kannan 0 siblings, 1 reply; 31+ messages in thread From: Daniel Lezcano @ 2020-03-16 14:57 UTC (permalink / raw) To: Saravana Kannan; +Cc: Thomas Gleixner, Android Kernel Team, LKML 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 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v1] clocksource: Avoid creating dead devices 2020-03-16 14:57 ` Daniel Lezcano @ 2020-03-16 17:49 ` Saravana Kannan 2020-03-16 18:07 ` Daniel Lezcano 0 siblings, 1 reply; 31+ messages in thread From: Saravana Kannan @ 2020-03-16 17:49 UTC (permalink / raw) To: Daniel Lezcano; +Cc: Thomas Gleixner, Android Kernel Team, LKML 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v1] clocksource: Avoid creating dead devices 2020-03-16 17:49 ` Saravana Kannan @ 2020-03-16 18:07 ` Daniel Lezcano 2020-03-16 18:15 ` Saravana Kannan 0 siblings, 1 reply; 31+ messages in thread From: Daniel Lezcano @ 2020-03-16 18:07 UTC (permalink / raw) To: Saravana Kannan; +Cc: Thomas Gleixner, Android Kernel Team, LKML, Paul Cercueil 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v1] clocksource: Avoid creating dead devices 2020-03-16 18:07 ` Daniel Lezcano @ 2020-03-16 18:15 ` Saravana Kannan [not found] ` <5e70b653.1c69fb81.b03d8.d2bbSMTPIN_ADDED_MISSING@mx.google.com> 0 siblings, 1 reply; 31+ messages in thread From: Saravana Kannan @ 2020-03-16 18:15 UTC (permalink / raw) To: Daniel Lezcano; +Cc: Thomas Gleixner, Android Kernel Team, LKML, Paul Cercueil 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 > ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <5e70b653.1c69fb81.b03d8.d2bbSMTPIN_ADDED_MISSING@mx.google.com>]
* Re: [PATCH v1] clocksource: Avoid creating dead devices [not found] ` <5e70b653.1c69fb81.b03d8.d2bbSMTPIN_ADDED_MISSING@mx.google.com> @ 2020-03-17 18:08 ` Saravana Kannan 2020-03-17 18:18 ` Daniel Lezcano 0 siblings, 1 reply; 31+ messages in thread From: Saravana Kannan @ 2020-03-17 18:08 UTC (permalink / raw) To: Paul Cercueil; +Cc: Daniel Lezcano, Thomas Gleixner, Android Kernel Team, LKML 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 > >> > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v1] clocksource: Avoid creating dead devices 2020-03-17 18:08 ` Saravana Kannan @ 2020-03-17 18:18 ` Daniel Lezcano 0 siblings, 0 replies; 31+ messages in thread From: Daniel Lezcano @ 2020-03-17 18:18 UTC (permalink / raw) To: Saravana Kannan, Paul Cercueil; +Cc: Thomas Gleixner, Android Kernel Team, LKML 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices 2020-01-11 5:21 [PATCH v1] clocksource: Avoid creating dead devices Saravana Kannan 2020-02-19 2:20 ` Saravana Kannan 2020-02-27 9:06 ` Daniel Lezcano @ 2020-03-19 8:47 ` tip-bot2 for Saravana Kannan 2020-03-24 17:59 ` Ionela Voinescu 2 siblings, 1 reply; 31+ messages in thread From: tip-bot2 for Saravana Kannan @ 2020-03-19 8:47 UTC (permalink / raw) To: linux-tip-commits; +Cc: Saravana Kannan, Daniel Lezcano, x86, LKML 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); ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices 2020-03-19 8:47 ` [tip: timers/core] clocksource/drivers/timer-probe: " tip-bot2 for Saravana Kannan @ 2020-03-24 17:59 ` Ionela Voinescu 2020-03-24 18:34 ` Saravana Kannan ` (2 more replies) 0 siblings, 3 replies; 31+ messages in thread From: Ionela Voinescu @ 2020-03-24 17:59 UTC (permalink / raw) To: linux-kernel, Saravana Kannan, Daniel Lezcano Cc: linux-tip-commits, x86, liviu.dudau, sudeep.holla, lorenzo.pieralisi 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices 2020-03-24 17:59 ` Ionela Voinescu @ 2020-03-24 18:34 ` Saravana Kannan 2020-03-24 19:56 ` Saravana Kannan [not found] ` <20200324175955.GA16972-5wv7dgnIgG8@public.gmane.org> 2020-03-28 10:30 ` [tip: timers/core] Revert "clocksource/drivers/timer-probe: Avoid creating dead devices" tip-bot2 for Thomas Gleixner 2 siblings, 1 reply; 31+ messages in thread From: Saravana Kannan @ 2020-03-24 18:34 UTC (permalink / raw) To: Ionela Voinescu Cc: LKML, Daniel Lezcano, linux-tip-commits, x86, liviu.dudau, sudeep.holla, Lorenzo Pieralisi 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices 2020-03-24 18:34 ` Saravana Kannan @ 2020-03-24 19:56 ` Saravana Kannan 2020-03-25 21:47 ` Thomas Gleixner 0 siblings, 1 reply; 31+ messages in thread From: Saravana Kannan @ 2020-03-24 19:56 UTC (permalink / raw) To: Ionela Voinescu Cc: LKML, Daniel Lezcano, linux-tip-commits, x86, liviu.dudau, sudeep.holla, Lorenzo Pieralisi 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" ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices 2020-03-24 19:56 ` Saravana Kannan @ 2020-03-25 21:47 ` Thomas Gleixner 2020-03-25 22:56 ` Saravana Kannan 0 siblings, 1 reply; 31+ messages in thread From: Thomas Gleixner @ 2020-03-25 21:47 UTC (permalink / raw) To: Saravana Kannan, Ionela Voinescu Cc: LKML, Daniel Lezcano, linux-tip-commits, x86, liviu.dudau, sudeep.holla, Lorenzo Pieralisi, Jon Hunter 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices 2020-03-25 21:47 ` Thomas Gleixner @ 2020-03-25 22:56 ` Saravana Kannan 2020-03-25 23:06 ` Saravana Kannan ` (2 more replies) 0 siblings, 3 replies; 31+ messages in thread From: Saravana Kannan @ 2020-03-25 22:56 UTC (permalink / raw) To: Thomas Gleixner Cc: Ionela Voinescu, LKML, Daniel Lezcano, linux-tip-commits, x86, liviu.dudau, sudeep.holla, Lorenzo Pieralisi, Jon Hunter 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices 2020-03-25 22:56 ` Saravana Kannan @ 2020-03-25 23:06 ` Saravana Kannan 2020-03-26 10:17 ` Daniel Lezcano 2020-03-26 10:33 ` Thomas Gleixner 2 siblings, 0 replies; 31+ messages in thread From: Saravana Kannan @ 2020-03-25 23:06 UTC (permalink / raw) To: Thomas Gleixner Cc: Ionela Voinescu, LKML, Daniel Lezcano, linux-tip-commits, x86, liviu.dudau, sudeep.holla, Lorenzo Pieralisi, Jon Hunter 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices 2020-03-25 22:56 ` Saravana Kannan 2020-03-25 23:06 ` Saravana Kannan @ 2020-03-26 10:17 ` Daniel Lezcano 2020-03-26 17:35 ` Saravana Kannan 2020-03-26 10:33 ` Thomas Gleixner 2 siblings, 1 reply; 31+ messages in thread From: Daniel Lezcano @ 2020-03-26 10:17 UTC (permalink / raw) To: Saravana Kannan, Thomas Gleixner Cc: Ionela Voinescu, LKML, linux-tip-commits, x86, liviu.dudau, sudeep.holla, Lorenzo Pieralisi, Jon Hunter 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices 2020-03-26 10:17 ` Daniel Lezcano @ 2020-03-26 17:35 ` Saravana Kannan 0 siblings, 0 replies; 31+ messages in thread From: Saravana Kannan @ 2020-03-26 17:35 UTC (permalink / raw) To: Daniel Lezcano Cc: Thomas Gleixner, Ionela Voinescu, LKML, linux-tip-commits, x86, liviu.dudau, sudeep.holla, Lorenzo Pieralisi, Jon Hunter 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices 2020-03-25 22:56 ` Saravana Kannan 2020-03-25 23:06 ` Saravana Kannan 2020-03-26 10:17 ` Daniel Lezcano @ 2020-03-26 10:33 ` Thomas Gleixner 2020-03-26 15:02 ` Rob Herring 2 siblings, 1 reply; 31+ messages in thread From: Thomas Gleixner @ 2020-03-26 10:33 UTC (permalink / raw) To: Saravana Kannan Cc: Ionela Voinescu, LKML, Daniel Lezcano, linux-tip-commits, x86, liviu.dudau, sudeep.holla, Lorenzo Pieralisi, Jon Hunter, Pawel Moll, Rob Herring, Mark Rutland 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices 2020-03-26 10:33 ` Thomas Gleixner @ 2020-03-26 15:02 ` Rob Herring 2020-03-26 18:08 ` Saravana Kannan 0 siblings, 1 reply; 31+ messages in thread From: Rob Herring @ 2020-03-26 15:02 UTC (permalink / raw) To: Thomas Gleixner, Saravana Kannan Cc: Ionela Voinescu, LKML, Daniel Lezcano, linux-tip-commits, x86, Liviu Dudau, Sudeep Holla, Lorenzo Pieralisi, Jon Hunter, Pawel Moll, Mark Rutland 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices 2020-03-26 15:02 ` Rob Herring @ 2020-03-26 18:08 ` Saravana Kannan 2020-03-28 2:23 ` Rob Herring 0 siblings, 1 reply; 31+ messages in thread From: Saravana Kannan @ 2020-03-26 18:08 UTC (permalink / raw) To: Rob Herring Cc: Thomas Gleixner, Ionela Voinescu, LKML, Daniel Lezcano, linux-tip-commits, x86, Liviu Dudau, Sudeep Holla, Lorenzo Pieralisi, Jon Hunter, Pawel Moll, Mark Rutland 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices 2020-03-26 18:08 ` Saravana Kannan @ 2020-03-28 2:23 ` Rob Herring 0 siblings, 0 replies; 31+ messages in thread From: Rob Herring @ 2020-03-28 2:23 UTC (permalink / raw) To: Saravana Kannan Cc: Thomas Gleixner, Ionela Voinescu, LKML, Daniel Lezcano, linux-tip-commits, x86, Liviu Dudau, Sudeep Holla, Lorenzo Pieralisi, Jon Hunter, Pawel Moll, Mark Rutland 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <20200324175955.GA16972-5wv7dgnIgG8@public.gmane.org>]
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices 2020-03-24 17:59 ` Ionela Voinescu @ 2020-03-25 21:29 ` Jon Hunter [not found] ` <20200324175955.GA16972-5wv7dgnIgG8@public.gmane.org> 2020-03-28 10:30 ` [tip: timers/core] Revert "clocksource/drivers/timer-probe: Avoid creating dead devices" tip-bot2 for Thomas Gleixner 2 siblings, 0 replies; 31+ messages in thread From: Jon Hunter @ 2020-03-25 21:29 UTC (permalink / raw) To: Ionela Voinescu, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Saravana Kannan, Daniel Lezcano Cc: linux-tip-commits-u79uwXL29TY76Z2rM5mHXA, x86, liviu.dudau-5wv7dgnIgG8, sudeep.holla-5wv7dgnIgG8, lorenzo.pieralisi-5wv7dgnIgG8, linux-tegra 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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> >> AuthorDate: Fri, 10 Jan 2020 21:21:25 -08:00 >> Committer: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> >> Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> >> Link: https://lore.kernel.org/r/20200111052125.238212-1-saravanak-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org >> --- >> 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices @ 2020-03-25 21:29 ` Jon Hunter 0 siblings, 0 replies; 31+ messages in thread From: Jon Hunter @ 2020-03-25 21:29 UTC (permalink / raw) To: Ionela Voinescu, linux-kernel, Saravana Kannan, Daniel Lezcano Cc: linux-tip-commits, x86, liviu.dudau, sudeep.holla, lorenzo.pieralisi, linux-tegra 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <e75ae529-264c-9aa6-f711-2afe28ceec36-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices 2020-03-25 21:29 ` Jon Hunter @ 2020-03-26 1:21 ` Dmitry Osipenko -1 siblings, 0 replies; 31+ messages in thread From: Dmitry Osipenko @ 2020-03-26 1:21 UTC (permalink / raw) To: Jon Hunter, Ionela Voinescu, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Saravana Kannan, Daniel Lezcano Cc: linux-tip-commits-u79uwXL29TY76Z2rM5mHXA, x86, liviu.dudau-5wv7dgnIgG8, sudeep.holla-5wv7dgnIgG8, lorenzo.pieralisi-5wv7dgnIgG8, linux-tegra 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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> >>> AuthorDate: Fri, 10 Jan 2020 21:21:25 -08:00 >>> Committer: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> >>> Link: https://lore.kernel.org/r/20200111052125.238212-1-saravanak-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org >>> --- >>> 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [tip: timers/core] clocksource/drivers/timer-probe: Avoid creating dead devices @ 2020-03-26 1:21 ` Dmitry Osipenko 0 siblings, 0 replies; 31+ messages in thread From: Dmitry Osipenko @ 2020-03-26 1:21 UTC (permalink / raw) To: Jon Hunter, Ionela Voinescu, linux-kernel, Saravana Kannan, Daniel Lezcano Cc: linux-tip-commits, x86, liviu.dudau, sudeep.holla, lorenzo.pieralisi, linux-tegra 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* [tip: timers/core] Revert "clocksource/drivers/timer-probe: Avoid creating dead devices" 2020-03-24 17:59 ` Ionela Voinescu 2020-03-24 18:34 ` Saravana Kannan [not found] ` <20200324175955.GA16972-5wv7dgnIgG8@public.gmane.org> @ 2020-03-28 10:30 ` tip-bot2 for Thomas Gleixner 2 siblings, 0 replies; 31+ messages in thread From: tip-bot2 for Thomas Gleixner @ 2020-03-28 10:30 UTC (permalink / raw) To: linux-tip-commits Cc: Ionela Voinescu, Jon Hunter, Thomas Gleixner, Saravana Kannan, Daniel Lezcano, x86, LKML 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); ^ permalink raw reply related [flat|nested] 31+ messages in thread
end of thread, other threads:[~2020-03-28 10:31 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-11 5:21 [PATCH v1] clocksource: Avoid creating dead devices Saravana Kannan 2020-02-19 2:20 ` Saravana Kannan 2020-02-27 9:06 ` Daniel Lezcano 2020-02-27 21:22 ` Saravana Kannan 2020-03-04 19:30 ` Saravana Kannan 2020-03-04 19:56 ` Daniel Lezcano 2020-03-08 5:53 ` Saravana Kannan 2020-03-16 14:57 ` Daniel Lezcano 2020-03-16 17:49 ` Saravana Kannan 2020-03-16 18:07 ` Daniel Lezcano 2020-03-16 18:15 ` Saravana Kannan [not found] ` <5e70b653.1c69fb81.b03d8.d2bbSMTPIN_ADDED_MISSING@mx.google.com> 2020-03-17 18:08 ` Saravana Kannan 2020-03-17 18:18 ` Daniel Lezcano 2020-03-19 8:47 ` [tip: timers/core] clocksource/drivers/timer-probe: " tip-bot2 for Saravana Kannan 2020-03-24 17:59 ` Ionela Voinescu 2020-03-24 18:34 ` Saravana Kannan 2020-03-24 19:56 ` Saravana Kannan 2020-03-25 21:47 ` Thomas Gleixner 2020-03-25 22:56 ` Saravana Kannan 2020-03-25 23:06 ` Saravana Kannan 2020-03-26 10:17 ` Daniel Lezcano 2020-03-26 17:35 ` Saravana Kannan 2020-03-26 10:33 ` Thomas Gleixner 2020-03-26 15:02 ` Rob Herring 2020-03-26 18:08 ` Saravana Kannan 2020-03-28 2:23 ` Rob Herring [not found] ` <20200324175955.GA16972-5wv7dgnIgG8@public.gmane.org> 2020-03-25 21:29 ` Jon Hunter 2020-03-25 21:29 ` Jon Hunter [not found] ` <e75ae529-264c-9aa6-f711-2afe28ceec36-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2020-03-26 1:21 ` Dmitry Osipenko 2020-03-26 1:21 ` Dmitry Osipenko 2020-03-28 10:30 ` [tip: timers/core] Revert "clocksource/drivers/timer-probe: Avoid creating dead devices" tip-bot2 for Thomas Gleixner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.