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

[Alexander Stein <alexander.stein@systec-electronic.com>]

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@systec-electronic.com>
> [...]
> > @@ -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@systec-electronic.com
Internet: http://www.systec-electronic.com

Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Jena, HRB 205563