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

[Ganapathi Bhat]

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:briannor...@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 <gb...@marvell.com>
> > wrote:
> > >> From: Ganapathi Bhat
> > >> > From: Brian Norris [mailto:briannor...@chromium.org] On Thu, Jan
> > >> > 25, 2018 at 09:59:04AM +, 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

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

[Ganapathi Bhat]

Hi Brian,

> -Original Message-
> From: Brian Norris [mailto:briannor...@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 <gb...@marvell.com>
> wrote:
> >> From: Ganapathi Bhat
> >> > From: Brian Norris [mailto:briannor...@chromium.org] On Thu, Jan
> >> > 25, 2018 at 09:59:04AM +, 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

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

[Brian Norris]

Hi,

On Sun, Jan 28, 2018 at 11:19 PM, Ganapathi Bhat  wrote:
>> From: Ganapathi Bhat
>> > From: Brian Norris [mailto:briannor...@chromium.org]
>> > On Thu, Jan 25, 2018 at 09:59:04AM +, 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

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

[Ganapathi Bhat]

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:briannor...@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 +, 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

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

[Ganapathi Bhat]

Hi Brian,

> -Original Message-
> From: Brian Norris [mailto:briannor...@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 +, 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

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

[Brian Norris]

On Thu, Jan 25, 2018 at 09:59:04AM +, 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

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

[Ganapathi Bhat]

Hi Kalle,

> -Original Message-
> From: Kalle Valo [mailto:kv...@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 <gb...@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 <c...@marvell.com>
> >> > Signed-off-by: Ganapathi Bhat <gb...@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

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

[Kalle Valo]

Ganapathi Bhat  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 
>> > Signed-off-by: Ganapathi Bhat 
>>
>> 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.

-- 
Kalle Valo

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

[Ganapathi Bhat]

Hi Kalle,

> --
> Kalle Valo  writes:
>
> > Ganapathi Bhat  wrote:
> >
> >> From: Shrenik Shikhare 
> >>
> >> 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 
> >> Signed-off-by: Ganapathi Bhat 
> >
> > 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

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

[Ganapathi Bhat]

Hi Kalle,
>
> --
> Ganapathi Bhat  wrote:
>
> > From: Shrenik Shikhare 
> >
> > 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 
> > Signed-off-by: Ganapathi Bhat 
>
> 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