Re: [PATCH 3/3] watchdog: realtek-otto: add RTL9607C support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Registers accessed before peripheral clock is enabled
- [High] Sleeping function `disable_irq()` called in atomic restart handler
- [High] Integer underflow in timeout calculation causes massively delayed watchdog reset
--

commit be1a33571f33f241ada324d2648ec90cdf1e9b91
Author: Rustam Adilov <adilov@disroot.org>

watchdog: realtek-otto: add RTL9607C support

The RTL9607C SoC has a watchdog timer identical to the already
supported SoCs. The peripheral is memory mapped at 0x18003260
and driven by the Lexra bus clock.

> diff --git a/drivers/watchdog/realtek_otto_wdt.c b/drivers/watchdog/realtek_otto_wdt.c
> index e5e9cb480f4f9..7ad196802eeb5 100644
> --- a/drivers/watchdog/realtek_otto_wdt.c
> +++ b/drivers/watchdog/realtek_otto_wdt.c
> @@ -355,6 +355,7 @@ static const struct of_device_id otto_wdt_ids[] = {
>  	{ .compatible = "realtek,rtl8390-wdt" },
>  	{ .compatible = "realtek,rtl9300-wdt" },
>  	{ .compatible = "realtek,rtl9310-wdt" },
> +	{ .compatible = "realtek,rtl9607-wdt" },

This isn't a bug introduced by this patch, but enabling this driver for the
new SoC exposes a few existing regressions in the code.

For example, can the driver safely access the peripheral's memory-mapped
registers before its clock domain is enabled?

otto_wdt_probe() {
[ ... ]
	/* Clear any old interrupts and reset initial state */
	regmap_write(ctrl->regmap, OTTO_WDT_REG_INTR,
		     OTTO_WDT_INTR_PHASE_1 | OTTO_WDT_INTR_PHASE_2);
	regmap_write(ctrl->regmap, OTTO_WDT_REG_CTRL, OTTO_WDT_CTRL_DEFAULT);

	ret = otto_wdt_probe_clk(ctrl);
[ ... ]
}

Writing to OTTO_WDT_REG_INTR and OTTO_WDT_REG_CTRL before calling
otto_wdt_probe_clk() might violate hardware access requirements and could
cause a synchronous external bus abort or system hang during driver probe.

Also, does the system restart handler sleep in an atomic context?

otto_wdt_restart() {
[ ... ]
	disable_irq(ctrl->irq_phase1);
[ ... ]
}

The otto_wdt_restart() function is a system restart handler executed in an
atomic context with local interrupts disabled. Since disable_irq() internally
invokes synchronize_irq(), which uses wait_event() to wait for executing
interrupt handlers to finish, it can sleep.

Calling a sleeping function with local interrupts disabled triggers a kernel
panic, and waiting for an IRQ handler to complete while other CPUs are stopped
during a machine restart can result in a hard deadlock. Should this use
disable_irq_nosync() instead?

Finally, is there an integer underflow in the timeout calculation?

otto_wdt_determine_timeouts() {
[ ... ]
		total_ticks = div_round_ticks(timeout_ms, tick_ms, 2);
		phase1_ticks = div_round_ticks(timeout_ms - pretimeout_ms, tick_ms, 1);
		phase2_ticks = total_ticks - phase1_ticks;
[ ... ]
	val |= FIELD_PREP(OTTO_WDT_CTRL_PHASE2, phase2_ticks - 1);
[ ... ]
}

If the pretimeout is configured to 0 (or a value small enough that it maps to
the same tick interval), total_ticks and phase1_ticks evaluate to the exact
same value, resulting in phase2_ticks = 0.

Since phase2_ticks is an unsigned integer, 0 - 1 underflows to 0xFFFFFFFF,
which FIELD_PREP masks to the maximum field value of 31. Does this configure
the hardware to delay the system reset by 32 ticks (over 5 seconds) instead
of the intended minimum of 1 tick?

>  	{ }
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509163101.722793-1-adilov@disroot.org?part=3