[PATCH v2 0/2] sdio: add quirk for spurious SDIO IRQ

[PATCH v2 0/2] sdio: add quirk for spurious SDIO IRQ

[Per Forlin]

From: Per Forlin <per.forlin@linaro.org>

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.

This patchset adds a quirk to support this spurious IRQ issue and also report
a warning if an SDIO interrupt is raised but none CCCR_INTx bits are set.

Changes since v1:
 * Replace public sdio function to enable disable SDIO single IRQ function
   with a quirk

Per Forlin (2):
  sdio: add quirk to handle pending IRQ in case of none CCCR_INTx bits
  sdio: report error if pending IRQ but none CCCR_INTx bits

 drivers/mmc/core/sdio_irq.c |    9 +++++++--
 include/linux/mmc/card.h    |    1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

-- 
1.7.4.1

[PATCH v2 0/2] sdio: add quirk for spurious SDIO IRQ

[Per Forlin]

From: Per Forlin <per.forlin@linaro.org>

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.

This patchset adds a quirk to support this spurious IRQ issue and also report
a warning if an SDIO interrupt is raised but none CCCR_INTx bits are set.

Changes since v1:
 * Replace public sdio function to enable disable SDIO single IRQ function
   with a quirk

Per Forlin (2):
  sdio: add quirk to handle pending IRQ in case of none CCCR_INTx bits
  sdio: report error if pending IRQ but none CCCR_INTx bits

 drivers/mmc/core/sdio_irq.c |    9 +++++++--
 include/linux/mmc/card.h    |    1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

-- 
1.7.4.1

[PATCH v2 1/2] sdio: add quirk to handle pending IRQ in case of none CCCR_INTx bits

[Per Forlin]

From: Per Forlin <per.forlin-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Add quirk MMC_QUIRK_SDIO_IRQ_CCCR_INTX_0 to handle SDIO device that may
have pending IRQ when none bits are set in CCCR_INTx.

This IRQ issue was identified in the libertas sdio driver where the
SDIO device seems to raise an interrupt even if there were none function
bits in CCCR_INTx set. SDIO single irq feature will call irq handler directly
in case of only one interrupt registered. It will not check the CCCR_INTx
register and miss if the register is 0 and call the irq handler anyway.
This IRQ issue is not defined by the SDIO spec.
Use this quirk to work around this SDIO IRQ issue for a specific device.

Signed-off-by: Per Forlin <per.forlin-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/mmc/core/sdio_irq.c |    4 ++--
 include/linux/mmc/card.h    |    1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index 03ead02..df1e946 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -204,7 +204,8 @@ static void sdio_single_irq_set(struct mmc_card *card)
 	int i;
 
 	card->sdio_single_irq = NULL;
