* [PATCH] mac80211: Don't buffer non-bufferable MMPDUs
@ 2016-01-05 14:37 Helmut Schaa
2016-01-05 15:35 ` Johannes Berg
2016-01-05 16:42 ` [PATCHv2] " Helmut Schaa
0 siblings, 2 replies; 6+ messages in thread
From: Helmut Schaa @ 2016-01-05 14:37 UTC (permalink / raw)
To: johannes; +Cc: linux-wireless, Helmut Schaa
Non-bufferable MMPDUs are sent out to STAs even while in PS mode
(for example probe responses). Applying filtered frame handling for
these doesn't seem to make much sense and will only create more
air utilization when the STA wakes up. Hence, apply filtered frame
handling only for bufferable MMPDUs.
Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
net/mac80211/status.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index 5bad05e..14bd53b 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -51,6 +51,12 @@ static void ieee80211_handle_filtered_frame(struct ieee80211_local *local,
struct ieee80211_hdr *hdr = (void *)skb->data;
int ac;
+ if (ieee80211_is_mgmt(hdr->frame_control) &&
+ !ieee80211_is_bufferable_mmpdu(hdr->frame_control)) {
+ ieee80211_free_txskb(&local->hw, skb);
+ return;
+ }
+
/*
* This skb 'survived' a round-trip through the driver, and
* hopefully the driver didn't mangle it too badly. However,
--
1.8.4.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] mac80211: Don't buffer non-bufferable MMPDUs
2016-01-05 14:37 [PATCH] mac80211: Don't buffer non-bufferable MMPDUs Helmut Schaa
@ 2016-01-05 15:35 ` Johannes Berg
2016-01-05 15:43 ` Helmut Schaa
2016-01-05 16:42 ` [PATCHv2] " Helmut Schaa
1 sibling, 1 reply; 6+ messages in thread
From: Johannes Berg @ 2016-01-05 15:35 UTC (permalink / raw)
To: Helmut Schaa; +Cc: linux-wireless
On Tue, 2016-01-05 at 15:37 +0100, Helmut Schaa wrote:
> Non-bufferable MMPDUs are sent out to STAs even while in PS mode
> (for example probe responses). Applying filtered frame handling for
> these doesn't seem to make much sense and will only create more
> air utilization when the STA wakes up. Hence, apply filtered frame
> handling only for bufferable MMPDUs.
>
> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
> ---
> net/mac80211/status.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/net/mac80211/status.c b/net/mac80211/status.c
> index 5bad05e..14bd53b 100644
> --- a/net/mac80211/status.c
> +++ b/net/mac80211/status.c
> @@ -51,6 +51,12 @@ static void ieee80211_handle_filtered_frame(struct
> ieee80211_local *local,
> struct ieee80211_hdr *hdr = (void *)skb->data;
> int ac;
>
> + if (ieee80211_is_mgmt(hdr->frame_control) &&
> + !ieee80211_is_bufferable_mmpdu(hdr->frame_control)) {
> + ieee80211_free_txskb(&local->hw, skb);
> + return;
> + }
>
I don't really see a problem per se with this patch, but it seems that
we could just check the flags instead? Perhaps we simply shouldn't
apply buffering filtered frames here to frames that were already marked
with IEEE80211_TX_CTL_NO_PS_BUFFER, since those frames shouldn't be
filtered by the driver to start with.
That said, it obviously also points to a driver bug not treating these
frames correctly, so you might want to investigate why you got here to
start with!
johannes
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mac80211: Don't buffer non-bufferable MMPDUs
2016-01-05 15:35 ` Johannes Berg
@ 2016-01-05 15:43 ` Helmut Schaa
2016-01-05 15:52 ` Johannes Berg
0 siblings, 1 reply; 6+ messages in thread
From: Helmut Schaa @ 2016-01-05 15:43 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
On Tue, Jan 5, 2016 at 4:35 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2016-01-05 at 15:37 +0100, Helmut Schaa wrote:
>> Non-bufferable MMPDUs are sent out to STAs even while in PS mode
>> (for example probe responses). Applying filtered frame handling for
>> these doesn't seem to make much sense and will only create more
>> air utilization when the STA wakes up. Hence, apply filtered frame
>> handling only for bufferable MMPDUs.
>>
>> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
>> ---
>> net/mac80211/status.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/net/mac80211/status.c b/net/mac80211/status.c
>> index 5bad05e..14bd53b 100644
>> --- a/net/mac80211/status.c
>> +++ b/net/mac80211/status.c
>> @@ -51,6 +51,12 @@ static void ieee80211_handle_filtered_frame(struct
>> ieee80211_local *local,
>> struct ieee80211_hdr *hdr = (void *)skb->data;
>> int ac;
>>
>> + if (ieee80211_is_mgmt(hdr->frame_control) &&
>> + !ieee80211_is_bufferable_mmpdu(hdr->frame_control)) {
>> + ieee80211_free_txskb(&local->hw, skb);
>> + return;
>> + }
>>
> I don't really see a problem per se with this patch, but it seems that
> we could just check the flags instead? Perhaps we simply shouldn't
> apply buffering filtered frames here to frames that were already marked
> with IEEE80211_TX_CTL_NO_PS_BUFFER, since those frames shouldn't be
> filtered by the driver to start with.
Good point, that should make the check a bit nicer. I'll change that.
> That said, it obviously also points to a driver bug not treating these
> frames correctly, so you might want to investigate why you got here to
> start with!
It was not the driver marking the frame as filtered but mac80211
itself in status.c:
acked = !!(info->flags & IEEE80211_TX_STAT_ACK);
if (!acked && test_sta_flag(sta, WLAN_STA_PS_STA)) {
/*
* The STA is in power save mode, so assume
* that this TX packet failed because of that.
*/
ieee80211_handle_filtered_frame(local, sta, skb);
rcu_read_unlock();
return;
}
However, I've put the code into ieee80211_handle_filtered_frame to
not grow ieee80211_tx_status even more :)
If you prefer to add it here directly I'm fine to change it.
Helmut
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mac80211: Don't buffer non-bufferable MMPDUs
2016-01-05 15:43 ` Helmut Schaa
@ 2016-01-05 15:52 ` Johannes Berg
2016-01-05 15:56 ` Helmut Schaa
0 siblings, 1 reply; 6+ messages in thread
From: Johannes Berg @ 2016-01-05 15:52 UTC (permalink / raw)
To: Helmut Schaa; +Cc: linux-wireless
On Tue, 2016-01-05 at 16:43 +0100, Helmut Schaa wrote:
> > That said, it obviously also points to a driver bug not treating
> > these frames correctly, so you might want to investigate why you
> > got here to start with!
>
> It was not the driver marking the frame as filtered but mac80211
> itself in status.c:
>
> acked = !!(info->flags & IEEE80211_TX_STAT_ACK);
> if (!acked && test_sta_flag(sta, WLAN_STA_PS_STA)) {
> /*
> * The STA is in power save mode, so assume
> * that this TX packet failed because of
> that.
> */
> ieee80211_handle_filtered_frame(local, sta,
> skb);
> rcu_read_unlock();
> return;
> }
Ah, yes, ok. So the frame simply didn't go through, for whatever
reason. Hopefully the driver actually transmitted it :)
> However, I've put the code into ieee80211_handle_filtered_frame to
> not grow ieee80211_tx_status even more :)
>
> If you prefer to add it here directly I'm fine to change it.
>
No, looks fine as is. I just misunderstood how we got here.
johannes
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mac80211: Don't buffer non-bufferable MMPDUs
2016-01-05 15:52 ` Johannes Berg
@ 2016-01-05 15:56 ` Helmut Schaa
0 siblings, 0 replies; 6+ messages in thread
From: Helmut Schaa @ 2016-01-05 15:56 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
On Tue, Jan 5, 2016 at 4:52 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2016-01-05 at 16:43 +0100, Helmut Schaa wrote:
>
>> > That said, it obviously also points to a driver bug not treating
>> > these frames correctly, so you might want to investigate why you
>> > got here to start with!
>>
>> It was not the driver marking the frame as filtered but mac80211
>> itself in status.c:
>>
>> acked = !!(info->flags & IEEE80211_TX_STAT_ACK);
>> if (!acked && test_sta_flag(sta, WLAN_STA_PS_STA)) {
>> /*
>> * The STA is in power save mode, so assume
>> * that this TX packet failed because of
>> that.
>> */
>> ieee80211_handle_filtered_frame(local, sta,
>> skb);
>> rcu_read_unlock();
>> return;
>> }
>
> Ah, yes, ok. So the frame simply didn't go through, for whatever
> reason. Hopefully the driver actually transmitted it :)
Yep, saw it on the air.
It was a STA sending probe requests while in PS mode and jumping to a new
channel before receiving/acking all probe responses. Hence, a probe response
ended up in the filtered queue. And it was easy to reproduce with this specific
STA (some old voip phone).
>> However, I've put the code into ieee80211_handle_filtered_frame to
>> not grow ieee80211_tx_status even more :)
>>
>> If you prefer to add it here directly I'm fine to change it.
>>
>
> No, looks fine as is. I just misunderstood how we got here.
Ok, I'd still like to change to checking IEEE80211_TX_CTL_NO_PS_BUFFER
instead -> v2 follows.
Helmut
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCHv2] mac80211: Don't buffer non-bufferable MMPDUs
2016-01-05 14:37 [PATCH] mac80211: Don't buffer non-bufferable MMPDUs Helmut Schaa
2016-01-05 15:35 ` Johannes Berg
@ 2016-01-05 16:42 ` Helmut Schaa
1 sibling, 0 replies; 6+ messages in thread
From: Helmut Schaa @ 2016-01-05 16:42 UTC (permalink / raw)
To: johannes; +Cc: linux-wireless, Helmut Schaa
Non-bufferable MMPDUs are sent out to STAs even while in PS mode
(for example probe responses). Applying filtered frame handling for
these doesn't seem to make much sense and will only create more
air utilization when the STA wakes up. Hence, apply filtered frame
handling only for bufferable MMPDUs.
Discovered while testing an old VOIP phone that started probing
for APs while in PS mode. The mac80211/ath9k AP where the STA is
associated would reply with a probe response but the phone sometimes
moved to a new channel already and couldn't ack the probe response
anymore. In that case mac80211 applied filtered frame handling
for the un-acked probe response.
Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
Changes in v2: Check IEEE80211_TX_CTL_NO_PS_BUFFER instead of frame_control field
net/mac80211/status.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index 5bad05e..6101deb 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -51,6 +51,11 @@ static void ieee80211_handle_filtered_frame(struct ieee80211_local *local,
struct ieee80211_hdr *hdr = (void *)skb->data;
int ac;
+ if (info->flags & IEEE80211_TX_CTL_NO_PS_BUFFER) {
+ ieee80211_free_txskb(&local->hw, skb);
+ return;
+ }
+
/*
* This skb 'survived' a round-trip through the driver, and
* hopefully the driver didn't mangle it too badly. However,
--
1.8.4.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-01-05 16:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-05 14:37 [PATCH] mac80211: Don't buffer non-bufferable MMPDUs Helmut Schaa
2016-01-05 15:35 ` Johannes Berg
2016-01-05 15:43 ` Helmut Schaa
2016-01-05 15:52 ` Johannes Berg
2016-01-05 15:56 ` Helmut Schaa
2016-01-05 16:42 ` [PATCHv2] " Helmut Schaa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).