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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] watchdog: acquiring cpus_read_lock() in clock notifier causes deadlock
- [High] watchdog: arm_twd_wdt: watchdog silently disabled across cpuidle deep sleep
- [High] watchdog: arm_twd_wdt: missed ping on concurrent clock rate change
--

commit 3df85baea3ae83b30f30fe157567af6f425f2a20
Author: Fil Dunsky <filipp.dunsky@gmail.com>

watchdog: Add ARM TWD (MPCore private) watchdog driver

This patch adds a watchdog driver for the ARM Cortex-A9/A5 and ARM11
MPCore private watchdog. It programs, pings and stops the watchdog on
all online CPUs and registers a hotplug state to handle CPUs coming
online and going offline.

> diff --git a/drivers/watchdog/arm_twd_wdt.c b/drivers/watchdog/arm_twd_wdt.c
> new file mode 100644
> index 0000000000000..18875ae24a243
> --- /dev/null
> +++ b/drivers/watchdog/arm_twd_wdt.c
[ ... ]
> +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: High]
Does this ping routine silently fail to re-enable the watchdog if the CPU
previously entered a deep cpuidle sleep state?

If cpuidle cuts power to the CPU's power domain, TWD_WDT_CONTROL might be
lost. Writing to TWD_WDT_LOAD alone without restoring TWD_WDT_CONTROL could
leave the watchdog permanently disabled upon wakeup.

[ ... ]
> +static int twd_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct twd_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	cpus_read_lock();
> +	on_each_cpu(twd_wdt_cpu_start, wdt, 1);
> +	wdt->armed = true;
> +	cpus_read_unlock();
> +
> +	return 0;
> +}

[Severity: High]
Could setting wdt->armed = true after calling on_each_cpu() cause a race
condition with a concurrent clock rate change?

Because this function uses a shared reader lock (cpus_read_lock()), it can
run concurrently with twd_wdt_clk_notify(). If the clock rate increases just
as the watchdog is started, twd_wdt_clk_notify() might see wdt->armed == false
and skip resynchronizing the ping, resulting in the hardware running with a
timeout calculated for the old, slower clock rate.

[ ... ]
> +static int twd_wdt_clk_notify(struct notifier_block *nb, unsigned long event,
> +			      void *data)
> +{
> +	struct twd_wdt *wdt = container_of(nb, struct twd_wdt, clk_nb);
> +	struct clk_notifier_data *cnd = data;
> +	unsigned long rate;
[ ... ]
> +	cpus_read_lock();
> +	wdt->rate = rate;
> +	if (wdt->armed)
> +		on_each_cpu(twd_wdt_cpu_ping, wdt, 1);
> +	cpus_read_unlock();
> +
> +	return NOTIFY_OK;
> +}

[Severity: High]
Can acquiring cpus_read_lock() here cause an AB-BA deadlock with CPU
hotplug?

The clock framework's prepare_lock is already held when this notifier is
called. The CPU hotplug path acquires the global hotplug lock for writing
via cpus_write_lock() and invokes callbacks, including cpuhp_cpufreq_online().

Cpufreq drivers update CPU frequencies during hotplug by calling
clk_set_rate(), which acquires the clock framework's prepare_lock mutex,
establishing a locking order of cpu_hotplug_lock -> prepare_lock.

Acquiring cpus_read_lock() here creates an inverted lock order of
prepare_lock -> cpu_hotplug_lock.

[ ... ]
> +static int twd_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct twd_wdt *wdt;
> +	struct clk *clk;
> +	int ret;
[ ... ]
> +	ret = cpuhp_state_add_instance(twd_wdt_hp_state, &wdt->node);
> +	if (ret)
> +		return ret;

[Severity: High]
Is a call to cpu_pm_register_notifier() missing here to ensure the watchdog
context is saved and restored across deep CPU idle states?

Without this, the watchdog could remain disabled if TWD hardware loses its
state during deep sleep.

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