imx8 regression: cyclic_register for watchdog at 30280000 failed

[Rasmus Villemoes]

On 25/10/2022 18.32, Tim Harvey wrote:
> Greetings,
> 
> I've noticed a regression since the merge of the cyclic framework use
> for watchdog on my imx8m boards:
> 
> cyclic_register for watchdog at 30280000 failed
> WDT:   Failed to start watchdog at 30280000
> 
> A bisect lead me to the following 3 sequential patches:
> 29caf9305b6f cyclic: Use schedule() instead of WATCHDOG_RESET()
> ^^^ bad
> 881d4108257a cyclic: Introduce schedule() function
> ^^^ unbuildable
> c2fd0ca1a822 watchdog: Integrate watchdog triggering into the cyclic framework
> ^^^ unbootable

Can you provide some more details on "unbootable" and "unbuildable"?
I.e., what build error do you get for the middle patch, and what do you
see on the console with the "unbootable" image?

Also, the .configs used in each case might be interesting, certainly all
lines containing CYCLIC, WATCHDOG or WDT.

One thing I notice is that I think wdt_stop should unregister the cyclic
function; there's really no point having the cyclic framework keep
calling wdt_cyclic() only to bail out at "if (!priv->running)" -
moreover, it's almost certainly a bug if the device is started again via
another wdt_start(), because then we have two cyclic instances
registered for the same device.

I also think the cyclic API is somewhat misdesigned. The storage for the
cyclic metadata should be provided by the caller (e.g. in the watchdog
case just embedded as part of struct wdt_priv), so that
cyclic_register() can never fail. Otherwise we have this awkward
situation where ops->start() have already been called and succeeded, but
then perhaps we fail to register the cyclic instance; should we then do
wdt_stop(), and what if _that_ then fails?

This also allows the callback to be a little more type-safe; let the
callback take a "struct cyclic_info *" as argument, and then the
callback can use e.g. container_of to get the containing wdt_priv.

Rasmus