Re: [PATCH] watchdog: stm32_iwdg: Base pretimeout on timeout when not specified

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] Userspace cannot disable the pretimeout feature, and the patch introduces a state inconsistency where the API reports the pretimeout as disabled (0) while the hardware interrupt remains active.

Pre-existing issues:
- [High] The driver fails to validate and clear an explicitly configured `pretimeout` when a new, shorter `timeout` is set, causing an arithmetic underflow and continuous interrupts.
- [High] The pretimeout calculation mathematically underflows when the software `timeout` exceeds the hardware's maximum supported limit, resulting in an immediate interrupt storm.
- [High] The driver fundamentally inverts the logic of the `pretimeout` UAPI, treating it as elapsed time instead of remaining time.
--

commit 53fd4e70debe020df0adba54327baa8bf331b885
Author: Benjamin Frost <[email protected]>

watchdog: stm32_iwdg: Base pretimeout on timeout when not specified

This commit introduces a local pretimeout variable to hold the effective
pretimeout value. This prevents the driver from modifying the configured
pretimeout, ensuring it is recalculated when the timeout changes.

> diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c
> index b356a272ff9a0..a38e204129ff4 100644
> --- a/drivers/watchdog/stm32_iwdg.c
> +++ b/drivers/watchdog/stm32_iwdg.c
[ ... ]
> @@ -97,17 +97,19 @@ static int stm32_iwdg_start(struct watchdog_device *wdd)
>  {
>  	struct stm32_iwdg *wdt = watchdog_get_drvdata(wdd);
> -	u32 tout, ptot, presc, iwdg_rlr, iwdg_ewcr, iwdg_pr, iwdg_sr;
> +	u32 tout, ptot, presc, iwdg_rlr, iwdg_ewcr, iwdg_pr, iwdg_sr, pretimeout;
>  	int ret;
>  
>  	dev_dbg(wdd->parent, "%s\n", __func__);
>  
> -	if (!wdd->pretimeout)
> -		wdd->pretimeout = 3 * wdd->timeout / 4;
> +	if (wdd->pretimeout)
> +		pretimeout = wdd->pretimeout;
> +	else
> +		pretimeout = 3 * wdd->timeout / 4;

[Severity: Medium]
If userspace disables the pretimeout by setting it to 0, doesn't this logic
force a fallback to 3/4 of the timeout using the local variable?

Because the hardware Early Wakeup Interrupt is unconditionally enabled
further down in this function:

	if (wdt->data->has_early_wakeup)
		reg_write(wdt->regs, IWDG_EWCR, iwdg_ewcr | EWCR_EWIE);

and wdd->pretimeout remains 0, the API will falsely report the pretimeout as
disabled while the hardware continuously generates interrupts.

[Severity: High]
This is a pre-existing issue, but stm32_iwdg_set_timeout() lacks logic to cap
or clear wdd->pretimeout when a new, shorter timeout is set.

If a user explicitly sets a pretimeout of 10s and later reduces the timeout to
5s, wdd->pretimeout remains 10s, which then leads to an underflow during the
subtraction below.

>  	tout = clamp_t(unsigned int, wdd->timeout,
>  		       wdd->min_timeout, wdd->max_hw_heartbeat_ms / 1000);

[Severity: High]
This isn't a bug introduced by this patch, but does this clamping logic break
when using long software-emulated timeouts?

If wdd->timeout is larger than the hardware limit, tout is clamped down
(e.g., to ~32s). However, pretimeout is derived from the unclamped software
wdd->timeout (e.g., for a 100s timeout, fallback pretimeout is 75s).

When the code calculates tout - pretimeout below, 32 - 75 will underflow,
causing ptot to be clamped to tout. The early wakeup register then equals
the reload register, causing the interrupt to fire instantly on every
worker ping.

> -	ptot = clamp_t(unsigned int, tout - wdd->pretimeout,
> +	ptot = clamp_t(unsigned int, tout - pretimeout,
>  		       wdd->min_timeout, tout);

[Severity: High]
This is a pre-existing issue, but does this calculation reverse the
watchdog UAPI semantics?

The UAPI defines pretimeout as the remaining time before the reset triggers.
The hardware uses a down-counter that triggers when it reaches IWDG_EWCR,
which means IWDG_EWCR should represent the remaining time.

By calculating ptot = tout - pretimeout, a 10s pretimeout on a 60s timeout
programs the register to 50s. The hardware starts at 60s and triggers at 50s,
meaning the interrupt fires after only 10 seconds of elapsed time (50 seconds
too early).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1