Re: Issue with Broadcom wireless in 5.2rc1 (was Re: [PATCH] mmc: sdhci: queue work after sdhci_defer_done())
From: Adrian Hunter <adrian.hunter@intel.com>
Date: 2019-05-27 12:08:27
Also in:
linux-arm-msm, linux-mmc, linux-wireless, lkml
Subsystem:
multimedia card (mmc), secure digital (sd) and sdio subsystem, secure digital host controller interface (sdhci) driver, the rest · Maintainers:
Ulf Hansson, Adrian Hunter, Linus Torvalds
On 27/05/19 12:37 PM, Brian Masney wrote:
On Sun, May 26, 2019 at 03:58:19PM -0400, Brian Masney wrote:quoted
I attached a patch that shows how I was able to determine what had already claimed the host.On Mon, May 27, 2019 at 10:48:24AM +0300, Adrian Hunter wrote:quoted
This is because SDHCI is using the IRQ thread to process the SDIO card interrupt (sdio_run_irqs()). When the card driver tries to use the card, it causes interrupts which deadlocks since c07a48c26519 ("mmc: sdhci: Remove finish_tasklet") has moved the tasklet processing to the IRQ thread. I would expect to be able to use the IRQ thread to complete requests, and it is desirable to do so because it is lower latency. Probably, SDHCI should use sdio_signal_irq() which queues a work item, and is what other drivers are doing. I will investigate some more and send a patch.
Please try the patch below:
From: Adrian Hunter <adrian.hunter@intel.com>
Date: Mon, 27 May 2019 14:45:55 +0300
Subject: [PATCH] mmc: sdhci: Fix SDIO IRQ thread deadlock
Since commit c07a48c26519 ("mmc: sdhci: Remove finish_tasklet"), the IRQ
thread might be used to complete requests, but the IRQ thread is also used
to process SDIO card interrupts. This can cause a deadlock when the SDIO
processing tries to access the card since that would also require the IRQ
thread. Change SDHCI to use sdio_signal_irq() to schedule a work item
instead. That also requires implementing the ->ack_sdio_irq() mmc host op.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Fixes: c07a48c26519 ("mmc: sdhci: Remove finish_tasklet")
---
drivers/mmc/host/sdhci.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 97158344b862..0cd5f2ce98df 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c@@ -2137,6 +2137,17 @@ void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable) } EXPORT_SYMBOL_GPL(sdhci_enable_sdio_irq); +static void sdhci_ack_sdio_irq(struct mmc_host *mmc) +{ + struct sdhci_host *host = mmc_priv(mmc); + unsigned long flags; + + spin_lock_irqsave(&host->lock, flags); + if (host->flags & SDHCI_SDIO_IRQ_ENABLED) + sdhci_enable_sdio_irq_nolock(host, true); + spin_unlock_irqrestore(&host->lock, flags); +} + int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios) {
@@ -2585,6 +2596,7 @@ static const struct mmc_host_ops sdhci_ops = { .get_ro = sdhci_get_ro, .hw_reset = sdhci_hw_reset, .enable_sdio_irq = sdhci_enable_sdio_irq, + .ack_sdio_irq = sdhci_ack_sdio_irq, .start_signal_voltage_switch = sdhci_start_signal_voltage_switch, .prepare_hs400_tuning = sdhci_prepare_hs400_tuning, .execute_tuning = sdhci_execute_tuning,
@@ -3087,8 +3099,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id) if ((intmask & SDHCI_INT_CARD_INT) && (host->ier & SDHCI_INT_CARD_INT)) { sdhci_enable_sdio_irq_nolock(host, false); - host->thread_isr |= SDHCI_INT_CARD_INT; - result = IRQ_WAKE_THREAD; + sdio_signal_irq(host->mmc); } intmask &= ~(SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE |
@@ -3160,15 +3171,6 @@ static irqreturn_t sdhci_thread_irq(int irq, void *dev_id) mmc_detect_change(mmc, msecs_to_jiffies(200)); } - if (isr & SDHCI_INT_CARD_INT) { - sdio_run_irqs(host->mmc); - - spin_lock_irqsave(&host->lock, flags); - if (host->flags & SDHCI_SDIO_IRQ_ENABLED) - sdhci_enable_sdio_irq_nolock(host, true); - spin_unlock_irqrestore(&host->lock, flags); - } - return IRQ_HANDLED; }
--
2.17.1