[PATCH v2] mac80211: don't drop null frames during software scan

[PATCH v2] mac80211: don't drop null frames during software scan

[Kalle Valo]

ieee80211_tx_h_check_assoc() was dropping everything else than probe
requests during software scan. So the null frame with the power save
bit was dropped and AP never received it. This meant that AP never
buffered any frames for the station during software scan.

Fix this by allowing to transmit both probe request and null frames
during software scan. Tested with stlc45xx.

Signed-off-by: Kalle Valo <kalle.valo@nokia.com>
---

 net/mac80211/scan.c |    5 +++++
 net/mac80211/tx.c   |    9 ++++++++-
 2 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 0e81e16..fae18c3 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -397,6 +397,11 @@ int ieee80211_start_scan(struct ieee80211_sub_if_data *scan_sdata,
 		return 0;
 	}
 
+	/*
+	 * While local->sw_scanning is true everything else but null
+	 * frames and probe requests will be dropped in
+	 * ieee80211_tx_h_check_assoc().
+	 */
 	local->sw_scanning = true;
 	if (local->ops->sw_scan_start)
 		local->ops->sw_scan_start(local_to_hw(local));
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index c3f0e95..bfe6ecd 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -193,7 +193,14 @@ ieee80211_tx_h_check_assoc(struct ieee80211_tx_data *tx)
 		return TX_CONTINUE;
 
 	if (unlikely(tx->local->sw_scanning) &&
-	    !ieee80211_is_probe_req(hdr->frame_control))
+	    !ieee80211_is_probe_req(hdr->frame_control) &&
+	    !ieee80211_is_nullfunc(hdr->frame_control))
+		/*
+		 * When software scanning only null frames (to notify the
+		 * sleep state to the AP) and probe requests (for the
+		 * active scan) are allowed, everything else should be
+		 * dropped.
+		 */
 		return TX_DROP;
 
 	if (tx->sdata->vif.type == NL80211_IFTYPE_MESH_POINT)

Re: [PATCH v2] mac80211: don't drop null frames during software scan

[Jouni Malinen]

On Sun, Mar 15, 2009 at 10:07:39PM +0200, Kalle Valo wrote:
> ieee80211_tx_h_check_assoc() was dropping everything else than probe
> requests during software scan. So the null frame with the power save
> bit was dropped and AP never received it. This meant that AP never
> buffered any frames for the station during software scan.
> 
> Fix this by allowing to transmit both probe request and null frames
> during software scan. Tested with stlc45xx.

I would assume the nullfunc frames are sent only just before the scan
and just after the scan, not really "during" the scan. Or am I missing
something here?

> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -193,7 +193,14 @@ ieee80211_tx_h_check_assoc(struct ieee80211_tx_data *tx)
>  		return TX_CONTINUE;
>  
>  	if (unlikely(tx->local->sw_scanning) &&
> -	    !ieee80211_is_probe_req(hdr->frame_control))
> +	    !ieee80211_is_probe_req(hdr->frame_control) &&
> +	    !ieee80211_is_nullfunc(hdr->frame_control))
> +		/*
> +		 * When software scanning only null frames (to notify the
> +		 * sleep state to the AP) and probe requests (for the
> +		 * active scan) are allowed, everything else should be
> +		 * dropped.
> +		 */
>  		return TX_DROP;

While this is probably the easiest way of fixing the issue you are
seeing, the more correct operation would be to allow nullfunc frames
only at the beginning and end of the scan operation, not during it,
i.e., there is no point allowing those frames to go out when we are not
on our operational channel. I would hope we do not currently send those
frames at such time, so this should not matter much, but the comment
could be made more clear about the different needs for nullfunc frames
(please also s/null frames/nullfunc frames/) and probe request frames.
The former are sent only on the operational channel in the beginning and
end of scan while the latter are sent on the channels to be scanned
during an active scan.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH v2] mac80211: don't drop null frames during software scan

[Kalle Valo]

Jouni Malinen <j@w1.fi> writes:

> On Sun, Mar 15, 2009 at 10:07:39PM +0200, Kalle Valo wrote:
>> ieee80211_tx_h_check_assoc() was dropping everything else than probe
>> requests during software scan. So the null frame with the power save
>> bit was dropped and AP never received it. This meant that AP never
>> buffered any frames for the station during software scan.
>> 
>> Fix this by allowing to transmit both probe request and null frames
>> during software scan. Tested with stlc45xx.
>
> I would assume the nullfunc frames are sent only just before the scan
> and just after the scan, not really "during" the scan. Or am I missing
> something here?

No, you are not missing anything. I just considered the start of scan
happening in the beginning of function ieee80211_start_scan() (which
sends the nullfunc frame) and that's why I chose the word "during".

>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> @@ -193,7 +193,14 @@ ieee80211_tx_h_check_assoc(struct ieee80211_tx_data *tx)
>>  		return TX_CONTINUE;
>>  
>>  	if (unlikely(tx->local->sw_scanning) &&
>> -	    !ieee80211_is_probe_req(hdr->frame_control))
>> +	    !ieee80211_is_probe_req(hdr->frame_control) &&
>> +	    !ieee80211_is_nullfunc(hdr->frame_control))
>> +		/*
>> +		 * When software scanning only null frames (to notify the
>> +		 * sleep state to the AP) and probe requests (for the
>> +		 * active scan) are allowed, everything else should be
>> +		 * dropped.
>> +		 */
>>  		return TX_DROP;
>
> While this is probably the easiest way of fixing the issue you are
> seeing, the more correct operation would be to allow nullfunc frames
> only at the beginning and end of the scan operation, not during it,
> i.e., there is no point allowing those frames to go out when we are not
> on our operational channel. I would hope we do not currently send those
> frames at such time,

