From: Markus Stockhausen markus.stockhausen@gmx.de
[ Upstream commit c445bffbf28f721e05d0ce06895045fc62aaff7c ]
During normal operation the timers are reprogrammed including an interrupt acknowledgement. This has no effect as the whole timer is setup from scratch afterwards. Especially in an interrupt this has already been done by rttm_timer_interrupt().
Change the behaviour as follows:
- Use rttm_disable_timer() during reprogramming - Keep rttm_stop_timer() for all other use cases.
Downstream has already tested and confirmed a patch. See https://github.com/openwrt/openwrt/pull/19468 https://forum.openwrt.org/t/support-for-rtl838x-based-managed-switches/57875...
Signed-off-by: Markus Stockhausen markus.stockhausen@gmx.de Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Tested-by: Stephen Howell howels@allthatwemight.be Tested-by: Bjørn Mork bjorn@mork.no Link: https://lore.kernel.org/r/20250804080328.2609287-4-markus.stockhausen@gmx.de Signed-off-by: Sasha Levin sashal@kernel.org ---
LLM Generated explanations, may be completely bogus:
YES
- What it fixes - The driver was acknowledging the interrupt (“W1C” PENDING bit) as part of routine timer reprogramming, not just in the interrupt handler. That read-modify-write ack can race with new pending interrupts and clear them, leading to occasional lost timer interrupts. The change confines IRQ acknowledgement to the interrupt handler and explicit stop/shutdown paths, preventing interference with in-flight or newly arriving interrupts.
- Exact code changes - In the reprogramming paths, `rttm_stop_timer()` (which disables the timer and acks the IRQ) is replaced with `rttm_disable_timer()` (disable only), so the PENDING bit is no longer touched during normal reprogramming: - `drivers/clocksource/timer-rtl-otto.c:141-146` changes reprogramming for oneshot next-event (now disable → set period → start, without ack). - `drivers/clocksource/timer-rtl-otto.c:153-159` changes `rttm_state_oneshot()` similarly. - `drivers/clocksource/timer-rtl-otto.c:166-172` changes `rttm_state_periodic()` similarly. - IRQ acknowledgement remains where it belongs: - Interrupt handler acks before invoking the event handler: `drivers/clocksource/timer-rtl-otto.c:97-106` and specifically the ack helper at `drivers/clocksource/timer-rtl-otto.c:77-80`. - Stop/shutdown/init paths still ack via `rttm_stop_timer()`: - Shutdown: `drivers/clocksource/timer-rtl-otto.c:175-182` - Setup: `drivers/clocksource/timer-rtl-otto.c:185-190` - `rttm_stop_timer()` itself still does disable + ack: `drivers/clocksource/timer-rtl-otto.c:125-129`.
- Why the original behavior is problematic - The ack function is implemented as a read-modify-write to a W1C bit: `ioread32(base + RTTM_INT) | RTTM_INT_PENDING` followed by a write (`drivers/clocksource/timer-rtl-otto.c:77-80`). If a new interrupt becomes pending between the read and the write, the write will still set the PENDING bit in the value and clear it on write, effectively dropping that freshly latched interrupt. Calling this sequence outside the ISR (e.g., during reprogramming) can therefore interfere with normal interrupt delivery.
- Why this change is safe - In-ISR reprogramming: The handler already acknowledges the interrupt at entry (`drivers/clocksource/timer-rtl-otto.c:102`). Removing a second ack during reprogramming eliminates a window where a new pending interrupt could be inadvertently cleared. - Non-ISR reprogramming: If a pending bit exists, not acking ensures it will be properly handled by the ISR when it fires, rather than being silently cleared by a stray reprogramming ack. - Ack is still performed at shutdown/setup where it is appropriate to clear stale state (`drivers/clocksource/timer-rtl-otto.c:175-190`), so there is no accumulation of stale flags.
- Context and related fixes - This change is part of a small, focused series addressing timer reliability on Realtek Otto platforms: - “Work around dying timers” added `rttm_bounce_timer()` to avoid reprogramming in a critical ~5us window before expiry (hardware peculiarity) and is used directly before reprogramming in all the altered paths (`drivers/clocksource/timer-rtl-otto.c:109-123` and calls at 141, 154, 167). - “Drop set_counter” cleaned up a no-op write to the current counter. - The series was tested downstream (OpenWrt) and carries multiple Tested-by tags; the commit under review also notes downstream confirmation.
- Backport considerations - Scope: Single driver file; changes are three substitutions of `rttm_stop_timer()` with `rttm_disable_timer()` in reprogramming paths. No functional/ABI changes outside this driver. - Dependencies: None strict. If a stable branch does not yet have `rttm_bounce_timer()`, the underlying correctness argument for using `rttm_disable_timer()` instead of `rttm_stop_timer()` during reprogramming still holds. For branches already including the bounce patch (as in newer stables), this applies cleanly. - Risk: Low. Potential for an extra immediate interrupt if a PENDING bit remained set is mitigated because the ISR acks and the clockevents layer tolerates such re-entries; conversely, the change removes a race that could drop interrupts, which is more severe.
- Stable policy fit - Fixes a real bug affecting users (lost or interfered interrupts on rtl-otto platforms). - Small, contained, and without architectural changes. - Confined to `drivers/clocksource/timer-rtl-otto.c`. - Already tested downstream and reviewed/merged upstream (commit c445bffbf28f7). - While there is no explicit “Cc: stable” in the commit message, the change meets stable backport criteria and aligns with the prior reliability fix series for this driver.
Conclusion: Backporting this patch reduces the risk of lost timer interrupts by avoiding unnecessary and racy IRQ acknowledgements during reprogramming, with minimal regression risk and limited scope.
drivers/clocksource/timer-rtl-otto.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/clocksource/timer-rtl-otto.c b/drivers/clocksource/timer-rtl-otto.c index 8be45a11fb8b6..24c4aa6a30131 100644 --- a/drivers/clocksource/timer-rtl-otto.c +++ b/drivers/clocksource/timer-rtl-otto.c @@ -147,7 +147,7 @@ static int rttm_next_event(unsigned long delta, struct clock_event_device *clkev
RTTM_DEBUG(to->of_base.base); rttm_bounce_timer(to->of_base.base, RTTM_CTRL_COUNTER); - rttm_stop_timer(to->of_base.base); + rttm_disable_timer(to->of_base.base); rttm_set_period(to->of_base.base, delta); rttm_start_timer(to, RTTM_CTRL_COUNTER);
@@ -160,7 +160,7 @@ static int rttm_state_oneshot(struct clock_event_device *clkevt)
RTTM_DEBUG(to->of_base.base); rttm_bounce_timer(to->of_base.base, RTTM_CTRL_COUNTER); - rttm_stop_timer(to->of_base.base); + rttm_disable_timer(to->of_base.base); rttm_set_period(to->of_base.base, RTTM_TICKS_PER_SEC / HZ); rttm_start_timer(to, RTTM_CTRL_COUNTER);
@@ -173,7 +173,7 @@ static int rttm_state_periodic(struct clock_event_device *clkevt)
RTTM_DEBUG(to->of_base.base); rttm_bounce_timer(to->of_base.base, RTTM_CTRL_TIMER); - rttm_stop_timer(to->of_base.base); + rttm_disable_timer(to->of_base.base); rttm_set_period(to->of_base.base, RTTM_TICKS_PER_SEC / HZ); rttm_start_timer(to, RTTM_CTRL_TIMER);