Re: [PATCH v3 0/6] watchdog: report effective timeout

On 21.08.26 19:18, Quentin Schulz wrote:
> 1. Is it really appropriate to have a timeout smaller than requested? 
> Maybe the answer is that we should fail if the user requests an 
> unsupported timeout?

I don't think we should. If wdt_start() fails, init_watchdog_dev() just
prints an error and returns, priv->running stays false and no watchdog
runs at all. Firing earlier than asked is usually better than that.

It would also change behaviour for quite a few boards. Most drivers
already clamp silently in hardware today (ast_wdt, at91sam9, cdns, imx,
mpc8xxx, octeontx, sbsa_gwdt, max6370), so a timeout above the hardware
maximum is not something anyone currently notices. arm_smc_wdt is the
exception and does return -EINVAL, but making that the uclass-wide rule
would drop the watchdog entirely on all the others.

Linux does not fail either, where it matters. watchdog_timeout_invalid()
only rejects a too-large timeout when max_hw_heartbeat_ms is unset. If
the driver sets it, the core takes the requested value and its worker
pings the hardware more often instead. Our clamp plus the cyclic
servicing is the bootloader equivalent of that.

What I do agree with is that clamping must not be silent, and v3 prints
", requested %ds" for that.

> 2. I'm not sure it is enough to clamp the max. The Rockchip RK806 PMIC 
> has a watchdog with only a few possible values as timeout: 50ms, 100ms, 
> 500ms, 2s, 10s, 1min and 10min.

Agreed, and we don't need the RK806 to run into it. max6370_wdt.c has
the same shape already: 1ms, 10ms, 30ms, 100ms, 1s, 10s, 60s, rounding
up to the next step.

designware_wdt is arguably worse, since it rounds up to powers of two:

	i = fls(timeout * clk_khz - 1) - 16;
	i = clamp(i, 0, 15);

At 25MHz, asking for 3s gets you 2^27 cycles, which is 5.37s. Nothing
hit a maximum there, so max_timeout_ms would not help and the print
stays wrong. booke_wdt and renesas_wwdt round as well.

So reporting the effective value is the more complete fix, yes, and I
think it is worth doing. Note that it is not a step backwards either.
Simon's suggestion to use max_timeout_ms came with the qualifier
"assuming this is a static value", and for sbsa_gwdt and octeontx that
holds, there is a real fixed hardware maximum. What your PMIC and
max6370 show is that the effective value is not always static, and that
is exactly the case the plat field cannot express.

For that case v1 already had the right shape:

	int (*get_timeout)(struct udevice *dev, u64 *timeout_ms);

with wdt_start() calling it after a successful start and printing what
comes back. Drivers that don't implement it keep the current behaviour,
and the ones that round or pick from a fixed set report what they
actually programmed. Simon already said in v1 how he'd want it built,
pass timeout_ms straight through and document that drivers only write
it on success.

I would rather not put it in the uclass plat data. It changes on every
start, so it is runtime state, and it would have to be cleared before
each ops->start() to not leave a stale value behind on the error path.
Overloading ops->start()'s return value would work too, there is
exactly one caller in wdt-uclass.c, but reviving the v1 op reuses a
design Simon has already reviewed.

There is one more reason to want this beyond the print. wdt_pre_probe()
derives the servicing interval from hw_margin_ms alone, with no relation
to the timeout at all. If the effective timeout ends up shorter than the
service interval, the watchdog fires during normal operation. I don't
think any in-tree DT hits that today, but once the uclass knows the real
timeout it can warn instead of letting people find out the hard way.

Juuso, I would like to take v3 as it stands. It fixes a real bug,
drivers that don't set max_timeout_ms are unaffected, and the
effective-value work sits on top of it rather than replacing it. I
applied it on current mainline here, checkpatch is clean and the wdt
tests pass on sandbox64; octeontx_81xx and lx2160ardb_tfa build fine
too. We are past -rc2, so this goes to next for the next merge window
and there is no hurry with the follow-up.

One thing though: none of the v3 patches carry Simon's Reviewed-by from
v2. Was that deliberate? 3/6 is byte-identical to v2 2/5, and 4/6 and
5/6 only changed in the ways Simon asked for, so those three should
have kept the tag. Only 2/6 really changed, the clamping before
ops->start() is new there, and I would not carry a tag over for that
one. If you use b4, "b4 trailers -u" picks the tags up for you.

No need to resend just for that. I can add them while applying if you
confirm that is what you meant.

Simon, 2/6 does something different from what you reviewed in v2, so a
look at that one would be welcome.

Thanks,
Stefan