Actually I was thinking of adding a WARN_ONCE just to catch that but I
decided to abandon the idea because all the blame I would get from the
warnings :)

> so this should not matter much, but the comment could be made more
> clear about the different needs for nullfunc frames (please also
> s/null frames/nullfunc frames/) and probe request frames. The former
> are sent only on the operational channel in the beginning and end of
> scan while the latter are sent on the channels to be scanned during
> an active scan.

Should the description be in ieee80211_start_scan() in scan.c? I think
it would make more sense to have it there instead of tx.c. I can then
add a reference to the comment above.

-- 
Kalle Valo

Re: [PATCH v2] mac80211: don't drop null frames during software scan

[Kalle Valo]

Kalle Valo <kalle.valo@nokia.com> writes:

>> so this should not matter much, but the comment could be made more
>> clear about the different needs for nullfunc frames (please also
>> s/null frames/nullfunc frames/) and probe request frames. The former
>> are sent only on the operational channel in the beginning and end of
>> scan while the latter are sent on the channels to be scanned during
>> an active scan.
>
> Should the description be in ieee80211_start_scan() in scan.c? I think
> it would make more sense to have it there instead of tx.c. I can then
> add a reference to the comment above.

Oh yeah, forgot to mention that I'll use the term "nullfunc frame" in
v3.

-- 
Kalle Valo

Re: [PATCH v2] mac80211: don't drop null frames during software scan

[Jouni Malinen]

On Mon, Mar 16, 2009 at 02:35:30PM +0200, Kalle Valo wrote:

> > so this should not matter much, but the comment could be made more
> > clear about the different needs for nullfunc frames (please also
> > s/null frames/nullfunc frames/) and probe request frames. The former
> > are sent only on the operational channel in the beginning and end of
> > scan while the latter are sent on the channels to be scanned during
> > an active scan.
> 
> Should the description be in ieee80211_start_scan() in scan.c? I think
> it would make more sense to have it there instead of tx.c. I can then
> add a reference to the comment above.

Either way works for me as long as there is something giving me enough
information (or pointer to that) next to the place that allows nullfunc
frames go through. Anyway, Johannes is correct about the proper longer
term fix being better mechanism to stop the queue so that we don't get
into this code in the first place and in that sense, this patch is more
of a workaround for the time being.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH v2] mac80211: don't drop null frames during software scan

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 3544 bytes --]

On Mon, 2009-03-16 at 10:57 +0200, Jouni Malinen wrote:
> On Sun, Mar 15, 2009 at 10:07:39PM +0200, Kalle Valo wrote:
> > ieee80211_tx_h_check_assoc() was dropping everything else than probe
> > requests during software scan. So the null frame with the power save
> > bit was dropped and AP never received it. This meant that AP never
> > buffered any frames for the station during software scan.
> > 
> > Fix this by allowing to transmit both probe request and null frames
> > during software scan. Tested with stlc45xx.
> 
> I would assume the nullfunc frames are sent only just before the scan
> and just after the scan, not really "during" the scan. Or am I missing
> something here?

Correct, but if we wanted to do this properly we'd have to iterate the
list twice and generally make the scan start code quite a bit more
complex. I don't think it's worth it, since we _should_ control quite
tightly what we're sending during scan.

> >  	if (unlikely(tx->local->sw_scanning) &&
> > -	    !ieee80211_is_probe_req(hdr->frame_control))
> > +	    !ieee80211_is_probe_req(hdr->frame_control) &&
> > +	    !ieee80211_is_nullfunc(hdr->frame_control))
> > +		/*
> > +		 * When software scanning only null frames (to notify the
> > +		 * sleep state to the AP) and probe requests (for the
> > +		 * active scan) are allowed, everything else should be
> > +		 * dropped.
> > +		 */
> >  		return TX_DROP;
> 
> While this is probably the easiest way of fixing the issue you are
> seeing, the more correct operation would be to allow nullfunc frames
> only at the beginning and end of the scan operation, not during it,
> i.e., there is no point allowing those frames to go out when we are not
> on our operational channel. I would hope we do not currently send those
> frames at such time, so this should not matter much, but the comment
> could be made more clear about the different needs for nullfunc frames
> (please also s/null frames/nullfunc frames/) and probe request frames.
> The former are sent only on the operational channel in the beginning and
> end of scan while the latter are sent on the channels to be scanned
> during an active scan.

The other thing is -- we shouldn't ever actually run into this code
dropping frames at all. Or rather, we should find a way to avoid it.

You have to realise that when the quoted piece of code executes for
frames other than nullfunc/preq frames, all is lost already! That means
we've had so many frames on the queues that we have frames stuck on the
software queues, and we'll start sending them while on the wrong
channel...

The proper way to fix this involves stopping the software queues,
flushing the the driver/hardware queues, and only _then_ leaving the
operational channel. Broadcom firmware will reject off-channel
transmissions (if set up correctly), but even that is not really
desirable here because it means we lose the packets too.

Once scanning starts, we have to start the voice queue again, of course,
and because it might contain frames still we should change the code
above to queue up the frames internally instead of simply dropping them.

If anyone wants to do anything about it, I would ask to wait for my
pending frames rework though, since that will make the last part simpler
and would most likely clash with any work done here.

Drivers implementing hw_scan are not affected, of course, if they do
things correctly. Which is probably only iwlwifi with its firmware
assisted scanning.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]