Messages in this thread | | | Subject | Re: [PATCH v1] clocksource: Avoid creating dead devices | From | Daniel Lezcano <> | Date | Tue, 17 Mar 2020 19:18:54 +0100 |
| |
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
| |