[PATCH] mac80211: fix delayed ADDBA response

[PATCH] mac80211: fix delayed ADDBA response

[Victor Goldenshtein]

Block frame processing during scan might delay the
ADDBA response, which eventually timeouts and
significantly reduces the device throughput.
Remove this constrain as it's not required for the
HW scan.

Signed-off-by: Victor Goldenshtein <victorg@ti.com>
---
 net/mac80211/iface.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 06fac29..a26ee36 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1063,7 +1063,7 @@ static void ieee80211_iface_work(struct work_struct *work)
 	if (!ieee80211_sdata_running(sdata))
 		return;
 
-	if (local->scanning)
+	if (local->scanning && !local->ops->hw_scan)
 		return;
 
 	/*
-- 
1.7.5.4

Re: [PATCH] mac80211: fix delayed ADDBA response

[Seth Forshee]

On Tue, Jan 08, 2013 at 04:16:37PM +0200, Victor Goldenshtein wrote:
> Block frame processing during scan might delay the
> ADDBA response, which eventually timeouts and
> significantly reduces the device throughput.
> Remove this constrain as it's not required for the
> HW scan.
> 
> Signed-off-by: Victor Goldenshtein <victorg@ti.com>
> ---
>  net/mac80211/iface.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
> index 06fac29..a26ee36 100644
> --- a/net/mac80211/iface.c
> +++ b/net/mac80211/iface.c
> @@ -1063,7 +1063,7 @@ static void ieee80211_iface_work(struct work_struct *work)
>  	if (!ieee80211_sdata_running(sdata))
>  		return;
>  
> -	if (local->scanning)
> +	if (local->scanning && !local->ops->hw_scan)

Wouldn't checking for SCAN_HW_SCANNING be better?

Seth

Re: [PATCH] mac80211: fix delayed ADDBA response

[Victor Goldenshtein]

On 08/01/13 19:41, Seth Forshee wrote:
> On Tue, Jan 08, 2013 at 04:16:37PM +0200, Victor Goldenshtein wrote:
>> Block frame processing during scan might delay the
>> ADDBA response, which eventually timeouts and
>> significantly reduces the device throughput.
>> Remove this constrain as it's not required for the
>> HW scan.
>>
>> Signed-off-by: Victor Goldenshtein<victorg@ti.com>
>> ---
>>   net/mac80211/iface.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
>> index 06fac29..a26ee36 100644
>> --- a/net/mac80211/iface.c
>> +++ b/net/mac80211/iface.c
>> @@ -1063,7 +1063,7 @@ static void ieee80211_iface_work(struct work_struct *work)
>>   	if (!ieee80211_sdata_running(sdata))
>>   		return;
>>
>> -	if (local->scanning)
>> +	if (local->scanning&&  !local->ops->hw_scan)
>
> Wouldn't checking for SCAN_HW_SCANNING be better?
>

IMHO it's totally the same, besides that mac80211 use this check in many 
other places, but lets see what Johannes thinks.

-- 
Thanks,
Victor.

Re: [PATCH] mac80211: fix delayed ADDBA response

[Johannes Berg]

On Tue, 2013-01-08 at 16:16 +0200, Victor Goldenshtein wrote:
> Block frame processing during scan might delay the
> ADDBA response, which eventually timeouts and
> significantly reduces the device throughput.
> Remove this constrain as it's not required for the
> HW scan.
> 
> Signed-off-by: Victor Goldenshtein <victorg@ti.com>
> ---
>  net/mac80211/iface.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
> index 06fac29..a26ee36 100644
> --- a/net/mac80211/iface.c
> +++ b/net/mac80211/iface.c
> @@ -1063,7 +1063,7 @@ static void ieee80211_iface_work(struct work_struct *work)
>  	if (!ieee80211_sdata_running(sdata))
>  		return;
>  
> -	if (local->scanning)
> +	if (local->scanning && !local->ops->hw_scan)
>  		return;

Regardless of whether it should check the HW scan flag (I think that'd
be better), this doesn't seem right.

Do we really want to allow arbitrary changes to the interfaces while
scanning? This could process a deauth frame for example, I'm worried
that might break things.

johannes

Re: [PATCH] mac80211: fix delayed ADDBA response

[Victor Goldenshtein]

On 09/01/13 14:38, Johannes Berg wrote:
> Do we really want to allow arbitrary changes to the interfaces while
> scanning? This could process a deauth frame for example, I'm worried
> that might break things.
>

Can you please elaborate why we shouldn't process deauth frames during 
HW scan ? This fix was tested with our 18xx, don't see how it can break 
things, but it's up to you whether to take it..

-- 
Thanks,
Victor.

Re: [PATCH] mac80211: fix delayed ADDBA response

[Johannes Berg]

On Wed, 2013-01-09 at 18:04 +0200, Victor Goldenshtein wrote:
> On 09/01/13 14:38, Johannes Berg wrote:
> > Do we really want to allow arbitrary changes to the interfaces while
> > scanning? This could process a deauth frame for example, I'm worried
> > that might break things.
> >
> 
> Can you please elaborate why we shouldn't process deauth frames during 
> HW scan ? This fix was tested with our 18xx, don't see how it can break 
> things, but it's up to you whether to take it..

But did you test disassociating in the middle of the scan? I suspect
that some devices at least might have trouble with reprogramming the
device completely while it's scanning.

Maybe you can make this behaviour opt-in with a hardware flag?

johannes

Re: [PATCH] mac80211: fix delayed ADDBA response

[Arik Nemtsov]

On Wed, Jan 9, 2013 at 6:32 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Wed, 2013-01-09 at 18:04 +0200, Victor Goldenshtein wrote:
>> On 09/01/13 14:38, Johannes Berg wrote:
>> > Do we really want to allow arbitrary changes to the interfaces while
>> > scanning? This could process a deauth frame for example, I'm worried
>> > that might break things.
>> >
>>
>> Can you please elaborate why we shouldn't process deauth frames during
>> HW scan ? This fix was tested with our 18xx, don't see how it can break
>> things, but it's up to you whether to take it..
>
> But did you test disassociating in the middle of the scan? I suspect
> that some devices at least might have trouble with reprogramming the
> device completely while it's scanning.
>
> Maybe you can make this behaviour opt-in with a hardware flag?
>

This is just a speed optimization for BA session management, so I'm
pretty sure the above was not tested.

Maybe we can make the "if" stricter and only allow it to pass if it's
hw_scanning and mgmt->u.action.category == WLAN_CATEGORY_BACK ? Would
that make sense?

Re: [PATCH] mac80211: fix delayed ADDBA response

[Johannes Berg]

On Wed, 2013-01-09 at 19:25 +0200, Arik Nemtsov wrote:

> This is just a speed optimization for BA session management, so I'm
> pretty sure the above was not tested.
> 
> Maybe we can make the "if" stricter and only allow it to pass if it's
> hw_scanning and mgmt->u.action.category == WLAN_CATEGORY_BACK ? Would
> that make sense?

Yeah that's the other option -- making the processing loop more
selective. That's more complex, but should work better.

johannes