Re: [PATCH v2 1/2] watchdog: orion_wdt: load the pretimeout counter on Armada 370/XP start

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] watchdog: orion_wdt: global orion_wdt_info struct contamination
- [Low] watchdog: orion_wdt: TIMER1 clock rate mismatch for pretimeout on Armada 370/375
--

commit dbf1fe3fb63d91004e0ddd635005322182202b47
Author: Igor Velkov <iav@iav.lv>

watchdog: orion_wdt: load the pretimeout counter on Armada 370/XP start

armada370_start() enables TIMER1 for the pretimeout but, unlike
armada375_start(), does not load its counter, so TIMER1 runs down from
whatever value it holds and the pretimeout can fire early. Load TIMER1 on
start the same way armada375_start() does.

Link: https://sashiko.dev/#/patchset/20261012-orion-wdt-suspend-v1-1-1676cb79e29e@iav.lv?part=1

> diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
> index 5d56e93f04064..ef64e37300012 100644
> --- a/drivers/watchdog/orion_wdt.c
> +++ b/drivers/watchdog/orion_wdt.c
> @@ -232,6 +232,9 @@ static int armada370_start(struct watchdog_device *wdt_dev)
>  	/* Set watchdog duration */
>  	writel(dev->clk_rate * wdt_dev->timeout,
>  	       dev->reg + dev->data->wdt_counter_offset);
> +	if (dev->wdt.info->options & WDIOF_PRETIMEOUT)
[Severity: Medium]
Is it possible for WDIOF_PRETIMEOUT to be set spuriously due to global
state contamination?

In drivers/watchdog/orion_wdt.c:orion_wdt_probe(), the global static
structure orion_wdt_info is unconditionally mutated if a pretimeout IRQ
is found:

static struct watchdog_info orion_wdt_info = {
    ...
};

orion_wdt_probe() {
    ...
    if (irq > 0) {
        orion_wdt_info.options |= WDIOF_PRETIMEOUT;
...
}

If a prior probe on a device or platform had the IRQ, a subsequent device
that does not have the IRQ will still inherit the flag across unbind/rebind
cycles or multiple instances.

Does this cause the newly added code here to erroneously write to
TIMER1_VAL_OFF and mistakenly enable the TIMER1 pretimeout interrupt, risking
an unhandled interrupt storm?

> +		writel(dev->clk_rate * (wdt_dev->timeout - wdt_dev->pretimeout),
> +		       dev->reg + TIMER1_VAL_OFF);
[Severity: Low]
Does this calculation incorrectly assume TIMER1 counts down at the
divided/fixed clock rate dev->clk_rate?

On Armada 370, drivers/watchdog/orion_wdt.c:armada370_wdt_clock_init() sets
WDT_A370_RATIO_MASK for the watchdog, but fails to set the corresponding
TIMER1_DIV:

static int armada370_wdt_clock_init(...)
{
    ...
    /* Setup watchdog input clock */
    atomic_io_modify(dev->reg + TIMER_CTRL,
            WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT),
            WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT));
    
    dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO;
    ...
}

Similarly on Armada 375, the initialization in armada375_wdt_clock_init()
sets WDT_AXP_FIXED_ENABLE_BIT but omits TIMER1_FIXED_ENABLE_BIT, unlike
Armada XP.

If a user boots Armada 370 or Armada 375 with a device tree that defines the
pretimeout interrupt, won't the pretimeout fire prematurely because TIMER1
counts down at the undivided/unfixed clock rate, while the initial counter
value is calculated using the slower dev->clk_rate?

>  
>  	/* Clear the watchdog expiration bit */
>  	atomic_io_modify(dev->reg + TIMER_A370_STATUS, WDT_A370_EXPIRED, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-orion-wdt-suspend-v2-0-f13cf71443ba@iav.lv?part=1