-	if ((card->host->caps & MMC_CAP_SDIO_IRQ) &&
+	if (!(card->quirks & MMC_QUIRK_SDIO_IRQ_CCCR_INTX_0) &&
+	    (card->host->caps & MMC_CAP_SDIO_IRQ) &&
 	    card->host->sdio_irqs == 1)
 		for (i = 0; i < card->sdio_funcs; i++) {
 		       func = card->sdio_func[i];
@@ -301,4 +302,3 @@ int sdio_release_irq(struct sdio_func *func)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(sdio_release_irq);
-
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 7b4fd7b..852021c 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -175,6 +175,7 @@ struct mmc_card {
 #define MMC_QUIRK_DISABLE_CD	(1<<5)		/* disconnect CD/DAT[3] resistor */
 #define MMC_QUIRK_INAND_CMD38	(1<<6)		/* iNAND devices have broken CMD38 */
 #define MMC_QUIRK_BLK_NO_CMD23	(1<<7)		/* Avoid CMD23 for regular multiblock */
+#define MMC_QUIRK_SDIO_IRQ_CCCR_INTX_0 (1<<7)	/* SDIO card has IRQ even if CCCR_INTx is 0 */
 
 	unsigned int		erase_size;	/* erase size in sectors */
  	unsigned int		erase_shift;	/* if erase unit is power 2 */
-- 
1.7.4.1

[PATCH v2 1/2] sdio: add quirk to handle pending IRQ in case of none CCCR_INTx bits

[Per Forlin]

From: Per Forlin <per.forlin@linaro.org>

Add quirk MMC_QUIRK_SDIO_IRQ_CCCR_INTX_0 to handle SDIO device that may
have pending IRQ when none bits are set in CCCR_INTx.

This IRQ issue was identified in the libertas sdio driver where the
SDIO device seems to raise an interrupt even if there were none function
bits in CCCR_INTx set. SDIO single irq feature will call irq handler directly
in case of only one interrupt registered. It will not check the CCCR_INTx
register and miss if the register is 0 and call the irq handler anyway.
This IRQ issue is not defined by the SDIO spec.
Use this quirk to work around this SDIO IRQ issue for a specific device.

Signed-off-by: Per Forlin <per.forlin@linaro.org>
---
 drivers/mmc/core/sdio_irq.c |    4 ++--
 include/linux/mmc/card.h    |    1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index 03ead02..df1e946 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -204,7 +204,8 @@ static void sdio_single_irq_set(struct mmc_card *card)
 	int i;
 
 	card->sdio_single_irq = NULL;
-	if ((card->host->caps & MMC_CAP_SDIO_IRQ) &&
+	if (!(card->quirks & MMC_QUIRK_SDIO_IRQ_CCCR_INTX_0) &&
+	    (card->host->caps & MMC_CAP_SDIO_IRQ) &&
 	    card->host->sdio_irqs == 1)
 		for (i = 0; i < card->sdio_funcs; i++) {
 		       func = card->sdio_func[i];
@@ -301,4 +302,3 @@ int sdio_release_irq(struct sdio_func *func)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(sdio_release_irq);
-
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 7b4fd7b..852021c 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -175,6 +175,7 @@ struct mmc_card {
 #define MMC_QUIRK_DISABLE_CD	(1<<5)		/* disconnect CD/DAT[3] resistor */
 #define MMC_QUIRK_INAND_CMD38	(1<<6)		/* iNAND devices have broken CMD38 */
 #define MMC_QUIRK_BLK_NO_CMD23	(1<<7)		/* Avoid CMD23 for regular multiblock */
+#define MMC_QUIRK_SDIO_IRQ_CCCR_INTX_0 (1<<7)	/* SDIO card has IRQ even if CCCR_INTx is 0 */
 
 	unsigned int		erase_size;	/* erase size in sectors */
  	unsigned int		erase_shift;	/* if erase unit is power 2 */
-- 
1.7.4.1

[PATCH v2 2/2] sdio: report error if pending IRQ but none CCCR_INTx bits

[Per Forlin]

From: Per Forlin <per.forlin@linaro.org>

Return error in case of pending IRQ but none functions bits
in CCCR_INTx are set.

Signed-off-by: Per Forlin <per.forlin@linaro.org>
---
 drivers/mmc/core/sdio_irq.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index df1e946..4288d92 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -50,6 +50,11 @@ static int process_sdio_pending_irqs(struct mmc_card *card)
 		return ret;
 	}
 
+	if (!pending) {
+		printk(KERN_WARNING "%s: pending IRQ but none function bits\n",
+		       mmc_card_id(card));
+		ret = -EINVAL;
+	}
 	count = 0;
 	for (i = 1; i <= 7; i++) {
 		if (pending & (1 << i)) {
-- 
1.7.4.1

[PATCH v2 2/2] sdio: report error if pending IRQ but none CCCR_INTx bits

[Per Forlin]

From: Per Forlin <per.forlin@linaro.org>

Return error in case of pending IRQ but none functions bits
in CCCR_INTx are set.

Signed-off-by: Per Forlin <per.forlin@linaro.org>
---
 drivers/mmc/core/sdio_irq.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index df1e946..4288d92 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -50,6 +50,11 @@ static int process_sdio_pending_irqs(struct mmc_card *card)
 		return ret;
 	}
 
+	if (!pending) {
+		printk(KERN_WARNING "%s: pending IRQ but none function bits\n",
+		       mmc_card_id(card));
+		ret = -EINVAL;
+	}
 	count = 0;
 	for (i = 1; i <= 7; i++) {
 		if (pending & (1 << i)) {
-- 
1.7.4.1

Re: [PATCH v2 0/2] sdio: add quirk for spurious SDIO IRQ

[Nicolas Pitre]

On Wed, 1 Jun 2011, Per Forlin wrote:

> From: Per Forlin <per.forlin@linaro.org>
> 
> 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.
> 
> This patchset adds a quirk to support this spurious IRQ issue and also report
> a warning if an SDIO interrupt is raised but none CCCR_INTx bits are set.

Given that the issue can be fixed locally to the libertas driver, I'd 
suggest not merging this series until truly unfixable issues come up, as 
Daniel said.


Nicolas

[PATCH v2 0/2] sdio: add quirk for spurious SDIO IRQ

[Nicolas Pitre]

On Wed, 1 Jun 2011, Per Forlin wrote:

> From: Per Forlin <per.forlin@linaro.org>
> 
> 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.
> 
> This patchset adds a quirk to support this spurious IRQ issue and also report
> a warning if an SDIO interrupt is raised but none CCCR_INTx bits are set.

Given that the issue can be fixed locally to the libertas driver, I'd 
suggest not merging this series until truly unfixable issues come up, as 
Daniel said.


Nicolas

Re: [PATCH v2 0/2] sdio: add quirk for spurious SDIO IRQ

[Chris Ball]

Hi,

On Wed, Jun 01 2011, Nicolas Pitre wrote:
>> This patchset adds a quirk to support this spurious IRQ issue and also report
>> a warning if an SDIO interrupt is raised but none CCCR_INTx bits are set.
>
> Given that the issue can be fixed locally to the libertas driver, I'd 
> suggest not merging this series until truly unfixable issues come up, as 
> Daniel said.

Yep, agreed.  Dan, please go ahead and submit the libertas patch and
let us know if there are any problems getting it in for 3.0.  (Ideally
by the next -rc!)

Thanks very much, everyone,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH v2 0/2] sdio: add quirk for spurious SDIO IRQ

[Chris Ball]

Hi,

On Wed, Jun 01 2011, Nicolas Pitre wrote:
>> This patchset adds a quirk to support this spurious IRQ issue and also report
>> a warning if an SDIO interrupt is raised but none CCCR_INTx bits are set.
>
> Given that the issue can be fixed locally to the libertas driver, I'd 
> suggest not merging this series until truly unfixable issues come up, as 
> Daniel said.

Yep, agreed.  Dan, please go ahead and submit the libertas patch and
let us know if there are any problems getting it in for 3.0.  (Ideally
by the next -rc!)

Thanks very much, everyone,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v2 1/2] sdio: add quirk to handle pending IRQ in case of none CCCR_INTx bits

[Troy Kisky]

On 6/1/2011 1:48 AM, Per Forlin wrote:
> From: Per Forlin <per.forlin@linaro.org>

> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 7b4fd7b..852021c 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -175,6 +175,7 @@ struct mmc_card {
>  #define MMC_QUIRK_DISABLE_CD	(1<<5)		/* disconnect CD/DAT[3] resistor */
>  #define MMC_QUIRK_INAND_CMD38	(1<<6)		/* iNAND devices have broken CMD38 */
>  #define MMC_QUIRK_BLK_NO_CMD23	(1<<7)		/* Avoid CMD23 for regular multiblock */
> +#define MMC_QUIRK_SDIO_IRQ_CCCR_INTX_0 (1<<7)	/* SDIO card has IRQ even if CCCR_INTx is 0 */

Using the same value as MMC_QUIRK_BLK_NO_CMD23 looks odd...

[PATCH v2 1/2] sdio: add quirk to handle pending IRQ in case of none CCCR_INTx bits

[Troy Kisky]

On 6/1/2011 1:48 AM, Per Forlin wrote:
> From: Per Forlin <per.forlin@linaro.org>

> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 7b4fd7b..852021c 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -175,6 +175,7 @@ struct mmc_card {
>  #define MMC_QUIRK_DISABLE_CD	(1<<5)		/* disconnect CD/DAT[3] resistor */
>  #define MMC_QUIRK_INAND_CMD38	(1<<6)		/* iNAND devices have broken CMD38 */
>  #define MMC_QUIRK_BLK_NO_CMD23	(1<<7)		/* Avoid CMD23 for regular multiblock */
> +#define MMC_QUIRK_SDIO_IRQ_CCCR_INTX_0 (1<<7)	/* SDIO card has IRQ even if CCCR_INTx is 0 */

Using the same value as MMC_QUIRK_BLK_NO_CMD23 looks odd...

Re: [PATCH v2 0/2] sdio: add quirk for spurious SDIO IRQ

[Per Forlin]

On 1 June 2011 17:59, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Wed, 1 Jun 2011, Per Forlin wrote:
>
>> From: Per Forlin <per.forlin@linaro.org>
>>
>> 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.
>>
>> This patchset adds a quirk to support this spurious IRQ issue and also report
>> a warning if an SDIO interrupt is raised but none CCCR_INTx bits are set.
>
> Given that the issue can be fixed locally to the libertas driver, I'd
> suggest not merging this series until truly unfixable issues come up, as
> Daniel said.
>
I agree too.

Thanks,
Per

[PATCH v2 0/2] sdio: add quirk for spurious SDIO IRQ

[Per Forlin]

On 1 June 2011 17:59, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Wed, 1 Jun 2011, Per Forlin wrote:
>
>> From: Per Forlin <per.forlin@linaro.org>
>>
>> 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.
>>
>> This patchset adds a quirk to support this spurious IRQ issue and also report
>> a warning if an SDIO interrupt is raised but none CCCR_INTx bits are set.
>
> Given that the issue can be fixed locally to the libertas driver, I'd
> suggest not merging this series until truly unfixable issues come up, as
> Daniel said.
>
I agree too.

Thanks,
Per

Re: [PATCH v2 1/2] sdio: add quirk to handle pending IRQ in case of none CCCR_INTx bits

[Per Forlin]

On 1 June 2011 23:36, Troy Kisky <troy.kisky@boundarydevices.com> wrote:
> On 6/1/2011 1:48 AM, Per Forlin wrote:
>> From: Per Forlin <per.forlin@linaro.org>
>
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index 7b4fd7b..852021c 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -175,6 +175,7 @@ struct mmc_card {
>>  #define MMC_QUIRK_DISABLE_CD (1<<5)          /* disconnect CD/DAT[3] resistor */
>>  #define MMC_QUIRK_INAND_CMD38        (1<<6)          /* iNAND devices have broken CMD38 */
>>  #define MMC_QUIRK_BLK_NO_CMD23       (1<<7)          /* Avoid CMD23 for regular multiblock */
>> +#define MMC_QUIRK_SDIO_IRQ_CCCR_INTX_0 (1<<7)        /* SDIO card has IRQ even if CCCR_INTx is 0 */
>
> Using the same value as MMC_QUIRK_BLK_NO_CMD23 looks odd...
>
Thanks for your observation.
Typo, it should be (1 << 8)
I'll update the patch but I don't plan post a new version yet. The
root cause issue in the libertas is resolved. If the fix turns out to
be needed later on I resend it.

Thanks,
Per

[PATCH v2 1/2] sdio: add quirk to handle pending IRQ in case of none CCCR_INTx bits

[Per Forlin]

On 1 June 2011 23:36, Troy Kisky <troy.kisky@boundarydevices.com> wrote:
> On 6/1/2011 1:48 AM, Per Forlin wrote:
>> From: Per Forlin <per.forlin@linaro.org>
>
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index 7b4fd7b..852021c 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -175,6 +175,7 @@ struct mmc_card {
>> ?#define MMC_QUIRK_DISABLE_CD (1<<5) ? ? ? ? ?/* disconnect CD/DAT[3] resistor */
>> ?#define MMC_QUIRK_INAND_CMD38 ? ? ? ?(1<<6) ? ? ? ? ?/* iNAND devices have broken CMD38 */
>> ?#define MMC_QUIRK_BLK_NO_CMD23 ? ? ? (1<<7) ? ? ? ? ?/* Avoid CMD23 for regular multiblock */
>> +#define MMC_QUIRK_SDIO_IRQ_CCCR_INTX_0 (1<<7) ? ? ? ?/* SDIO card has IRQ even if CCCR_INTx is 0 */
>
> Using the same value as MMC_QUIRK_BLK_NO_CMD23 looks odd...
>
Thanks for your observation.
Typo, it should be (1 << 8)
I'll update the patch but I don't plan post a new version yet. The
root cause issue in the libertas is resolved. If the fix turns out to
be needed later on I resend it.

Thanks,
Per