SDIO single irq optimization - Spurious interrupt notification to function driver

SDIO single irq optimization - Spurious interrupt notification to function driver

[Sujit Reddy Thumma]

Hi,

While I was debugging an issue where the sdio function driver panics 
upon receiving a spurious interrupt notification which is similar to the 
one found on libertas sdio 
http://permalink.gmane.org/gmane.linux.kernel.mmc/8338

I can confirm that my hardware not buggy and is not generating any 
interrupts during initialization and CCCR_INTx is all zero's.

After some analysis, I found that there is a notification of interrupt 
to function handler by sdio_irq_thread() even though there is no 
interrupt. The sequence is as follows.

sdio_claim_host();

sdio_claim_irq():
	...
	sdio_card_irq_get() -> kthread_run(sdio_irq_thread) -> 
wake_up_sdio_irq_thread
	sdio_single_irq_set() -> card->sdio_single_irq = func = sdio_func[i];
	...

sdio_release_host();

Since we woke up sdio_irq_thread(), while this thread get a chance to run:
...
__mmc_claim_host()
process_sdio_pending_irqs() -> since sdio_single_irq is non NULL the 
func->handler is directly called without checking pending interrupt bit 
CCCR_INTx.
mmc_release_host()
...

Irrespective of the function driver state, sdio driver should not call 
func->handler() if it is not real interrupt. I knew that function 
handler should be able to handle these interrupts gracefully. But it is 
just not the right thing for sdio driver to generate spurious interrupts.

I see this notification issue during mmc_sdio_resume() also explained by 
Nicolas:
http://permalink.gmane.org/gmane.linux.kernel.mmc/10871

This leads me to think of reverting commit 06e8935feb "optimized SDIO 
IRQ handling for single irq" instead of hacking every function driver to 
handle spurious interrupt notification gracefully.

Let me know your thoughts on this or suggest any alternate way instead 
of reverting it.

-- 
Thanks & Regards,
Sujit Reddy Thumma

Re: SDIO single irq optimization - Spurious interrupt notification to function driver

[Nicolas Pitre]

On Fri, 30 Mar 2012, Sujit Reddy Thumma wrote:

> Hi,
> 
> While I was debugging an issue where the sdio function driver panics upon
> receiving a spurious interrupt notification which is similar to the one found
> on libertas sdio http://permalink.gmane.org/gmane.linux.kernel.mmc/8338
> 
> I can confirm that my hardware not buggy and is not generating any interrupts
> during initialization and CCCR_INTx is all zero's.

Irrespective of whatever issue this optimization is uncovering, you 
should _never_ panic when receiving a spurious interrupt notification.  
This is just basic hardware driver programming practice to gracefully 
deal with unexpected hardware state.  In this case, you should simply 
notice that your function has no pending work and just return.  You 
probably know this already but this is still worth mentioning again.

> After some analysis, I found that there is a notification of interrupt to
> function handler by sdio_irq_thread() even though there is no interrupt. The
> sequence is as follows.
> 
> sdio_claim_host();
> 
> sdio_claim_irq():
> 	...
> 	sdio_card_irq_get() -> kthread_run(sdio_irq_thread) ->
> wake_up_sdio_irq_thread
> 	sdio_single_irq_set() -> card->sdio_single_irq = func = sdio_func[i];
> 	...
> 
> sdio_release_host();
> 
> Since we woke up sdio_irq_thread(), while this thread get a chance to run:
> ...
> __mmc_claim_host()
> process_sdio_pending_irqs() -> since sdio_single_irq is non NULL the
> func->handler is directly called without checking pending interrupt bit
> CCCR_INTx.
> mmc_release_host()
> ...
> 
> Irrespective of the function driver state, sdio driver should not call
> func->handler() if it is not real interrupt. I knew that function handler
> should be able to handle these interrupts gracefully. But it is just not the
> right thing for sdio driver to generate spurious interrupts.

I agree with you.  There is no point calling interrupt handlers 
spuriously if we can.  The patch below should fix that.  But that 
doesn't mean that your driver is not buggy as well.  In other words, the 
patch below is no excuse for you not to fix your code too!

----- >8
Subject: [PATCH] mmc: sdio: avoid spurious calls to interrupt handlers

Commit 06e8935feb "optimized SDIO IRQ handling for single irq" 
introduced some spurious calls to SDIO function interrupt handlers,
such as when the SDIO IRQ thread is started, or the safety check 
performed upon a system resume.  Let's add a flag to perform the
optimization only when a real interrupt is signaled by the host
driver and we know there is no point confirming it.

Reported-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
Signed-off-by: Nicolas Pitre <nico@linaro.org>
Cc: stable@kernel.org

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index f573e7f9f7..3d8ceb4084 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -28,18 +28,20 @@
 
 #include "sdio_ops.h"
 
-static int process_sdio_pending_irqs(struct mmc_card *card)
+static int process_sdio_pending_irqs(struct mmc_host *host)
 {
+	struct mmc_card *card = host->card;
 	int i, ret, count;
 	unsigned char pending;
 	struct sdio_func *func;
 
 	/*
 	 * Optimization, if there is only 1 function interrupt registered
-	 * call irq handler directly
+	 * and we know an IRQ was signaled then call irq handler directly.
+	 * Otherwise do the full probe.
 	 */
 	func = card->sdio_single_irq;
-	if (func) {
+	if (func && host->sdio_irq_pending) {
 		func->irq_handler(func);
 		return 1;
 	}
@@ -116,7 +118,8 @@ static int sdio_irq_thread(void *_host)
 		ret = __mmc_claim_host(host, &host->sdio_irq_thread_abort);
 		if (ret)
 			break;
-		ret = process_sdio_pending_irqs(host->card);
+		ret = process_sdio_pending_irqs(host);
+		host->sdio_irq_pending = false;
 		mmc_release_host(host);
 
 		/*
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index ee2b0363c0..557aa4cd66 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -323,6 +323,7 @@ struct mmc_host {
 
 	unsigned int		sdio_irqs;
 	struct task_struct	*sdio_irq_thread;
+	bool			sdio_irq_pending;
 	atomic_t		sdio_irq_thread_abort;
 
 	mmc_pm_flag_t		pm_flags;	/* requested pm features */
@@ -378,6 +379,7 @@ extern int mmc_cache_ctrl(struct mmc_host *, u8);
 static inline void mmc_signal_sdio_irq(struct mmc_host *host)
 {
 	host->ops->enable_sdio_irq(host, 0);
+	host->sdio_irq_pending = true;
 	wake_up_process(host->sdio_irq_thread);
 }