diff mbox

[2/2] mwifiex: use more_rx_task_flag to avoid USB RX stall

Message ID 1516633497-6584-3-git-send-email-gbhat@marvell.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show

Commit Message

Ganapathi Bhat Jan. 22, 2018, 3:04 p.m. UTC
From: Shrenik Shikhare <shrenik@marvell.com>

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 <cluo@marvell.com>
Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
---
 drivers/net/wireless/marvell/mwifiex/main.c | 10 +++++++++-
 drivers/net/wireless/marvell/mwifiex/main.h |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Kalle Valo Jan. 25, 2018, 7:12 a.m. UTC | #1
Ganapathi Bhat <gbhat@marvell.com> wrote:

> From: Shrenik Shikhare <shrenik@marvell.com>
> 
> 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 <cluo@marvell.com>
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>

I can't find any commit with id c7dbdcb2a4e1, is it correct?
Kalle Valo Jan. 25, 2018, 7:13 a.m. UTC | #2
Kalle Valo <kvalo@codeaurora.org> writes:

> Ganapathi Bhat <gbhat@marvell.com> wrote:
>
>> From: Shrenik Shikhare <shrenik@marvell.com>
>> 
>> 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 <cluo@marvell.com>
>> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
>
> 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.
Ganapathi Bhat Jan. 25, 2018, 9:59 a.m. UTC | #3
Hi Kalle,
>

> ----------------------------------------------------------------------

> Ganapathi Bhat <gbhat@marvell.com> wrote:

>

> > From: Shrenik Shikhare <shrenik@marvell.com>

> >

> > 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 <cluo@marvell.com>

> > Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>

>

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

Correct. Actually the commit id c7dbdcb2a4e1 is the PATCH [1/2] sent in this series.
>

> --

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

>

> https://wireless.wiki.kernel.org/en/developers/documentation/submittingp

> atches

Regards,
Ganapathi
Ganapathi Bhat Jan. 25, 2018, 10:02 a.m. UTC | #4
Hi Kalle,

> ----------------------------------------------------------------------
> Kalle Valo <kvalo@codeaurora.org> writes:
>
> > Ganapathi Bhat <gbhat@marvell.com> wrote:
> >
> >> From: Shrenik Shikhare <shrenik@marvell.com>
> >>
> >> 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 <cluo@marvell.com>
> >> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> >
> > 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
Kalle Valo Jan. 25, 2018, 12:59 p.m. UTC | #5
Ganapathi Bhat <gbhat@marvell.com> writes:

>> > 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 <cluo@marvell.com>
>> > Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
>>
>> I can't find any commit with id c7dbdcb2a4e1, is it correct?
>
> Correct. Actually the commit id c7dbdcb2a4e1 is the PATCH [1/2] sent in this series.

Actually the commit id will be different, I just tested it to be sure:

$ git reset --hard master
HEAD is now at b69c1df47281 brcmfmac: separate firmware errors from i/o errors
$ git am -s -3 1.patch
Applying: mwifiex: schedule rx_work on RX interrupt for USB
$ git log --oneline -1 | cat
676bc4833907 mwifiex: schedule rx_work on RX interrupt for USB
$ git reset --hard master
HEAD is now at b69c1df47281 brcmfmac: separate firmware errors from i/o errors
$ git am -s -3 1.patch
Applying: mwifiex: schedule rx_work on RX interrupt for USB
$ git log --oneline -1 | cat
74c5fc1d45b4 mwifiex: schedule rx_work on RX interrupt for USB
$ 

So the date, and most likely also the commiter, is included when
calculating the hash. So you can't really refer to uncommited patches
using a commit id as the id is determined only once the maintainer
applies the patch.
Ganapathi Bhat Jan. 25, 2018, 1:26 p.m. UTC | #6
Hi Kalle,

