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

[Daniel Lezcano]

On 17/03/2020 19:08, Saravana Kannan wrote:
> On Tue, Mar 17, 2020 at 4:36 AM Paul Cercueil <paul@xxxxxxxxxxxxxxx> 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