[PATCH 3.12] mwifiex: fix SDIO interrupt lost issue

[PATCH 3.12] mwifiex: fix SDIO interrupt lost issue

[Bing Zhao]

From: Amitkumar Karwar <akarwar@marvell.com>

601216e "mwifiex: process RX packets in SDIO IRQ thread directly"
introduced a command timeout issue which can be reproduced easily on
an AM33xx platform using a test application written by Daniel Mack:

https://gist.github.com/zonque/6579314

mwifiex_main_process() is called from both the SDIO handler and
the workqueue. In case an interrupt occurs right after the
int_status check, but before updating the mwifiex_processing flag,
this interrupt gets lost, resulting in a command timeout and
consequently a card reset.

Let main_proc_lock protect both int_status and mwifiex_processing
flag. This fixes the interrupt lost issue.

Cc: <stable@vger.kernel.org> # 3.7+
Reported-by: Sven Neumann <s.neumann@raumfeld.com>
Reported-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
Tested-by: Daniel Mack <zonque@gmail.com>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
Signed-off-by: Paul Stewart <pstew@chromium.org>
---
 drivers/net/wireless/mwifiex/main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/main.c b/drivers/net/wireless/mwifiex/main.c
index fd77833..c2b91f5 100644
--- a/drivers/net/wireless/mwifiex/main.c
+++ b/drivers/net/wireless/mwifiex/main.c
@@ -358,10 +358,12 @@ process_start:
 		}
 	} while (true);
 
-	if ((adapter->int_status) || IS_CARD_RX_RCVD(adapter))
+	spin_lock_irqsave(&adapter->main_proc_lock, flags);
+	if ((adapter->int_status) || IS_CARD_RX_RCVD(adapter)) {
+		spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
 		goto process_start;
+	}
 
-	spin_lock_irqsave(&adapter->main_proc_lock, flags);
 	adapter->mwifiex_processing = false;
 	spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
 
-- 
1.8.2.3

Re: [PATCH 3.12] mwifiex: fix SDIO interrupt lost issue

[Daniel Mack]

On 27.09.2013 19:55, Bing Zhao wrote:
> From: Amitkumar Karwar <akarwar@marvell.com>
> 
> 601216e "mwifiex: process RX packets in SDIO IRQ thread directly"
> introduced a command timeout issue which can be reproduced easily on
> an AM33xx platform using a test application written by Daniel Mack:
> 
> https://gist.github.com/zonque/6579314

Actually, just out of pure curiosity, I would be interested whether the
issue is also as easily reproducible on other platform like Chromebooks.
Could anyone give that a try maybe?


Thanks,
Daniel


> 
> mwifiex_main_process() is called from both the SDIO handler and
> the workqueue. In case an interrupt occurs right after the
> int_status check, but before updating the mwifiex_processing flag,
> this interrupt gets lost, resulting in a command timeout and
> consequently a card reset.
> 
> Let main_proc_lock protect both int_status and mwifiex_processing
> flag. This fixes the interrupt lost issue.
> 
> Cc: <stable@vger.kernel.org> # 3.7+
> Reported-by: Sven Neumann <s.neumann@raumfeld.com>
> Reported-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> Tested-by: Daniel Mack <zonque@gmail.com>
> Reviewed-by: Dylan Reid <dgreid@chromium.org>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> Signed-off-by: Paul Stewart <pstew@chromium.org>
> ---
>  drivers/net/wireless/mwifiex/main.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/mwifiex/main.c b/drivers/net/wireless/mwifiex/main.c
> index fd77833..c2b91f5 100644
> --- a/drivers/net/wireless/mwifiex/main.c
> +++ b/drivers/net/wireless/mwifiex/main.c
> @@ -358,10 +358,12 @@ process_start:
>  		}
>  	} while (true);
>  
> -	if ((adapter->int_status) || IS_CARD_RX_RCVD(adapter))
> +	spin_lock_irqsave(&adapter->main_proc_lock, flags);
> +	if ((adapter->int_status) || IS_CARD_RX_RCVD(adapter)) {
> +		spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
>  		goto process_start;
> +	}
>  
> -	spin_lock_irqsave(&adapter->main_proc_lock, flags);
>  	adapter->mwifiex_processing = false;
>  	spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
>  
> 

Re: [PATCH 3.12] mwifiex: fix SDIO interrupt lost issue

[Daniel Mack]

