[PATCH 0/2] sdio: make sdio_single_irq optional due to suprious IRQ

[PATCH 0/2] sdio: make sdio_single_irq optional due to suprious IRQ

[Per Forlin]

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 intends to resolve the libertas issue by making
sdio_single_irq feature configurable and also report a
warning if an SDIO interrupt is raised but none CCCR_INTx bits are set.

Per Forlin (2):
  sdio: add function to enable and disable sdio_single_irq optimization
  sdio: report error if pending IRQ but none function bits

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

-- 
1.7.4.1

[PATCH 1/2] sdio: add function to enable and disable sdio_single_irq optimization

[Per Forlin]

Make sdio single irq run time configurable, default is disable. This is due
to an issue in libertas where the SDIO device seems to raise interrupt
even if there are none function bits in CCCR_INTx set. This behaviour
is not defined by the SDIO spec.

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

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index 03ead02..2f81ddc 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->sdio_single_irq_en &&
+	    (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];
@@ -302,3 +303,17 @@ int sdio_release_irq(struct sdio_func *func)
 }
 EXPORT_SYMBOL_GPL(sdio_release_irq);
 
+/**
+ *	sdio_single_irq_enable - enable or disable SDIO single IRQ function
+ *	@card: card to enable SDIO single irq
+ *	@value: true to enable SDIO single irq function, false to disable
+ *
+ *	If there is only 1 function interrupt registered and SDIO single IRQ
+ *	is enable, the irq handler is called directly without reading
+ *	the CCCR registers
+ */
+void sdio_single_irq_enable(struct mmc_card *card, bool value)
+{
+	card->sdio_single_irq_en = value;
+}
+EXPORT_SYMBOL_GPL(sdio_single_irq_enable);
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 7b4fd7b..f8b0537 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -196,6 +196,7 @@ struct mmc_card {
 	struct sdio_cis		cis;		/* common tuple info */
 	struct sdio_func	*sdio_func[SDIO_MAX_FUNCS]; /* SDIO functions (devices) */
 	struct sdio_func	*sdio_single_irq; /* SDIO function when only one IRQ active */
+	bool			sdio_single_irq_en; /* enable single irq */
 	unsigned		num_info;	/* number of info strings */
 	const char		**info;		/* info strings */
 	struct sdio_func_tuple	*tuples;	/* unknown common tuples */
-- 
1.7.4.1

[PATCH 1/2] sdio: add function to enable and disable sdio_single_irq optimization

[Linus Walleij]

2011/5/31 Per Forlin <per.forlin@linaro.org>:

> +/**
> + * ? ? sdio_single_irq_enable - enable or disable SDIO single IRQ function
> + * ? ? @card: card to enable SDIO single irq
> + * ? ? @value: true to enable SDIO single irq function, false to disable
> + *
> + * ? ? If there is only 1 function interrupt registered and SDIO single IRQ
> + * ? ? is enable, the irq handler is called directly without reading
> + * ? ? the CCCR registers
> + */
> +void sdio_single_irq_enable(struct mmc_card *card, bool value)
> +{
> + ? ? ? card->sdio_single_irq_en = value;
> +}
> +EXPORT_SYMBOL_GPL(sdio_single_irq_enable);

Can we use a quirk for implementing this for the specific problematic
card instead?

Daniel, do you have the vendor and device ID for the problematic
Libertas card you're working on so this can be quirked explicitly
in drivers/mmc/core/quirks.c?

Yours,
Linus Walleij

[PATCH 1/2] sdio: add function to enable and disable sdio_single_irq optimization

[Per Forlin]

On 1 June 2011 09:30, Linus Walleij <linus.walleij@linaro.org> wrote:
> 2011/5/31 Per Forlin <per.forlin@linaro.org>:
>
>> +/**
>> + * ? ? sdio_single_irq_enable - enable or disable SDIO single IRQ function
>> + * ? ? @card: card to enable SDIO single irq
>> + * ? ? @value: true to enable SDIO single irq function, false to disable
>> + *
>> + * ? ? If there is only 1 function interrupt registered and SDIO single IRQ
>> + * ? ? is enable, the irq handler is called directly without reading
>> + * ? ? the CCCR registers
>> + */
>> +void sdio_single_irq_enable(struct mmc_card *card, bool value)
>> +{
>> + ? ? ? card->sdio_single_irq_en = value;
>> +}
>> +EXPORT_SYMBOL_GPL(sdio_single_irq_enable);
>
> Can we use a quirk for implementing this for the specific problematic
> card instead?
>
Yes, quirks is the thing I should use.
I'll remove this function and replace it with a quirk. The default
state could then be sdio_single_irq enable and for all none supported
hardware (device ID) sdio_single_irq will be disable.

> Daniel, do you have the vendor and device ID for the problematic
> Libertas card you're working on so this can be quirked explicitly
> in drivers/mmc/core/quirks.c?
>
Even if Daniel fix this issue in libertas it is still good to have a
sdio_single_irq quirk in place for other SDIO devices with the same
hardware issue.

> Yours,
> Linus Walleij
>
Thanks,
Per

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

[Per Forlin]

Return error in case of pending IRQ but none functions bits
in CCCR_INTx is 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 2f81ddc..8184b6e 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 2/2] sdio: report error if pending IRQ but none function bits

[Nicolas Pitre]

On Tue, 31 May 2011, Per Forlin wrote:

> Return error in case of pending IRQ but none functions bits
> in CCCR_INTx is 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 2f81ddc..8184b6e 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;
> +	}

Nope, this won't work with the polled interrupt mode.


Nicolas

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

[Per Forlin]

On 1 June 2011 16:30, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Tue, 31 May 2011, Per Forlin wrote:
>
>> Return error in case of pending IRQ but none functions bits
>> in CCCR_INTx is 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 2f81ddc..8184b6e 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;
>> + ? ? }
>
> Nope, this won't work with the polled interrupt mode.
I'll drop this patch.

Thanks for your comment,
Per

[PATCH 0/2] sdio: make sdio_single_irq optional due to suprious IRQ

[Daniel Drake]

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);

[PATCH 0/2] sdio: make sdio_single_irq optional due to suprious IRQ

[Nicolas Pitre]

On Tue, 31 May 2011, Daniel Drake wrote:

> 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?

This is definitively the best approach.  Thanks for fixing the root 
cause.


Nicolas