From: Daniel Drake <dsd@laptop.org> To: Per Forlin <per.forlin@linaro.org> Cc: linux-mmc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Nicolas Pitre <nicolas.pitre@linaro.org>, linaro-dev@lists.linaro.org, Chris Ball <cjb@laptop.org> Subject: Re: [PATCH 0/2] sdio: make sdio_single_irq optional due to suprious IRQ Date: Tue, 31 May 2011 22:52:57 +0100 [thread overview] Message-ID: <BANLkTimRB8z16wjyH1fuXbiybQuaJL4Qug@mail.gmail.com> (raw) In-Reply-To: <1306874008-28867-1-git-send-email-per.forlin@linaro.org> [-- Attachment #1: Type: text/plain, Size: 1746 bytes --] On 31 May 2011 21:33, Per Forlin <per.forlin@linaro.org> wrote: > Daniel Drake reported an issue in the libertas sdio client that was > triggered by the sdio_single_irq functionality. His SDIO device seems to > raise an interrupt even though there are no bits set in the CCCR_INTx > register. This behaviour is not supported by the sdio_single_irq feature nor > the SDIO spec. The purpose of the sdio_single_irq feature is to avoid the > overhead of checking the CCCR_INTx registers, this result in no error > handling of the case if there is a pending IRQ with none CCCR_INTx bits set. Thanks a lot for diagnosing this and nice work on figuring out the root cause presumably without even having access to the hardware! I've looked further, based on your findings, and have found that you are correct. During initialisation, exactly one interrupt is received with CCCR_INTx=0. Previously the mmc stack threw this interrupt away, after the optimization it now calls into libertas before it is ready to handle interrupts, leading to the crash. From that point, all other interrupts that come in are "normal". This is definitely a weird hardware issue, and it would annoy me for this hardware to cause a second generic mmc layer feature be disabled by default! And actually it is not much work to harden up the libertas driver to be able to accept that spurious IRQ, and during the process of fixing that it actually made the spurious IRQ go away completely. Patch attached. So, I vote for that we work around this little hardware issue in the libertas driver, and leave this optimization enabled by default until we find a hardware issue that is more difficult to workaround. I can take on submission of the libertas patch. Thoughts? Daniel [-- Attachment #2: lbs_sdio_irq.txt --] [-- Type: text/plain, Size: 1554 bytes --] diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c index a7b5cb0..224e985 100644 --- a/drivers/net/wireless/libertas/if_sdio.c +++ b/drivers/net/wireless/libertas/if_sdio.c @@ -907,7 +907,7 @@ static void if_sdio_interrupt(struct sdio_func *func) card = sdio_get_drvdata(func); cause = sdio_readb(card->func, IF_SDIO_H_INT_STATUS, &ret); - if (ret) + if (ret || !cause) goto out; lbs_deb_sdio("interrupt: 0x%X\n", (unsigned)cause); @@ -1008,10 +1008,6 @@ static int if_sdio_probe(struct sdio_func *func, if (ret) goto release; - ret = sdio_claim_irq(func, if_sdio_interrupt); - if (ret) - goto disable; - /* For 1-bit transfers to the 8686 model, we need to enable the * interrupt flag in the CCCR register. Set the MMC_QUIRK_LENIENT_FN0 * bit to allow access to non-vendor registers. */ @@ -1083,6 +1079,21 @@ static int if_sdio_probe(struct sdio_func *func, card->rx_unit = 0; /* + * Set up the interrupt handler late. + * + * If we set it up earlier, the (buggy) hardware generates a spurious + * interrupt, even before the interrupt has been enabled, with + * CCCR_INTx = 0. + * + * We register the interrupt handler late so that we can handle any + * spurious interrupts, and also to avoid generation of that known + * spurious interrupt in the first place. + */ + ret = sdio_claim_irq(func, if_sdio_interrupt); + if (ret) + goto disable; + + /* * Enable interrupts now that everything is set up */ sdio_writeb(func, 0x0f, IF_SDIO_H_INT_MASK, &ret);
WARNING: multiple messages have this Message-ID (diff)
From: dsd@laptop.org (Daniel Drake) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 0/2] sdio: make sdio_single_irq optional due to suprious IRQ Date: Tue, 31 May 2011 22:52:57 +0100 [thread overview] Message-ID: <BANLkTimRB8z16wjyH1fuXbiybQuaJL4Qug@mail.gmail.com> (raw) In-Reply-To: <1306874008-28867-1-git-send-email-per.forlin@linaro.org> On 31 May 2011 21:33, Per Forlin <per.forlin@linaro.org> wrote: > Daniel Drake reported an issue in the libertas sdio client that was > triggered by the sdio_single_irq functionality. His SDIO device seems to > raise an interrupt even though there are no bits set in the CCCR_INTx > register. This behaviour is not supported by the sdio_single_irq feature nor > the SDIO spec. The purpose of the sdio_single_irq feature is to avoid the > overhead of checking the CCCR_INTx registers, this result in no error > handling of the case if there is a pending IRQ with none CCCR_INTx bits set. Thanks a lot for diagnosing this and nice work on figuring out the root cause presumably without even having access to the hardware! I've looked further, based on your findings, and have found that you are correct. During initialisation, exactly one interrupt is received with CCCR_INTx=0. Previously the mmc stack threw this interrupt away, after the optimization it now calls into libertas before it is ready to handle interrupts, leading to the crash. From that point, all other interrupts that come in are "normal". This is definitely a weird hardware issue, and it would annoy me for this hardware to cause a second generic mmc layer feature be disabled by default! And actually it is not much work to harden up the libertas driver to be able to accept that spurious IRQ, and during the process of fixing that it actually made the spurious IRQ go away completely. Patch attached. So, I vote for that we work around this little hardware issue in the libertas driver, and leave this optimization enabled by default until we find a hardware issue that is more difficult to workaround. I can take on submission of the libertas patch. Thoughts? Daniel -------------- next part -------------- diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c index a7b5cb0..224e985 100644 --- a/drivers/net/wireless/libertas/if_sdio.c +++ b/drivers/net/wireless/libertas/if_sdio.c @@ -907,7 +907,7 @@ static void if_sdio_interrupt(struct sdio_func *func) card = sdio_get_drvdata(func); cause = sdio_readb(card->func, IF_SDIO_H_INT_STATUS, &ret); - if (ret) + if (ret || !cause) goto out; lbs_deb_sdio("interrupt: 0x%X\n", (unsigned)cause); @@ -1008,10 +1008,6 @@ static int if_sdio_probe(struct sdio_func *func, if (ret) goto release; - ret = sdio_claim_irq(func, if_sdio_interrupt); - if (ret) - goto disable; - /* For 1-bit transfers to the 8686 model, we need to enable the * interrupt flag in the CCCR register. Set the MMC_QUIRK_LENIENT_FN0 * bit to allow access to non-vendor registers. */ @@ -1083,6 +1079,21 @@ static int if_sdio_probe(struct sdio_func *func, card->rx_unit = 0; /* + * Set up the interrupt handler late. + * + * If we set it up earlier, the (buggy) hardware generates a spurious + * interrupt, even before the interrupt has been enabled, with + * CCCR_INTx = 0. + * + * We register the interrupt handler late so that we can handle any + * spurious interrupts, and also to avoid generation of that known + * spurious interrupt in the first place. + */ + ret = sdio_claim_irq(func, if_sdio_interrupt); + if (ret) + goto disable; + + /* * Enable interrupts now that everything is set up */ sdio_writeb(func, 0x0f, IF_SDIO_H_INT_MASK, &ret);
next prev parent reply other threads:[~2011-05-31 21:52 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-05-31 20:33 [PATCH 0/2] sdio: make sdio_single_irq optional due to suprious IRQ Per Forlin 2011-05-31 20:33 ` Per Forlin 2011-05-31 20:33 ` [PATCH 1/2] sdio: add function to enable and disable sdio_single_irq optimization Per Forlin 2011-05-31 20:33 ` Per Forlin 2011-06-01 7:30 ` Linus Walleij 2011-06-01 7:30 ` Linus Walleij 2011-06-01 7:39 ` Per Forlin 2011-06-01 7:39 ` Per Forlin 2011-05-31 20:33 ` [PATCH 2/2] sdio: report error if pending IRQ but none function bits Per Forlin 2011-05-31 20:33 ` Per Forlin 2011-06-01 14:30 ` Nicolas Pitre 2011-06-01 14:30 ` Nicolas Pitre 2011-06-07 7:02 ` Per Forlin 2011-06-07 7:02 ` Per Forlin 2011-05-31 21:52 ` Daniel Drake [this message] 2011-05-31 21:52 ` [PATCH 0/2] sdio: make sdio_single_irq optional due to suprious IRQ Daniel Drake 2011-06-01 14:29 ` Nicolas Pitre 2011-06-01 14:29 ` Nicolas Pitre
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=BANLkTimRB8z16wjyH1fuXbiybQuaJL4Qug@mail.gmail.com \ --to=dsd@laptop.org \ --cc=cjb@laptop.org \ --cc=linaro-dev@lists.linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-mmc@vger.kernel.org \ --cc=nicolas.pitre@linaro.org \ --cc=per.forlin@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.