* [PATCH 0/4] mmc: dw_mmc: replace three fallback timers with a single watchdog
@ 2026-08-27 7:58 Shawn Lin
2026-08-27 7:58 ` [PATCH 1/4] mmc: dw_mmc: add central watchdog and convert CTO onto it Shawn Lin
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Shawn Lin @ 2026-08-27 7:58 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jaehoon Chung, Marek Szyprowski, linux-mmc, linux-kernel,
Shawn Lin
The driver carries three fallback timers (cmd11_timer, cto_timer,
dto_timer) added piecemeal after the hardware failed in the field.
dto_timer exists because Synopsys confirmed that the DTO interrupt can
be lost entirely, leaving no hardware fallback and blocking the request
forever without software help (57e104864bc4); cto_timer came later,
when Rockchip's reworked sample circuit was root-caused to swallow both
CMD_DONE and response-timeout interrupts across their whole dwmmc
family (03de19212ea3); cmd11_timer papered over voltage-switch hangs
that reproduced reliably when ejecting/inserting UHS cards on rk3288
(5c935165da79).
Because each callback races against the very interrupt it supplements,
all three grew the same copy-pasted defenses: re-read MINTSTS in case
the interrupt is merely late, check whether pending_events has been set
meanwhile, validate host->state against the leg being guarded, and only
then synthesize the missed event -- with timer_delete_sync() calls from
softirq context eventually needed just to contain them. Meanwhile the
special cases keep piling up: the EXTENDED_TMOUT quirk makes the DTO
story differ per platform, fault injection can post DATA_ERROR ahead of
any real completion, and every future change has to reason about up to
three timers at once.
The observation enabling the cleanup is that command, data and
voltage-switch legs run strictly serially within a request, so a single
hrtimer suffices. Under irq_lock it records which pending_events bits
are awaited along with a snapshot of host->state (dw_mci_wd_arm()),
and producers clear that awaited mask instead of deleting any timer
(dw_mci_wd_deliver()). On expiry the one callback classifies what
expired by comparing the awaited set against the named
DW_MCI_WD_{CMD,DATA}_EVENTS masks, keeps the old MINTSTS latency check
but re-arms instead of going passive so an interrupt lost for good
can no longer wedge the request forever, and only then synthesizes
exactly what its predecessor would have (RTO + command complete, or
DRTO + data error/complete). The per-leg deadlines -- CTO formula,
DRTO formula, 500ms CMD11 budget -- are carried over unchanged.
Apart from three behavioral deltas called out in the individual commit
messages as well -- a single surviving watch during voltage switch
instead of two timers racing, a stuck CMD11 aborting after exactly
500ms instead of racing min(cto_ms, 500ms) with -ETIMEDOUT unchanged,
and bounded recovery from interrupt-latency peaks instead of an
unbounded hang -- no functional change is intended.
Tested on Rockchip platforms with SD card and eMMC (rv1126/rk3568/
rk3576 boards): normal IO, suspend/resume and card removal during
transfer. Compile tested on every variant consuming dw_mmc.h.
Next steps: first split dw_mci_work_func() into per-state handlers to
make the transitions explicit, then go further in the sdhci direction:
shrink the eight-state machine, drop the redundant completed_events
bookkeeping, and end up where other host drivers already are: one state
machine, one timer, no separate event flags.
Shawn Lin (4):
mmc: dw_mmc: add central watchdog and convert CTO onto it
mmc: dw_mmc: convert DTO onto the central watchdog
mmc: dw_mmc: absorb CMD11 timeout into the central watchdog
mmc: dw_mmc: expose the watchdog state in debugfs
drivers/mmc/host/dw_mmc.c | 316 +++++++++++++++++++++++-----------------------
drivers/mmc/host/dw_mmc.h | 15 ++-
2 files changed, 165 insertions(+), 166 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/4] mmc: dw_mmc: add central watchdog and convert CTO onto it
2026-08-27 7:58 [PATCH 0/4] mmc: dw_mmc: replace three fallback timers with a single watchdog Shawn Lin
@ 2026-08-27 7:58 ` Shawn Lin
2026-08-27 7:58 ` [PATCH 2/4] mmc: dw_mmc: convert DTO onto the central watchdog Shawn Lin
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Shawn Lin @ 2026-08-27 7:58 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jaehoon Chung, Marek Szyprowski, linux-mmc, linux-kernel,
Shawn Lin
The driver keeps three independent fallback timers (cmd11, cto, dto)
whose callbacks all re-implement the same race handling: peek MINTSTS
in case the interrupt is in flight, check whether the event was
delivered meanwhile, verify host->state matches the leg being guarded,
and finally synthesize the missed event. This series replaces them
with a single hrtimer watchdog.
This first step introduces the watchdog and moves the command timeout
onto it; cto_timer is deleted. Later steps convert the data timeout
and the voltage-switch timer onto the same watch.
The new protocol relies on two simple facts which hold for all current
event producers:
* every site posting EVENT_CMD_COMPLETE (dw_mci_cmd_interrupt() and
the command-error branch of dw_mci_interrupt()) runs under
irq_lock,
* every site arming a watch does so under irq_lock as well.
Consequently a watchdog callback holding irq_lock can neither miss nor
race an already-delivered event: the bookkeeping part of the former
're-read MINTSTS' paranoia is subsumed by checking the awaited mask
against pending_events under the same lock the producers use. The
hardware-latency part of that paranoia is kept verbatim, see below.
A callback that raced past every check nonetheless degrades to at most
one idempotent extra state machine run instead of completing a foreign
leg.
The callback classifies what expired by comparing the awaited set
against the named DW_MCI_WD_{CMD,DATA}_EVENTS masks so that subsequent
conversions only add call sites. dw_mci_wd_arm() takes a separate
'already delivered' precheck mask because guarding the data legs must
tolerate a DATA_ERROR that arrived while still waiting for the paired
completion -- exactly like mod_timer() paths did before.
Behavioral notes for review:
* dw_mci_wd_arm() replaces any previously armed watch. During a
voltage switch (CMD11) both cto_timer and cmd11_timer were armed
concurrently before, racing each other with duplicated warnings;
now only the last arm on that path survives.
* The stale-timer defensiveness of dw_mci_clear_pending_cmd_complete()
(WARN_ON + timer_delete_sync) is dropped because the callback is now
idempotent by construction; timer_delete_sync from the BH would also
be wrong-context sleeping on hrtimers.
* Before declaring a timeout the callback re-reads MINTSTS: when the
completion interrupt is already latched in hardware and only its
handler has not been scheduled yet, the firing grants further
DW_MCI_WD_INFLIGHT_GRACE_MS rounds instead of failing an about-to-
complete transfer. This replicates the interrupt-latency paranoia
of the retired cto_timer()/dto_timer() callbacks; unlike them it
keeps re-watching rather than going passive, so if that latched
interrupt is ultimately lost the request still unwedges with a
timeout error instead of hanging forever.
No functional change intended beyond the deduplication described above.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/host/dw_mmc.c | 216 ++++++++++++++++++++++++++++++----------------
drivers/mmc/host/dw_mmc.h | 11 ++-
2 files changed, 150 insertions(+), 77 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index cf5cecc..cefc873 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -44,6 +44,135 @@
#define DW_MCI_FREQ_MAX 200000000 /* unit: HZ */
#define DW_MCI_FREQ_MIN 100000 /* unit: HZ */
+/* Event sets guarded by the central watchdog, see below. */
+#define DW_MCI_WD_CMD_EVENTS BIT(EVENT_CMD_COMPLETE)
+#define DW_MCI_WD_DATA_EVENTS (BIT(EVENT_DATA_ERROR) | \
+ BIT(EVENT_DATA_COMPLETE))
+/* Extra watch rounds granted while the completion IRQ is merely late. */
+#define DW_MCI_WD_INFLIGHT_GRACE_MS 10
+
+/*
+ * Central watchdog
+ * ================
+ *
+ * A single hrtimer guards the legs of a request (command, data, voltage
+ * switch) against the hardware going silent: whenever a completion event
+ * that was expected within a bounded time does not arrive in time, the
+ * callback synthesizes it so that the request state machine can error out.
+ *
+ * @wd_events holds the pending_events bits still being waited for and
+ * @wd_states the host->state values for which the watch is valid; both
+ * are protected by irq_lock. Every producer of the guarded events posts
+ * them under irq_lock as well, so a callback holding irq_lock can never
+ * miss an already-delivered event. A stale callback which raced past
+ * all checks therefore degrades to at most one redundant state-machine
+ * run instead of completing a foreign leg. Arming replaces whatever
+ * watch was outstanding before: command and data legs are strictly
+ * serial, so consecutive arms on the same path collapse onto the latest
+ * deadline.
+ *
+ * On expiry, before declaring a timeout, the callback double-checks that
+ * the completion interrupt is not already latched in hardware; if it is,
+ * only the interrupt latency is to blame and the watch is granted extra
+ * rounds instead of failing a transfer that is about to complete.
+ */
+static void dw_mci_wd_deliver(struct dw_mci *host, unsigned long events)
+{
+ host->wd_events &= ~events;
+ if (!host->wd_events)
+ hrtimer_try_to_cancel(&host->wd_timer);
+}
+
+/*
+ * Arm a new watch for @events valid while host->state is one of @states,
+ * expiring after @ms milliseconds. @already lists bits whose delivery
+ * makes the watch pointless before it is even armed: arming is skipped
+ * when any of them has been posted concurrently. Must be called with
+ * irq_lock held.
+ */
+static void dw_mci_wd_arm(struct dw_mci *host, unsigned long ms,
+ unsigned long already, unsigned long events,
+ unsigned long states)
+{
+ lockdep_assert_held(&host->irq_lock);
+
+ if (host->pending_events & already) {
+ /*
+ * The event was delivered concurrently; no need for a
+ * watch anymore.
+ */
+ host->wd_events = 0;
+ hrtimer_try_to_cancel(&host->wd_timer);
+ return;
+ }
+
+ host->wd_events = events;
+ host->wd_states = states;
+ hrtimer_start(&host->wd_timer, ms_to_ktime(ms), HRTIMER_MODE_REL);
+}
+
+/* Called from hardirq context on watchdog expiry. */
+static enum hrtimer_restart dw_mci_watchdog_fn(struct hrtimer *t)
+{
+ struct dw_mci *host = container_of(t, struct dw_mci, wd_timer);
+ unsigned long irqflags;
+ u32 pending;
+
+ spin_lock_irqsave(&host->irq_lock, irqflags);
+
+ /*
+ * Last line of defense against a false timeout: when the completion
+ * interrupt is already latched in MINTSTS but its handler has not
+ * been scheduled yet, the leg is about to finish normally. Warn
+ * about the latency and keep watching instead of failing the
+ * transfer; delivery by the real interrupt will stop the watch.
+ * The retired per-timer callbacks went passive here and relied on
+ * the latched interrupt arriving; rearming bounds the damage if
+ * that assumption is ever broken.
+ */
+ if (host->wd_events) {
+ pending = mci_readl(host, MINTSTS); /* read-only mask reg */
+ if ((host->wd_events == DW_MCI_WD_CMD_EVENTS &&
+ (pending & (DW_MCI_CMD_ERROR_FLAGS |
+ SDMMC_INT_CMD_DONE))) ||
+ (host->wd_events == DW_MCI_WD_DATA_EVENTS &&
+ (pending & (SDMMC_INT_DATA_OVER |
+ DW_MCI_DATA_ERROR_FLAGS)))) {
+ dev_warn(host->dev,
+ "Interrupt latency, state %d (watched %#lx)\n",
+ host->state, host->wd_events);
+ hrtimer_forward_now(&host->wd_timer,
+ ms_to_ktime(DW_MCI_WD_INFLIGHT_GRACE_MS));
+ spin_unlock_irqrestore(&host->irq_lock, irqflags);
+ return HRTIMER_RESTART;
+ }
+ }
+
+ if (host->wd_events == DW_MCI_WD_DATA_EVENTS) {
+ dev_warn(host->dev,
+ "Data timeout, state %d\n", host->state);
+ host->data_status = SDMMC_INT_DRTO;
+ set_bit(EVENT_DATA_ERROR, &host->pending_events);
+ set_bit(EVENT_DATA_COMPLETE, &host->pending_events);
+ } else if (host->wd_events) {
+ dev_warn(host->dev,
+ "Command timeout, state %d (watched %#lx)\n",
+ host->state, host->wd_events);
+ host->cmd_status = SDMMC_INT_RTO;
+ set_bit(EVENT_CMD_COMPLETE, &host->pending_events);
+ }
+
+ if (host->wd_events)
+ queue_work(system_bh_wq, &host->bh_work);
+
+ host->wd_events = 0;
+ host->wd_states = 0;
+
+ spin_unlock_irqrestore(&host->irq_lock, irqflags);
+
+ return HRTIMER_NORESTART;
+}
+
#define IDMAC_INT_CLR (SDMMC_IDMAC_INT_AI | SDMMC_IDMAC_INT_NI | \
SDMMC_IDMAC_INT_CES | SDMMC_IDMAC_INT_DU | \
SDMMC_IDMAC_INT_FBE | SDMMC_IDMAC_INT_RI | \
@@ -366,18 +495,17 @@ static inline void dw_mci_set_cto(struct dw_mci *host)
* extra careful about synchronization here. Specifically in hardware a
* command timeout is _at most_ 5.1 ms, so that means we expect an
* interrupt (either command done or timeout) to come rather quickly
- * after the mci_writel. ...but just in case we have a long interrupt
- * latency let's add a bit of paranoia.
+ * after the mci_writel.
*
- * In general we'll assume that at least an interrupt will be asserted
- * in hardware by the time the cto_timer runs. ...and if it hasn't
- * been asserted in hardware by that time then we'll assume it'll never
- * come.
+ * dw_mci_wd_arm() skips arming when the event was already delivered,
+ * and the watchdog callback rechecks under the same lock before
+ * declaring a timeout, so both orderings of the race are covered.
*/
spin_lock_irqsave(&host->irq_lock, irqflags);
- if (!test_bit(EVENT_CMD_COMPLETE, &host->pending_events))
- mod_timer(&host->cto_timer,
- jiffies + msecs_to_jiffies(cto_ms) + 1);
+ dw_mci_wd_arm(host, cto_ms, DW_MCI_WD_CMD_EVENTS,
+ DW_MCI_WD_CMD_EVENTS,
+ BIT(STATE_SENDING_CMD) | BIT(STATE_SENDING_STOP) |
+ BIT(STATE_SENDING_CMD11));
spin_unlock_irqrestore(&host->irq_lock, irqflags);
}
@@ -1881,14 +2009,6 @@ static bool dw_mci_clear_pending_cmd_complete(struct dw_mci *host)
if (!test_bit(EVENT_CMD_COMPLETE, &host->pending_events))
return false;
- /*
- * Really be certain that the timer has stopped. This is a bit of
- * paranoia and could only really happen if we had really bad
- * interrupt latency and the interrupt routine and timeout were
- * running concurrently so that the timer_delete() in the interrupt
- * handler couldn't run.
- */
- WARN_ON(timer_delete_sync(&host->cto_timer));
clear_bit(EVENT_CMD_COMPLETE, &host->pending_events);
return true;
@@ -2633,7 +2753,7 @@ static void dw_mci_write_data_pio(struct dw_mci *host)
static void dw_mci_cmd_interrupt(struct dw_mci *host, u32 status)
{
- timer_delete(&host->cto_timer);
+ dw_mci_wd_deliver(host, BIT(EVENT_CMD_COMPLETE));
if (!host->cmd_status)
host->cmd_status = status;
@@ -2680,7 +2800,7 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
if (pending & DW_MCI_CMD_ERROR_FLAGS) {
spin_lock(&host->irq_lock);
- timer_delete(&host->cto_timer);
+ dw_mci_wd_deliver(host, BIT(EVENT_CMD_COMPLETE));
mci_writel(host, RINTSTS, DW_MCI_CMD_ERROR_FLAGS);
host->cmd_status = pending;
smp_wmb(); /* drain writebuffer */
@@ -3008,61 +3128,6 @@ static void dw_mci_cmd11_timer(struct timer_list *t)
queue_work(system_bh_wq, &host->bh_work);
}
-static void dw_mci_cto_timer(struct timer_list *t)
-{
- struct dw_mci *host = timer_container_of(host, t, cto_timer);
- unsigned long irqflags;
- u32 pending;
-
- spin_lock_irqsave(&host->irq_lock, irqflags);
-
- /*
- * If somehow we have very bad interrupt latency it's remotely possible
- * that the timer could fire while the interrupt is still pending or
- * while the interrupt is midway through running. Let's be paranoid
- * and detect those two cases. Note that this is paranoia is somewhat
- * justified because in this function we don't actually cancel the
- * pending command in the controller--we just assume it will never come.
- */
- pending = mci_readl(host, MINTSTS); /* read-only mask reg */
- if (pending & (DW_MCI_CMD_ERROR_FLAGS | SDMMC_INT_CMD_DONE)) {
- /* The interrupt should fire; no need to act but we can warn */
- dev_warn(host->dev, "Unexpected interrupt latency\n");
- goto exit;
- }
- if (test_bit(EVENT_CMD_COMPLETE, &host->pending_events)) {
- /* Presumably interrupt handler couldn't delete the timer */
- dev_warn(host->dev, "CTO timeout when already completed\n");
- goto exit;
- }
-
- /*
- * Continued paranoia to make sure we're in the state we expect.
- * This paranoia isn't really justified but it seems good to be safe.
- */
- switch (host->state) {
- case STATE_SENDING_CMD11:
- case STATE_SENDING_CMD:
- case STATE_SENDING_STOP:
- /*
- * If CMD_DONE interrupt does NOT come in sending command
- * state, we should notify the driver to terminate current
- * transfer and report a command timeout to the core.
- */
- host->cmd_status = SDMMC_INT_RTO;
- set_bit(EVENT_CMD_COMPLETE, &host->pending_events);
- queue_work(system_bh_wq, &host->bh_work);
- break;
- default:
- dev_warn(host->dev, "Unexpected command timeout, state %d\n",
- host->state);
- break;
- }
-
-exit:
- spin_unlock_irqrestore(&host->irq_lock, irqflags);
-}
-
static void dw_mci_dto_timer(struct timer_list *t)
{
struct dw_mci *host = timer_container_of(host, t, dto_timer);
@@ -3261,8 +3326,9 @@ int dw_mci_probe(struct dw_mci *host)
}
}
+ hrtimer_setup(&host->wd_timer, dw_mci_watchdog_fn, CLOCK_MONOTONIC,
+ HRTIMER_MODE_REL);
timer_setup(&host->cmd11_timer, dw_mci_cmd11_timer, 0);
- timer_setup(&host->cto_timer, dw_mci_cto_timer, 0);
timer_setup(&host->dto_timer, dw_mci_dto_timer, 0);
spin_lock_init(&host->lock);
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 38610c8..7a14f3f 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -122,8 +122,12 @@ struct dw_mci_dma_slave {
* @irq_flags: The flags to be passed to request_irq.
* @irq: The irq value to be passed to request_irq.
* @sdio_irq: SDIO interrupt bit in interrupt registers.
+ * @wd_timer: Central watchdog guarding request legs against hardware going
+ * silent; fires when an expected completion event fails to arrive in
+ * time so that the state machine can synthesize an error.
+ * @wd_events: pending_events bits still awaited by the armed watch.
+ * @wd_states: host->state values for which the armed watch is valid.
* @cmd11_timer: Timer for SD3.0 voltage switch over scheme.
- * @cto_timer: Timer for broken command transfer over scheme.
* @dto_timer: Timer for broken data transfer over scheme.
* @mmc: The mmc_host representing this dw_mci.
* @flags: Random state bits associated with the host.
@@ -235,8 +239,11 @@ struct dw_mci {
int sdio_irq;
+ struct hrtimer wd_timer;
+ unsigned long wd_events;
+ unsigned long wd_states;
+
struct timer_list cmd11_timer;
- struct timer_list cto_timer;
struct timer_list dto_timer;
#ifdef CONFIG_FAULT_INJECTION
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/4] mmc: dw_mmc: convert DTO onto the central watchdog
2026-08-27 7:58 [PATCH 0/4] mmc: dw_mmc: replace three fallback timers with a single watchdog Shawn Lin
2026-08-27 7:58 ` [PATCH 1/4] mmc: dw_mmc: add central watchdog and convert CTO onto it Shawn Lin
@ 2026-08-27 7:58 ` Shawn Lin
2026-08-27 7:58 ` [PATCH 3/4] mmc: dw_mmc: absorb CMD11 timeout into " Shawn Lin
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Shawn Lin @ 2026-08-27 7:58 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jaehoon Chung, Marek Szyprowski, linux-mmc, linux-kernel,
Shawn Lin
The data timeout joins the command timeout on the central watchdog;
dto_timer is deleted.
dw_mci_set_drto() arms DW_MCI_WD_DATA_EVENTS with EVENT_DATA_COMPLETE
as its precheck mask: a DATA_ERROR that arrived while still waiting
for the paired completion must not prevent the watch -- the legacy
mod_timer() guard tested exactly that one bit, and the fault-injection
machinery relies on this by injecting DATA_ERROR early.
The EXTENDED_TMOUT quirk semantics fall out naturally now:
* On quirk hosts the data-error branch delivers the whole watched
set, stopping the watch since no further data events will come --
this mirrors the former conditional timer_delete() plus the manual
EVENT_DATA_COMPLETE side-post.
* Without the quirk nothing is delivered there and the outstanding
watch keeps guarding until a genuine DATA_OVER arrives, exactly
like leaving dto_timer running did.
The DATA_OVER branch delivers unconditionally, superseding its
unconditional timer_delete(). The stale-timer WARN_ON +
timer_delete_sync() dance in dw_mci_clear_pending_data_complete() goes
away for the same reason as on the command leg: a callback racing past
its checks is idempotent under irq_lock.
No functional change intended.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/host/dw_mmc.c | 64 ++++-------------------------------------------
drivers/mmc/host/dw_mmc.h | 2 --
2 files changed, 5 insertions(+), 61 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index cefc873..31cf728 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1998,9 +1998,9 @@ static void dw_mci_set_drto(struct dw_mci *host)
drto_ms += 10;
spin_lock_irqsave(&host->irq_lock, irqflags);
- if (!test_bit(EVENT_DATA_COMPLETE, &host->pending_events))
- mod_timer(&host->dto_timer,
- jiffies + msecs_to_jiffies(drto_ms));
+ dw_mci_wd_arm(host, drto_ms, BIT(EVENT_DATA_COMPLETE),
+ DW_MCI_WD_DATA_EVENTS,
+ BIT(STATE_SENDING_DATA) | BIT(STATE_DATA_BUSY));
spin_unlock_irqrestore(&host->irq_lock, irqflags);
}
@@ -2019,8 +2019,6 @@ static bool dw_mci_clear_pending_data_complete(struct dw_mci *host)
if (!test_bit(EVENT_DATA_COMPLETE, &host->pending_events))
return false;
- /* Extra paranoia just like dw_mci_clear_pending_cmd_complete() */
- WARN_ON(timer_delete_sync(&host->dto_timer));
clear_bit(EVENT_DATA_COMPLETE, &host->pending_events);
return true;
@@ -2813,7 +2811,7 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
spin_lock(&host->irq_lock);
if (host->quirks & DW_MMC_QUIRK_EXTENDED_TMOUT)
- timer_delete(&host->dto_timer);
+ dw_mci_wd_deliver(host, DW_MCI_WD_DATA_EVENTS);
/* if there is an error report DATA_ERROR */
mci_writel(host, RINTSTS, DW_MCI_DATA_ERROR_FLAGS);
@@ -2834,7 +2832,7 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
if (pending & SDMMC_INT_DATA_OVER) {
spin_lock(&host->irq_lock);
- timer_delete(&host->dto_timer);
+ dw_mci_wd_deliver(host, DW_MCI_WD_DATA_EVENTS);
mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
if (!host->data_status)
@@ -3128,57 +3126,6 @@ static void dw_mci_cmd11_timer(struct timer_list *t)
queue_work(system_bh_wq, &host->bh_work);
}
-static void dw_mci_dto_timer(struct timer_list *t)
-{
- struct dw_mci *host = timer_container_of(host, t, dto_timer);
- unsigned long irqflags;
- u32 pending;
-
- spin_lock_irqsave(&host->irq_lock, irqflags);
-
- /*
- * The DTO timer is much longer than the CTO timer, so it's even less
- * likely that we'll these cases, but it pays to be paranoid.
- */
- pending = mci_readl(host, MINTSTS); /* read-only mask reg */
- if (pending & SDMMC_INT_DATA_OVER) {
- /* The interrupt should fire; no need to act but we can warn */
- dev_warn(host->dev, "Unexpected data interrupt latency\n");
- goto exit;
- }
- if (test_bit(EVENT_DATA_COMPLETE, &host->pending_events)) {
- /* Presumably interrupt handler couldn't delete the timer */
- dev_warn(host->dev, "DTO timeout when already completed\n");
- goto exit;
- }
-
- /*
- * Continued paranoia to make sure we're in the state we expect.
- * This paranoia isn't really justified but it seems good to be safe.
- */
- switch (host->state) {
- case STATE_SENDING_DATA:
- case STATE_DATA_BUSY:
- /*
- * If DTO interrupt does NOT come in sending data state,
- * we should notify the driver to terminate current transfer
- * and report a data timeout to the core.
- */
- host->data_status = SDMMC_INT_DRTO;
- set_bit(EVENT_DATA_ERROR, &host->pending_events);
- set_bit(EVENT_DATA_COMPLETE, &host->pending_events);
- queue_work(system_bh_wq, &host->bh_work);
- break;
- default:
- dev_warn(host->dev, "Unexpected data timeout, state %d\n",
- host->state);
- break;
- }
-
-exit:
- spin_unlock_irqrestore(&host->irq_lock, irqflags);
-}
-
static int dw_mci_parse_dt(struct dw_mci *host)
{
struct device *dev = host->dev;
@@ -3329,7 +3276,6 @@ int dw_mci_probe(struct dw_mci *host)
hrtimer_setup(&host->wd_timer, dw_mci_watchdog_fn, CLOCK_MONOTONIC,
HRTIMER_MODE_REL);
timer_setup(&host->cmd11_timer, dw_mci_cmd11_timer, 0);
- timer_setup(&host->dto_timer, dw_mci_dto_timer, 0);
spin_lock_init(&host->lock);
spin_lock_init(&host->irq_lock);
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 7a14f3f..7af2b45 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -128,7 +128,6 @@ struct dw_mci_dma_slave {
* @wd_events: pending_events bits still awaited by the armed watch.
* @wd_states: host->state values for which the armed watch is valid.
* @cmd11_timer: Timer for SD3.0 voltage switch over scheme.
- * @dto_timer: Timer for broken data transfer over scheme.
* @mmc: The mmc_host representing this dw_mci.
* @flags: Random state bits associated with the host.
* @ctype: Card type for this host.
@@ -244,7 +243,6 @@ struct dw_mci {
unsigned long wd_states;
struct timer_list cmd11_timer;
- struct timer_list dto_timer;
#ifdef CONFIG_FAULT_INJECTION
struct fault_attr fail_data_crc;
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/4] mmc: dw_mmc: absorb CMD11 timeout into the central watchdog
2026-08-27 7:58 [PATCH 0/4] mmc: dw_mmc: replace three fallback timers with a single watchdog Shawn Lin
2026-08-27 7:58 ` [PATCH 1/4] mmc: dw_mmc: add central watchdog and convert CTO onto it Shawn Lin
2026-08-27 7:58 ` [PATCH 2/4] mmc: dw_mmc: convert DTO onto the central watchdog Shawn Lin
@ 2026-08-27 7:58 ` Shawn Lin
2026-08-27 7:58 ` [PATCH 4/4] mmc: dw_mmc: expose the watchdog state in debugfs Shawn Lin
2026-09-10 16:05 ` [PATCH 0/4] mmc: dw_mmc: replace three fallback timers with a single watchdog Ulf Hansson
4 siblings, 0 replies; 6+ messages in thread
From: Shawn Lin @ 2026-08-27 7:58 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jaehoon Chung, Marek Szyprowski, linux-mmc, linux-kernel,
Shawn Lin
The voltage switch (CMD11) keeps its dedicated 500ms deadline, but it
is now just another arm of the central watchdog; cmd11_timer is
deleted. The synthesized payload is identical to what the command leg
watchdog produces (cmd_status = RTO plus EVENT_CMD_COMPLETE), so the
request state machine cannot tell the difference.
Behavior notes for review:
* The extra jiffy in the legacy '500ms + 1' arming was pure jiffies
rollover paranoia and disappears together with the jiffies math.
* Since patch 1 arms the regular command watch on every RESP_EXP
command -- including voltage switches -- the subsequent arm here
replaces it, as documented there. For a genuinely stuck CMD11 the
abort latency therefore becomes exactly 500ms instead of racing
min(cto_ms, 500ms) between two timers as before; the reported
error (-ETIMEDOUT either way) is unchanged.
* dw_mci_cmd_interrupt() already delivers the watched events under
irq_lock on any completion path, so the former out-of-lock
timer_delete() next to the VOLT_SWITCH branch simply goes away.
No functional change intended.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/host/dw_mmc.c | 30 ++++--------------------------
drivers/mmc/host/dw_mmc.h | 3 ---
2 files changed, 4 insertions(+), 29 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 31cf728..6852c05 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1393,17 +1393,16 @@ static void dw_mci_start_request(struct dw_mci *host, struct mmc_command *cmd)
/*
* Databook says to fail after 2ms w/ no response, but evidence
* shows that sometimes the cmd11 interrupt takes over 130ms.
- * We'll set to 500ms, plus an extra jiffy just in case jiffies
- * is just about to roll over.
+ * We'll set to 500ms.
*
* We do this whole thing under spinlock and only if the
* command hasn't already completed (indicating the irq
* already ran so we don't want the timeout).
*/
spin_lock_irqsave(&host->irq_lock, irqflags);
- if (!test_bit(EVENT_CMD_COMPLETE, &host->pending_events))
- mod_timer(&host->cmd11_timer,
- jiffies + msecs_to_jiffies(500) + 1);
+ dw_mci_wd_arm(host, 500, DW_MCI_WD_CMD_EVENTS,
+ DW_MCI_WD_CMD_EVENTS,
+ BIT(STATE_SENDING_CMD11));
spin_unlock_irqrestore(&host->irq_lock, irqflags);
}
@@ -2784,15 +2783,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
mci_writel(host, RINTSTS, SDMMC_INT_VOLT_SWITCH);
pending &= ~SDMMC_INT_VOLT_SWITCH;
- /*
- * Hold the lock; we know cmd11_timer can't be kicked
- * off after the lock is released, so safe to delete.
- */
spin_lock(&host->irq_lock);
dw_mci_cmd_interrupt(host, pending);
spin_unlock(&host->irq_lock);
-
- timer_delete(&host->cmd11_timer);
}
if (pending & DW_MCI_CMD_ERROR_FLAGS) {
@@ -3112,20 +3105,6 @@ static void dw_mci_init_dma(struct dw_mci *host)
host->use_dma = TRANS_MODE_PIO;
}
-static void dw_mci_cmd11_timer(struct timer_list *t)
-{
- struct dw_mci *host = timer_container_of(host, t, cmd11_timer);
-
- if (host->state != STATE_SENDING_CMD11) {
- dev_warn(host->dev, "Unexpected CMD11 timeout\n");
- return;
- }
-
- host->cmd_status = SDMMC_INT_RTO;
- set_bit(EVENT_CMD_COMPLETE, &host->pending_events);
- queue_work(system_bh_wq, &host->bh_work);
-}
-
static int dw_mci_parse_dt(struct dw_mci *host)
{
struct device *dev = host->dev;
@@ -3275,7 +3254,6 @@ int dw_mci_probe(struct dw_mci *host)
hrtimer_setup(&host->wd_timer, dw_mci_watchdog_fn, CLOCK_MONOTONIC,
HRTIMER_MODE_REL);
- timer_setup(&host->cmd11_timer, dw_mci_cmd11_timer, 0);
spin_lock_init(&host->lock);
spin_lock_init(&host->irq_lock);
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 7af2b45..7b70392 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -127,7 +127,6 @@ struct dw_mci_dma_slave {
* time so that the state machine can synthesize an error.
* @wd_events: pending_events bits still awaited by the armed watch.
* @wd_states: host->state values for which the armed watch is valid.
- * @cmd11_timer: Timer for SD3.0 voltage switch over scheme.
* @mmc: The mmc_host representing this dw_mci.
* @flags: Random state bits associated with the host.
* @ctype: Card type for this host.
@@ -242,8 +241,6 @@ struct dw_mci {
unsigned long wd_events;
unsigned long wd_states;
- struct timer_list cmd11_timer;
-
#ifdef CONFIG_FAULT_INJECTION
struct fault_attr fail_data_crc;
struct hrtimer fault_timer;
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 4/4] mmc: dw_mmc: expose the watchdog state in debugfs
2026-08-27 7:58 [PATCH 0/4] mmc: dw_mmc: replace three fallback timers with a single watchdog Shawn Lin
` (2 preceding siblings ...)
2026-08-27 7:58 ` [PATCH 3/4] mmc: dw_mmc: absorb CMD11 timeout into " Shawn Lin
@ 2026-08-27 7:58 ` Shawn Lin
2026-09-10 16:05 ` [PATCH 0/4] mmc: dw_mmc: replace three fallback timers with a single watchdog Ulf Hansson
4 siblings, 0 replies; 6+ messages in thread
From: Shawn Lin @ 2026-08-27 7:58 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jaehoon Chung, Marek Szyprowski, linux-mmc, linux-kernel,
Shawn Lin
Debugging hangs on the request legs now means asking 'what was the
watchdog guarding and until when?' Expose the awaited events, the
valid states, and the absolute deadline of the current watch next to
the existing pending_events/completed_events nodes; all three zero out
once a leg is settled or the watch fired.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/host/dw_mmc.c | 6 ++++++
drivers/mmc/host/dw_mmc.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 6852c05..ff0b25d 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -108,6 +108,7 @@ static void dw_mci_wd_arm(struct dw_mci *host, unsigned long ms,
host->wd_events = events;
host->wd_states = states;
+ host->wd_deadline_ns = ktime_get_ns() + (u64)ms * NSEC_PER_MSEC;
hrtimer_start(&host->wd_timer, ms_to_ktime(ms), HRTIMER_MODE_REL);
}
@@ -167,6 +168,7 @@ static enum hrtimer_restart dw_mci_watchdog_fn(struct hrtimer *t)
host->wd_events = 0;
host->wd_states = 0;
+ host->wd_deadline_ns = 0;
spin_unlock_irqrestore(&host->irq_lock, irqflags);
@@ -300,6 +302,10 @@ static void dw_mci_init_debugfs(struct dw_mci *host)
&host->pending_events);
debugfs_create_xul("completed_events", 0400, root,
&host->completed_events);
+ debugfs_create_x64("wd_deadline_ns", 0400, root,
+ &host->wd_deadline_ns);
+ debugfs_create_xul("wd_events", 0400, root, &host->wd_events);
+ debugfs_create_xul("wd_states", 0400, root, &host->wd_states);
#ifdef CONFIG_FAULT_INJECTION
fault_create_debugfs_attr("fail_data_crc", root, &host->fail_data_crc);
#endif
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 7b70392..12b88e7 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -240,6 +240,7 @@ struct dw_mci {
struct hrtimer wd_timer;
unsigned long wd_events;
unsigned long wd_states;
+ u64 wd_deadline_ns;
#ifdef CONFIG_FAULT_INJECTION
struct fault_attr fail_data_crc;
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/4] mmc: dw_mmc: replace three fallback timers with a single watchdog
2026-08-27 7:58 [PATCH 0/4] mmc: dw_mmc: replace three fallback timers with a single watchdog Shawn Lin
` (3 preceding siblings ...)
2026-08-27 7:58 ` [PATCH 4/4] mmc: dw_mmc: expose the watchdog state in debugfs Shawn Lin
@ 2026-09-10 16:05 ` Ulf Hansson
4 siblings, 0 replies; 6+ messages in thread
From: Ulf Hansson @ 2026-09-10 16:05 UTC (permalink / raw)
To: Shawn Lin
Cc: Ulf Hansson, Jaehoon Chung, Marek Szyprowski, linux-mmc,
linux-kernel
On Thu, Aug 27, 2026 at 9:58 AM Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
>
> The driver carries three fallback timers (cmd11_timer, cto_timer,
> dto_timer) added piecemeal after the hardware failed in the field.
> dto_timer exists because Synopsys confirmed that the DTO interrupt can
> be lost entirely, leaving no hardware fallback and blocking the request
> forever without software help (57e104864bc4); cto_timer came later,
> when Rockchip's reworked sample circuit was root-caused to swallow both
> CMD_DONE and response-timeout interrupts across their whole dwmmc
> family (03de19212ea3); cmd11_timer papered over voltage-switch hangs
> that reproduced reliably when ejecting/inserting UHS cards on rk3288
> (5c935165da79).
>
> Because each callback races against the very interrupt it supplements,
> all three grew the same copy-pasted defenses: re-read MINTSTS in case
> the interrupt is merely late, check whether pending_events has been set
> meanwhile, validate host->state against the leg being guarded, and only
> then synthesize the missed event -- with timer_delete_sync() calls from
> softirq context eventually needed just to contain them. Meanwhile the
> special cases keep piling up: the EXTENDED_TMOUT quirk makes the DTO
> story differ per platform, fault injection can post DATA_ERROR ahead of
> any real completion, and every future change has to reason about up to
> three timers at once.
>
> The observation enabling the cleanup is that command, data and
> voltage-switch legs run strictly serially within a request, so a single
> hrtimer suffices. Under irq_lock it records which pending_events bits
> are awaited along with a snapshot of host->state (dw_mci_wd_arm()),
> and producers clear that awaited mask instead of deleting any timer
> (dw_mci_wd_deliver()). On expiry the one callback classifies what
> expired by comparing the awaited set against the named
> DW_MCI_WD_{CMD,DATA}_EVENTS masks, keeps the old MINTSTS latency check
> but re-arms instead of going passive so an interrupt lost for good
> can no longer wedge the request forever, and only then synthesizes
> exactly what its predecessor would have (RTO + command complete, or
> DRTO + data error/complete). The per-leg deadlines -- CTO formula,
> DRTO formula, 500ms CMD11 budget -- are carried over unchanged.
>
> Apart from three behavioral deltas called out in the individual commit
> messages as well -- a single surviving watch during voltage switch
> instead of two timers racing, a stuck CMD11 aborting after exactly
> 500ms instead of racing min(cto_ms, 500ms) with -ETIMEDOUT unchanged,
> and bounded recovery from interrupt-latency peaks instead of an
> unbounded hang -- no functional change is intended.
>
> Tested on Rockchip platforms with SD card and eMMC (rv1126/rk3568/
> rk3576 boards): normal IO, suspend/resume and card removal during
> transfer. Compile tested on every variant consuming dw_mmc.h.
>
> Next steps: first split dw_mci_work_func() into per-state handlers to
> make the transitions explicit, then go further in the sdhci direction:
> shrink the eight-state machine, drop the redundant completed_events
> bookkeeping, and end up where other host drivers already are: one state
> machine, one timer, no separate event flags.
>
>
>
> Shawn Lin (4):
> mmc: dw_mmc: add central watchdog and convert CTO onto it
> mmc: dw_mmc: convert DTO onto the central watchdog
> mmc: dw_mmc: absorb CMD11 timeout into the central watchdog
> mmc: dw_mmc: expose the watchdog state in debugfs
>
> drivers/mmc/host/dw_mmc.c | 316 +++++++++++++++++++++++-----------------------
> drivers/mmc/host/dw_mmc.h | 15 ++-
> 2 files changed, 165 insertions(+), 166 deletions(-)
>
> --
> 2.7.4
>
The series applied for next, thanks!
Kind regards
Uffe
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-10 16:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-27 7:58 [PATCH 0/4] mmc: dw_mmc: replace three fallback timers with a single watchdog Shawn Lin
2026-08-27 7:58 ` [PATCH 1/4] mmc: dw_mmc: add central watchdog and convert CTO onto it Shawn Lin
2026-08-27 7:58 ` [PATCH 2/4] mmc: dw_mmc: convert DTO onto the central watchdog Shawn Lin
2026-08-27 7:58 ` [PATCH 3/4] mmc: dw_mmc: absorb CMD11 timeout into " Shawn Lin
2026-08-27 7:58 ` [PATCH 4/4] mmc: dw_mmc: expose the watchdog state in debugfs Shawn Lin
2026-09-10 16:05 ` [PATCH 0/4] mmc: dw_mmc: replace three fallback timers with a single watchdog Ulf Hansson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox