Re: [PATCH v3 1/1] watchdog: realtek-otto: Change to use regmap API

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

Pre-existing issues:
- [High] Calling `disable_irq()` in the restart handler causes a scheduling while atomic bug and a deadlock when triggered by a pretimeout panic.
- [High] The watchdog timeout setup logic does not clamp user timeouts to hardware limits, breaking the watchdog core's software-extended timeout feature.
--

--- Patch [1]: [PATCH v3 1/1] watchdog: realtek-otto: Change to use regmap API ---
commit df79c826fd293fb9a9e926fad614d84ecc05598a
Author: Rustam Adilov <[email protected]>

watchdog: realtek-otto: Change to use regmap API

To make the realtek watchdog driver functional when SWAP_IO_SPACE
config is enabled, change all of the register access to be done
by regmap API which helps us to tweak endianness with big-endian
or little-endian property from within the device tree node.

Add the REGMAP_MMIO as a select to REALTEK_OTTO_WDT now that the
regmap is used.

> diff --git a/drivers/watchdog/realtek_otto_wdt.c b/drivers/watchdog/realtek_otto_wdt.c
> index 9094f2189f555..5c48ffba70a20 100644
> --- a/drivers/watchdog/realtek_otto_wdt.c
> +++ b/drivers/watchdog/realtek_otto_wdt.c

[ ... ]

> @@ -141,14 +134,11 @@ static int otto_wdt_determine_timeouts(struct watchdog_device *wdev, unsigned in
>  	} while (phase1_ticks > OTTO_WDT_PHASE_TICKS_MAX
>  		|| phase2_ticks > OTTO_WDT_PHASE_TICKS_MAX);
>  
> -	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
> -
> -	v &= ~(OTTO_WDT_CTRL_PRESCALE | OTTO_WDT_CTRL_PHASE1 | OTTO_WDT_CTRL_PHASE2);
> -	v |= FIELD_PREP(OTTO_WDT_CTRL_PHASE1, phase1_ticks - 1);
> -	v |= FIELD_PREP(OTTO_WDT_CTRL_PHASE2, phase2_ticks - 1);
> -	v |= FIELD_PREP(OTTO_WDT_CTRL_PRESCALE, prescale);
> -
> -	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
> +	mask = OTTO_WDT_CTRL_PRESCALE | OTTO_WDT_CTRL_PHASE1 | OTTO_WDT_CTRL_PHASE2;
> +	val = FIELD_PREP(OTTO_WDT_CTRL_PHASE1, phase1_ticks - 1);
> +	val |= FIELD_PREP(OTTO_WDT_CTRL_PHASE2, phase2_ticks - 1);
> +	val |= FIELD_PREP(OTTO_WDT_CTRL_PRESCALE, prescale);
> +	regmap_update_bits(ctrl->regmap, OTTO_WDT_REG_CTRL, mask, val);
>  
>  	timeout_ms = total_ticks * tick_ms;
>  	ctrl->wdev.timeout = timeout_ms / 1000;

[Severity: High]
This is a pre-existing issue, but does otto_wdt_determine_timeouts() break
the watchdog core's software heartbeat extension?

When a user requests a timeout larger than the hardware's maximum capability,
the prescale calculation loop earlier in this function exceeds
OTTO_WDT_PRESCALE_MAX and returns -EINVAL.

It seems the driver fails to clamp the requested timeout to its hardware
capability before programming the registers, and overwrites wdev.timeout with
the calculated hardware ticks. This prevents the core from maintaining the
full user-requested timeout via periodic pings.

[ ... ]

> @@ -192,7 +182,7 @@ static int otto_wdt_restart(struct watchdog_device *wdev, unsigned long reboot_m
>  
>  	/* Configure for shortest timeout and wait for reset to occur */
>  	v = FIELD_PREP(OTTO_WDT_CTRL_RST_MODE, reset_mode) | OTTO_WDT_CTRL_ENABLE;
> -	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
> +	regmap_write(ctrl->regmap, OTTO_WDT_REG_CTRL, v);

[Severity: High]
This isn't a bug introduced by this patch, but can the call to disable_irq()
earlier in otto_wdt_restart() cause a deadlock during a pretimeout panic?

If the pretimeout governor is set to panic, otto_wdt_phase1_isr() calls
watchdog_notify_pretimeout() which triggers a kernel panic. The panic routine
disables local interrupts and calls machine_restart(), which invokes
otto_wdt_restart().

Because otto_wdt_restart() calls disable_irq(), it internally calls
synchronize_irq() which will spin waiting for the currently active interrupt
handler (otto_wdt_phase1_isr) to complete.

Since we are already inside that handler, this will hang indefinitely during
reboot. Additionally, calling disable_irq() in an atomic context with
interrupts disabled could trigger a scheduling while atomic bug.

>  
>  	mdelay(3 * otto_wdt_tick_ms(ctrl, 0));

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