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

[Sander Vanheule <sander@svanheule.net>]

As a general remark, it would be good if this bot could use the original To/Cc
list of the message this was sent in reply to.


On Sat, 2026-05-09 at 17:27 +0000, sashiko-bot@kernel.org wrote:
> 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.

otto_wdt_probe_clk() can be moved up to come before the first bus accesses.

This would not cause issues on real hardware, as the bootloader will have
ensured the watchdog's bus is up since it is shared with the UART console.



> 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?

I don't see why it couldn't use disable_irq_nosync().


> 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?

While this does indeed delay the system reset to the maximum time, the
pretimeout interrupt (end of PHASE1) would still occur at the intended time.
During PHASE2, the watchdog cannot be pinged anymore and system reset remains
inevitable.

I'll send a set of patches to resolve these issues, as they don't really relate
to adding RTL9706C support.

Best,
Sander