> -----Original Message-----
> From: Kalle Valo [mailto:kvalo@codeaurora.org]
> Sent: Thursday, January 25, 2018 6:29 PM
> To: Ganapathi Bhat
> Cc: linux-wireless@vger.kernel.org; Brian Norris; Cathy Luo; Xinming Hu;
> Zhiyuan Yang; James Cao; Mangesh Malusare; Shrenik Shikhare
> Subject: Re: [EXT] Re: [2/2] mwifiex: use more_rx_task_flag to avoid USB RX
> stall
>
> Ganapathi Bhat <gbhat@marvell.com> writes:
>
> >> > 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 <cluo@marvell.com>
> >> > Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> >>
> >> I can't find any commit with id c7dbdcb2a4e1, is it correct?
> >
> > Correct. Actually the commit id c7dbdcb2a4e1 is the PATCH [1/2] sent in this
> series.
>
> Actually the commit id will be different, I just tested it to be sure:
>
> $ git reset --hard master
> HEAD is now at b69c1df47281 brcmfmac: separate firmware errors from i/o
> errors $ git am -s -3 1.patch
> Applying: mwifiex: schedule rx_work on RX interrupt for USB $ git log --
> oneline -1 | cat
> 676bc4833907 mwifiex: schedule rx_work on RX interrupt for USB $ git reset -
> -hard master HEAD is now at b69c1df47281 brcmfmac: separate firmware
> errors from i/o errors $ git am -s -3 1.patch
> Applying: mwifiex: schedule rx_work on RX interrupt for USB $ git log --
> oneline -1 | cat
> 74c5fc1d45b4 mwifiex: schedule rx_work on RX interrupt for USB $
>
> So the date, and most likely also the commiter, is included when calculating
> the hash. So you can't really refer to uncommited patches using a commit id
> as the id is determined only once the maintainer applies the patch.
Ok. So, what can be done in this case. Should we wait for 1st patch to be committed and then send v3 of second patch?
>
> --
> Kalle Valo
Regards,
Ganapathi
Brian Norris Jan. 25, 2018, 6:33 p.m. UTC | #7
On Thu, Jan 25, 2018 at 09:59:04AM +0000, Ganapathi Bhat wrote:
> > I can't find any commit with id c7dbdcb2a4e1, is it correct?
> Correct. Actually the commit id c7dbdcb2a4e1 is the PATCH [1/2] sent in this series.

What? Why would you introduce a bug and only fix it in the next patch?
Does that mean you should just combine the two? Or reverse the order, if
patch 2 doesn't cause problems on its own?

Brian
Ganapathi Bhat Jan. 29, 2018, 7:17 a.m. UTC | #8
Hi Brian,

> -----Original Message-----
> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Friday, January 26, 2018 12:04 AM
> To: Ganapathi Bhat
> Cc: Kalle Valo; linux-wireless@vger.kernel.org; Cathy Luo; Xinming Hu;
> Zhiyuan Yang; James Cao; Mangesh Malusare; Shrenik Shikhare
> Subject: Re: [EXT] Re: [2/2] mwifiex: use more_rx_task_flag to avoid USB RX
> stall
>
> On Thu, Jan 25, 2018 at 09:59:04AM +0000, Ganapathi Bhat wrote:
> > > I can't find any commit with id c7dbdcb2a4e1, is it correct?
> > Correct. Actually the commit id c7dbdcb2a4e1 is the PATCH [1/2] sent in this
> series.
>
> What? Why would you introduce a bug and only fix it in the next patch?
With the first patch the original issue took considerably longer time to recreate. Also it followed a different path to get recreated(shared in commit message).
> Does that mean you should just combine the two?
Let us know if that is fine to merge them. We can modify the commit message accordingly.
> Or reverse the order, if patch 2 doesn't cause problems on its own?
Patch 2 has a dependency on patch 1.
>
> Brian

Regards,
Ganapathi
Ganapathi Bhat Jan. 29, 2018, 7:19 a.m. UTC | #9
Hi Brian,

> -----Original Message-----
> From: Ganapathi Bhat
> Sent: Monday, January 29, 2018 12:47 PM
> To: 'Brian Norris'
> Cc: Kalle Valo; linux-wireless@vger.kernel.org; Cathy Luo; Xinming Hu;
> Zhiyuan Yang; James Cao; Mangesh Malusare; Shrenik Shikhare
> Subject: RE: [EXT] Re: [2/2] mwifiex: use more_rx_task_flag to avoid USB RX
> stall
>
> Hi Brian,
>
> > -----Original Message-----
> > From: Brian Norris [mailto:briannorris@chromium.org]
> > Sent: Friday, January 26, 2018 12:04 AM
> > To: Ganapathi Bhat
> > Cc: Kalle Valo; linux-wireless@vger.kernel.org; Cathy Luo; Xinming Hu;
> > Zhiyuan Yang; James Cao; Mangesh Malusare; Shrenik Shikhare
> > Subject: Re: [EXT] Re: [2/2] mwifiex: use more_rx_task_flag to avoid
> > USB RX stall
> >
> > On Thu, Jan 25, 2018 at 09:59:04AM +0000, Ganapathi Bhat wrote:
> > > > I can't find any commit with id c7dbdcb2a4e1, is it correct?
> > > Correct. Actually the commit id c7dbdcb2a4e1 is the PATCH [1/2] sent
> > > in this
> > series.
> >
> > What? Why would you introduce a bug and only fix it in the next patch?
> With the first patch the original issue took considerably longer time to
> recreate. Also it followed a different path to get recreated(shared in commit
> message).
> > Does that mean you should just combine the two?
> Let us know if that is fine to merge them. We can modify the commit
> message accordingly.
> > Or reverse the order, if patch 2 doesn't cause problems on its own?
> Patch 2 has a dependency on patch 1.
One correction. There is no commit dependency between patch 1 and 2(they can be applied in any order). But the result would be same. We need both fixes. Let us know if we can combine them.
> >
> > Brian
>
> Regards,
> Ganapathi

Regards,
Ganapathi
Brian Norris Jan. 29, 2018, 10:08 p.m. UTC | #10
Hi,

On Sun, Jan 28, 2018 at 11:19 PM, Ganapathi Bhat <gbhat@marvell.com> wrote:
>> From: Ganapathi Bhat
>> > From: Brian Norris [mailto:briannorris@chromium.org]
>> > On Thu, Jan 25, 2018 at 09:59:04AM +0000, Ganapathi Bhat wrote:
>> > > > I can't find any commit with id c7dbdcb2a4e1, is it correct?
>> > > Correct. Actually the commit id c7dbdcb2a4e1 is the PATCH [1/2] sent
>> > > in this
>> > series.
>> >
>> > What? Why would you introduce a bug and only fix it in the next patch?
>> With the first patch the original issue took considerably longer time to
>> recreate. Also it followed a different path to get recreated(shared in commit
>> message).
>> > Does that mean you should just combine the two?
>> Let us know if that is fine to merge them. We can modify the commit
>> message accordingly.
>> > Or reverse the order, if patch 2 doesn't cause problems on its own?
>> Patch 2 has a dependency on patch 1.
> One correction. There is no commit dependency between patch 1 and 2(they can be applied in any order). But the result would be same. We need both fixes. Let us know if we can combine them.

I haven't closely looked at patch 2 yet. My only statement was that it
makes no sense to have 2 commits, with the second one labeled as
"Fixing" the first one. From your descriptions, it sounds like patch 2
should actually come first, but I'm not really sure. I only looked far
enough to say that I didn't like patch 1 as-is :)

Brian
Ganapathi Bhat Jan. 30, 2018, 3:25 p.m. UTC | #11
Hi Brian,

> -----Original Message-----

> From: Brian Norris [mailto:briannorris@chromium.org]

> Sent: Tuesday, January 30, 2018 3:38 AM

> To: Ganapathi Bhat

> Cc: Kalle Valo; linux-wireless@vger.kernel.org; Cathy Luo; Xinming Hu;

> Zhiyuan Yang; James Cao; Mangesh Malusare; Shrenik Shikhare

> Subject: Re: [EXT] Re: [2/2] mwifiex: use more_rx_task_flag to avoid USB RX

> stall

>

> Hi,

>

> On Sun, Jan 28, 2018 at 11:19 PM, Ganapathi Bhat <gbhat@marvell.com>

> wrote:

> >> From: Ganapathi Bhat

> >> > From: Brian Norris [mailto:briannorris@chromium.org] On Thu, Jan

> >> > 25, 2018 at 09:59:04AM +0000, Ganapathi Bhat wrote:

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

> >> > > Correct. Actually the commit id c7dbdcb2a4e1 is the PATCH [1/2]

> >> > > sent in this

> >> > series.

> >> >

> >> > What? Why would you introduce a bug and only fix it in the next patch?

