On 17/03/2020 13.21, Wolfgang Denk wrote:
Dear Rasmus,
In message 1b6c7efd-8264-6cb5-0b39-3223bae5f8dc@prevas.dk you wrote:
Or do you not agree that __udelay is supposed to be a raw primitive that does the delay and nothing else?
I agree, and it does that - it converts the microseconds into ticks, and calls wait_ticks(), and does nothing else.
It's the wait_ticks() implementation which references the macro WATCHDOG_RESET.
That's hair-splitting, wait_ticks only has that single caller. And regardless of whether __udelay is a leaf function or not, the call graph below it is what determines whether it can be considered a primitive. Currently, it is not.
There are not that many __udelay() calls, so I doubt this causes a regression for anyone. Callers of udelay() are not affected, since udelay() itself does one WATCHDOG_RESET() per __udelay() call.
Which exact platforms have you tested this on?
An MPC8309-derived board which does not utilize the SOCs watchdog but has an external gpio-triggered (always-running) watchdog circuit.
This is not even close to global coverage.
No, of course not. Shall I list all __udelay() calls in the tree and exclude the ones that are in board-specific code for non-ppc boards? I'm pretty sure that leaves a very small handful. I can do that.
Are you aware that there is the PPC-global implementation of wait_ticks() in arch/powerpc/lib/ticks.S , plus another MPC83xx specific one in drivers/timer/mpc83xx_timer.c?
Yes, that latter one doesn't even compile in the presence of CONFIG_WATCHDOG or CONFIG_HW_WATCHDOG.
I don't even understand why MPC83xx needs special treatment.
It doesn't. I don't understand why powerpc's __udelay needs to be different from all other architectures'. This is not really about the quirks of ppc SOCs' watchdogs or not, it's about providing a __udelay primitive that generic code can rely on has certain properties, and in particular, that watchdog drivers can use without risking being called recursively.
In any case it seems to me a board specific redefinition of the WATCHDOG_RESET macro would be less intrusive and risky than changing code that has been there since the beginning of time (well, at least more than 18 years).
The point is, we're being told that everything is moving to DM and better convert your board or else..., and nowadays CONFIG_WDT comes with it's own watchdog_reset() which is a rather more complicated beast than the board-specific ones that used to be sprinkled throughout (and out-of) the tree. So yes, for the past 18 years, nothing bad has probably come from doing a WATCHDOG_RESET even deep down in the guts of arch-specific primitives.
Rasmus