On 27.09.2013 23:11, Paul Stewart wrote:
> On Fri, Sep 27, 2013 at 11:55 AM, Daniel Mack <zonque@gmail.com
> <mailto:zonque@gmail.com>> wrote:
> 
>     On 27.09.2013 19 <tel:27.09.2013%2019>:55, Bing Zhao wrote:
>     > From: Amitkumar Karwar <akarwar@marvell.com
>     <mailto:akarwar@marvell.com>>
>     >
>     > 601216e "mwifiex: process RX packets in SDIO IRQ thread directly"
>     > introduced a command timeout issue which can be reproduced easily on
>     > an AM33xx platform using a test application written by Daniel Mack:
>     >
>     > https://gist.github.com/zonque/6579314
> 
>     Actually, just out of pure curiosity, I would be interested whether the
>     issue is also as easily reproducible on other platform like Chromebooks.
>     Could anyone give that a try maybe?
> 
> I would hesitate to say "easily".   Even on a kernel before your change,
> running your test application

That might be a dumb remark, but you noticed that the interface name in
the code doesn't actually match the name of the interface mwifiex
creates by default? We changed that to "wlan0" in our system to bring it
in line with other platforms.

I changed the test tool in the gist so it requires you to pass the name
as first argument.


Daniel

Re: [PATCH 3.12] mwifiex: fix SDIO interrupt lost issue

[Daniel Mack]

On 27.09.2013 19:55, Bing Zhao wrote:
> From: Amitkumar Karwar <akarwar@marvell.com>
> 
> 601216e "mwifiex: process RX packets in SDIO IRQ thread directly"
> introduced a command timeout issue which can be reproduced easily on
> an AM33xx platform using a test application written by Daniel Mack:
> 
> https://gist.github.com/zonque/6579314
> 
> mwifiex_main_process() is called from both the SDIO handler and
> the workqueue. In case an interrupt occurs right after the
> int_status check, but before updating the mwifiex_processing flag,
> this interrupt gets lost, resulting in a command timeout and
> consequently a card reset.
> 
> Let main_proc_lock protect both int_status and mwifiex_processing
> flag. This fixes the interrupt lost issue.

John, could you take this patch trough your tree?


Thanks,
Daniel


> 
> Cc: <stable@vger.kernel.org> # 3.7+
> Reported-by: Sven Neumann <s.neumann@raumfeld.com>
> Reported-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> Tested-by: Daniel Mack <zonque@gmail.com>
> Reviewed-by: Dylan Reid <dgreid@chromium.org>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> Signed-off-by: Paul Stewart <pstew@chromium.org>
> ---
>  drivers/net/wireless/mwifiex/main.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/mwifiex/main.c b/drivers/net/wireless/mwifiex/main.c
> index fd77833..c2b91f5 100644
> --- a/drivers/net/wireless/mwifiex/main.c
> +++ b/drivers/net/wireless/mwifiex/main.c
> @@ -358,10 +358,12 @@ process_start:
>  		}
>  	} while (true);
>  
> -	if ((adapter->int_status) || IS_CARD_RX_RCVD(adapter))
> +	spin_lock_irqsave(&adapter->main_proc_lock, flags);
> +	if ((adapter->int_status) || IS_CARD_RX_RCVD(adapter)) {
> +		spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
>  		goto process_start;
> +	}
>  
> -	spin_lock_irqsave(&adapter->main_proc_lock, flags);
>  	adapter->mwifiex_processing = false;
>  	spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
>  
> 

Re: [PATCH 3.12] mwifiex: fix SDIO interrupt lost issue

[John W. Linville]

On Tue, Oct 15, 2013 at 12:38:50PM +0200, Daniel Mack wrote: > On 27.09.2013 19:55, Bing Zhao wrote:
> > From: Amitkumar Karwar <akarwar@marvell.com>
> > 
> > 601216e "mwifiex: process RX packets in SDIO IRQ thread directly"
> > introduced a command timeout issue which can be reproduced easily on
> > an AM33xx platform using a test application written by Daniel Mack:
> > 
> > https://gist.github.com/zonque/6579314
> > 
> > mwifiex_main_process() is called from both the SDIO handler and
> > the workqueue. In case an interrupt occurs right after the
> > int_status check, but before updating the mwifiex_processing flag,
> > this interrupt gets lost, resulting in a command timeout and
> > consequently a card reset.
> > 
> > Let main_proc_lock protect both int_status and mwifiex_processing
> > flag. This fixes the interrupt lost issue.
> 
> John, could you take this patch trough your tree?

I already did, and it is already in Dave's tree too...

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.