> >> With the first patch the original issue took considerably longer time

> >> to recreate. Also it followed a different path to get

> >> recreated(shared in commit message).

> >> > Does that mean you should just combine the two?

> >> Let us know if that is fine to merge them. We can modify the commit

> >> message accordingly.

> >> > Or reverse the order, if patch 2 doesn't cause problems on its own?

> >> Patch 2 has a dependency on patch 1.

> > One correction. There is no commit dependency between patch 1 and

> 2(they can be applied in any order). But the result would be same. We need

> both fixes. Let us know if we can combine them.

>

> I haven't closely looked at patch 2 yet. My only statement was that it makes

> no sense to have 2 commits, with the second one labeled as "Fixing" the first

> one. From your descriptions, it sounds like patch 2 should actually come first,

Ok. I understand. I will reorder them and send v3(along with spinlock change).
> but I'm not really sure. I only looked far enough to say that I didn't like patch

> 1 as-is :)

>

> Brian

Regards,
Ganapathi
Ganapathi Bhat Jan. 31, 2018, 6:59 a.m. UTC | #12
Hi Brian,

> -----Original Message-----

> From: Ganapathi Bhat

> Sent: Tuesday, January 30, 2018 8:55 PM

> To: 'Brian Norris'

> Cc: Kalle Valo; linux-wireless@vger.kernel.org; Cathy Luo; Xinming Hu;

> Zhiyuan Yang; James Cao; Mangesh Malusare; Shrenik Shikhare

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

> stall

>

> Hi Brian,

>

> > -----Original Message-----

> > From: Brian Norris [mailto:briannorris@chromium.org]

> > Sent: Tuesday, January 30, 2018 3:38 AM

> > To: Ganapathi Bhat

> > Cc: Kalle Valo; linux-wireless@vger.kernel.org; Cathy Luo; Xinming Hu;

> > Zhiyuan Yang; James Cao; Mangesh Malusare; Shrenik Shikhare

> > Subject: Re: [EXT] Re: [2/2] mwifiex: use more_rx_task_flag to avoid

> > USB RX stall

> >

> > Hi,

> >

> > On Sun, Jan 28, 2018 at 11:19 PM, Ganapathi Bhat <gbhat@marvell.com>

> > wrote:

> > >> From: Ganapathi Bhat

> > >> > From: Brian Norris [mailto:briannorris@chromium.org] On Thu, Jan

> > >> > 25, 2018 at 09:59:04AM +0000, Ganapathi Bhat wrote:

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

> > >> > > Correct. Actually the commit id c7dbdcb2a4e1 is the PATCH [1/2]

> > >> > > sent in this

> > >> > series.

> > >> >

> > >> > What? Why would you introduce a bug and only fix it in the next

> patch?

> > >> With the first patch the original issue took considerably longer

> > >> time to recreate. Also it followed a different path to get

> > >> recreated(shared in commit message).

> > >> > Does that mean you should just combine the two?

> > >> Let us know if that is fine to merge them. We can modify the commit

> > >> message accordingly.

> > >> > Or reverse the order, if patch 2 doesn't cause problems on its own?

> > >> Patch 2 has a dependency on patch 1.

> > > One correction. There is no commit dependency between patch 1 and

> > 2(they can be applied in any order). But the result would be same. We

> > need both fixes. Let us know if we can combine them.

> >

> > I haven't closely looked at patch 2 yet. My only statement was that it

> > makes no sense to have 2 commits, with the second one labeled as

> > "Fixing" the first one. From your descriptions, it sounds like patch 2

> > should actually come first,

> Ok. I understand. I will reorder them and send v3(along with spinlock

> change).

I was trying to reorder the patch but found patch 2 is indeed dependent on patch 1. Consider the first point in commit message of patch 2:
1. USB receives an RX data interrupt, queues rx_work
This is the change added in patch 1. Earlier on receive of RX data interrupt, driver would queue main_work, which in turn queued rx_work. But after patch 1 driver tries to  queue rx_work in interrupt context.

Let us please know your thoughts on this.

> > but I'm not really sure. I only looked far enough to say that I didn't

> > like patch

> > 1 as-is :)

> >

> > Brian

> Regards,

> Ganapathi
diff mbox

Patch

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 */