| Message ID | 20221114093857.491695-1-peter.suti@streamunlimited.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: meson-gx: fix SDIO interrupt handling | expand |
On 14.11.2022 10:38, Peter Suti wrote: > With the interrupt support introduced in commit 066ecde sometimes the > Marvell-8987 wifi chip entered a deadlock using the marvell-sd-uapsta-8987 > vendor driver. The cause seems to be that sometimes the interrupt handler > handles 2 IRQs and one of them disables the interrupts which are not reenabled > when all interrupts are finished. To work around this, disable all interrupts > when we are in the IRQ context and reenable them when the current IRQ is handled. > IIRC I had a similar/same problem in mind when discussing the following: https://lore.kernel.org/linux-arm-kernel/CAPDyKFoameOb7d3cn8_ki1O6DbMEAFvkQh1uUsYp4S-Lkq41oQ@mail.gmail.com/ Not sure though whether it's related to the issue you're facing. > Fixes: 066ecde ("mmc: meson-gx: add SDIO interrupt support") > > Signed-off-by: Peter Suti <peter.suti@streamunlimited.com> > --- > drivers/mmc/host/meson-gx-mmc.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > index 6e5ea0213b47..972024d57d1c 100644 > --- a/drivers/mmc/host/meson-gx-mmc.c > +++ b/drivers/mmc/host/meson-gx-mmc.c > @@ -950,6 +950,10 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) > struct mmc_command *cmd; > u32 status, raw_status; > irqreturn_t ret = IRQ_NONE; > + unsigned long flags; > + > + spin_lock_irqsave(&host->lock, flags); Typically you wouldn't have to use _irqsave version in a hard irq handler. Or is to deal with forced threaded irq handlers? Do you use forced threaded handlers on your system? > + __meson_mmc_enable_sdio_irq(host->mmc, 0); > > raw_status = readl(host->regs + SD_EMMC_STATUS); > status = raw_status & (IRQ_EN_MASK | IRQ_SDIO); > @@ -958,11 +962,11 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) > dev_dbg(host->dev, > "Unexpected IRQ! irq_en 0x%08lx - status 0x%08x\n", > IRQ_EN_MASK | IRQ_SDIO, raw_status); > - return IRQ_NONE; > + goto out_unlock; > } > > if (WARN_ON(!host)) > - return IRQ_NONE; > + goto out_unlock; > > /* ack all raised interrupts */ > writel(status, host->regs + SD_EMMC_STATUS); > @@ -970,17 +974,16 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) > cmd = host->cmd; > > if (status & IRQ_SDIO) { > - spin_lock(&host->lock); > - __meson_mmc_enable_sdio_irq(host->mmc, 0); > sdio_signal_irq(host->mmc); > - spin_unlock(&host->lock); > status &= ~IRQ_SDIO; > - if (!status) > + if (!status) { > + spin_unlock_irqrestore(&host->lock, flags); > return IRQ_HANDLED; > + } > } > > if (WARN_ON(!cmd)) > - return IRQ_NONE; > + goto out_unlock; > > cmd->error = 0; > if (status & IRQ_CRC_ERR) { > @@ -1023,6 +1026,10 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) > if (ret == IRQ_HANDLED) > meson_mmc_request_done(host->mmc, cmd->mrq); > > +out_unlock: > + __meson_mmc_enable_sdio_irq(host->mmc, 1); > + spin_unlock_irqrestore(&host->lock, flags); > + > return ret; > } >
On Mon, 14 Nov 2022 at 10:40, Peter Suti <peter.suti@streamunlimited.com> wrote: > > With the interrupt support introduced in commit 066ecde sometimes the > Marvell-8987 wifi chip entered a deadlock using the marvell-sd-uapsta-8987 > vendor driver. The cause seems to be that sometimes the interrupt handler > handles 2 IRQs and one of them disables the interrupts which are not reenabled > when all interrupts are finished. To work around this, disable all interrupts > when we are in the IRQ context and reenable them when the current IRQ is handled. > > Fixes: 066ecde ("mmc: meson-gx: add SDIO interrupt support") > > Signed-off-by: Peter Suti <peter.suti@streamunlimited.com> > --- > drivers/mmc/host/meson-gx-mmc.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > index 6e5ea0213b47..972024d57d1c 100644 > --- a/drivers/mmc/host/meson-gx-mmc.c > +++ b/drivers/mmc/host/meson-gx-mmc.c > @@ -950,6 +950,10 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) > struct mmc_command *cmd; > u32 status, raw_status; > irqreturn_t ret = IRQ_NONE; > + unsigned long flags; > + > + spin_lock_irqsave(&host->lock, flags); > + __meson_mmc_enable_sdio_irq(host->mmc, 0); > > raw_status = readl(host->regs + SD_EMMC_STATUS); > status = raw_status & (IRQ_EN_MASK | IRQ_SDIO); > @@ -958,11 +962,11 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) > dev_dbg(host->dev, > "Unexpected IRQ! irq_en 0x%08lx - status 0x%08x\n", > IRQ_EN_MASK | IRQ_SDIO, raw_status); > - return IRQ_NONE; > + goto out_unlock; This may end up re-enabling the sdio irqs, even if it was not enabled to start with. This is probably not what we want. > } > > if (WARN_ON(!host)) > - return IRQ_NONE; > + goto out_unlock; > > /* ack all raised interrupts */ > writel(status, host->regs + SD_EMMC_STATUS); > @@ -970,17 +974,16 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) > cmd = host->cmd; > > if (status & IRQ_SDIO) { > - spin_lock(&host->lock); > - __meson_mmc_enable_sdio_irq(host->mmc, 0); > sdio_signal_irq(host->mmc); > - spin_unlock(&host->lock); > status &= ~IRQ_SDIO; > - if (!status) > + if (!status) { > + spin_unlock_irqrestore(&host->lock, flags); > return IRQ_HANDLED; > + } > } > > if (WARN_ON(!cmd)) > - return IRQ_NONE; > + goto out_unlock; > > cmd->error = 0; > if (status & IRQ_CRC_ERR) { > @@ -1023,6 +1026,10 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) > if (ret == IRQ_HANDLED) > meson_mmc_request_done(host->mmc, cmd->mrq); > > +out_unlock: > + __meson_mmc_enable_sdio_irq(host->mmc, 1); > + spin_unlock_irqrestore(&host->lock, flags); > + > return ret; > } > Kind regards Uffe
diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c index 6e5ea0213b47..972024d57d1c 100644 --- a/drivers/mmc/host/meson-gx-mmc.c +++ b/drivers/mmc/host/meson-gx-mmc.c @@ -950,6 +950,10 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) struct mmc_command *cmd; u32 status, raw_status; irqreturn_t ret = IRQ_NONE; + unsigned long flags; + + spin_lock_irqsave(&host->lock, flags); + __meson_mmc_enable_sdio_irq(host->mmc, 0); raw_status = readl(host->regs + SD_EMMC_STATUS); status = raw_status & (IRQ_EN_MASK | IRQ_SDIO); @@ -958,11 +962,11 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) dev_dbg(host->dev, "Unexpected IRQ! irq_en 0x%08lx - status 0x%08x\n", IRQ_EN_MASK | IRQ_SDIO, raw_status); - return IRQ_NONE; + goto out_unlock; } if (WARN_ON(!host)) - return IRQ_NONE; + goto out_unlock; /* ack all raised interrupts */ writel(status, host->regs + SD_EMMC_STATUS); @@ -970,17 +974,16 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) cmd = host->cmd; if (status & IRQ_SDIO) { - spin_lock(&host->lock); - __meson_mmc_enable_sdio_irq(host->mmc, 0); sdio_signal_irq(host->mmc); - spin_unlock(&host->lock); status &= ~IRQ_SDIO; - if (!status) + if (!status) { + spin_unlock_irqrestore(&host->lock, flags); return IRQ_HANDLED; + } } if (WARN_ON(!cmd)) - return IRQ_NONE; + goto out_unlock; cmd->error = 0; if (status & IRQ_CRC_ERR) { @@ -1023,6 +1026,10 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) if (ret == IRQ_HANDLED) meson_mmc_request_done(host->mmc, cmd->mrq); +out_unlock: + __meson_mmc_enable_sdio_irq(host->mmc, 1); + spin_unlock_irqrestore(&host->lock, flags); + return ret; }
With the interrupt support introduced in commit 066ecde sometimes the Marvell-8987 wifi chip entered a deadlock using the marvell-sd-uapsta-8987 vendor driver. The cause seems to be that sometimes the interrupt handler handles 2 IRQs and one of them disables the interrupts which are not reenabled when all interrupts are finished. To work around this, disable all interrupts when we are in the IRQ context and reenable them when the current IRQ is handled. Fixes: 066ecde ("mmc: meson-gx: add SDIO interrupt support") Signed-off-by: Peter Suti <peter.suti@streamunlimited.com> --- drivers/mmc/host/meson-gx-mmc.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)