diff mbox series

mac80211: Fix wrong channel bandwidths reported for aggregates

Message ID 20220718222804.21708-1-linus.luessing@c0d3.blue (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series mac80211: Fix wrong channel bandwidths reported for aggregates | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5 this patch: 5
netdev/cc_maintainers warning 1 maintainers not CCed: johannes@sipsolutions.net
netdev/build_clang success Errors and warnings before: 6 this patch: 6
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4 this patch: 4
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 65 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Linus Lüssing July 18, 2022, 10:28 p.m. UTC 
From: Linus Lüssing <ll@simonwunderlich.de>

AR9003 based wifi chips have a hardware bug, they always report a
channel bandwidth of HT40 for any sub-frame of an aggregate which is
not the last one. Only the last sub-frame has correct channel bandwidth
information.

This can be easily reproduced by setting an ath9k based wifi to HT20 and
running an iperf test. Then "iw dev wlan0 station dump" will occasionally,
wrongly show something like:

  rx bitrate:     121.5 MBit/s MCS 6 40MHz

Debug output in ath9k_hw_process_rxdesc_edma() confirmed that it is
always frames with (rxs->rs_isaggr && !rxs->rs_moreaggr) and no others
which report RATE_INFO_BW_40.

Unfortunately we cannot easily fix this within ath9k as in ath9k we
cannot peek at the rate/bandwidth info of the last aggregate
sub-frame and there is no queueing within ath9k after receiving the
frame from the wifi chip, it is directly handed over to mac80211.

Therefore fixing this within mac80211: For an aggergated AMPDU only
update the RX "last_rate" variable from the last sub-frame of an
aggregate. In theory, without hardware bugs, rate/bandwidth info
should be the same for all sub-frames of an aggregate anyway.

This change only affects ath9k, ath9k-htc and ath10k as these are
currently the only drivers implementing the RX_FLAG_AMPDU_LAST_KNOWN
flag.

Tested-on: 8devices Lima board, QCA9531 WiFi

Cc: Sven Eckelmann <sven@narfation.org>
Cc: Simon Wunderlich <sw@simonwunderlich.de>
Cc: Linus Lüssing <linus.luessing@c0d3.blue>
Signed-off-by: Linus Lüssing <ll@simonwunderlich.de>
---
 net/mac80211/rx.c       |  8 ++++----
 net/mac80211/sta_info.h | 16 +++++++++++++---
 2 files changed, 17 insertions(+), 7 deletions(-)

Comments

Adrian Chadd July 19, 2022, 3:03 p.m. UTC | #1 
On Mon, 18 Jul 2022 at 15:28, Linus Lüssing <linus.luessing@c0d3.blue> wrote:
>
> From: Linus Lüssing <ll@simonwunderlich.de>
>
> AR9003 based wifi chips have a hardware bug, they always report a
> channel bandwidth of HT40 for any sub-frame of an aggregate which is
> not the last one. Only the last sub-frame has correct channel bandwidth
> information.

Hi!

It's not a hardware bug. Dating back to the original AR5416 11n chip,
most flags aren't valid for subframes in an aggregate. Only the final
frame has valid flags. This was explicitly covered internally way back
when.




-adrian
Linus Lüssing July 19, 2022, 3:36 p.m. UTC | #2 
On 19/07/2022 17:03, Adrian Chadd wrote:
> Hi!
> 
> It's not a hardware bug. Dating back to the original AR5416 11n chip,
> most flags aren't valid for subframes in an aggregate. Only the final
> frame has valid flags. This was explicitly covered internally way back
> when.

Ah, thanks for the clarification! I see it in the datasheet for the 
QCA9531, too, now. And thanks for the confirmation, that what we are 
doing so far is not correct for ath9k.

Words 0+2 are valid for all RX descriptors, 0+2+11 valid for the last RX 
descriptor of each packet and 0-11 for the last RX descriptor of an 
aggregate or last RX descriptor of a stand-alone packet. Or in other 
words, word 4, which contains the 20 vs. 40 MHz indicator, is invalid 
for any aggregate sub-frame other than the last one. I can rename that 
in the commit message.


Another approach that also came to my mind was introducing more explicit 
flags in cfg80211.h's "struct rate_info", like a RATE_INFO_BW_UNKNOWN in 
"enum rate_info_bw" and/or RATE_INFO_FLAGS_UNKNOWN in "enum 
rate_info_flags". And setting those flags in ath9k_cmn_process_rate().

The current approach is smaller though, as it simply uses the already 
existing flags. If anyone has any preferences, please let me know.

Regards, Linus
Toke Høiland-Jørgensen July 20, 2022, 10:57 a.m. UTC | #3 
Linus Lüssing <ll@simonwunderlich.de> writes:

> On 19/07/2022 17:03, Adrian Chadd wrote:
>> Hi!
>> 
>> It's not a hardware bug. Dating back to the original AR5416 11n chip,
>> most flags aren't valid for subframes in an aggregate. Only the final
>> frame has valid flags. This was explicitly covered internally way back
>> when.
>
> Ah, thanks for the clarification! I see it in the datasheet for the 
> QCA9531, too, now. And thanks for the confirmation, that what we are 
> doing so far is not correct for ath9k.
>
> Words 0+2 are valid for all RX descriptors, 0+2+11 valid for the last RX 
> descriptor of each packet and 0-11 for the last RX descriptor of an 
> aggregate or last RX descriptor of a stand-alone packet. Or in other 
> words, word 4, which contains the 20 vs. 40 MHz indicator, is invalid 
> for any aggregate sub-frame other than the last one. I can rename that 
> in the commit message.
>
>
> Another approach that also came to my mind was introducing more explicit 
> flags in cfg80211.h's "struct rate_info", like a RATE_INFO_BW_UNKNOWN in 
> "enum rate_info_bw" and/or RATE_INFO_FLAGS_UNKNOWN in "enum 
> rate_info_flags". And setting those flags in ath9k_cmn_process_rate().
>
> The current approach is smaller though, as it simply uses the already 
> existing flags. If anyone has any preferences, please let me know.

I have no objections to doing it in mac80211 like you're proposing here :)

-Toke
Johannes Berg July 21, 2022, 2:43 p.m. UTC | #4 
On Tue, 2022-07-19 at 00:28 +0200, Linus Lüssing wrote:
> 
> Therefore fixing this within mac80211: For an aggergated AMPDU only
> update the RX "last_rate" variable from the last sub-frame of an
> aggregate. In theory, without hardware bugs, rate/bandwidth info
> should be the same for all sub-frames of an aggregate anyway.
> 

What if other drivers do it only on the first? :)

