| Message ID | 1452004632-20263-1-git-send-email-helmut.schaa@googlemail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(+)