Re: Issue with Broadcom wireless in 5.2rc1 (was Re: [PATCH] mmc: sdhci: queue work after sdhci_defer_done())

[Adrian Hunter]

On 27/05/19 12:37 PM, Brian Masney wrote:
> On Sun, May 26, 2019 at 03:58:19PM -0400, Brian Masney wrote:
>> 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:
>> 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@xxxxxxxxx>
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@xxxxxxxxx>
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