mmc: core: Fix bitfield race between retune and host claiming
Subsystems (2)
PATCH 1/1 mmc: core: Fix bitfield race between retune and host claiming
The host->claimed flag shares a bitfield storage word with several retune flags (retune_now, retune_paused, can_retune, doing_retune, doing_init_tune). Updating those flags without host->lock can RMW the shared word and clear claimed, triggering spurious WARN_ON(!host->claimed).
Serialize all retune bitfield updates with host->lock. Provide lockless __mmc_retune_* helpers so callers that already hold host->lock can avoid deadlocks while public wrappers serialize updates. Also protect doing_init_tune and the CQE retune_now assignment with host->lock.
--- drivers/mmc/core/host.c | 60 +++++++++++++++++++++++++++++++--------- drivers/mmc/core/host.h | 35 ++++++++++++++++++++++- drivers/mmc/core/mmc.c | 6 ++++ drivers/mmc/core/queue.c | 3 ++ include/linux/mmc/host.h | 4 +++ 5 files changed, 94 insertions(+), 14 deletions(-)
drivers/mmc/core/host.c +47 -13
@@ -109,7 +109,11 @@ void mmc_unregister_host_class(void)*/ void mmc_retune_enable (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->can_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->retune_period)mod_timer (&host->retune_timer, jiffies + host->retune_period *HZ );
@@ -121,18 +125,31 @@ void mmc_retune_enable(struct mmc_host *host)*/ void mmc_retune_pause (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (!host->retune_paused) { host->retune_paused =1 ; -mmc_retune_hold (host); +__mmc_retune_hold (host); } +spin_unlock_irqrestore (&host->lock, flags); }EXPORT_SYMBOL (mmc_retune_pause);void mmc_retune_unpause (struct mmc_host *host) { +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags);if (host->retune_paused) { host->retune_paused =0 ; -mmc_retune_release (host); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released) +WARN_ON (1 ); + }else { +spin_unlock_irqrestore (&host->lock, flags); } }EXPORT_SYMBOL (mmc_retune_unpause);
@@ -145,8 +162,12 @@ EXPORT_SYMBOL(mmc_retune_unpause);*/ void mmc_retune_disable (struct mmc_host *host) { +unsigned long flags; +mmc_retune_unpause (host); +spin_lock_irqsave (&host->lock, flags); host->can_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);timer_delete_sync (&host->retune_timer);mmc_retune_clear (host); }
@@ -159,16 +180,22 @@ EXPORT_SYMBOL(mmc_retune_timer_stop);void mmc_retune_hold (struct mmc_host *host) { -if (!host->hold_retune) - host->retune_now =1 ; - host->hold_retune +=1 ; +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_hold (host); +spin_unlock_irqrestore (&host->lock, flags); }void mmc_retune_release (struct mmc_host *host) { -if (host->hold_retune) - host->hold_retune -=1 ; -else +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released)WARN_ON (1 ); }EXPORT_SYMBOL (mmc_retune_release);
@@ -177,18 +204,23 @@ int mmc_retune(struct mmc_host *host) {bool return_to_hs400 = false;int err; +unsigned long flags; -if (host->retune_now) - host->retune_now =0 ; -else +spin_lock_irqsave (&host->lock, flags); +if (!host->retune_now) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } + host->retune_now =0 ; -if (!host->need_retune || host->doing_retune || !host->card) +if (!host->need_retune || host->doing_retune || !host->card) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } host->need_retune =0 ; - host->doing_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->ios.timing ==MMC_TIMING_MMC_HS400 ) { err =mmc_hs400_to_hs200 (host->card);
@@ -205,7 +237,9 @@ int mmc_retune(struct mmc_host *host)if (return_to_hs400) err =mmc_hs200_to_hs400 (host->card); out: +spin_lock_irqsave (&host->lock, flags); host->doing_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);return err; }
drivers/mmc/core/host.h +34 -1
@@ -21,22 +21,55 @@ int mmc_retune(struct mmc_host *host);void mmc_retune_pause (struct mmc_host *host);void mmc_retune_unpause (struct mmc_host *host); -static inline void mmc_retune_clear (struct mmc_host *host) +static inline void __mmc_retune_clear (struct mmc_host *host) { host->retune_now =0 ; host->need_retune =0 ; } +static inline void mmc_retune_clear (struct mmc_host *host) +{ +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_clear (host); +spin_unlock_irqrestore (&host->lock, flags); +} + +static inline void __mmc_retune_hold (struct mmc_host *host) +{ +if (!host->hold_retune) + host->retune_now =1 ; + host->hold_retune +=1 ; +} + +static inline bool __mmc_retune_release (struct mmc_host *host) +{ +if (host->hold_retune) { + host->hold_retune -=1 ; +return true; + } +return false; +} +static inline void mmc_retune_hold_now (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->retune_now =0 ; host->hold_retune +=1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline void mmc_retune_recheck (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->hold_retune <=1 ) host->retune_now =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline int mmc_host_can_cmd23 (struct mmc_host *host)
drivers/mmc/core/mmc.c +6 -0
@@ -1820,13 +1820,19 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, goto free_card;if (mmc_card_hs200 (card)) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =1 ; +spin_unlock_irqrestore (&host->lock, flags); err =mmc_hs200_tuning (card);if (!err) err =mmc_select_hs400 (card); +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =0 ; +spin_unlock_irqrestore (&host->lock, flags);if (err) goto free_card;
drivers/mmc/core/queue.c +3 -0
@@ -237,6 +237,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,enum mmc_issue_type issue_type;enum mmc_issued issued;bool get_card, cqe_retune_ok; +unsigned long flags;blk_status_t ret;if (mmc_card_removed (mq->card)) {
@@ -297,8 +298,10 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,mmc_get_card (card, &mq->ctx);if (host->cqe_enabled) { +spin_lock_irqsave (&host->lock, flags); host->retune_now = host->need_retune && cqe_retune_ok && !host->hold_retune; +spin_unlock_irqrestore (&host->lock, flags); }blk_mq_start_request (req);
include/linux/mmc/host.h +4 -0
@@ -713,8 +713,12 @@ void mmc_retune_timer_stop(struct mmc_host *host);static inline void mmc_retune_needed (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->can_retune) host->need_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline bool mmc_can_retune (struct mmc_host *host)
Signature · 1 line
2.43.0
+ Adrian
Quoted 7 lines · Penghe Geng
The
host->claimedflag shares a bitfield storage word with several retune flags (retune_now,retune_paused,can_retune,doing_retune,doing_init_tune). Updating those flags withouthost->lockcan RMW the shared word and clear claimed, triggering spuriousWARN_ON(!host->claimed).
Quoted 8 lines
Serialize all retune bitfield updates with
host->lock. Provide lockless __mmc_retune_* helpers so callers that already holdhost->lockcan avoid deadlocks while public wrappers serialize updates. Also protectdoing_init_tuneand the CQEretune_nowassignment withhost->lock.Fixes dfa13ebbe334 ("mmc: host: Add facility to support re-tuning")Signed-off-by Penghe Geng <[email protected]>
Thanks for the patch! I have looped in Adrian to get his opinion on this.
Kind regards
Uffe
Quoted 267 lines
--- drivers/mmc/core/host.c | 60 +++++++++++++++++++++++++++++++--------- drivers/mmc/core/host.h | 35 ++++++++++++++++++++++- drivers/mmc/core/mmc.c | 6 ++++ drivers/mmc/core/queue.c | 3 ++ include/linux/mmc/host.h | 4 +++ 5 files changed, 94 insertions(+), 14 deletions(-)drivers/mmc/core/host.c +47
-13@@ -109,7 +109,11 @@ void mmc_unregister_host_class(void)*/ void mmc_retune_enable (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->can_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->retune_period)mod_timer (&host->retune_timer, jiffies + host->retune_period *HZ );@@ -121,18 +125,31 @@ void mmc_retune_enable(struct mmc_host *host)*/ void mmc_retune_pause (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (!host->retune_paused) { host->retune_paused =1 ; -mmc_retune_hold (host); +__mmc_retune_hold (host); } +spin_unlock_irqrestore (&host->lock, flags); }EXPORT_SYMBOL (mmc_retune_pause);void mmc_retune_unpause (struct mmc_host *host) { +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags);if (host->retune_paused) { host->retune_paused =0 ; -mmc_retune_release (host); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released) +WARN_ON (1 ); + }else { +spin_unlock_irqrestore (&host->lock, flags); } }EXPORT_SYMBOL (mmc_retune_unpause);@@ -145,8 +162,12 @@ EXPORT_SYMBOL(mmc_retune_unpause);*/ void mmc_retune_disable (struct mmc_host *host) { +unsigned long flags; +mmc_retune_unpause (host); +spin_lock_irqsave (&host->lock, flags); host->can_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);timer_delete_sync (&host->retune_timer);mmc_retune_clear (host); }@@ -159,16 +180,22 @@ EXPORT_SYMBOL(mmc_retune_timer_stop);void mmc_retune_hold (struct mmc_host *host) { -if (!host->hold_retune) - host->retune_now =1 ; - host->hold_retune +=1 ; +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_hold (host); +spin_unlock_irqrestore (&host->lock, flags); }void mmc_retune_release (struct mmc_host *host) { -if (host->hold_retune) - host->hold_retune -=1 ; -else +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released)WARN_ON (1 ); }EXPORT_SYMBOL (mmc_retune_release);@@ -177,18 +204,23 @@ int mmc_retune(struct mmc_host *host) {bool return_to_hs400 = false;int err; +unsigned long flags; -if (host->retune_now) - host->retune_now =0 ; -else +spin_lock_irqsave (&host->lock, flags); +if (!host->retune_now) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } + host->retune_now =0 ; -if (!host->need_retune || host->doing_retune || !host->card) +if (!host->need_retune || host->doing_retune || !host->card) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } host->need_retune =0 ; - host->doing_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->ios.timing ==MMC_TIMING_MMC_HS400 ) { err =mmc_hs400_to_hs200 (host->card);@@ -205,7 +237,9 @@ int mmc_retune(struct mmc_host *host)if (return_to_hs400) err =mmc_hs200_to_hs400 (host->card); out: +spin_lock_irqsave (&host->lock, flags); host->doing_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);return err; }drivers/mmc/core/host.h +34
-1@@ -21,22 +21,55 @@ int mmc_retune(struct mmc_host *host);void mmc_retune_pause (struct mmc_host *host);void mmc_retune_unpause (struct mmc_host *host); -static inline void mmc_retune_clear (struct mmc_host *host) +static inline void __mmc_retune_clear (struct mmc_host *host) { host->retune_now =0 ; host->need_retune =0 ; } +static inline void mmc_retune_clear (struct mmc_host *host) +{ +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_clear (host); +spin_unlock_irqrestore (&host->lock, flags); +} + +static inline void __mmc_retune_hold (struct mmc_host *host) +{ +if (!host->hold_retune) + host->retune_now =1 ; + host->hold_retune +=1 ; +} + +static inline bool __mmc_retune_release (struct mmc_host *host) +{ +if (host->hold_retune) { + host->hold_retune -=1 ; +return true; + } +return false; +} +static inline void mmc_retune_hold_now (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->retune_now =0 ; host->hold_retune +=1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline void mmc_retune_recheck (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->hold_retune <=1 ) host->retune_now =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline int mmc_host_can_cmd23 (struct mmc_host *host)drivers/mmc/core/mmc.c +6
-0@@ -1820,13 +1820,19 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, goto free_card;if (mmc_card_hs200 (card)) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =1 ; +spin_unlock_irqrestore (&host->lock, flags); err =mmc_hs200_tuning (card);if (!err) err =mmc_select_hs400 (card); +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =0 ; +spin_unlock_irqrestore (&host->lock, flags);if (err) goto free_card;drivers/mmc/core/queue.c +3
-0@@ -237,6 +237,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,enum mmc_issue_type issue_type;enum mmc_issued issued;bool get_card, cqe_retune_ok; +unsigned long flags;blk_status_t ret;if (mmc_card_removed (mq->card)) {@@ -297,8 +298,10 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,mmc_get_card (card, &mq->ctx);if (host->cqe_enabled) { +spin_lock_irqsave (&host->lock, flags); host->retune_now = host->need_retune && cqe_retune_ok && !host->hold_retune; +spin_unlock_irqrestore (&host->lock, flags); }blk_mq_start_request (req);include/linux/mmc/host.h +4
-1@@ -713,8 +713,12 @@ void mmc_retune_timer_stop(struct mmc_host *host);static inline void mmc_retune_needed (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->can_retune) host->need_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline bool mmc_can_retune (struct mmc_host *host) --2.43.0
Quoted 5 lines · Penghe Geng
The
host->claimedflag shares a bitfield storage word with several retune flags (retune_now,retune_paused,can_retune,doing_retune,doing_init_tune). Updating those flags withouthost->lockcan RMW the shared word and clear claimed, triggering spuriousWARN_ON(!host->claimed).
Thanks for finding this!
The design is that those members are protected by the host->claimed lock itself.
mmc operations are primarily single-threaded, protected by the host->claimed lock, although the block driver does allow multiple transfers at the same time in some cases.
There are also other contexts like interrupt handlers.
Can you provide some information about when WARN_ON(!host->claimed) is being hit? Including the stack dump?
What kernel version?
Is it eMMC, SDIO or SD card?
Is CQE being used?
Are there any I/O errors happening also?
What controller driver is it?
In any case, the use of bit fields seems to add complexity unnecessarily, so we should consider converting some or all of them to bool.
Quoted 273 lines
Serialize all retune bitfield updates with
host->lock. Provide lockless __mmc_retune_* helpers so callers that already holdhost->lockcan avoid deadlocks while public wrappers serialize updates. Also protectdoing_init_tuneand the CQEretune_nowassignment withhost->lock.Fixes dfa13ebbe334 ("mmc: host: Add facility to support re-tuning")Signed-off-by Penghe Geng <[email protected]>--- drivers/mmc/core/host.c | 60 +++++++++++++++++++++++++++++++--------- drivers/mmc/core/host.h | 35 ++++++++++++++++++++++- drivers/mmc/core/mmc.c | 6 ++++ drivers/mmc/core/queue.c | 3 ++ include/linux/mmc/host.h | 4 +++ 5 files changed, 94 insertions(+), 14 deletions(-)drivers/mmc/core/host.c +47
-13@@ -109,7 +109,11 @@ void mmc_unregister_host_class(void)*/ void mmc_retune_enable (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->can_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->retune_period)mod_timer (&host->retune_timer, jiffies + host->retune_period *HZ );@@ -121,18 +125,31 @@ void mmc_retune_enable(struct mmc_host *host)*/ void mmc_retune_pause (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (!host->retune_paused) { host->retune_paused =1 ; -mmc_retune_hold (host); +__mmc_retune_hold (host); } +spin_unlock_irqrestore (&host->lock, flags); }EXPORT_SYMBOL (mmc_retune_pause);void mmc_retune_unpause (struct mmc_host *host) { +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags);if (host->retune_paused) { host->retune_paused =0 ; -mmc_retune_release (host); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released) +WARN_ON (1 ); + }else { +spin_unlock_irqrestore (&host->lock, flags); } }EXPORT_SYMBOL (mmc_retune_unpause);@@ -145,8 +162,12 @@ EXPORT_SYMBOL(mmc_retune_unpause);*/ void mmc_retune_disable (struct mmc_host *host) { +unsigned long flags; +mmc_retune_unpause (host); +spin_lock_irqsave (&host->lock, flags); host->can_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);timer_delete_sync (&host->retune_timer);mmc_retune_clear (host); }@@ -159,16 +180,22 @@ EXPORT_SYMBOL(mmc_retune_timer_stop);void mmc_retune_hold (struct mmc_host *host) { -if (!host->hold_retune) - host->retune_now =1 ; - host->hold_retune +=1 ; +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_hold (host); +spin_unlock_irqrestore (&host->lock, flags); }void mmc_retune_release (struct mmc_host *host) { -if (host->hold_retune) - host->hold_retune -=1 ; -else +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released)WARN_ON (1 ); }EXPORT_SYMBOL (mmc_retune_release);@@ -177,18 +204,23 @@ int mmc_retune(struct mmc_host *host) {bool return_to_hs400 = false;int err; +unsigned long flags; -if (host->retune_now) - host->retune_now =0 ; -else +spin_lock_irqsave (&host->lock, flags); +if (!host->retune_now) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } + host->retune_now =0 ; -if (!host->need_retune || host->doing_retune || !host->card) +if (!host->need_retune || host->doing_retune || !host->card) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } host->need_retune =0 ; - host->doing_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->ios.timing ==MMC_TIMING_MMC_HS400 ) { err =mmc_hs400_to_hs200 (host->card);@@ -205,7 +237,9 @@ int mmc_retune(struct mmc_host *host)if (return_to_hs400) err =mmc_hs200_to_hs400 (host->card); out: +spin_lock_irqsave (&host->lock, flags); host->doing_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);return err; }drivers/mmc/core/host.h +34
-1@@ -21,22 +21,55 @@ int mmc_retune(struct mmc_host *host);void mmc_retune_pause (struct mmc_host *host);void mmc_retune_unpause (struct mmc_host *host); -static inline void mmc_retune_clear (struct mmc_host *host) +static inline void __mmc_retune_clear (struct mmc_host *host) { host->retune_now =0 ; host->need_retune =0 ; } +static inline void mmc_retune_clear (struct mmc_host *host) +{ +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_clear (host); +spin_unlock_irqrestore (&host->lock, flags); +} + +static inline void __mmc_retune_hold (struct mmc_host *host) +{ +if (!host->hold_retune) + host->retune_now =1 ; + host->hold_retune +=1 ; +} + +static inline bool __mmc_retune_release (struct mmc_host *host) +{ +if (host->hold_retune) { + host->hold_retune -=1 ; +return true; + } +return false; +} +static inline void mmc_retune_hold_now (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->retune_now =0 ; host->hold_retune +=1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline void mmc_retune_recheck (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->hold_retune <=1 ) host->retune_now =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline int mmc_host_can_cmd23 (struct mmc_host *host)drivers/mmc/core/mmc.c +6
-0@@ -1820,13 +1820,19 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, goto free_card;if (mmc_card_hs200 (card)) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =1 ; +spin_unlock_irqrestore (&host->lock, flags); err =mmc_hs200_tuning (card);if (!err) err =mmc_select_hs400 (card); +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =0 ; +spin_unlock_irqrestore (&host->lock, flags);if (err) goto free_card;drivers/mmc/core/queue.c +3
-0@@ -237,6 +237,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,enum mmc_issue_type issue_type;enum mmc_issued issued;bool get_card, cqe_retune_ok; +unsigned long flags;blk_status_t ret;if (mmc_card_removed (mq->card)) {@@ -297,8 +298,10 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,mmc_get_card (card, &mq->ctx);if (host->cqe_enabled) { +spin_lock_irqsave (&host->lock, flags); host->retune_now = host->need_retune && cqe_retune_ok && !host->hold_retune; +spin_unlock_irqrestore (&host->lock, flags); }blk_mq_start_request (req);include/linux/mmc/host.h +4
-0@@ -713,8 +713,12 @@ void mmc_retune_timer_stop(struct mmc_host *host);static inline void mmc_retune_needed (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->can_retune) host->need_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline bool mmc_can_retune (struct mmc_host *host)
Hi Adrian,
Thanks for the feedback. Below are the details you asked for.
Kernel versions:
- Seen on 5.15.120.bsk.business.6‑arm64 (custom tree).
- Also observed on 6.1.0‑11‑arm64 (Debian 6.1.38‑4).
Media: eMMC
Controllers:
- BlueField‑2: Synopsys DesignWare MMC (drivers/mmc/host/dw_mmc-bluefield.c)
- BlueField‑3: Synopsys DWC MSHC (drivers/mmc/host/sdhci-of-dwcmshc.c in‑tree) and also with OOT sdhci-of-dwcmshc-bf3
CQE:
- Not enabled at runtime. CONFIG_MMC_CQHCI=m.
- lsmod | grep cq is empty by default.
- modprobe cqhci loads with 0 users and no CQE/CQHCI enable messages in dmesg. So CQE is not in use by the active host.
I/O errors:
- None observed around the WARN.
Repro:
- Intermittent, mostly during boot under stress.
- Roughly 0.1–1% depending on distro/platform.
Example stack (BF3, in-tree sdhci-of-dwcmshc):
------------[ cut here ]------------ mmcblk0boot1: mmc0:0001 Y29128 31.9 MiB WARNING: CPU: 8 PID: 240 at drivers/mmc/core/core.c:349 mmc_start_request+0xb4/0xc4 Modules linked in: crc16(E) mbcache(E) jbd2(E) nvme_tcp(OE) nvme_rdma(OE) rdma_cm(OE) iw_cm(OE) ib_cm(OE) ib_core(OE) nvme_fabrics(OE) configfs(E) nls_ascii(E) nls_cp437(E) nls_cp850(E) msdos(E) efivarfs(E) nvme(OE) nvme_core(OE) virtio_net(E) net_failover(E) virtio_console(E) failover(E) mlxbf_tmfifo(OE) mlx_compat(OE) virtio(E) t10_pi(E) sbsa_gwdt(E) mlxbf_bootctl(OE) sdhci_of_dwcmshc(OE) virtio_ring(E) mmcblk0rpmb: mmc0:0001 Y29128 4.00 MiB, chardev (245:0) CPU: 8 PID: 240 Comm: kworker/8:1H Tainted: G OE 5.15.120.bsk.business.6-arm64 #5.15.120.bsk.business.6 Hardware name: https://www.mellanox.com BlueField-3 DPU/BlueField-3 DPU, BIOS 4.9.2.13576 Mar 18 2025 Workqueue: kblockd blk_mq_run_work_fn pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : mmc_start_request+0xb4/0xc4 lr : mmc_start_request+0x68/0xc4
sp : ffff80000932ba90
x29: ffff80000932ba90 x28: ffff000081b3b308 x27: 0000000000000000
x26: 0000000000000001 x25: 0000000000000000 x24: ffff000082aec000
x23: ffff000082485800 x22: ffff000082aec000 x21: 0000000000000000
x20: ffff000081b3b3d8 x19: ffff000082aec000 x18: 0000000000000000
x17: 0000000000000000 x16: ffffbab212913c40 x15: 0000000000000000
x14: 0000000000000000 x13: 0000000000000038 x12: ffff00008f06b000
x11: 7f7f7f7f7f7f7f7f x10: ffffbab2144acbc8 x9 : ffffbab2130ad368 x8 : ffff000081b3b548 x7 : ffff000082aec000 x6 : 0000000000000000 x5 : ffff000081b3b458 x4 : ffff00008a4b5e80 x3 : ffff0000824859b0 x2 : 0000000000000000 x1 : ffff000081b3b4c8 x0 : 0000000000000020
Call trace: mmc_start_request+0xb4/0xc4 mmc_blk_mq_issue_rq+0x310/0x8fc mmc_mq_queue_rq+0x154/0x3e0 blk_mq_dispatch_rq_list+0x13c/0xa44 blk_mq_do_dispatch_sched+0x2cc/0x33c __blk_mq_sched_dispatch_requests+0x154/0x1b0 blk_mq_sched_dispatch_requests+0x40/0x80 __blk_mq_run_hw_queue+0x58/0xa0 blk_mq_run_work_fn+0x28/0x34 process_one_work+0x1f8/0x4c0 worker_thread+0x180/0x580 kthread+0x128/0x13c kthread_return_to_user+0x0/0x10 ---[ end trace fc3df73f08f7c8ee ]---
I agree the bitfield usage adds complexity. I can work on a follow-up to convert the retune-related flags to bools if that’s the preferred direction.
Thanks,
Penghe
Quoted 306 lines · Adrian Hunter
External email: Use caution opening links or attachments
Quoted 5 lines · Penghe Geng
The
host->claimedflag shares a bitfield storage word with several retune flags (retune_now,retune_paused,can_retune,doing_retune,doing_init_tune). Updating those flags withouthost->lockcan RMW the shared word and clear claimed, triggering spuriousWARN_ON(!host->claimed).Thanks for finding this!
The design is that those members are protected by the
host->claimedlock itself.mmc operations are primarily single-threaded, protected by the
host->claimedlock, although the block driver does allow multiple transfers at the same time in some cases.There are also other contexts like interrupt handlers.
Can you provide some information about when
WARN_ON(!host->claimed)is being hit? Including the stack dump?
What kernel version?
Is it eMMC, SDIO or SD card?
Is CQE being used?
Are there any I/O errors happening also?
What controller driver is it?In any case, the use of bit fields seems to add complexity unnecessarily, so we should consider converting some or all of them to bool.
Quoted 273 lines
Serialize all retune bitfield updates with
host->lock. Provide lockless __mmc_retune_* helpers so callers that already holdhost->lockcan avoid deadlocks while public wrappers serialize updates. Also protectdoing_init_tuneand the CQEretune_nowassignment withhost->lock.Fixes dfa13ebbe334 ("mmc: host: Add facility to support re-tuning")Signed-off-by Penghe Geng <[email protected]>--- drivers/mmc/core/host.c | 60 +++++++++++++++++++++++++++++++--------- drivers/mmc/core/host.h | 35 ++++++++++++++++++++++- drivers/mmc/core/mmc.c | 6 ++++ drivers/mmc/core/queue.c | 3 ++ include/linux/mmc/host.h | 4 +++ 5 files changed, 94 insertions(+), 14 deletions(-)drivers/mmc/core/host.c +47
-13@@ -109,7 +109,11 @@ void mmc_unregister_host_class(void)*/ void mmc_retune_enable (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->can_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->retune_period)mod_timer (&host->retune_timer, jiffies + host->retune_period *HZ );@@ -121,18 +125,31 @@ void mmc_retune_enable(struct mmc_host *host)*/ void mmc_retune_pause (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (!host->retune_paused) { host->retune_paused =1 ; -mmc_retune_hold (host); +__mmc_retune_hold (host); } +spin_unlock_irqrestore (&host->lock, flags); }EXPORT_SYMBOL (mmc_retune_pause);void mmc_retune_unpause (struct mmc_host *host) { +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags);if (host->retune_paused) { host->retune_paused =0 ; -mmc_retune_release (host); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released) +WARN_ON (1 ); + }else { +spin_unlock_irqrestore (&host->lock, flags); } }EXPORT_SYMBOL (mmc_retune_unpause);@@ -145,8 +162,12 @@ EXPORT_SYMBOL(mmc_retune_unpause);*/ void mmc_retune_disable (struct mmc_host *host) { +unsigned long flags; +mmc_retune_unpause (host); +spin_lock_irqsave (&host->lock, flags); host->can_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);timer_delete_sync (&host->retune_timer);mmc_retune_clear (host); }@@ -159,16 +180,22 @@ EXPORT_SYMBOL(mmc_retune_timer_stop);void mmc_retune_hold (struct mmc_host *host) { -if (!host->hold_retune) - host->retune_now =1 ; - host->hold_retune +=1 ; +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_hold (host); +spin_unlock_irqrestore (&host->lock, flags); }void mmc_retune_release (struct mmc_host *host) { -if (host->hold_retune) - host->hold_retune -=1 ; -else +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released)WARN_ON (1 ); }EXPORT_SYMBOL (mmc_retune_release);@@ -177,18 +204,23 @@ int mmc_retune(struct mmc_host *host) {bool return_to_hs400 = false;int err; +unsigned long flags; -if (host->retune_now) - host->retune_now =0 ; -else +spin_lock_irqsave (&host->lock, flags); +if (!host->retune_now) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } + host->retune_now =0 ; -if (!host->need_retune || host->doing_retune || !host->card) +if (!host->need_retune || host->doing_retune || !host->card) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } host->need_retune =0 ; - host->doing_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->ios.timing ==MMC_TIMING_MMC_HS400 ) { err =mmc_hs400_to_hs200 (host->card);@@ -205,7 +237,9 @@ int mmc_retune(struct mmc_host *host)if (return_to_hs400) err =mmc_hs200_to_hs400 (host->card); out: +spin_lock_irqsave (&host->lock, flags); host->doing_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);return err; }drivers/mmc/core/host.h +34
-1@@ -21,22 +21,55 @@ int mmc_retune(struct mmc_host *host);void mmc_retune_pause (struct mmc_host *host);void mmc_retune_unpause (struct mmc_host *host); -static inline void mmc_retune_clear (struct mmc_host *host) +static inline void __mmc_retune_clear (struct mmc_host *host) { host->retune_now =0 ; host->need_retune =0 ; } +static inline void mmc_retune_clear (struct mmc_host *host) +{ +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_clear (host); +spin_unlock_irqrestore (&host->lock, flags); +} + +static inline void __mmc_retune_hold (struct mmc_host *host) +{ +if (!host->hold_retune) + host->retune_now =1 ; + host->hold_retune +=1 ; +} + +static inline bool __mmc_retune_release (struct mmc_host *host) +{ +if (host->hold_retune) { + host->hold_retune -=1 ; +return true; + } +return false; +} +static inline void mmc_retune_hold_now (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->retune_now =0 ; host->hold_retune +=1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline void mmc_retune_recheck (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->hold_retune <=1 ) host->retune_now =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline int mmc_host_can_cmd23 (struct mmc_host *host)drivers/mmc/core/mmc.c +6
-0@@ -1820,13 +1820,19 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, goto free_card;if (mmc_card_hs200 (card)) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =1 ; +spin_unlock_irqrestore (&host->lock, flags); err =mmc_hs200_tuning (card);if (!err) err =mmc_select_hs400 (card); +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =0 ; +spin_unlock_irqrestore (&host->lock, flags);if (err) goto free_card;drivers/mmc/core/queue.c +3
-0@@ -237,6 +237,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,enum mmc_issue_type issue_type;enum mmc_issued issued;bool get_card, cqe_retune_ok; +unsigned long flags;blk_status_t ret;if (mmc_card_removed (mq->card)) {@@ -297,8 +298,10 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,mmc_get_card (card, &mq->ctx);if (host->cqe_enabled) { +spin_lock_irqsave (&host->lock, flags); host->retune_now = host->need_retune && cqe_retune_ok && !host->hold_retune; +spin_unlock_irqrestore (&host->lock, flags); }blk_mq_start_request (req);include/linux/mmc/host.h +4
-0@@ -713,8 +713,12 @@ void mmc_retune_timer_stop(struct mmc_host *host);static inline void mmc_retune_needed (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->can_retune) host->need_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline bool mmc_can_retune (struct mmc_host *host)
Quoted 306 lines · Adrian Hunter
External email: Use caution opening links or attachments
Quoted 5 lines · Penghe Geng
The
host->claimedflag shares a bitfield storage word with several retune flags (retune_now,retune_paused,can_retune,doing_retune,doing_init_tune). Updating those flags withouthost->lockcan RMW the shared word and clear claimed, triggering spuriousWARN_ON(!host->claimed).Thanks for finding this!
The design is that those members are protected by the
host->claimedlock itself.mmc operations are primarily single-threaded, protected by the
host->claimedlock, although the block driver does allow multiple transfers at the same time in some cases.There are also other contexts like interrupt handlers.
Can you provide some information about when
WARN_ON(!host->claimed)is being hit? Including the stack dump?
What kernel version?
Is it eMMC, SDIO or SD card?
Is CQE being used?
Are there any I/O errors happening also?
What controller driver is it?In any case, the use of bit fields seems to add complexity unnecessarily, so we should consider converting some or all of them to bool.
Quoted 273 lines
Serialize all retune bitfield updates with
host->lock. Provide lockless __mmc_retune_* helpers so callers that already holdhost->lockcan avoid deadlocks while public wrappers serialize updates. Also protectdoing_init_tuneand the CQEretune_nowassignment withhost->lock.Fixes dfa13ebbe334 ("mmc: host: Add facility to support re-tuning")Signed-off-by Penghe Geng <[email protected]>--- drivers/mmc/core/host.c | 60 +++++++++++++++++++++++++++++++--------- drivers/mmc/core/host.h | 35 ++++++++++++++++++++++- drivers/mmc/core/mmc.c | 6 ++++ drivers/mmc/core/queue.c | 3 ++ include/linux/mmc/host.h | 4 +++ 5 files changed, 94 insertions(+), 14 deletions(-)drivers/mmc/core/host.c +47
-13@@ -109,7 +109,11 @@ void mmc_unregister_host_class(void)*/ void mmc_retune_enable (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->can_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->retune_period)mod_timer (&host->retune_timer, jiffies + host->retune_period *HZ );@@ -121,18 +125,31 @@ void mmc_retune_enable(struct mmc_host *host)*/ void mmc_retune_pause (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (!host->retune_paused) { host->retune_paused =1 ; -mmc_retune_hold (host); +__mmc_retune_hold (host); } +spin_unlock_irqrestore (&host->lock, flags); }EXPORT_SYMBOL (mmc_retune_pause);void mmc_retune_unpause (struct mmc_host *host) { +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags);if (host->retune_paused) { host->retune_paused =0 ; -mmc_retune_release (host); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released) +WARN_ON (1 ); + }else { +spin_unlock_irqrestore (&host->lock, flags); } }EXPORT_SYMBOL (mmc_retune_unpause);@@ -145,8 +162,12 @@ EXPORT_SYMBOL(mmc_retune_unpause);*/ void mmc_retune_disable (struct mmc_host *host) { +unsigned long flags; +mmc_retune_unpause (host); +spin_lock_irqsave (&host->lock, flags); host->can_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);timer_delete_sync (&host->retune_timer);mmc_retune_clear (host); }@@ -159,16 +180,22 @@ EXPORT_SYMBOL(mmc_retune_timer_stop);void mmc_retune_hold (struct mmc_host *host) { -if (!host->hold_retune) - host->retune_now =1 ; - host->hold_retune +=1 ; +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_hold (host); +spin_unlock_irqrestore (&host->lock, flags); }void mmc_retune_release (struct mmc_host *host) { -if (host->hold_retune) - host->hold_retune -=1 ; -else +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released)WARN_ON (1 ); }EXPORT_SYMBOL (mmc_retune_release);@@ -177,18 +204,23 @@ int mmc_retune(struct mmc_host *host) {bool return_to_hs400 = false;int err; +unsigned long flags; -if (host->retune_now) - host->retune_now =0 ; -else +spin_lock_irqsave (&host->lock, flags); +if (!host->retune_now) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } + host->retune_now =0 ; -if (!host->need_retune || host->doing_retune || !host->card) +if (!host->need_retune || host->doing_retune || !host->card) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } host->need_retune =0 ; - host->doing_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->ios.timing ==MMC_TIMING_MMC_HS400 ) { err =mmc_hs400_to_hs200 (host->card);@@ -205,7 +237,9 @@ int mmc_retune(struct mmc_host *host)if (return_to_hs400) err =mmc_hs200_to_hs400 (host->card); out: +spin_lock_irqsave (&host->lock, flags); host->doing_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);return err; }drivers/mmc/core/host.h +34
-1@@ -21,22 +21,55 @@ int mmc_retune(struct mmc_host *host);void mmc_retune_pause (struct mmc_host *host);void mmc_retune_unpause (struct mmc_host *host); -static inline void mmc_retune_clear (struct mmc_host *host) +static inline void __mmc_retune_clear (struct mmc_host *host) { host->retune_now =0 ; host->need_retune =0 ; } +static inline void mmc_retune_clear (struct mmc_host *host) +{ +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_clear (host); +spin_unlock_irqrestore (&host->lock, flags); +} + +static inline void __mmc_retune_hold (struct mmc_host *host) +{ +if (!host->hold_retune) + host->retune_now =1 ; + host->hold_retune +=1 ; +} + +static inline bool __mmc_retune_release (struct mmc_host *host) +{ +if (host->hold_retune) { + host->hold_retune -=1 ; +return true; + } +return false; +} +static inline void mmc_retune_hold_now (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->retune_now =0 ; host->hold_retune +=1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline void mmc_retune_recheck (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->hold_retune <=1 ) host->retune_now =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline int mmc_host_can_cmd23 (struct mmc_host *host)drivers/mmc/core/mmc.c +6
-0@@ -1820,13 +1820,19 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, goto free_card;if (mmc_card_hs200 (card)) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =1 ; +spin_unlock_irqrestore (&host->lock, flags); err =mmc_hs200_tuning (card);if (!err) err =mmc_select_hs400 (card); +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =0 ; +spin_unlock_irqrestore (&host->lock, flags);if (err) goto free_card;drivers/mmc/core/queue.c +3
-0@@ -237,6 +237,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,enum mmc_issue_type issue_type;enum mmc_issued issued;bool get_card, cqe_retune_ok; +unsigned long flags;blk_status_t ret;if (mmc_card_removed (mq->card)) {@@ -297,8 +298,10 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,mmc_get_card (card, &mq->ctx);if (host->cqe_enabled) { +spin_lock_irqsave (&host->lock, flags); host->retune_now = host->need_retune && cqe_retune_ok && !host->hold_retune; +spin_unlock_irqrestore (&host->lock, flags); }blk_mq_start_request (req);include/linux/mmc/host.h +4
-0@@ -713,8 +713,12 @@ void mmc_retune_timer_stop(struct mmc_host *host);static inline void mmc_retune_needed (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->can_retune) host->need_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline bool mmc_can_retune (struct mmc_host *host)
Quoted 50 lines · Penghe Geng
Hi Adrian,
Thanks for the feedback. Below are the details you asked for.
Kernel versions:
- Seen on 5.15.120.bsk.business.6‑arm64 (custom tree).
- Also observed on 6.1.0‑11‑arm64 (Debian 6.1.38‑4).Media: eMMC
Controllers:
- BlueField‑2: Synopsys DesignWare MMC (drivers/mmc/host/dw_mmc-bluefield.c)
- BlueField‑3: Synopsys DWC MSHC (drivers/mmc/host/sdhci-of-dwcmshc.cin‑tree) and also with OOT sdhci-of-dwcmshc-bf3CQE:
- Not enabled at runtime.CONFIG_MMC_CQHCI=m.
- lsmod | grep cq is empty by default.
- modprobe cqhci loads with 0 users and no CQE/CQHCI enable messages in dmesg. So CQE is not in use by the active host.I/O errors:
- None observed around the WARN.Repro:
- Intermittent, mostly during boot under stress.
- Roughly 0.1–1% depending on distro/platform.Example stack (BF3, in-tree sdhci-of-dwcmshc):
------------[ cut here ]------------ mmcblk0boot1: mmc0:0001 Y29128 31.9 MiB WARNING: CPU: 8 PID: 240 at drivers/mmc/core/core.c:349 mmc_start_request+0xb4/0xc4 Modules linked in: crc16(E) mbcache(E) jbd2(E) nvme_tcp(OE) nvme_rdma(OE) rdma_cm(OE) iw_cm(OE) ib_cm(OE) ib_core(OE) nvme_fabrics(OE) configfs(E) nls_ascii(E) nls_cp437(E) nls_cp850(E) msdos(E) efivarfs(E) nvme(OE) nvme_core(OE) virtio_net(E) net_failover(E) virtio_console(E) failover(E) mlxbf_tmfifo(OE) mlx_compat(OE) virtio(E) t10_pi(E) sbsa_gwdt(E) mlxbf_bootctl(OE) sdhci_of_dwcmshc(OE) virtio_ring(E) mmcblk0rpmb: mmc0:0001 Y29128 4.00 MiB, chardev (245:0) CPU: 8 PID: 240 Comm: kworker/8:1H Tainted: G OE 5.15.120.bsk.business.6-arm64 #5.15.120.bsk.business.6 Hardware name: https://www.mellanox.com BlueField-3 DPU/BlueField-3 DPU, BIOS 4.9.2.13576 Mar 18 2025 Workqueue: kblockd blk_mq_run_work_fn pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : mmc_start_request+0xb4/0xc4 lr : mmc_start_request+0x68/0xc4sp :
ffff80000932ba90
x29:ffff80000932ba90x28:ffff000081b3b308x27: 0000000000000000
x26: 0000000000000001 x25: 0000000000000000 x24:ffff000082aec000
x23:ffff000082485800x22:ffff000082aec000x21: 0000000000000000
x20:ffff000081b3b3d8x19:ffff000082aec000x18: 0000000000000000
x17: 0000000000000000 x16:ffffbab212913c40x15: 0000000000000000
x14: 0000000000000000 x13: 0000000000000038 x12:ffff00008f06b000
x11:7f7f7f7f7f7f7f7fx10:ffffbab2144acbc8x9 :ffffbab2130ad368x8 :ffff000081b3b548x7 :ffff000082aec000x6 : 0000000000000000 x5 :ffff000081b3b458x4 :ffff00008a4b5e80x3 :ffff0000824859b0x2 : 0000000000000000 x1 :ffff000081b3b4c8x0 : 0000000000000020
Quoted 15 lines
Call trace: mmc_start_request+0xb4/0xc4 mmc_blk_mq_issue_rq+0x310/0x8fc mmc_mq_queue_rq+0x154/0x3e0 blk_mq_dispatch_rq_list+0x13c/0xa44 blk_mq_do_dispatch_sched+0x2cc/0x33c __blk_mq_sched_dispatch_requests+0x154/0x1b0 blk_mq_sched_dispatch_requests+0x40/0x80 __blk_mq_run_hw_queue+0x58/0xa0 blk_mq_run_work_fn+0x28/0x34 process_one_work+0x1f8/0x4c0 worker_thread+0x180/0x580 kthread+0x128/0x13c kthread_return_to_user+0x0/0x10 ---[ end trace fc3df73f08f7c8ee ]---
Not much to go on
Quoted 4 lines
I agree the bitfield usage adds complexity. I can work on a follow-up to convert the retune-related flags to bools if that’s the preferred direction.
There are 2 suspect cases that I notice:
1. host->claimed in __mmc_claim_host()
The block driver allows more than 1 request to be inflight, which means __mmc_claim_host() could itself overwrite other bitfields in a asynchronous context if the host has alrady been claimed for the same ctx.
int __mmc_claim_host(struct mmc_host *host, struct mmc_ctx *ctx,
atomic_t *abort)
{
struct task_struct *task = ctx ? NULL : current;
DECLARE_WAITQUEUE(wait, current);
unsigned long flags;
int stop;
bool pm = false;
might_sleep();
add_wait_queue(&host->wq, &wait);
spin_lock_irqsave(&host->lock, flags);
while (1) {
set_current_state(TASK_UNINTERRUPTIBLE);
stop = abort ? atomic_read(abort) : 0;
if (stop || !host->claimed || mmc_ctx_matches(host, ctx, task))
break;
spin_unlock_irqrestore(&host->lock, flags);
schedule();
spin_lock_irqsave(&host->lock, flags);
}
set_current_state(TASK_RUNNING);
if (!stop) {
host->claimed = 1; <- overwrites other bitfields
mmc_ctx_set_claimer(host, ctx, task);
host->claim_cnt += 1;
if (host->claim_cnt == 1)
pm = true;
} else
wake_up(&host->wq);
spin_unlock_irqrestore(&host->lock, flags);
remove_wait_queue(&host->wq, &wait);
if (pm)
pm_runtime_get_sync(mmc_dev(host));
return stop;
}2. host->retune_now in mmc_mq_queue_rq()
For the same reason as 1, host->retune_now update can overwrite other bitfields in an asynchronous context.
if (host->cqe_enabled) {
host->retune_now = host->need_retune && cqe_retune_ok &&
!host->hold_retune;
}I suggest changing bitfields to bool for the above 2 cases and also in cases that are relatively difficult to fully understand:
include/linux/mmc/host.h +5 -4
@@ -486,14 +486,12 @@ struct mmc_host {struct mmc_ios ios;/* current io bus settings */ +bool claimed;/* host exclusively claimed */ +/* group bitfields together to minimize padding */ unsigned int use_spi_crc:1; - unsigned int claimed:1; /* host exclusively claimed */ unsigned int doing_init_tune:1; /* initial tuning in progress */ - unsigned int can_retune:1; /* re-tuning can be used */ unsigned int doing_retune:1; /* re-tuning in progress */ - unsigned int retune_now:1; /* do re-tuning at next req */ - unsigned int retune_paused:1; /* re-tuning is temporarily disabled */ unsigned int retune_crc_disable:1; /* don't trigger retune upon crc */ unsigned int can_dma_map_merge:1; /* merging can be used */ unsigned int vqmmc_enabled:1; /* vqmmc regulator is enabled */
@@ -508,6 +506,9 @@ struct mmc_host {int rescan_disable;/* disable card detection */ int rescan_entered;/* used with nonremovable devices */ +bool can_retune;/* re-tuning can be used */ +bool retune_now;/* do re-tuning at next req */ +bool retune_paused;/* re-tuning is temporarily disabled */ int need_retune;/* re-tuning is needed */ int hold_retune;/* hold off re-tuning */ unsigned int retune_period;/* re-tuning period in secs */
For which fixes tags could be:
Fixes: 6c0cedd1ef952 "mmc: core: Introduce host claiming by context" Fixes: 1e8e55b67030c "mmc: block: Add CQE support"
Quoted 619 lines
Thanks,
PengheQuoted 306 lines · Adrian Hunter
External email: Use caution opening links or attachments
Quoted 5 lines · Penghe Geng
The
host->claimedflag shares a bitfield storage word with several retune flags (retune_now,retune_paused,can_retune,doing_retune,doing_init_tune). Updating those flags withouthost->lockcan RMW the shared word and clear claimed, triggering spuriousWARN_ON(!host->claimed).Thanks for finding this!
The design is that those members are protected by the
host->claimedlock itself.mmc operations are primarily single-threaded, protected by the
host->claimedlock, although the block driver does allow multiple transfers at the same time in some cases.There are also other contexts like interrupt handlers.
Can you provide some information about when
WARN_ON(!host->claimed)is being hit? Including the stack dump?
What kernel version?
Is it eMMC, SDIO or SD card?
Is CQE being used?
Are there any I/O errors happening also?
What controller driver is it?In any case, the use of bit fields seems to add complexity unnecessarily, so we should consider converting some or all of them to bool.
Quoted 273 lines
Serialize all retune bitfield updates with
host->lock. Provide lockless __mmc_retune_* helpers so callers that already holdhost->lockcan avoid deadlocks while public wrappers serialize updates. Also protectdoing_init_tuneand the CQEretune_nowassignment withhost->lock.Fixes dfa13ebbe334 ("mmc: host: Add facility to support re-tuning")Signed-off-by Penghe Geng <[email protected]>--- drivers/mmc/core/host.c | 60 +++++++++++++++++++++++++++++++--------- drivers/mmc/core/host.h | 35 ++++++++++++++++++++++- drivers/mmc/core/mmc.c | 6 ++++ drivers/mmc/core/queue.c | 3 ++ include/linux/mmc/host.h | 4 +++ 5 files changed, 94 insertions(+), 14 deletions(-)drivers/mmc/core/host.c +47
-13@@ -109,7 +109,11 @@ void mmc_unregister_host_class(void)*/ void mmc_retune_enable (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->can_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->retune_period)mod_timer (&host->retune_timer, jiffies + host->retune_period *HZ );@@ -121,18 +125,31 @@ void mmc_retune_enable(struct mmc_host *host)*/ void mmc_retune_pause (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (!host->retune_paused) { host->retune_paused =1 ; -mmc_retune_hold (host); +__mmc_retune_hold (host); } +spin_unlock_irqrestore (&host->lock, flags); }EXPORT_SYMBOL (mmc_retune_pause);void mmc_retune_unpause (struct mmc_host *host) { +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags);if (host->retune_paused) { host->retune_paused =0 ; -mmc_retune_release (host); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released) +WARN_ON (1 ); + }else { +spin_unlock_irqrestore (&host->lock, flags); } }EXPORT_SYMBOL (mmc_retune_unpause);@@ -145,8 +162,12 @@ EXPORT_SYMBOL(mmc_retune_unpause);*/ void mmc_retune_disable (struct mmc_host *host) { +unsigned long flags; +mmc_retune_unpause (host); +spin_lock_irqsave (&host->lock, flags); host->can_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);timer_delete_sync (&host->retune_timer);mmc_retune_clear (host); }@@ -159,16 +180,22 @@ EXPORT_SYMBOL(mmc_retune_timer_stop);void mmc_retune_hold (struct mmc_host *host) { -if (!host->hold_retune) - host->retune_now =1 ; - host->hold_retune +=1 ; +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_hold (host); +spin_unlock_irqrestore (&host->lock, flags); }void mmc_retune_release (struct mmc_host *host) { -if (host->hold_retune) - host->hold_retune -=1 ; -else +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released)WARN_ON (1 ); }EXPORT_SYMBOL (mmc_retune_release);@@ -177,18 +204,23 @@ int mmc_retune(struct mmc_host *host) {bool return_to_hs400 = false;int err; +unsigned long flags; -if (host->retune_now) - host->retune_now =0 ; -else +spin_lock_irqsave (&host->lock, flags); +if (!host->retune_now) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } + host->retune_now =0 ; -if (!host->need_retune || host->doing_retune || !host->card) +if (!host->need_retune || host->doing_retune || !host->card) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } host->need_retune =0 ; - host->doing_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->ios.timing ==MMC_TIMING_MMC_HS400 ) { err =mmc_hs400_to_hs200 (host->card);@@ -205,7 +237,9 @@ int mmc_retune(struct mmc_host *host)if (return_to_hs400) err =mmc_hs200_to_hs400 (host->card); out: +spin_lock_irqsave (&host->lock, flags); host->doing_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);return err; }drivers/mmc/core/host.h +34
-1@@ -21,22 +21,55 @@ int mmc_retune(struct mmc_host *host);void mmc_retune_pause (struct mmc_host *host);void mmc_retune_unpause (struct mmc_host *host); -static inline void mmc_retune_clear (struct mmc_host *host) +static inline void __mmc_retune_clear (struct mmc_host *host) { host->retune_now =0 ; host->need_retune =0 ; } +static inline void mmc_retune_clear (struct mmc_host *host) +{ +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_clear (host); +spin_unlock_irqrestore (&host->lock, flags); +} + +static inline void __mmc_retune_hold (struct mmc_host *host) +{ +if (!host->hold_retune) + host->retune_now =1 ; + host->hold_retune +=1 ; +} + +static inline bool __mmc_retune_release (struct mmc_host *host) +{ +if (host->hold_retune) { + host->hold_retune -=1 ; +return true; + } +return false; +} +static inline void mmc_retune_hold_now (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->retune_now =0 ; host->hold_retune +=1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline void mmc_retune_recheck (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->hold_retune <=1 ) host->retune_now =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline int mmc_host_can_cmd23 (struct mmc_host *host)drivers/mmc/core/mmc.c +6
-0@@ -1820,13 +1820,19 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, goto free_card;if (mmc_card_hs200 (card)) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =1 ; +spin_unlock_irqrestore (&host->lock, flags); err =mmc_hs200_tuning (card);if (!err) err =mmc_select_hs400 (card); +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =0 ; +spin_unlock_irqrestore (&host->lock, flags);if (err) goto free_card;drivers/mmc/core/queue.c +3
-0@@ -237,6 +237,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,enum mmc_issue_type issue_type;enum mmc_issued issued;bool get_card, cqe_retune_ok; +unsigned long flags;blk_status_t ret;if (mmc_card_removed (mq->card)) {@@ -297,8 +298,10 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,mmc_get_card (card, &mq->ctx);if (host->cqe_enabled) { +spin_lock_irqsave (&host->lock, flags); host->retune_now = host->need_retune && cqe_retune_ok && !host->hold_retune; +spin_unlock_irqrestore (&host->lock, flags); }blk_mq_start_request (req);include/linux/mmc/host.h +4
-0@@ -713,8 +713,12 @@ void mmc_retune_timer_stop(struct mmc_host *host);static inline void mmc_retune_needed (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->can_retune) host->need_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline bool mmc_can_retune (struct mmc_host *host)Quoted 306 lines · Adrian Hunter
External email: Use caution opening links or attachments
Quoted 5 lines · Penghe Geng
The
host->claimedflag shares a bitfield storage word with several retune flags (retune_now,retune_paused,can_retune,doing_retune,doing_init_tune). Updating those flags withouthost->lockcan RMW the shared word and clear claimed, triggering spuriousWARN_ON(!host->claimed).Thanks for finding this!
The design is that those members are protected by the
host->claimedlock itself.mmc operations are primarily single-threaded, protected by the
host->claimedlock, although the block driver does allow multiple transfers at the same time in some cases.There are also other contexts like interrupt handlers.
Can you provide some information about when
WARN_ON(!host->claimed)is being hit? Including the stack dump?
What kernel version?
Is it eMMC, SDIO or SD card?
Is CQE being used?
Are there any I/O errors happening also?
What controller driver is it?In any case, the use of bit fields seems to add complexity unnecessarily, so we should consider converting some or all of them to bool.
Quoted 273 lines
Serialize all retune bitfield updates with
host->lock. Provide lockless __mmc_retune_* helpers so callers that already holdhost->lockcan avoid deadlocks while public wrappers serialize updates. Also protectdoing_init_tuneand the CQEretune_nowassignment withhost->lock.Fixes dfa13ebbe334 ("mmc: host: Add facility to support re-tuning")Signed-off-by Penghe Geng <[email protected]>--- drivers/mmc/core/host.c | 60 +++++++++++++++++++++++++++++++--------- drivers/mmc/core/host.h | 35 ++++++++++++++++++++++- drivers/mmc/core/mmc.c | 6 ++++ drivers/mmc/core/queue.c | 3 ++ include/linux/mmc/host.h | 4 +++ 5 files changed, 94 insertions(+), 14 deletions(-)drivers/mmc/core/host.c +47
-13@@ -109,7 +109,11 @@ void mmc_unregister_host_class(void)*/ void mmc_retune_enable (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->can_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->retune_period)mod_timer (&host->retune_timer, jiffies + host->retune_period *HZ );@@ -121,18 +125,31 @@ void mmc_retune_enable(struct mmc_host *host)*/ void mmc_retune_pause (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (!host->retune_paused) { host->retune_paused =1 ; -mmc_retune_hold (host); +__mmc_retune_hold (host); } +spin_unlock_irqrestore (&host->lock, flags); }EXPORT_SYMBOL (mmc_retune_pause);void mmc_retune_unpause (struct mmc_host *host) { +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags);if (host->retune_paused) { host->retune_paused =0 ; -mmc_retune_release (host); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released) +WARN_ON (1 ); + }else { +spin_unlock_irqrestore (&host->lock, flags); } }EXPORT_SYMBOL (mmc_retune_unpause);@@ -145,8 +162,12 @@ EXPORT_SYMBOL(mmc_retune_unpause);*/ void mmc_retune_disable (struct mmc_host *host) { +unsigned long flags; +mmc_retune_unpause (host); +spin_lock_irqsave (&host->lock, flags); host->can_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);timer_delete_sync (&host->retune_timer);mmc_retune_clear (host); }@@ -159,16 +180,22 @@ EXPORT_SYMBOL(mmc_retune_timer_stop);void mmc_retune_hold (struct mmc_host *host) { -if (!host->hold_retune) - host->retune_now =1 ; - host->hold_retune +=1 ; +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_hold (host); +spin_unlock_irqrestore (&host->lock, flags); }void mmc_retune_release (struct mmc_host *host) { -if (host->hold_retune) - host->hold_retune -=1 ; -else +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released)WARN_ON (1 ); }EXPORT_SYMBOL (mmc_retune_release);@@ -177,18 +204,23 @@ int mmc_retune(struct mmc_host *host) {bool return_to_hs400 = false;int err; +unsigned long flags; -if (host->retune_now) - host->retune_now =0 ; -else +spin_lock_irqsave (&host->lock, flags); +if (!host->retune_now) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } + host->retune_now =0 ; -if (!host->need_retune || host->doing_retune || !host->card) +if (!host->need_retune || host->doing_retune || !host->card) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } host->need_retune =0 ; - host->doing_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->ios.timing ==MMC_TIMING_MMC_HS400 ) { err =mmc_hs400_to_hs200 (host->card);@@ -205,7 +237,9 @@ int mmc_retune(struct mmc_host *host)if (return_to_hs400) err =mmc_hs200_to_hs400 (host->card); out: +spin_lock_irqsave (&host->lock, flags); host->doing_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);return err; }drivers/mmc/core/host.h +34
-1@@ -21,22 +21,55 @@ int mmc_retune(struct mmc_host *host);void mmc_retune_pause (struct mmc_host *host);void mmc_retune_unpause (struct mmc_host *host); -static inline void mmc_retune_clear (struct mmc_host *host) +static inline void __mmc_retune_clear (struct mmc_host *host) { host->retune_now =0 ; host->need_retune =0 ; } +static inline void mmc_retune_clear (struct mmc_host *host) +{ +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_clear (host); +spin_unlock_irqrestore (&host->lock, flags); +} + +static inline void __mmc_retune_hold (struct mmc_host *host) +{ +if (!host->hold_retune) + host->retune_now =1 ; + host->hold_retune +=1 ; +} + +static inline bool __mmc_retune_release (struct mmc_host *host) +{ +if (host->hold_retune) { + host->hold_retune -=1 ; +return true; + } +return false; +} +static inline void mmc_retune_hold_now (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->retune_now =0 ; host->hold_retune +=1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline void mmc_retune_recheck (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->hold_retune <=1 ) host->retune_now =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline int mmc_host_can_cmd23 (struct mmc_host *host)drivers/mmc/core/mmc.c +6
-0@@ -1820,13 +1820,19 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, goto free_card;if (mmc_card_hs200 (card)) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =1 ; +spin_unlock_irqrestore (&host->lock, flags); err =mmc_hs200_tuning (card);if (!err) err =mmc_select_hs400 (card); +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =0 ; +spin_unlock_irqrestore (&host->lock, flags);if (err) goto free_card;drivers/mmc/core/queue.c +3
-0@@ -237,6 +237,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,enum mmc_issue_type issue_type;enum mmc_issued issued;bool get_card, cqe_retune_ok; +unsigned long flags;blk_status_t ret;if (mmc_card_removed (mq->card)) {@@ -297,8 +298,10 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,mmc_get_card (card, &mq->ctx);if (host->cqe_enabled) { +spin_lock_irqsave (&host->lock, flags); host->retune_now = host->need_retune && cqe_retune_ok && !host->hold_retune; +spin_unlock_irqrestore (&host->lock, flags); }blk_mq_start_request (req);include/linux/mmc/host.h +4
-0@@ -713,8 +713,12 @@ void mmc_retune_timer_stop(struct mmc_host *host);static inline void mmc_retune_needed (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->can_retune) host->need_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline bool mmc_can_retune (struct mmc_host *host)
Hi Adrian,
Apologies for the late reply, and thanks for the analysis. I’ve sent a v2 patch that moves claimed and the retune control flags out of the shared bitfield word, per your suggestion.
Patch subject:
[PATCH mmc v2] mmc: core: Avoid bitfield RMW for claim/retune flags
Thanks,
Penghe Geng
Quoted 795 lines · Adrian Hunter
External email: Use caution opening links or attachments
Quoted 50 lines · Penghe Geng
Hi Adrian,
Thanks for the feedback. Below are the details you asked for.
Kernel versions:
- Seen on 5.15.120.bsk.business.6‑arm64 (custom tree).
- Also observed on 6.1.0‑11‑arm64 (Debian 6.1.38‑4).Media: eMMC
Controllers:
- BlueField‑2: Synopsys DesignWare MMC (drivers/mmc/host/dw_mmc-bluefield.c)
- BlueField‑3: Synopsys DWC MSHC (drivers/mmc/host/sdhci-of-dwcmshc.cin‑tree) and also with OOT sdhci-of-dwcmshc-bf3CQE:
- Not enabled at runtime.CONFIG_MMC_CQHCI=m.
- lsmod | grep cq is empty by default.
- modprobe cqhci loads with 0 users and no CQE/CQHCI enable messages in dmesg. So CQE is not in use by the active host.I/O errors:
- None observed around the WARN.Repro:
- Intermittent, mostly during boot under stress.
- Roughly 0.1–1% depending on distro/platform.Example stack (BF3, in-tree sdhci-of-dwcmshc):
------------[ cut here ]------------ mmcblk0boot1: mmc0:0001 Y29128 31.9 MiB WARNING: CPU: 8 PID: 240 at drivers/mmc/core/core.c:349 mmc_start_request+0xb4/0xc4 Modules linked in: crc16(E) mbcache(E) jbd2(E) nvme_tcp(OE) nvme_rdma(OE) rdma_cm(OE) iw_cm(OE) ib_cm(OE) ib_core(OE) nvme_fabrics(OE) configfs(E) nls_ascii(E) nls_cp437(E) nls_cp850(E) msdos(E) efivarfs(E) nvme(OE) nvme_core(OE) virtio_net(E) net_failover(E) virtio_console(E) failover(E) mlxbf_tmfifo(OE) mlx_compat(OE) virtio(E) t10_pi(E) sbsa_gwdt(E) mlxbf_bootctl(OE) sdhci_of_dwcmshc(OE) virtio_ring(E) mmcblk0rpmb: mmc0:0001 Y29128 4.00 MiB, chardev (245:0) CPU: 8 PID: 240 Comm: kworker/8:1H Tainted: G OE 5.15.120.bsk.business.6-arm64 #5.15.120.bsk.business.6 Hardware name: https://www.mellanox.com BlueField-3 DPU/BlueField-3 DPU, BIOS 4.9.2.13576 Mar 18 2025 Workqueue: kblockd blk_mq_run_work_fn pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : mmc_start_request+0xb4/0xc4 lr : mmc_start_request+0x68/0xc4sp :
ffff80000932ba90
x29:ffff80000932ba90x28:ffff000081b3b308x27: 0000000000000000
x26: 0000000000000001 x25: 0000000000000000 x24:ffff000082aec000
x23:ffff000082485800x22:ffff000082aec000x21: 0000000000000000
x20:ffff000081b3b3d8x19:ffff000082aec000x18: 0000000000000000
x17: 0000000000000000 x16:ffffbab212913c40x15: 0000000000000000
x14: 0000000000000000 x13: 0000000000000038 x12:ffff00008f06b000
x11:7f7f7f7f7f7f7f7fx10:ffffbab2144acbc8x9 :ffffbab2130ad368x8 :ffff000081b3b548x7 :ffff000082aec000x6 : 0000000000000000 x5 :ffff000081b3b458x4 :ffff00008a4b5e80x3 :ffff0000824859b0x2 : 0000000000000000 x1 :ffff000081b3b4c8x0 : 0000000000000020Quoted 15 lines
Call trace: mmc_start_request+0xb4/0xc4 mmc_blk_mq_issue_rq+0x310/0x8fc mmc_mq_queue_rq+0x154/0x3e0 blk_mq_dispatch_rq_list+0x13c/0xa44 blk_mq_do_dispatch_sched+0x2cc/0x33c __blk_mq_sched_dispatch_requests+0x154/0x1b0 blk_mq_sched_dispatch_requests+0x40/0x80 __blk_mq_run_hw_queue+0x58/0xa0 blk_mq_run_work_fn+0x28/0x34 process_one_work+0x1f8/0x4c0 worker_thread+0x180/0x580 kthread+0x128/0x13c kthread_return_to_user+0x0/0x10 ---[ end trace fc3df73f08f7c8ee ]---Not much to go on
Quoted 4 lines
I agree the bitfield usage adds complexity. I can work on a follow-up to convert the retune-related flags to bools if that’s the preferred direction.
There are 2 suspect cases that I notice:
1.
host->claimedin__mmc_claim_host()The block driver allows more than 1 request to be inflight, which means
__mmc_claim_host()could itself overwrite other bitfields in a asynchronous context if the host has alrady been claimed for the same ctx.int __mmc_claim_host(struct
mmc_host*host, structmmc_ctx*ctx,
atomic_t*abort){ struct task_struct *task = ctx ? NULL : current; DECLARE_WAITQUEUE(wait, current); unsigned long flags; int stop; bool pm = false; might_sleep(); add_wait_queue(&host->wq, &wait); spin_lock_irqsave(&host->lock, flags); while (1) { set_current_state(TASK_UNINTERRUPTIBLE); stop = abort ? atomic_read(abort) : 0; if (stop || !host->claimed || mmc_ctx_matches(host, ctx, task)) break; spin_unlock_irqrestore(&host->lock, flags); schedule(); spin_lock_irqsave(&host->lock, flags); } set_current_state(TASK_RUNNING); if (!stop) { host->claimed = 1; <- overwrites other bitfields mmc_ctx_set_claimer(host, ctx, task); host->claim_cnt += 1; if (host->claim_cnt == 1) pm = true; } else wake_up(&host->wq); spin_unlock_irqrestore(&host->lock, flags); remove_wait_queue(&host->wq, &wait); if (pm) pm_runtime_get_sync(mmc_dev(host)); return stop; }2.
host->retune_nowinmmc_mq_queue_rq()For the same reason as 1,
host->retune_nowupdate can overwrite other bitfields in an asynchronous context.if (host->cqe_enabled) { host->retune_now = host->need_retune && cqe_retune_ok && !host->hold_retune; }I suggest changing bitfields to bool for the above 2 cases and also in cases that are relatively difficult to fully understand:
include/linux/mmc/host.h +5
-4@@ -486,14 +486,12 @@ struct mmc_host {struct mmc_ios ios;/* current io bus settings */ +bool claimed;/* host exclusively claimed */ +/* group bitfields together to minimize padding */ unsigned int use_spi_crc:1; - unsigned int claimed:1; /* host exclusively claimed */ unsigned int doing_init_tune:1; /* initial tuning in progress */ - unsigned int can_retune:1; /* re-tuning can be used */ unsigned int doing_retune:1; /* re-tuning in progress */ - unsigned int retune_now:1; /* do re-tuning at next req */ - unsigned int retune_paused:1; /* re-tuning is temporarily disabled */ unsigned int retune_crc_disable:1; /* don't trigger retune upon crc */ unsigned int can_dma_map_merge:1; /* merging can be used */ unsigned int vqmmc_enabled:1; /* vqmmc regulator is enabled */@@ -508,6 +506,9 @@ struct mmc_host {int rescan_disable;/* disable card detection */ int rescan_entered;/* used with nonremovable devices */ +bool can_retune;/* re-tuning can be used */ +bool retune_now;/* do re-tuning at next req */ +bool retune_paused;/* re-tuning is temporarily disabled */ int need_retune;/* re-tuning is needed */ int hold_retune;/* hold off re-tuning */ unsigned int retune_period;/* re-tuning period in secs */ For which fixes tags could be:
Fixes 6c0cedd1ef952 "mmc: core: Introduce host claiming by context"Fixes 1e8e55b67030c "mmc: block: Add CQE support"Quoted 619 lines
Thanks,
PengheQuoted 306 lines · Adrian Hunter
External email: Use caution opening links or attachments
Quoted 5 lines · Penghe Geng
The
host->claimedflag shares a bitfield storage word with several retune flags (retune_now,retune_paused,can_retune,doing_retune,doing_init_tune). Updating those flags withouthost->lockcan RMW the shared word and clear claimed, triggering spuriousWARN_ON(!host->claimed).Thanks for finding this!
The design is that those members are protected by the
host->claimedlock itself.mmc operations are primarily single-threaded, protected by the
host->claimedlock, although the block driver does allow multiple transfers at the same time in some cases.There are also other contexts like interrupt handlers.
Can you provide some information about when
WARN_ON(!host->claimed)is being hit? Including the stack dump?
What kernel version?
Is it eMMC, SDIO or SD card?
Is CQE being used?
Are there any I/O errors happening also?
What controller driver is it?In any case, the use of bit fields seems to add complexity unnecessarily, so we should consider converting some or all of them to bool.
Quoted 273 lines
Serialize all retune bitfield updates with
host->lock. Provide lockless __mmc_retune_* helpers so callers that already holdhost->lockcan avoid deadlocks while public wrappers serialize updates. Also protectdoing_init_tuneand the CQEretune_nowassignment withhost->lock.Fixes dfa13ebbe334 ("mmc: host: Add facility to support re-tuning")Signed-off-by Penghe Geng <[email protected]>--- drivers/mmc/core/host.c | 60 +++++++++++++++++++++++++++++++--------- drivers/mmc/core/host.h | 35 ++++++++++++++++++++++- drivers/mmc/core/mmc.c | 6 ++++ drivers/mmc/core/queue.c | 3 ++ include/linux/mmc/host.h | 4 +++ 5 files changed, 94 insertions(+), 14 deletions(-)drivers/mmc/core/host.c +47
-13@@ -109,7 +109,11 @@ void mmc_unregister_host_class(void)*/ void mmc_retune_enable (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->can_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->retune_period)mod_timer (&host->retune_timer, jiffies + host->retune_period *HZ );@@ -121,18 +125,31 @@ void mmc_retune_enable(struct mmc_host *host)*/ void mmc_retune_pause (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (!host->retune_paused) { host->retune_paused =1 ; -mmc_retune_hold (host); +__mmc_retune_hold (host); } +spin_unlock_irqrestore (&host->lock, flags); }EXPORT_SYMBOL (mmc_retune_pause);void mmc_retune_unpause (struct mmc_host *host) { +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags);if (host->retune_paused) { host->retune_paused =0 ; -mmc_retune_release (host); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released) +WARN_ON (1 ); + }else { +spin_unlock_irqrestore (&host->lock, flags); } }EXPORT_SYMBOL (mmc_retune_unpause);@@ -145,8 +162,12 @@ EXPORT_SYMBOL(mmc_retune_unpause);*/ void mmc_retune_disable (struct mmc_host *host) { +unsigned long flags; +mmc_retune_unpause (host); +spin_lock_irqsave (&host->lock, flags); host->can_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);timer_delete_sync (&host->retune_timer);mmc_retune_clear (host); }@@ -159,16 +180,22 @@ EXPORT_SYMBOL(mmc_retune_timer_stop);void mmc_retune_hold (struct mmc_host *host) { -if (!host->hold_retune) - host->retune_now =1 ; - host->hold_retune +=1 ; +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_hold (host); +spin_unlock_irqrestore (&host->lock, flags); }void mmc_retune_release (struct mmc_host *host) { -if (host->hold_retune) - host->hold_retune -=1 ; -else +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released)WARN_ON (1 ); }EXPORT_SYMBOL (mmc_retune_release);@@ -177,18 +204,23 @@ int mmc_retune(struct mmc_host *host) {bool return_to_hs400 = false;int err; +unsigned long flags; -if (host->retune_now) - host->retune_now =0 ; -else +spin_lock_irqsave (&host->lock, flags); +if (!host->retune_now) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } + host->retune_now =0 ; -if (!host->need_retune || host->doing_retune || !host->card) +if (!host->need_retune || host->doing_retune || !host->card) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } host->need_retune =0 ; - host->doing_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->ios.timing ==MMC_TIMING_MMC_HS400 ) { err =mmc_hs400_to_hs200 (host->card);@@ -205,7 +237,9 @@ int mmc_retune(struct mmc_host *host)if (return_to_hs400) err =mmc_hs200_to_hs400 (host->card); out: +spin_lock_irqsave (&host->lock, flags); host->doing_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);return err; }drivers/mmc/core/host.h +34
-1@@ -21,22 +21,55 @@ int mmc_retune(struct mmc_host *host);void mmc_retune_pause (struct mmc_host *host);void mmc_retune_unpause (struct mmc_host *host); -static inline void mmc_retune_clear (struct mmc_host *host) +static inline void __mmc_retune_clear (struct mmc_host *host) { host->retune_now =0 ; host->need_retune =0 ; } +static inline void mmc_retune_clear (struct mmc_host *host) +{ +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_clear (host); +spin_unlock_irqrestore (&host->lock, flags); +} + +static inline void __mmc_retune_hold (struct mmc_host *host) +{ +if (!host->hold_retune) + host->retune_now =1 ; + host->hold_retune +=1 ; +} + +static inline bool __mmc_retune_release (struct mmc_host *host) +{ +if (host->hold_retune) { + host->hold_retune -=1 ; +return true; + } +return false; +} +static inline void mmc_retune_hold_now (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->retune_now =0 ; host->hold_retune +=1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline void mmc_retune_recheck (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->hold_retune <=1 ) host->retune_now =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline int mmc_host_can_cmd23 (struct mmc_host *host)drivers/mmc/core/mmc.c +6
-0@@ -1820,13 +1820,19 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, goto free_card;if (mmc_card_hs200 (card)) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =1 ; +spin_unlock_irqrestore (&host->lock, flags); err =mmc_hs200_tuning (card);if (!err) err =mmc_select_hs400 (card); +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =0 ; +spin_unlock_irqrestore (&host->lock, flags);if (err) goto free_card;drivers/mmc/core/queue.c +3
-0@@ -237,6 +237,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,enum mmc_issue_type issue_type;enum mmc_issued issued;bool get_card, cqe_retune_ok; +unsigned long flags;blk_status_t ret;if (mmc_card_removed (mq->card)) {@@ -297,8 +298,10 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,mmc_get_card (card, &mq->ctx);if (host->cqe_enabled) { +spin_lock_irqsave (&host->lock, flags); host->retune_now = host->need_retune && cqe_retune_ok && !host->hold_retune; +spin_unlock_irqrestore (&host->lock, flags); }blk_mq_start_request (req);include/linux/mmc/host.h +4
-0@@ -713,8 +713,12 @@ void mmc_retune_timer_stop(struct mmc_host *host);static inline void mmc_retune_needed (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->can_retune) host->need_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline bool mmc_can_retune (struct mmc_host *host)Quoted 306 lines · Adrian Hunter
External email: Use caution opening links or attachments
Quoted 5 lines · Penghe Geng
The
host->claimedflag shares a bitfield storage word with several retune flags (retune_now,retune_paused,can_retune,doing_retune,doing_init_tune). Updating those flags withouthost->lockcan RMW the shared word and clear claimed, triggering spuriousWARN_ON(!host->claimed).Thanks for finding this!
The design is that those members are protected by the
host->claimedlock itself.mmc operations are primarily single-threaded, protected by the
host->claimedlock, although the block driver does allow multiple transfers at the same time in some cases.There are also other contexts like interrupt handlers.
Can you provide some information about when
WARN_ON(!host->claimed)is being hit? Including the stack dump?
What kernel version?
Is it eMMC, SDIO or SD card?
Is CQE being used?
Are there any I/O errors happening also?
What controller driver is it?In any case, the use of bit fields seems to add complexity unnecessarily, so we should consider converting some or all of them to bool.
Quoted 273 lines
Serialize all retune bitfield updates with
host->lock. Provide lockless __mmc_retune_* helpers so callers that already holdhost->lockcan avoid deadlocks while public wrappers serialize updates. Also protectdoing_init_tuneand the CQEretune_nowassignment withhost->lock.Fixes dfa13ebbe334 ("mmc: host: Add facility to support re-tuning")Signed-off-by Penghe Geng <[email protected]>--- drivers/mmc/core/host.c | 60 +++++++++++++++++++++++++++++++--------- drivers/mmc/core/host.h | 35 ++++++++++++++++++++++- drivers/mmc/core/mmc.c | 6 ++++ drivers/mmc/core/queue.c | 3 ++ include/linux/mmc/host.h | 4 +++ 5 files changed, 94 insertions(+), 14 deletions(-)drivers/mmc/core/host.c +47
-13@@ -109,7 +109,11 @@ void mmc_unregister_host_class(void)*/ void mmc_retune_enable (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->can_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->retune_period)mod_timer (&host->retune_timer, jiffies + host->retune_period *HZ );@@ -121,18 +125,31 @@ void mmc_retune_enable(struct mmc_host *host)*/ void mmc_retune_pause (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (!host->retune_paused) { host->retune_paused =1 ; -mmc_retune_hold (host); +__mmc_retune_hold (host); } +spin_unlock_irqrestore (&host->lock, flags); }EXPORT_SYMBOL (mmc_retune_pause);void mmc_retune_unpause (struct mmc_host *host) { +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags);if (host->retune_paused) { host->retune_paused =0 ; -mmc_retune_release (host); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released) +WARN_ON (1 ); + }else { +spin_unlock_irqrestore (&host->lock, flags); } }EXPORT_SYMBOL (mmc_retune_unpause);@@ -145,8 +162,12 @@ EXPORT_SYMBOL(mmc_retune_unpause);*/ void mmc_retune_disable (struct mmc_host *host) { +unsigned long flags; +mmc_retune_unpause (host); +spin_lock_irqsave (&host->lock, flags); host->can_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);timer_delete_sync (&host->retune_timer);mmc_retune_clear (host); }@@ -159,16 +180,22 @@ EXPORT_SYMBOL(mmc_retune_timer_stop);void mmc_retune_hold (struct mmc_host *host) { -if (!host->hold_retune) - host->retune_now =1 ; - host->hold_retune +=1 ; +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_hold (host); +spin_unlock_irqrestore (&host->lock, flags); }void mmc_retune_release (struct mmc_host *host) { -if (host->hold_retune) - host->hold_retune -=1 ; -else +unsigned long flags; +bool released; + +spin_lock_irqsave (&host->lock, flags); + released =__mmc_retune_release (host); +spin_unlock_irqrestore (&host->lock, flags); +if (!released)WARN_ON (1 ); }EXPORT_SYMBOL (mmc_retune_release);@@ -177,18 +204,23 @@ int mmc_retune(struct mmc_host *host) {bool return_to_hs400 = false;int err; +unsigned long flags; -if (host->retune_now) - host->retune_now =0 ; -else +spin_lock_irqsave (&host->lock, flags); +if (!host->retune_now) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } + host->retune_now =0 ; -if (!host->need_retune || host->doing_retune || !host->card) +if (!host->need_retune || host->doing_retune || !host->card) { +spin_unlock_irqrestore (&host->lock, flags);return 0 ; + } host->need_retune =0 ; - host->doing_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags);if (host->ios.timing ==MMC_TIMING_MMC_HS400 ) { err =mmc_hs400_to_hs200 (host->card);@@ -205,7 +237,9 @@ int mmc_retune(struct mmc_host *host)if (return_to_hs400) err =mmc_hs200_to_hs400 (host->card); out: +spin_lock_irqsave (&host->lock, flags); host->doing_retune =0 ; +spin_unlock_irqrestore (&host->lock, flags);return err; }drivers/mmc/core/host.h +34
-1@@ -21,22 +21,55 @@ int mmc_retune(struct mmc_host *host);void mmc_retune_pause (struct mmc_host *host);void mmc_retune_unpause (struct mmc_host *host); -static inline void mmc_retune_clear (struct mmc_host *host) +static inline void __mmc_retune_clear (struct mmc_host *host) { host->retune_now =0 ; host->need_retune =0 ; } +static inline void mmc_retune_clear (struct mmc_host *host) +{ +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); +__mmc_retune_clear (host); +spin_unlock_irqrestore (&host->lock, flags); +} + +static inline void __mmc_retune_hold (struct mmc_host *host) +{ +if (!host->hold_retune) + host->retune_now =1 ; + host->hold_retune +=1 ; +} + +static inline bool __mmc_retune_release (struct mmc_host *host) +{ +if (host->hold_retune) { + host->hold_retune -=1 ; +return true; + } +return false; +} +static inline void mmc_retune_hold_now (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->retune_now =0 ; host->hold_retune +=1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline void mmc_retune_recheck (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->hold_retune <=1 ) host->retune_now =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline int mmc_host_can_cmd23 (struct mmc_host *host)drivers/mmc/core/mmc.c +6
-0@@ -1820,13 +1820,19 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, goto free_card;if (mmc_card_hs200 (card)) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =1 ; +spin_unlock_irqrestore (&host->lock, flags); err =mmc_hs200_tuning (card);if (!err) err =mmc_select_hs400 (card); +spin_lock_irqsave (&host->lock, flags); host->doing_init_tune =0 ; +spin_unlock_irqrestore (&host->lock, flags);if (err) goto free_card;drivers/mmc/core/queue.c +3
-0@@ -237,6 +237,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,enum mmc_issue_type issue_type;enum mmc_issued issued;bool get_card, cqe_retune_ok; +unsigned long flags;blk_status_t ret;if (mmc_card_removed (mq->card)) {@@ -297,8 +298,10 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,mmc_get_card (card, &mq->ctx);if (host->cqe_enabled) { +spin_lock_irqsave (&host->lock, flags); host->retune_now = host->need_retune && cqe_retune_ok && !host->hold_retune; +spin_unlock_irqrestore (&host->lock, flags); }blk_mq_start_request (req);include/linux/mmc/host.h +4
-0@@ -713,8 +713,12 @@ void mmc_retune_timer_stop(struct mmc_host *host);static inline void mmc_retune_needed (struct mmc_host *host) { +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags);if (host->can_retune) host->need_retune =1 ; +spin_unlock_irqrestore (&host->lock, flags); }static inline bool mmc_can_retune (struct mmc_host *host)
PATCH v2 1/1 mmc: core: Avoid bitfield RMW for claim/retune flags
In reply to Penghe GengMove claimed and retune control flags out of the bitfield word to avoid unrelated RMW side effects in asynchronous contexts.
The host->claimed bit shared a word with retune flags. Writes to claimed in __mmc_claim_host() or retune_now in mmc_mq_queue_rq() can overwrite other bits when concurrent updates happen in other contexts, triggering spurious WARN_ON(!host->claimed). Convert claimed, can_retune, retune_now and retune_paused to bool to remove shared-word coupling.
--- include/linux/mmc/host.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
include/linux/mmc/host.h +5 -4
@@ -486,14 +486,12 @@ struct mmc_host {struct mmc_ios ios;/* current io bus settings */ +bool claimed;/* host exclusively claimed */ +/* group bitfields together to minimize padding */ unsigned int use_spi_crc:1; - unsigned int claimed:1; /* host exclusively claimed */ unsigned int doing_init_tune:1; /* initial tuning in progress */ - unsigned int can_retune:1; /* re-tuning can be used */ unsigned int doing_retune:1; /* re-tuning in progress */ - unsigned int retune_now:1; /* do re-tuning at next req */ - unsigned int retune_paused:1; /* re-tuning is temporarily disabled */ unsigned int retune_crc_disable:1; /* don't trigger retune upon crc */ unsigned int can_dma_map_merge:1; /* merging can be used */ unsigned int vqmmc_enabled:1; /* vqmmc regulator is enabled */
@@ -508,6 +506,9 @@ struct mmc_host {int rescan_disable;/* disable card detection */ int rescan_entered;/* used with nonremovable devices */ +bool can_retune;/* re-tuning can be used */ +bool retune_now;/* do re-tuning at next req */ +bool retune_paused;/* re-tuning is temporarily disabled */ int need_retune;/* re-tuning is needed */ int hold_retune;/* hold off re-tuning */ unsigned int retune_period;/* re-tuning period in secs */
Signature · 1 line
2.43.0
Re: [PATCH mmc v2] mmc: core: Avoid bitfield RMW for claim/retune flags
Quoted 9 lines · Penghe Geng
Move claimed and retune control flags out of the bitfield word to avoid unrelated RMW side effects in asynchronous contexts.
The
host->claimedbit shared a word with retune flags. Writes to claimed in__mmc_claim_host()orretune_nowinmmc_mq_queue_rq()can overwrite other bits when concurrent updates happen in other contexts, triggering spuriousWARN_ON(!host->claimed). Convert claimed,can_retune,retune_nowandretune_pausedto bool to remove shared-word coupling.
Quoted 5 lines
Fixes 6c0cedd1ef952 ("mmc: core: Introduce host claiming by context")Fixes 1e8e55b67030c ("mmc: block: Add CQE support")Suggested-by Adrian Hunter <[email protected]>Signed-off-by Penghe Geng <[email protected]>
Quoted 35 lines
--- include/linux/mmc/host.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)include/linux/mmc/host.h +5
-4@@ -486,14 +486,12 @@ struct mmc_host {struct mmc_ios ios;/* current io bus settings */ +bool claimed;/* host exclusively claimed */ +/* group bitfields together to minimize padding */ unsigned int use_spi_crc:1; - unsigned int claimed:1; /* host exclusively claimed */ unsigned int doing_init_tune:1; /* initial tuning in progress */ - unsigned int can_retune:1; /* re-tuning can be used */ unsigned int doing_retune:1; /* re-tuning in progress */ - unsigned int retune_now:1; /* do re-tuning at next req */ - unsigned int retune_paused:1; /* re-tuning is temporarily disabled */ unsigned int retune_crc_disable:1; /* don't trigger retune upon crc */ unsigned int can_dma_map_merge:1; /* merging can be used */ unsigned int vqmmc_enabled:1; /* vqmmc regulator is enabled */@@ -508,6 +506,9 @@ struct mmc_host {int rescan_disable;/* disable card detection */ int rescan_entered;/* used with nonremovable devices */ +bool can_retune;/* re-tuning can be used */ +bool retune_now;/* do re-tuning at next req */ +bool retune_paused;/* re-tuning is temporarily disabled */ int need_retune;/* re-tuning is needed */ int hold_retune;/* hold off re-tuning */ unsigned int retune_period;/* re-tuning period in secs */
Re: [PATCH mmc v2] mmc: core: Avoid bitfield RMW for claim/retune flags
In reply to Penghe GengQuoted 10 lines · Penghe Geng
Move claimed and retune control flags out of the bitfield word to avoid unrelated RMW side effects in asynchronous contexts.
The
host->claimedbit shared a word with retune flags. Writes to claimed in__mmc_claim_host()orretune_nowinmmc_mq_queue_rq()can overwrite other bits when concurrent updates happen in other contexts, triggering spuriousWARN_ON(!host->claimed). Convert claimed,can_retune,retune_nowandretune_pausedto bool to remove shared-word coupling.
Quoted 5 lines
Fixes 6c0cedd1ef952 ("mmc: core: Introduce host claiming by context")Fixes 1e8e55b67030c ("mmc: block: Add CQE support")Suggested-by Adrian Hunter <[email protected]>Signed-off-by Penghe Geng <[email protected]>
Applied for fixes, thanks!
Kind regards
Uffe
Quoted 39 lines
--- include/linux/mmc/host.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)include/linux/mmc/host.h +5
-5@@ -486,14 +486,12 @@ struct mmc_host {struct mmc_ios ios;/* current io bus settings */ +bool claimed;/* host exclusively claimed */ +/* group bitfields together to minimize padding */ unsigned int use_spi_crc:1; - unsigned int claimed:1; /* host exclusively claimed */ unsigned int doing_init_tune:1; /* initial tuning in progress */ - unsigned int can_retune:1; /* re-tuning can be used */ unsigned int doing_retune:1; /* re-tuning in progress */ - unsigned int retune_now:1; /* do re-tuning at next req */ - unsigned int retune_paused:1; /* re-tuning is temporarily disabled */ unsigned int retune_crc_disable:1; /* don't trigger retune upon crc */ unsigned int can_dma_map_merge:1; /* merging can be used */ unsigned int vqmmc_enabled:1; /* vqmmc regulator is enabled */@@ -508,6 +506,9 @@ struct mmc_host {int rescan_disable;/* disable card detection */ int rescan_entered;/* used with nonremovable devices */ +bool can_retune;/* re-tuning can be used */ +bool retune_now;/* do re-tuning at next req */ +bool retune_paused;/* re-tuning is temporarily disabled */ int need_retune;/* re-tuning is needed */ int hold_retune;/* hold off re-tuning */ unsigned int retune_period;/* re-tuning period in secs */ --2.43.0