[PATCH 3.12] mwifiex: queue main work from main process when bailing on races

[PATCH 3.12] mwifiex: queue main work from main process when bailing on races

[Daniel Mack]

Queue main_work in case mwifiex_main_process() bails due to an already
processed transaction. This is particularly necessary because
mwifiex_main_process() is called from both the SDIO interrupt handler
and the workqueue. In case an interrupt occurs while the main process
is currently executed from the workqueue, the interrupt is lost,
resulting in a command timeout and consequently a card reset.

I'm marking this for stable kernel in version 3.7+, because on our
platform, the issue appears since 601216e12c ("mwifiex: process RX
packets in SDIO IRQ thread directly") went in.

Signed-off-by: Daniel Mack <zonque@gmail.com>
Reported-by: Sven Neumann <s.neumann@raumfeld.com>
Reported-by: Andreas Fenkart <afenkart@gmail.com>
Cc: Bing Zhao <bzhao@marvell.com>
Cc: <stable@vger.kernel.org> [v3.7+]
---
 drivers/net/wireless/mwifiex/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/mwifiex/main.c b/drivers/net/wireless/mwifiex/main.c
index ff4ed96..0700bc2 100644
--- a/drivers/net/wireless/mwifiex/main.c
+++ b/drivers/net/wireless/mwifiex/main.c
@@ -235,6 +235,7 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
 	/* Check if already processing */
 	if (adapter->mwifiex_processing) {
 		spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
+		queue_work(adapter->workqueue, &adapter->main_work);
 		goto exit_main_proc;
 	} else {
 		adapter->mwifiex_processing = true;
-- 
1.8.3.1

RE: [PATCH 3.12] mwifiex: queue main work from main process when bailing on races

[Bing Zhao]

Hi Daniel,

> Queue main_work in case mwifiex_main_process() bails due to an already
> processed transaction. This is particularly necessary because
> mwifiex_main_process() is called from both the SDIO interrupt handler and
> the workqueue. In case an interrupt occurs while the main process is
> currently executed from the workqueue, the interrupt is lost, resulting in a
> command timeout and consequently a card reset.
> 
> I'm marking this for stable kernel in version 3.7+, because on our platform,
> the issue appears since 601216e12c ("mwifiex: process RX packets in SDIO
> IRQ thread directly") went in.
> 
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> Reported-by: Sven Neumann <s.neumann@raumfeld.com>
> Reported-by: Andreas Fenkart <afenkart@gmail.com>
> Cc: Bing Zhao <bzhao@marvell.com>
> Cc: <stable@vger.kernel.org> [v3.7+]

Acked-by: Bing Zhao <bzhao@marvell.com>

Thanks,
Bing

> ---
>  drivers/net/wireless/mwifiex/main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/wireless/mwifiex/main.c
> b/drivers/net/wireless/mwifiex/main.c
> index ff4ed96..0700bc2 100644
> --- a/drivers/net/wireless/mwifiex/main.c
> +++ b/drivers/net/wireless/mwifiex/main.c
> @@ -235,6 +235,7 @@ int mwifiex_main_process(struct mwifiex_adapter
> *adapter)
>  	/* Check if already processing */
>  	if (adapter->mwifiex_processing) {
>  		spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
> +		queue_work(adapter->workqueue, &adapter->main_work);
>  		goto exit_main_proc;
>  	} else {
>  		adapter->mwifiex_processing = true;
> --
> 1.8.3.1

Re: [PATCH 3.12] mwifiex: queue main work from main process when bailing on races

[Daniel Mack]

On 17.09.2013 21:15, Bing Zhao wrote:
>> Queue main_work in case mwifiex_main_process() bails due to an already
>> processed transaction. This is particularly necessary because
>> mwifiex_main_process() is called from both the SDIO interrupt handler and
>> the workqueue. In case an interrupt occurs while the main process is
>> currently executed from the workqueue, the interrupt is lost, resulting in a
>> command timeout and consequently a card reset.
>>
>> I'm marking this for stable kernel in version 3.7+, because on our platform,
>> the issue appears since 601216e12c ("mwifiex: process RX packets in SDIO
>> IRQ thread directly") went in.
>>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>> Reported-by: Sven Neumann <s.neumann@raumfeld.com>
>> Reported-by: Andreas Fenkart <afenkart@gmail.com>
>> Cc: Bing Zhao <bzhao@marvell.com>
>> Cc: <stable@vger.kernel.org> [v3.7+]
> 
> Acked-by: Bing Zhao <bzhao@marvell.com>

John, could you pick this one?


Many thanks,
Daniel


> 
> Thanks,
> Bing
> 
>> ---
>>  drivers/net/wireless/mwifiex/main.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/wireless/mwifiex/main.c
>> b/drivers/net/wireless/mwifiex/main.c
>> index ff4ed96..0700bc2 100644
>> --- a/drivers/net/wireless/mwifiex/main.c
>> +++ b/drivers/net/wireless/mwifiex/main.c
>> @@ -235,6 +235,7 @@ int mwifiex_main_process(struct mwifiex_adapter
>> *adapter)
>>  	/* Check if already processing */
>>  	if (adapter->mwifiex_processing) {
>>  		spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
>> +		queue_work(adapter->workqueue, &adapter->main_work);
>>  		goto exit_main_proc;
>>  	} else {
>>  		adapter->mwifiex_processing = true;
>> --
>> 1.8.3.1
> 

RE: [PATCH 3.12] mwifiex: queue main work from main process when bailing on races

[Bing Zhao]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

Hi Daniel,

> > Acked-by: Bing Zhao <bzhao@marvell.com>
> 
> John, could you pick this one?

We found that this patch causes CPU utilization issue on Chromebooks.
Could you please try attached patch on your platform? Basically this patch reverts your change and add main_proc_lock protection for int_status and IS_CARD_RX_RCVD access to avoid the missing interrupt handling.

Thanks,
Bing
 

[-- Attachment #2: 0001-mwifiex-avoid-redundant-main-process-queueing.patch --]
[-- Type: application/octet-stream, Size: 1965 bytes --]

From 43b3fcb3e58b6d10b70c08f5b2259a64a195f1a5 Mon Sep 17 00:00:00 2001
From: Amitkumar Karwar <akarwar@marvell.com>
Date: Wed, 25 Sep 2013 21:21:29 +0530
Subject: [PATCH] mwifiex: avoid redundant main process queueing

As we use single threaded workqueue, the only possiblity
for concurrent mwifiex_main_process() calls is during SDIO
interrupts where the routine is directly called.

Recently queue_work() call is added to fix corner case
SDIO interrupt lost issue. With this change main process
is sometimes unnecessarily queued multiple times which results
in increased CPU utilization.

This patch makes sure that currently running main process
will always take care of pending interrupts. We don't need to
queue an extra main process. This fixes interrupt lost issue.

Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
 drivers/net/wireless/mwifiex/main.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/main.c b/drivers/net/wireless/mwifiex/main.c
index 516b80e..dd6548e 100644
--- a/drivers/net/wireless/mwifiex/main.c
+++ b/drivers/net/wireless/mwifiex/main.c
@@ -235,7 +235,6 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
 	/* Check if already processing */
 	if (adapter->mwifiex_processing) {
 		spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
-		queue_work(adapter->workqueue, &adapter->main_work);
 		goto exit_main_proc;
 	} else {
 		adapter->mwifiex_processing = true;
@@ -359,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.7.3.4

Re: [PATCH 3.12] mwifiex: queue main work from main process when bailing on races

[Daniel Mack]

Hi Bing,

On 25.09.2013 18:19, Bing Zhao wrote:
>>> Acked-by: Bing Zhao <bzhao@marvell.com>
>> 
>> John, could you pick this one?
> 
> We found that this patch causes CPU utilization issue on
> Chromebooks. Could you please try attached patch on your platform?
> Basically this patch reverts your change and add main_proc_lock
> protection for int_status and IS_CARD_RX_RCVD access to avoid the
> missing interrupt handling.

Ok, that seems to work fine for me as well. Thanks for the update :)

Fell free to add my

	Tested-by: Daniel Mack <zonque@gmail.com>

if you need it.


Thanks,
Daniel