2018-01-22 15:09:27

by Ganapathi Bhat

[permalink] [raw]
Subject: [PATCH 0/2] mwifiex: fix RX data stall issue on USB interface

This patch series contains 2 fixes to avoid RX data stall on
observed on high through put scenarios on some slower platforms.

Shrenik Shikhare (2):
mwifiex: schedule rx_work on RX interrupt for USB
mwifiex: use more_rx_task_flag to avoid USB RX stall

drivers/net/wireless/marvell/mwifiex/main.c | 17 +++++++++++++----
drivers/net/wireless/marvell/mwifiex/main.h | 2 ++
drivers/net/wireless/marvell/mwifiex/usb.c | 2 ++
3 files changed, 17 insertions(+), 4 deletions(-)

--
1.9.1


2018-01-25 07:10:53

by Kalle Valo

[permalink] [raw]
Subject: Re: [1/2] mwifiex: schedule rx_work on RX interrupt for USB

Ganapathi Bhat <[email protected]> wrote:

> From: Shrenik Shikhare <[email protected]>
>
> There is race for data_received flag between main thread and
> RX data interrupt(mwifiex_usb_rx_complete()):
> 1. USB received an RX data interrupt, set data_received flag
> 2. main thread checks data_received, if set queues rx_work
> 3. rx worker thread independently start processing rx_data_q
> 4. rx work exits (once rx_data_q is empty)
> 5. main thread resets the data_received flag(after #2)
> 6. Now at the corner case there will be high RX data interrupts
> between #4 and #5
> 7. Driver stops submitting URBs to firmware, once rx_pending
> exceeds HIGH_RX_PENDING
> 8. The flag data_received(cleared in #5) will remain unset since
> there will be no interrupts from firmware to set it(after #7)
>
> Above scenario causes RX stall in driver, which will finally
> result in command/TX timeouts in firmware.
>
> As a fix, queue rx_work directly in mwifiex_usb_rx_complete()
> callback, instead in the main thread. This removes dependency
> of RX processing on data_received flag.
>
> Signed-off-by: Cathy Luo <[email protected]>
> Signed-off-by: Ganapathi Bhat <[email protected]>

Brian, did you have a chance to review these two?

--
https://patchwork.kernel.org/patch/10178731/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2018-01-25 07:13:47

by Kalle Valo

[permalink] [raw]
Subject: Re: [2/2] mwifiex: use more_rx_task_flag to avoid USB RX stall

Kalle Valo <[email protected]> writes:

> Ganapathi Bhat <[email protected]> wrote:
>
>> From: Shrenik Shikhare <[email protected]>
>>
>> There is a race condition for acquiring rx_proc_lock between
>> rx worker thread and USB RX data interrupt
>> (mwifiex_usb_rx_complete):
>>
>> 1. USB receives an RX data interrupt, queues rx_work
>> 2. rx_work empties rx_data_q, tries to acquire rx_proc_lock (to
>> clear rx_processing flag)
>> 3. While #2 is yet to acquire rx_proc_lock, driver receives
>> continuous RX data interupts(mwifiex_usb_rx_complete)
>> 3. For each interrupt at #3, driver acquires rx_proc_lock(it gets
>> the lock since it is in interrupt context), tries to queue
>> rx_work, but fails to do so since rx_processing is still set(#2)
>> 4. When rx_pending exceeds HIGH_RX_PENDING, driver stops
>> submitting URBs back to USB subsystem and thus firmware stops
>> uploading RX data to driver
>> 5. Now finally #2 will acquire rx_proc_lock, but because of #4,
>> there are no further triggers to schedule rx_work again
>>
>> The above scenario occurs in some platforms where the RX
>> processing is comparitively slower. This results in RX stall in
>> driver, command/TX timeouts in firmware. The above scenario is
>> introduced after commit c7dbdcb2a4e1
>> ("mwifiex: schedule rx_work on RX interrupt for USB")
>>
>> To fix this set a new more_rx_task_flag whenever RX data callback
>> is trying to schedule rx_work but rx_processing is not yet
>> cleared. This will let the current rx_work(which was waiting for
>> rx_proc_lock) to loopback and process newly arrived RX packets.
>>
>> Signed-off-by: Cathy Luo <[email protected]>
>> Signed-off-by: Ganapathi Bhat <[email protected]>
>
> I can't find any commit with id c7dbdcb2a4e1, is it correct?

Oh, and please use Fixes line to mark the commit which broke this.

--
Kalle Valo

2018-01-22 15:09:31

by Ganapathi Bhat

[permalink] [raw]
Subject: [PATCH 2/2] mwifiex: use more_rx_task_flag to avoid USB RX stall

From: Shrenik Shikhare <[email protected]>

There is a race condition for acquiring rx_proc_lock between
rx worker thread and USB RX data interrupt
(mwifiex_usb_rx_complete):

1. USB receives an RX data interrupt, queues rx_work
2. rx_work empties rx_data_q, tries to acquire rx_proc_lock (to
clear rx_processing flag)
3. While #2 is yet to acquire rx_proc_lock, driver receives
continuous RX data interupts(mwifiex_usb_rx_complete)
3. For each interrupt at #3, driver acquires rx_proc_lock(it gets
the lock since it is in interrupt context), tries to queue
rx_work, but fails to do so since rx_processing is still set(#2)
4. When rx_pending exceeds HIGH_RX_PENDING, driver stops
submitting URBs back to USB subsystem and thus firmware stops
uploading RX data to driver
5. Now finally #2 will acquire rx_proc_lock, but because of #4,
there are no further triggers to schedule rx_work again

The above scenario occurs in some platforms where the RX
processing is comparitively slower. This results in RX stall in
driver, command/TX timeouts in firmware. The above scenario is
introduced after commit c7dbdcb2a4e1
("mwifiex: schedule rx_work on RX interrupt for USB")

To fix this set a new more_rx_task_flag whenever RX data callback
is trying to schedule rx_work but rx_processing is not yet
cleared. This will let the current rx_work(which was waiting for
rx_proc_lock) to loopback and process newly arrived RX packets.

Signed-off-by: Cathy Luo <[email protected]>
Signed-off-by: Ganapathi Bhat <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/main.c | 10 +++++++++-
drivers/net/wireless/marvell/mwifiex/main.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 6e6e1a7..ea87c7c 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -163,6 +163,7 @@ void mwifiex_queue_main_work(struct mwifiex_adapter *adapter)
spin_lock_irqsave(&adapter->main_proc_lock, flags);
if (adapter->mwifiex_processing) {
adapter->more_task_flag = true;
+ adapter->more_rx_task_flag = true;
spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
} else {
spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
@@ -177,6 +178,7 @@ void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter)

spin_lock_irqsave(&adapter->rx_proc_lock, flags);
if (adapter->rx_processing) {
+ adapter->more_rx_task_flag = true;
spin_unlock_irqrestore(&adapter->rx_proc_lock, flags);
} else {
spin_unlock_irqrestore(&adapter->rx_proc_lock, flags);
@@ -193,13 +195,14 @@ static int mwifiex_process_rx(struct mwifiex_adapter *adapter)

spin_lock_irqsave(&adapter->rx_proc_lock, flags);
if (adapter->rx_processing || adapter->rx_locked) {
+ adapter->more_rx_task_flag = true;
spin_unlock_irqrestore(&adapter->rx_proc_lock, flags);
goto exit_rx_proc;
} else {
adapter->rx_processing = true;
spin_unlock_irqrestore(&adapter->rx_proc_lock, flags);
}
-
+rx_process_start:
/* Check for Rx data */
while ((skb = skb_dequeue(&adapter->rx_data_q))) {
atomic_dec(&adapter->rx_pending);
@@ -221,6 +224,11 @@ static int mwifiex_process_rx(struct mwifiex_adapter *adapter)
}
}
spin_lock_irqsave(&adapter->rx_proc_lock, flags);
+ if (adapter->more_rx_task_flag) {
+ adapter->more_rx_task_flag = false;
+ spin_unlock_irqrestore(&adapter->rx_proc_lock, flags);
+ goto rx_process_start;
+ }
adapter->rx_processing = false;
spin_unlock_irqrestore(&adapter->rx_proc_lock, flags);

diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 66ba95c..242e05e 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -891,6 +891,7 @@ struct mwifiex_adapter {
spinlock_t main_proc_lock;
u32 mwifiex_processing;
u8 more_task_flag;
+ u8 more_rx_task_flag;
u16 tx_buf_size;
u16 curr_tx_buf_size;
/* sdio single port rx aggregation capability */
--
1.9.1

2018-01-25 10:02:50

by Ganapathi Bhat

[permalink] [raw]
Subject: RE: [EXT] Re: [2/2] mwifiex: use more_rx_task_flag to avoid USB RX stall

Hi Kalle,

> ----------------------------------------------------------------------
> Kalle Valo <[email protected]> writes:
>
> > Ganapathi Bhat <[email protected]> wrote:
> >
> >> From: Shrenik Shikhare <[email protected]>
> >>
> >> There is a race condition for acquiring rx_proc_lock between rx
> >> worker thread and USB RX data interrupt
> >> (mwifiex_usb_rx_complete):
> >>
> >> 1. USB receives an RX data interrupt, queues rx_work 2. rx_work
> >> empties rx_data_q, tries to acquire rx_proc_lock (to clear
> >> rx_processing flag) 3. While #2 is yet to acquire rx_proc_lock,
> >> driver receives continuous RX data interupts(mwifiex_usb_rx_complete)
> >> 3. For each interrupt at #3, driver acquires rx_proc_lock(it gets the
> >> lock since it is in interrupt context), tries to queue rx_work, but
> >> fails to do so since rx_processing is still set(#2) 4. When
> >> rx_pending exceeds HIGH_RX_PENDING, driver stops submitting URBs
> back
> >> to USB subsystem and thus firmware stops uploading RX data to driver
> >> 5. Now finally #2 will acquire rx_proc_lock, but because of #4, there
> >> are no further triggers to schedule rx_work again
> >>
> >> The above scenario occurs in some platforms where the RX processing
> >> is comparitively slower. This results in RX stall in driver,
> >> command/TX timeouts in firmware. The above scenario is introduced
> >> after commit c7dbdcb2a4e1
> >> ("mwifiex: schedule rx_work on RX interrupt for USB")
> >>
> >> To fix this set a new more_rx_task_flag whenever RX data callback is
> >> trying to schedule rx_work but rx_processing is not yet cleared. This
> >> will let the current rx_work(which was waiting for
> >> rx_proc_lock) to loopback and process newly arrived RX packets.
> >>
> >> Signed-off-by: Cathy Luo <[email protected]>
> >> Signed-off-by: Ganapathi Bhat <[email protected]>
> >
> > I can't find any commit with id c7dbdcb2a4e1, is it correct?
>
> Oh, and please use Fixes line to mark the commit which broke this.
Ok Sure. I will do that and send v2.
>
> --
> Kalle Valo
Regards,
Ganapathi

2018-01-25 18:59:51

by Brian Norris

[permalink] [raw]
Subject: Re: [PATCH 1/2] mwifiex: schedule rx_work on RX interrupt for USB

On Mon, Jan 22, 2018 at 08:34:56PM +0530, Ganapathi Bhat wrote:
> From: Shrenik Shikhare <[email protected]>
>
> There is race for data_received flag between main thread and
> RX data interrupt(mwifiex_usb_rx_complete()):
> 1. USB received an RX data interrupt, set data_received flag
> 2. main thread checks data_received, if set queues rx_work

Stop right there.

There is a flag, data_received, and as you say, you are setting it one
thread, and reading it in another thread (and later clearing it; step
#5). Where is the locking? There is none. Therefore, you have a data
race.

You are not resolving any locking problems here, so you're not really
solving the entire problem.

Brian

> 3. rx worker thread independently start processing rx_data_q
> 4. rx work exits (once rx_data_q is empty)
> 5. main thread resets the data_received flag(after #2)
> 6. Now at the corner case there will be high RX data interrupts
> between #4 and #5
> 7. Driver stops submitting URBs to firmware, once rx_pending
> exceeds HIGH_RX_PENDING
> 8. The flag data_received(cleared in #5) will remain unset since
> there will be no interrupts from firmware to set it(after #7)
>
> Above scenario causes RX stall in driver, which will finally
> result in command/TX timeouts in firmware.
>
> As a fix, queue rx_work directly in mwifiex_usb_rx_complete()
> callback, instead in the main thread. This removes dependency
> of RX processing on data_received flag.
>
> Signed-off-by: Cathy Luo <[email protected]>
> Signed-off-by: Ganapathi Bhat <[email protected]>
> ---
> drivers/net/wireless/marvell/mwifiex/main.c | 7 ++++---
> drivers/net/wireless/marvell/mwifiex/main.h | 1 +
> drivers/net/wireless/marvell/mwifiex/usb.c | 2 ++
> 3 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index 12e7399..6e6e1a7 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -171,7 +171,7 @@ void mwifiex_queue_main_work(struct mwifiex_adapter *adapter)
> }
> EXPORT_SYMBOL_GPL(mwifiex_queue_main_work);
>
> -static void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter)
> +void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter)
> {
> unsigned long flags;
>
> @@ -183,6 +183,7 @@ static void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter)
> queue_work(adapter->rx_workqueue, &adapter->rx_work);
> }
> }
> +EXPORT_SYMBOL_GPL(mwifiex_queue_rx_work);
>
> static int mwifiex_process_rx(struct mwifiex_adapter *adapter)
> {
> @@ -283,10 +284,10 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
> mwifiex_process_hs_config(adapter);
> if (adapter->if_ops.process_int_status)
> adapter->if_ops.process_int_status(adapter);
> + if (adapter->rx_work_enabled && adapter->data_received)
> + mwifiex_queue_rx_work(adapter);
> }
>
> - if (adapter->rx_work_enabled && adapter->data_received)
> - mwifiex_queue_rx_work(adapter);
>
> /* Need to wake up the card ? */
> if ((adapter->ps_state == PS_STATE_SLEEP) &&
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> index 6b5539b..66ba95c 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -1667,6 +1667,7 @@ u8 mwifiex_adjust_data_rate(struct mwifiex_private *priv,
> void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter);
> void *mwifiex_alloc_dma_align_buf(int rx_len, gfp_t flags);
> void mwifiex_queue_main_work(struct mwifiex_adapter *adapter);
> +void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter);
> int mwifiex_get_wakeup_reason(struct mwifiex_private *priv, u16 action,
> int cmd_type,
> struct mwifiex_ds_wakeup_reason *wakeup_reason);
> diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
> index 4bc2448..d20fda1 100644
> --- a/drivers/net/wireless/marvell/mwifiex/usb.c
> +++ b/drivers/net/wireless/marvell/mwifiex/usb.c
> @@ -144,6 +144,8 @@ static int mwifiex_usb_recv(struct mwifiex_adapter *adapter,
> skb_queue_tail(&adapter->rx_data_q, skb);
> adapter->data_received = true;
> atomic_inc(&adapter->rx_pending);
> + if (adapter->rx_work_enabled)
> + mwifiex_queue_rx_work(adapter);
> break;
> default:
> mwifiex_dbg(adapter, ERROR,
> --
> 1.9.1
>

2018-01-22 15:09:28

by Ganapathi Bhat

[permalink] [raw]
Subject: [PATCH 1/2] mwifiex: schedule rx_work on RX interrupt for USB

From: Shrenik Shikhare <[email protected]>

There is race for data_received flag between main thread and
RX data interrupt(mwifiex_usb_rx_complete()):
1. USB received an RX data interrupt, set data_received flag
2. main thread checks data_received, if set queues rx_work
3. rx worker thread independently start processing rx_data_q
4. rx work exits (once rx_data_q is empty)
5. main thread resets the data_received flag(after #2)
6. Now at the corner case there will be high RX data interrupts
between #4 and #5
7. Driver stops submitting URBs to firmware, once rx_pending
exceeds HIGH_RX_PENDING
8. The flag data_received(cleared in #5) will remain unset since
there will be no interrupts from firmware to set it(after #7)

Above scenario causes RX stall in driver, which will finally
result in command/TX timeouts in firmware.

As a fix, queue rx_work directly in mwifiex_usb_rx_complete()
callback, instead in the main thread. This removes dependency
of RX processing on data_received flag.

Signed-off-by: Cathy Luo <[email protected]>
Signed-off-by: Ganapathi Bhat <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/main.c | 7 ++++---
drivers/net/wireless/marvell/mwifiex/main.h | 1 +
drivers/net/wireless/marvell/mwifiex/usb.c | 2 ++
3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 12e7399..6e6e1a7 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -171,7 +171,7 @@ void mwifiex_queue_main_work(struct mwifiex_adapter *adapter)
}
EXPORT_SYMBOL_GPL(mwifiex_queue_main_work);

-static void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter)
+void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter)
{
unsigned long flags;

@@ -183,6 +183,7 @@ static void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter)
queue_work(adapter->rx_workqueue, &adapter->rx_work);
}
}
+EXPORT_SYMBOL_GPL(mwifiex_queue_rx_work);

static int mwifiex_process_rx(struct mwifiex_adapter *adapter)
{
@@ -283,10 +284,10 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
mwifiex_process_hs_config(adapter);
if (adapter->if_ops.process_int_status)
adapter->if_ops.process_int_status(adapter);
+ if (adapter->rx_work_enabled && adapter->data_received)
+ mwifiex_queue_rx_work(adapter);
}

- if (adapter->rx_work_enabled && adapter->data_received)
- mwifiex_queue_rx_work(adapter);

/* Need to wake up the card ? */
if ((adapter->ps_state == PS_STATE_SLEEP) &&
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 6b5539b..66ba95c 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1667,6 +1667,7 @@ u8 mwifiex_adjust_data_rate(struct mwifiex_private *priv,
void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter);
void *mwifiex_alloc_dma_align_buf(int rx_len, gfp_t flags);
void mwifiex_queue_main_work(struct mwifiex_adapter *adapter);
+void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter);
int mwifiex_get_wakeup_reason(struct mwifiex_private *priv, u16 action,
int cmd_type,
struct mwifiex_ds_wakeup_reason *wakeup_reason);
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 4bc2448..d20fda1 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -144,6 +144,8 @@ static int mwifiex_usb_recv(struct mwifiex_adapter *adapter,
skb_queue_tail(&adapter->rx_data_q, skb);
adapter->data_received = true;
atomic_inc(&adapter->rx_pending);
+ if (adapter->rx_work_enabled)
+ mwifiex_queue_rx_work(adapter);
break;
default:
mwifiex_dbg(adapter, ERROR,
--
1.9.1

2018-01-25 07:12:02

by Kalle Valo

[permalink] [raw]
Subject: Re: [2/2] mwifiex: use more_rx_task_flag to avoid USB RX stall

Ganapathi Bhat <[email protected]> wrote:

> From: Shrenik Shikhare <[email protected]>
>
> There is a race condition for acquiring rx_proc_lock between
> rx worker thread and USB RX data interrupt
> (mwifiex_usb_rx_complete):
>
> 1. USB receives an RX data interrupt, queues rx_work
> 2. rx_work empties rx_data_q, tries to acquire rx_proc_lock (to
> clear rx_processing flag)
> 3. While #2 is yet to acquire rx_proc_lock, driver receives
> continuous RX data interupts(mwifiex_usb_rx_complete)
> 3. For each interrupt at #3, driver acquires rx_proc_lock(it gets
> the lock since it is in interrupt context), tries to queue
> rx_work, but fails to do so since rx_processing is still set(#2)
> 4. When rx_pending exceeds HIGH_RX_PENDING, driver stops
> submitting URBs back to USB subsystem and thus firmware stops
> uploading RX data to driver
> 5. Now finally #2 will acquire rx_proc_lock, but because of #4,
> there are no further triggers to schedule rx_work again
>
> The above scenario occurs in some platforms where the RX
> processing is comparitively slower. This results in RX stall in
> driver, command/TX timeouts in firmware. The above scenario is
> introduced after commit c7dbdcb2a4e1
> ("mwifiex: schedule rx_work on RX interrupt for USB")
>
> To fix this set a new more_rx_task_flag whenever RX data callback
> is trying to schedule rx_work but rx_processing is not yet
> cleared. This will let the current rx_work(which was waiting for
> rx_proc_lock) to loopback and process newly arrived RX packets.
>
> Signed-off-by: Cathy Luo <[email protected]>
> Signed-off-by: Ganapathi Bhat <[email protected]>

I can't find any commit with id c7dbdcb2a4e1, is it correct?

--
https://patchwork.kernel.org/patch/10178729/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2018-01-29 07:21:26

by Ganapathi Bhat

[permalink] [raw]
Subject: RE: [EXT] Re: [PATCH 1/2] mwifiex: schedule rx_work on RX interrupt for USB

Hi Brian,
> -----Original Message-----
> From: Brian Norris [mailto:[email protected]]
> Sent: Friday, January 26, 2018 12:30 AM
> To: Ganapathi Bhat
> Cc: [email protected]; Cathy Luo; Xinming Hu; Zhiyuan Yang;
> James Cao; Mangesh Malusare; Shrenik Shikhare
> Subject: [EXT] Re: [PATCH 1/2] mwifiex: schedule rx_work on RX interrupt for
> USB
>
> External Email
>
> ----------------------------------------------------------------------
> On Mon, Jan 22, 2018 at 08:34:56PM +0530, Ganapathi Bhat wrote:
> > From: Shrenik Shikhare <[email protected]>
> >
> > There is race for data_received flag between main thread and RX data
> > interrupt(mwifiex_usb_rx_complete()):
> > 1. USB received an RX data interrupt, set data_received flag 2. main
> > thread checks data_received, if set queues rx_work
>
> Stop right there.
>
> There is a flag, data_received, and as you say, you are setting it one thread,
> and reading it in another thread (and later clearing it; step #5). Where is the
> locking? There is none. Therefore, you have a data race.
Yes. We missed it. We will add the locking and send it in v3.
>
> You are not resolving any locking problems here, so you're not really solving
> the entire problem.
>
> Brian
>
> > 3. rx worker thread independently start processing rx_data_q 4. rx
> > work exits (once rx_data_q is empty) 5. main thread resets the
> > data_received flag(after #2) 6. Now at the corner case there will be
> > high RX data interrupts between #4 and #5 7. Driver stops submitting
> > URBs to firmware, once rx_pending exceeds HIGH_RX_PENDING 8. The
> flag
> > data_received(cleared in #5) will remain unset since there will be no
> > interrupts from firmware to set it(after #7)
> >
> > Above scenario causes RX stall in driver, which will finally result in
> > command/TX timeouts in firmware.
> >
> > As a fix, queue rx_work directly in mwifiex_usb_rx_complete()
> > callback, instead in the main thread. This removes dependency of RX
> > processing on data_received flag.
> >
> > Signed-off-by: Cathy Luo <[email protected]>
> > Signed-off-by: Ganapathi Bhat <[email protected]>
> > ---
> > drivers/net/wireless/marvell/mwifiex/main.c | 7 ++++---
> > drivers/net/wireless/marvell/mwifiex/main.h | 1 +
> > drivers/net/wireless/marvell/mwifiex/usb.c | 2 ++
> > 3 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c
> > b/drivers/net/wireless/marvell/mwifiex/main.c
> > index 12e7399..6e6e1a7 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -171,7 +171,7 @@ void mwifiex_queue_main_work(struct
> > mwifiex_adapter *adapter) }
> > EXPORT_SYMBOL_GPL(mwifiex_queue_main_work);
> >
> > -static void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter)
> > +void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter)
> > {
> > unsigned long flags;
> >
> > @@ -183,6 +183,7 @@ static void mwifiex_queue_rx_work(struct
> mwifiex_adapter *adapter)
> > queue_work(adapter->rx_workqueue, &adapter->rx_work);
> > }
> > }
> > +EXPORT_SYMBOL_GPL(mwifiex_queue_rx_work);
> >
> > static int mwifiex_process_rx(struct mwifiex_adapter *adapter) { @@
> > -283,10 +284,10 @@ int mwifiex_main_process(struct mwifiex_adapter
> *adapter)
> > mwifiex_process_hs_config(adapter);
> > if (adapter->if_ops.process_int_status)
> > adapter-
> >if_ops.process_int_status(adapter);
> > + if (adapter->rx_work_enabled && adapter-
> >data_received)
> > + mwifiex_queue_rx_work(adapter);
> > }
> >
> > - if (adapter->rx_work_enabled && adapter->data_received)
> > - mwifiex_queue_rx_work(adapter);
> >
> > /* Need to wake up the card ? */
> > if ((adapter->ps_state == PS_STATE_SLEEP) && diff --git
> > a/drivers/net/wireless/marvell/mwifiex/main.h
> > b/drivers/net/wireless/marvell/mwifiex/main.h
> > index 6b5539b..66ba95c 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.h
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> > @@ -1667,6 +1667,7 @@ u8 mwifiex_adjust_data_rate(struct
> > mwifiex_private *priv, void mwifiex_upload_device_dump(struct
> > mwifiex_adapter *adapter); void *mwifiex_alloc_dma_align_buf(int
> > rx_len, gfp_t flags); void mwifiex_queue_main_work(struct
> > mwifiex_adapter *adapter);
> > +void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter);
> > int mwifiex_get_wakeup_reason(struct mwifiex_private *priv, u16 action,
> > int cmd_type,
> > struct mwifiex_ds_wakeup_reason
> *wakeup_reason); diff --git
> > a/drivers/net/wireless/marvell/mwifiex/usb.c
> > b/drivers/net/wireless/marvell/mwifiex/usb.c
> > index 4bc2448..d20fda1 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/usb.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/usb.c
> > @@ -144,6 +144,8 @@ static int mwifiex_usb_recv(struct mwifiex_adapter
> *adapter,
> > skb_queue_tail(&adapter->rx_data_q, skb);
> > adapter->data_received = true;
> > atomic_inc(&adapter->rx_pending);
> > + if (adapter->rx_work_enabled)
> > + mwifiex_queue_rx_work(adapter);
> > break;
> > default:
> > mwifiex_dbg(adapter, ERROR,
> > --
> > 1.9.1
> >
Regards,
Ganapathi