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-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
