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 <[email protected]>
---
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
On Tue, Jan 5, 2016 at 4:52 PM, Johannes Berg <[email protected]> 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
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
---
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
On Tue, Jan 5, 2016 at 4:35 PM, Johannes Berg <[email protected]> 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 <[email protected]>
>> ---
>> 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