Re: [PATCH v2 07/11] watchdog: Add ARM TWD (MPCore private) watchdog driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] watchdog: arm_twd_wdt: stale clock rate causes spurious reboots
- [Medium] watchdog: arm_twd_wdt: boot CPU watchdog fails to restart after resume
--

commit 6655d83c448f4cf87494cf40f7b8891a218a895c
Author: Fil Dunsky <filipp.dunsky@gmail.com>

watchdog: Add ARM TWD (MPCore private) watchdog driver

ARM11 MPCore, Cortex-A5 and Cortex-A9 MPCore systems have a watchdog next to
the local timer of every CPU. This commit adds a driver to program, ping,
and stop the watchdogs of all online CPUs together.

> diff --git a/drivers/watchdog/arm_twd_wdt.c b/drivers/watchdog/arm_twd_wdt.c
> new file mode 100644
> index 0000000000000..a582234e3d953
> --- /dev/null
> +++ b/drivers/watchdog/arm_twd_wdt.c
> @@ -0,0 +1,395 @@

[ ... ]

> +static u32 twd_wdt_ticks(struct twd_wdt *wdt, unsigned int secs)
> +{
> +	u64 ticks = div_u64((u64)wdt->rate * secs, TWD_WDT_PRESCALE + 1);
> +
> +	return min_t(u64, ticks, U32_MAX);
> +}

[Severity: High]
Does this calculation use a stale clock rate if the CPU frequency scales up?

The ARM TWD clock is tied to the CPU frequency on many MPCore systems. If
the CPU scales up its frequency via cpufreq, the actual clock rate will be
higher than the cached wdt->rate.

Could this result in computing too few ticks for the requested timeout,
leading to a premature expiration and spurious system reboots?

[ ... ]

> +static void twd_wdt_cpu_ping(void *data)
> +{
> +	struct twd_wdt *wdt = data;
> +
> +	/* In watchdog mode a write to the load register reloads the counter */
> +	writel(twd_wdt_ticks(wdt, wdt->wdd.timeout), wdt->base + TWD_WDT_LOAD);
> +}

[Severity: Medium]
The ping function only writes to the LOAD register. If the system resumes
from suspend-to-RAM, the watchdog core's ping worker will invoke this
method. Since it does not restore the CONTROL register, will the boot
CPU's watchdog silently remain disabled after power loss?

[ ... ]

> +static int twd_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct twd_wdt *wdt;
> +	struct clk *clk;

[ ... ]

> +	clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
> +
> +	wdt->rate = clk_get_rate(clk);

[Severity: High]
The clock rate is cached statically during probe. Since the clk pointer is
not saved, does this prevent dynamic rate updates if the CPU frequency
changes via cpufreq without modifying the driver?

> +	if (!wdt->rate)
> +		return dev_err_probe(dev, -EINVAL, "clock rate is zero\n");

[ ... ]

> +static struct platform_driver twd_wdt_driver = {
> +	.probe = twd_wdt_probe,
> +	.driver = {
> +		.name = "arm-twd-wdt",
> +		.of_match_table = twd_wdt_of_match,
> +	},
> +};

[Severity: Medium]
This platform driver appears to lack a .pm field or syscore_ops. 

During system suspend, the TWD block loses power and its CONTROL register
is cleared. While non-boot CPUs are hotplugged and restart their watchdogs
via the online hook, the boot CPU is not hotplugged. 

Does this mean the boot CPU's watchdog state is never restored after resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-b4-cs75xx-s2-v2-0-94f8c0a90cda@gmail.com?part=7