Skip to contentSkip to conversation

watchdog: msc313e: Fix premature reset during timeout update

PATCH Applied Tzung-Bi Shih 3 participants 3 messages Sep 13 – Sep 17

Subsystems (2)
Watchdog device drivers Arm/MStar/Sigmastar Armv7 SoC support
lore
Share & tools
lore

Tzung-Bi Shih

author
#0 lore raw

PATCH 1/1 watchdog: msc313e: Fix premature reset during timeout update

Updating the 32-bit hardware timeout requires writing to two 16-bit registers sequentially. If the watchdog is actively running, this non-atomic update might trigger a premature system reset.

Clear the watchdog counter before updating the registers to prevent the timer from timing out prematurely against an intermediate threshold.

Fixes e9800b799464 ("watchdog: Add Mstar MSC313e WDT driver")
Signed-off-by Tzung-Bi Shih <[email protected]>

---
This fixes a Sashiko report [1].

[1] https://lore.kernel.org/all/[email protected]

---
 drivers/watchdog/msc313e_wdt.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/watchdog/msc313e_wdt.c +6 -0
index dc7bdd5660e6..0a68be7a217b 100644
@@ -46,6 +46,9 @@ static void msc313e_wdt_set_hw_timeout(struct msc313e_wdt_priv *priv,
 {
 	u32 t = timeout * clk_get_rate(priv->clk);
 
+	/* Clear before to prevent premature reset during non-atomic updates. */
+	writew(1, priv->base + REG_WDT_CLR);
+
 	writew(t & 0xffff, priv->base + REG_WDT_MAX_PRD_L);
 	writew((t >> 16) & 0xffff, priv->base + REG_WDT_MAX_PRD_H);
 	writew(1, priv->base + REG_WDT_CLR);

@@ -76,6 +79,9 @@ static int msc313e_wdt_stop(struct watchdog_device *wdev)
 {
 	struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
 
+	/* Clear before to prevent premature reset during non-atomic updates. */
+	writew(1, priv->base + REG_WDT_CLR);
+
 	writew(0, priv->base + REG_WDT_MAX_PRD_L);
 	writew(0, priv->base + REG_WDT_MAX_PRD_H);
 	writew(0, priv->base + REG_WDT_CLR);
Signature · 1 line
2.53.0
AI review
#1 lore raw
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete race condition fix leading to premature system reset during watchdog timeout update. --

commit 7f35d2e3193543673ce5c7858773cd48582d5c52
Author: Tzung-Bi Shih <[email protected]>

watchdog: msc313e: Fix premature reset during timeout update

Updating the 32-bit hardware timeout requires writing to two 16-bit registers sequentially. Clear the watchdog counter before updating the registers to prevent the timer from timing out prematurely.

drivers/watchdog/msc313e_wdt.c static void msc313e_wdt_set_hw_timeout(struct msc313e_wdt_priv *priv, · line 53

Quoted 13 lines
drivers/watchdog/msc313e_wdt.c +3 -0
index 4a5cce2a16b18..a743087e922d5 100644
@@ -46,6 +46,9 @@ static void msc313e_wdt_set_hw_timeout(struct msc313e_wdt_priv *priv,
 {
 	u32 t = timeout * clk_get_rate(priv->clk);
 
+	/* Clear before to prevent premature reset during non-atomic updates. */
+	writew(1, priv->base + REG_WDT_CLR);
+
 	writew(t & 0xffff, priv->base + REG_WDT_MAX_PRD_L);
 	writew((t >> 16) & 0xffff, priv->base + REG_WDT_MAX_PRD_H);

[Severity: High]
This is a pre-existing issue, but does this non-atomic update still leave a race condition window?

If a preemption or interrupt occurs between these two writes in msc313e_wdt_set_hw_timeout(), the active threshold becomes a hybrid of the old high bits and the new low bits.

Since the counter was just cleared to 0 immediately prior to this, if the old high bits are 0 and the new low bits are small, the intermediate threshold will be very low.

Could the counter reach this small intermediate threshold during the preemption window and still trigger a premature hardware reset?

Quoted 2 lines
drivers/watchdog/msc313e_wdt.c +0 -0
 	writew(1, priv->base + REG_WDT_CLR);

}

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
maintainer Applied
#2 lore raw
In reply to Tzung-Bi Shih
Quoted 9 lines · Tzung-Bi Shih

Updating the 32-bit hardware timeout requires writing to two 16-bit registers sequentially. If the watchdog is actively running, this non-atomic update might trigger a premature system reset.

Clear the watchdog counter before updating the registers to prevent the timer from timing out prematurely against an intermediate threshold.

Fixes e9800b799464 ("watchdog: Add Mstar MSC313e WDT driver")
Signed-off-by Tzung-Bi Shih <[email protected]>

Applied.

Thanks,
Guenter