Re: [PATCH 2/3] mmc: sdhci: check interrupt flags in ISR again

From: Alexander Stein
Date: Wed Mar 14 2012 - 03:53:07 EST


Hello,

Am Mittwoch, 14. März 2012, 09:39:02 schrieb Adrian Hunter:
> On 13/03/12 19:16, Alexander Stein wrote:
> > When using MSI it is possible that a new MSI is sent while an earlier
> > MSI is currently handled. In this case SDHCI_INT_STATUS only contains
> > SDHCI_INT_RESPONSE and the ISR would not be called again. But at the end
> > of the ISR SDHCI_INT_DATA_END is now also pending which would be
> > ignored.
> >
> > Fix this by rereading the interrupt flags in the ISR until no interrupt
> > we care is pending.
> >
> > Signed-off-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxxxxxxxx>
> [...]
> > @@ -2336,6 +2338,14 @@ static irqreturn_t sdhci_irq(int irq, void
> > *dev_id)>
> > sdhci_writel(host, SDHCI_INT_BUS_POWER, SDHCI_INT_STATUS);
> >
> > }
> >
> > + intmask_unhandled = intmask;
> > +
> > + intmask = sdhci_readl(host, SDHCI_INT_STATUS);
> > +
> > + /* Do interrupt handling again if we got new flags */
> > + if (intmask & ~intmask_unhandled)
> > + goto again;
> > +
> >
> > intmask &= ~SDHCI_INT_BUS_POWER;
> >
> > if (intmask & SDHCI_INT_CARD_INT)
>
> Why not just replace mmiowb() i.e.
>
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 8d66706..da8a101 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2353,7 +2353,9 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>
> result = IRQ_HANDLED;
>
> - mmiowb();
> + intmask = sdhci_readl(host, SDHCI_INT_STATUS);
> + if (intmask)
> + goto again;
> out:
> spin_unlock(&host->lock);
>

Well, I chose this way to only printk the error once. With your suggestion it
might be printed in each loop, dunno how often/fast these IRQ stats are set
again after clearing. This would end in an endless loop if error flags are set
again fast enough, but see below.
But in general I like this approach.

> But maybe it would be safer limiting the number of loops i.e.
>
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 8d66706..d88247d 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2268,7 +2268,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
> irqreturn_t result;
> struct sdhci_host *host = dev_id;
> u32 intmask;
> - int cardint = 0;
> + int cardint = 0, max_loops = 16;
>
> spin_lock(&host->lock);
>
> @@ -2353,7 +2353,9 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>
> result = IRQ_HANDLED;
>
> - mmiowb();
> + intmask = sdhci_readl(host, SDHCI_INT_STATUS);
> + if (intmask && --max_loops)
> + goto again;
> out:
> spin_unlock(&host->lock);

The actual problem I saw was a CMD6 command with an R1b response where the IRQ
for the 'not busy' event was sent during ISR for the response. So I think
normally this should only occur once.
Regarding error flags I masked the unhandled flags out in order to print an
error only once, even if they might be set again in the next loop. With a
simple check on intmask they might occur up to 16 times in the kernel log.
IMHO it makes no sense to repeatedly print errors about interrupt flags we
don't handle.

Suggestions to get a more clean way?

Best regards,
Alexander
--
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
August-Bebel-Str. 29
D-07973 Greiz

Tel: +49-3661-6279-0, Fax: +49-3661-6279-99
eMail: Alexander.Stein@xxxxxxxxxxxxxxxxxxxxx
Internet: http://www.systec-electronic.com

Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Jena, HRB 205563
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/