diff mbox series

clocksource/drivers/sun4i: Wait for pending reload before CTRL write

Message ID 20260725113341.2250687-1-megi@xff.cz (mailing list archive)
State New
Headers
Series clocksource/drivers/sun4i: Wait for pending reload before CTRL write |

Commit Message

Ondřej Jirman July 25, 2026, 11:33 a.m. UTC 
From: Ondrej Jirman <megi@xff.cz>

The A13 manual says of TMR0_RELOAD: "After the bit is set, it can not be
written again before it's cleared automatically."  Both time_stop() and
time_start() read-modify-write the control register, so a reload still in
flight gets rewritten and the write can be dropped, leaving the timer
unarmed.  In oneshot mode this happens on every tick, and the tick
eventually stops for good.

Wait for the reload to clear first.  All control register updates go
through sun4i_clkevt_time_stop().

Without this my Allwinner A13 based Pocketbook Touch Lux 3 stops
scheduling processes during boot or within a few seconds of executing
userspace. Only sysrq+t over serial port works at this stage, which is
how I discovered the root cause.

Fixes: 7e14183469d8 ("clocksource: sun4i: Fix bug when switching from periodic to oneshot modes")
Signed-off-by: Ondrej Jirman <megi@xff.cz>
---
BTW, similar issue will likely be also in timer-sun5i.c but I don't have any
device that would exercise it. And it's possible that this triggers more easily
with CONFIG_HZ=1000 which I use.

 drivers/clocksource/timer-sun4i.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

Comments

sashiko-bot@kernel.org July 25, 2026, 11:43 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the same hardware limitation where writing to the control register while the RELOAD bit is set causes the write to be dropped is unaddressed in `timer-sun5i.c`.
--

--- Patch [1]: [PATCH] clocksource/drivers/sun4i: Wait for pending reload before CTRL write ---
commit 936f66d7fd8614d6358ab1a1c7241c4675c9fff4
Author: Ondrej Jirman <megi@xff.cz>

clocksource/drivers/sun4i: Wait for pending reload before CTRL write

This commit fixes a bug in the sun4i timer driver where writes to the
control register are silently dropped if the RELOAD bit is set. It introduces
a polling mechanism to wait for the reload bit to clear before modifying
the register.

> BTW, similar issue will likely be also in timer-sun5i.c but I don't have any
> device that would exercise it. And it's possible that this triggers more easily
> with CONFIG_HZ=1000 which I use.

[Severity: High]
This isn't a bug introduced by this patch, but as you suspected, the exact
same hardware limitation is unaddressed in drivers/clocksource/timer-sun5i.c.

Looking at sun5i_clkevt_time_stop(), the unprotected read-modify-write of
the control register occurs here:

drivers/clocksource/timer-sun5i.c:sun5i_clkevt_time_stop() {
    ...
    writel(val & ~TIMER_CTL_ENABLE,
           ce->base + TIMER_CTL_REG(timer, offset));
    ...
}

If rapid back-to-back scheduling of high-resolution timers causes this to
be called while the RELOAD bit from a prior time_start() is still set,
wouldn't the write be dropped, failing to clear the ENABLE bit? 

Since this can permanently stop clock events and cause the system to freeze
by halting process scheduling, should a similar wait mechanism be applied
to the sun5i timer to protect devices that use it as a clockevent device?
Ondřej Jirman July 25, 2026, 12:40 p.m. UTC | #2 
Well, that's what I stated in the original patch comment. I don't have the HW
to test. I can send a blind fix, or whoever cares and has SoC that uses the
other timer driver that may have the same issue, can do it.

I'll do whatever the real maintainers suggest. ;)

	.o

On Sat, Jul 25, 2026 at 11:43:38AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the same hardware limitation where writing to the control register while the RELOAD bit is set causes the write to be dropped is unaddressed in `timer-sun5i.c`.
> --
> 
> --- Patch [1]: [PATCH] clocksource/drivers/sun4i: Wait for pending reload before CTRL write ---
> commit 936f66d7fd8614d6358ab1a1c7241c4675c9fff4
> Author: Ondrej Jirman <megi@xff.cz>
> 
> clocksource/drivers/sun4i: Wait for pending reload before CTRL write
> 
> This commit fixes a bug in the sun4i timer driver where writes to the
> control register are silently dropped if the RELOAD bit is set. It introduces
> a polling mechanism to wait for the reload bit to clear before modifying
> the register.
> 
> > BTW, similar issue will likely be also in timer-sun5i.c but I don't have any
> > device that would exercise it. And it's possible that this triggers more easily
> > with CONFIG_HZ=1000 which I use.
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but as you suspected, the exact
> same hardware limitation is unaddressed in drivers/clocksource/timer-sun5i.c.
> 
> Looking at sun5i_clkevt_time_stop(), the unprotected read-modify-write of
> the control register occurs here:
> 
> drivers/clocksource/timer-sun5i.c:sun5i_clkevt_time_stop() {
>     ...
>     writel(val & ~TIMER_CTL_ENABLE,
>            ce->base + TIMER_CTL_REG(timer, offset));
>     ...
> }
> 
> If rapid back-to-back scheduling of high-resolution timers causes this to
> be called while the RELOAD bit from a prior time_start() is still set,
> wouldn't the write be dropped, failing to clear the ENABLE bit? 
> 
> Since this can permanently stop clock events and cause the system to freeze
> by halting process scheduling, should a similar wait mechanism be applied
> to the sun5i timer to protect devices that use it as a clockevent device?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260725113341.2250687-1-megi@xff.cz?part=1
diff mbox series

Patch

diff --git a/drivers/clocksource/timer-sun4i.c b/drivers/clocksource/timer-sun4i.c
index 7bdcc60ad43c..2e7457f671c4 100644
--- a/drivers/clocksource/timer-sun4i.c
+++ b/drivers/clocksource/timer-sun4i.c
@@ -39,6 +39,9 @@ 
 
 #define TIMER_SYNC_TICKS	3
 
+/* The reload bit clears a couple of source clock cycles after it is set. */
+#define TIMER_RELOAD_MAX_POLL	100
+
 /*
  * When we disable a timer, we need to wait at least for 2 cycles of
  * the timer source clock. We will use for that the clocksource timer
@@ -53,9 +56,30 @@  static void sun4i_clkevt_sync(void __iomem *base)
 		cpu_relax();
 }
 
+/*
+ * The control register must not be written while a reload is still in
+ * flight, or the write can be dropped.
+ */
+static int sun4i_clkevt_wait_reload(void __iomem *base, u8 timer)
+{
+	int i;
+
+	for (i = 0; i < TIMER_RELOAD_MAX_POLL; i++) {
+		if (!(readl(base + TIMER_CTL_REG(timer)) & TIMER_CTL_RELOAD))
+			return 0;
+		cpu_relax();
+	}
+
+	return -ETIME;
+}
+
 static void sun4i_clkevt_time_stop(void __iomem *base, u8 timer)
 {
-	u32 val = readl(base + TIMER_CTL_REG(timer));
+	u32 val;
+
+	sun4i_clkevt_wait_reload(base, timer);
+
+	val = readl(base + TIMER_CTL_REG(timer));
 	writel(val & ~TIMER_CTL_ENABLE, base + TIMER_CTL_REG(timer));
 	sun4i_clkevt_sync(base);
 }