2013-01-08 14:18:53

by Victor Goldenshtein

[permalink] [raw]
Subject: [PATCH] mac80211: fix delayed ADDBA response

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 <[email protected]>
---
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



2013-01-09 17:29:24

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: fix delayed ADDBA response

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


2013-01-09 17:25:19

by Arik Nemtsov

[permalink] [raw]
Subject: Re: [PATCH] mac80211: fix delayed ADDBA response

On Wed, Jan 9, 2013 at 6:32 PM, Johannes Berg <[email protected]> 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?

2013-01-09 16:06:11

by Victor Goldenshtein

[permalink] [raw]
Subject: Re: [PATCH] mac80211: fix delayed ADDBA response

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.

2013-01-09 12:37:56

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: fix delayed ADDBA response

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 <[email protected]>
> ---
> 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


2013-01-08 17:41:15

by Seth Forshee

[permalink] [raw]
Subject: Re: [PATCH] mac80211: fix delayed ADDBA response

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 <[email protected]>
> ---
> 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

2013-01-09 08:41:40

by Victor Goldenshtein

[permalink] [raw]
Subject: Re: [PATCH] mac80211: fix delayed ADDBA response

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<[email protected]>
>> ---
>> 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.

2013-01-09 16:31:49

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: fix delayed ADDBA response

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