diff mbox series

mmc: tmio_mmc_core: don't claim spurious interrupts

Message ID f2295f99-2354-a2ad-5013-0501ecb0d3a1@cogentembedded.com (mailing list archive)
State New, archived
Headers show
Series mmc: tmio_mmc_core: don't claim spurious interrupts | expand

Commit Message

Sergei Shtylyov Aug. 17, 2018, 11:41 a.m. UTC
I have encountered an interrupt storm  during the eMMC chip probing (and
the chip finally didn't get detected). It  turned out  that U-Boot  left
the DMAC interrupts enabled while the  Linux driver didn't use those.
The SDHI driver's interrupt handler somehow  assumes that, even if a
SDIO interrupt didn't happen, it should return IRQ_HANDLED. I think that
if none of the enabled interrupts happened and got handled, we should
return IRQ_NONE -- that  way the kernel IRQ code recoginizes a spurious
interrupt and masks  it  off pretty quickly...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
The patch is against Ulf Hansson's 'mmc.git' repo's 'fixes' branch.

 drivers/mmc/host/tmio_mmc_core.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Wolfram Sang Aug. 20, 2018, 4:55 p.m. UTC | #1
On Fri, Aug 17, 2018 at 02:41:02PM +0300, Sergei Shtylyov wrote:
> I have encountered an interrupt storm  during the eMMC chip probing (and
> the chip finally didn't get detected). It  turned out  that U-Boot  left
> the DMAC interrupts enabled while the  Linux driver didn't use those.
> The SDHI driver's interrupt handler somehow  assumes that, even if a
> SDIO interrupt didn't happen, it should return IRQ_HANDLED. I think that
> if none of the enabled interrupts happened and got handled, we should
> return IRQ_NONE -- that  way the kernel IRQ code recoginizes a spurious
> interrupt and masks  it  off pretty quickly...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Can be argued. Should come after Yamada-san's patches, I'd think. BTW
Sergei, did you test with SDIO devices?
Sergei Shtylyov Aug. 20, 2018, 5:02 p.m. UTC | #2
On 08/20/2018 07:55 PM, Wolfram Sang wrote:

>> I have encountered an interrupt storm  during the eMMC chip probing (and
>> the chip finally didn't get detected). It  turned out  that U-Boot  left
>> the DMAC interrupts enabled while the  Linux driver didn't use those.
>> The SDHI driver's interrupt handler somehow  assumes that, even if a
>> SDIO interrupt didn't happen, it should return IRQ_HANDLED. I think that
>> if none of the enabled interrupts happened and got handled, we should
>> return IRQ_NONE -- that  way the kernel IRQ code recoginizes a spurious
>> interrupt and masks  it  off pretty quickly...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> Can be argued.

   You get interrupt storm anyway, just not as quickly stopped as with this fix
(I hadn't understood *how* it gets finally stopped, I only know there's ~600000
interrupts before it gets quiet instead of 100000 with IRQ_NONE).

> Should come after Yamada-san's patches, I'd think.

   What are these?

> BTW Sergei, did you test with SDIO devices?

   No. But I really think the current code is borked. If you don't have an interrupt
to handle, you should idicate that.

MBR, Sergei
Wolfram Sang Aug. 20, 2018, 5:08 p.m. UTC | #3
> > Should come after Yamada-san's patches, I'd think.
> 
>    What are these?

A more complex series adding another SDHI user.

> > BTW Sergei, did you test with SDIO devices?
> 
>    No. But I really think the current code is borked. If you don't have an interrupt
> to handle, you should idicate that.

I think the patch is justified, I am not talking about that. I just
wondered if you tested it. So, you are not missing something in the SDIO
code path which you changed.
diff mbox series

Patch

Index: mmc/drivers/mmc/host/tmio_mmc_core.c
===================================================================
--- mmc.orig/drivers/mmc/host/tmio_mmc_core.c
+++ mmc/drivers/mmc/host/tmio_mmc_core.c
@@ -691,7 +691,7 @@  static bool __tmio_mmc_sdcard_irq(struct
 	return false;
 }
 
-static void __tmio_mmc_sdio_irq(struct tmio_mmc_host *host)
+static bool __tmio_mmc_sdio_irq(struct tmio_mmc_host *host)
 {
 	struct mmc_host *mmc = host->mmc;
 	struct tmio_mmc_data *pdata = host->pdata;
@@ -699,7 +699,7 @@  static void __tmio_mmc_sdio_irq(struct t
 	unsigned int sdio_status;
 
 	if (!(pdata->flags & TMIO_MMC_SDIO_IRQ))
-		return;
+		return false;
 
 	status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
 	ireg = status & TMIO_SDIO_MASK_ALL & ~host->sdio_irq_mask;
@@ -712,6 +712,8 @@  static void __tmio_mmc_sdio_irq(struct t
 
 	if (mmc->caps & MMC_CAP_SDIO_IRQ && ireg & TMIO_SDIO_STAT_IOIRQ)
 		mmc_signal_sdio_irq(mmc);
+
+	return ireg ? true : false;
 }
 
 irqreturn_t tmio_mmc_irq(int irq, void *devid)
@@ -730,9 +732,10 @@  irqreturn_t tmio_mmc_irq(int irq, void *
 	if (__tmio_mmc_sdcard_irq(host, ireg, status))
 		return IRQ_HANDLED;
 
-	__tmio_mmc_sdio_irq(host);
+	if (__tmio_mmc_sdio_irq(host))
+		return IRQ_HANDLED;
 
-	return IRQ_HANDLED;
+	return IRQ_NONE;
 }
 EXPORT_SYMBOL_GPL(tmio_mmc_irq);