| 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 |
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?
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
> > 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.
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);
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(-)