I'd be more inclined to squeeze in a "RATE_INVALID" flag or so somewhere
there in the rx status, and make it depend on that.

johannes
diff mbox series

Patch

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 304b9909f025..988dbf058489 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1723,6 +1723,7 @@  ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx)
 	struct sk_buff *skb = rx->skb;
 	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+	u32 *last_rate = &sta->deflink.rx_stats.last_rate;
 	int i;
 
 	if (!sta)
@@ -1744,8 +1745,7 @@  ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx)
 			sta->deflink.rx_stats.last_rx = jiffies;
 			if (ieee80211_is_data(hdr->frame_control) &&
 			    !is_multicast_ether_addr(hdr->addr1))
-				sta->deflink.rx_stats.last_rate =
-					sta_stats_encode_rate(status);
+				sta_stats_encode_rate(status, last_rate);
 		}
 	} else if (rx->sdata->vif.type == NL80211_IFTYPE_OCB) {
 		sta->deflink.rx_stats.last_rx = jiffies;
@@ -1757,7 +1757,7 @@  ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx)
 		 */
 		sta->deflink.rx_stats.last_rx = jiffies;
 		if (ieee80211_is_data(hdr->frame_control))
-			sta->deflink.rx_stats.last_rate = sta_stats_encode_rate(status);
+			sta_stats_encode_rate(status, last_rate);
 	}
 
 	sta->deflink.rx_stats.fragments++;
@@ -4502,7 +4502,7 @@  static void ieee80211_rx_8023(struct ieee80211_rx_data *rx,
 	/* end of statistics */
 
 	stats->last_rx = jiffies;
-	stats->last_rate = sta_stats_encode_rate(status);
+	sta_stats_encode_rate(status, &stats->last_rate);
 
 	stats->fragments++;
 	stats->packets++;
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 70ee55ec5518..67f9c1647567 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -941,10 +941,19 @@  enum sta_stats_type {
 
 #define STA_STATS_RATE_INVALID		0
 
-static inline u32 sta_stats_encode_rate(struct ieee80211_rx_status *s)
+static inline void
+sta_stats_encode_rate(struct ieee80211_rx_status *s, u32 *rate)
 {
 	u32 r;
 
+	/* some drivers (notably ath9k) only report a valid bandwidth
+	 * in the last subframe of an aggregate, skip the others
+	 * in that case
+	 */
+	if (s->flag & RX_FLAG_AMPDU_LAST_KNOWN &&
+	    !(s->flag & RX_FLAG_AMPDU_IS_LAST))
+		return;
+
 	r = STA_STATS_FIELD(BW, s->bw);
 
 	if (s->enc_flags & RX_ENC_FLAG_SHORT_GI)
@@ -975,10 +984,11 @@  static inline u32 sta_stats_encode_rate(struct ieee80211_rx_status *s)
 		break;
 	default:
 		WARN_ON(1);
-		return STA_STATS_RATE_INVALID;
+		*rate = STA_STATS_RATE_INVALID;
+		return;
 	}
 
-	return r;
+	*rate = r;
 }
 
 #endif /* STA_INFO_H */