lkml.org 
[lkml]   [2012]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 2/3] mmc: sdhci: check interrupt flags in ISR again
    Date
    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-03-14 08:55    [W:0.025 / U:31.796 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site