rt2x00: Fix TX status reporting

PATCH In review Ivo van Doorn 2 participants 3 messages Aug 29, 2009 – Aug 30, 2009 Networking drivers (wireless)

Share & tools

Ivo van Doorn

author

2009-08-29 17:10 UTC #0 lore raw

PATCH 1/1 rt2x00: Fix TX status reporting

Not all values of the TX status enumeration were covered during updating of the TX statistics. This could lead to wrong bitrate tuning but also wrong behavior in tools like hostapd.

Signed-off-by Ivo van Doorn <[email protected]>

---
 drivers/net/wireless/rt2x00/rt2x00dev.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

drivers/net/wireless/rt2x00/rt2x00dev.c +16 ~~-12~~

index 5db613f..71761b3 100644
@@ -206,6 +206,7 @@ void rt2x00lib_txdone(struct queue_entry *entry,
 	unsigned int header_length = ieee80211_get_hdrlen_from_skb(entry->skb);
 	u8 rate_idx, rate_flags, retry_rates;
 	unsigned int i;
+	bool success;
 
 	/*
 	 * Unmap the skb.

@@ -234,13 +235,18 @@ void rt2x00lib_txdone(struct queue_entry *entry,
 	rt2x00debug_dump_frame(rt2x00dev, DUMP_FRAME_TXDONE, entry->skb);
 
 	/*
-	 * Update TX statistics.
+	 * Determine if the frame has been successfully transmitted.
 	 */
-	rt2x00dev->link.qual.tx_success +=
+	success =
 	    test_bit(TXDONE_SUCCESS, &txdesc->flags) ||
-	    test_bit(TXDONE_UNKNOWN, &txdesc->flags);
-	rt2x00dev->link.qual.tx_failed +=
-	    test_bit(TXDONE_FAILURE, &txdesc->flags);
+	    test_bit(TXDONE_UNKNOWN, &txdesc->flags) ||
+	    test_bit(TXDONE_FALLBACK, &txdesc->flags);
+
+	/*
+	 * Update TX statistics.
+	 */
+	rt2x00dev->link.qual.tx_success += success;
+	rt2x00dev->link.qual.tx_failed += !success;
 
 	rate_idx = skbdesc->tx_rate_idx;
 	rate_flags = skbdesc->tx_rate_flags;

@@ -263,22 +269,20 @@ void rt2x00lib_txdone(struct queue_entry *entry,
 		tx_info->status.rates[i].flags = rate_flags;
 		tx_info->status.rates[i].count = 1;
 	}
-	if (i < (IEEE80211_TX_MAX_RATES -1))
+	if (i < (IEEE80211_TX_MAX_RATES - 1))
 		tx_info->status.rates[i].idx = -1; /* terminate */
 
 	if (!(tx_info->flags & IEEE80211_TX_CTL_NO_ACK)) {
-		if (test_bit(TXDONE_SUCCESS, &txdesc->flags) ||
-				test_bit(TXDONE_UNKNOWN, &txdesc->flags))
+		if (success)
 			tx_info->flags |= IEEE80211_TX_STAT_ACK;
-		else if (test_bit(TXDONE_FAILURE, &txdesc->flags))
+		else
 			rt2x00dev->low_level_stats.dot11ACKFailureCount++;
 	}
 
 	if (rate_flags & IEEE80211_TX_RC_USE_RTS_CTS) {
-		if (test_bit(TXDONE_SUCCESS, &txdesc->flags) ||
-				test_bit(TXDONE_UNKNOWN, &txdesc->flags))
+		if (success)
 			rt2x00dev->low_level_stats.dot11RTSSuccessCount++;
-		else if (test_bit(TXDONE_FAILURE, &txdesc->flags))
+		else
 			rt2x00dev->low_level_stats.dot11RTSFailureCount++;
 	}

Signature · 1 line

1.6.4.1

Benoit PAPILLAULT

2009-08-29 20:31 UTC #1 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 5 lines · Ivo van Doorn

Not all values of the TX status enumeration were covered during updating of the TX statistics. This could lead to wrong bitrate tuning but also wrong behavior in tools like hostapd.
Signed-off-by Ivo van Doorn <[email protected]>

I have a pending patch about TX status that changed the meaning of TXDONE_FALLBACK just a bit... just to avoid this change in fact. I should be able to send those patches in few hours.

In my code, you success/failure of the packet is reported with TXDONE_SUCCESS / TXDONE_FAILURE. The TXDONE_FALLBACK is set independently to indicate that a fallback rate table has been used (and this could be the case for either success or failure).

Regards,
Benoit

Quoted 16 lines

--- drivers/net/wireless/rt2x00/rt2x00dev.c | 28
++++++++++++++++------------ 1 files changed, 16 insertions(+), 12 deletions(-)
(quoted diff) +0 -0
b/drivers/net/wireless/rt2x00/rt2x00dev.c index 5db613f..71761b3 100644 --- a/drivers/net/wireless/rt2x00/rt2x00dev.c +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c @@ -206,6 +206,7 @@ void rt2x00lib_txdone(struct queue_entry *entry, unsigned int header_length = ieee80211_get_hdrlen_from_skb(entry->skb); u8
rate_idx, rate_flags, retry_rates; unsigned int i; + bool success;
/* * Unmap the skb. @@ -234,13 +235,18 @@ void rt2x00lib_txdone(struct queue_entry *entry, rt2x00debug_dump_frame(rt2x00dev, DUMP_FRAME_TXDONE, entry->skb);

Quoted 7 lines

/* - * Update TX statistics. + * Determine if the frame has been
successfully transmitted. */ - rt2x00dev->link.qual.tx_success += +
success = test_bit(TXDONE_SUCCESS, &txdesc->flags) || - test_bit(TXDONE_UNKNOWN, &txdesc->flags); -
rt2x00dev->link.qual.tx_failed += - test_bit(TXDONE_FAILURE,
&txdesc->flags); + test_bit(TXDONE_UNKNOWN, &txdesc->flags) ||
+ test_bit(TXDONE_FALLBACK, &txdesc->flags); + + /* + *

Update

Quoted 18 lines

TX statistics. + */ + rt2x00dev->link.qual.tx_success += success;
+ rt2x00dev->link.qual.tx_failed += !success;
rate_idx = skbdesc->tx_rate_idx; rate_flags = skbdesc->tx_rate_flags; @@ -263,22 +269,20 @@ void rt2x00lib_txdone(struct queue_entry *entry, tx_info->status.rates[i].flags = rate_flags;
tx_info->status.rates[i].count = 1; } - if (i <
(IEEE80211_TX_MAX_RATES -1)) + if (i < (IEEE80211_TX_MAX_RATES -
1)) tx_info->status.rates[i].idx = -1; /* terminate */
if (!(tx_info->flags & IEEE80211_TX_CTL_NO_ACK)) { - if
(test_bit(TXDONE_SUCCESS, &txdesc->flags) || -
test_bit(TXDONE_UNKNOWN, &txdesc->flags)) + if (success)
tx_info->flags |= IEEE80211_TX_STAT_ACK; - else if
(test_bit(TXDONE_FAILURE, &txdesc->flags)) + else
rt2x00dev->low_level_stats.dot11ACKFailureCount++; }

Quoted 7 lines

if (rate_flags & IEEE80211_TX_RC_USE_RTS_CTS) { - if
(test_bit(TXDONE_SUCCESS, &txdesc->flags) || -
test_bit(TXDONE_UNKNOWN, &txdesc->flags)) + if (success)
rt2x00dev->low_level_stats.dot11RTSSuccessCount++; - else if
(test_bit(TXDONE_FAILURE, &txdesc->flags)) + else
rt2x00dev->low_level_stats.dot11RTSFailureCount++; }

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkqZkB8ACgkQOR6EySwP7oKnaQCglW+lu167n88CR3Chv1i4uF1P I1kAoLrVvvXCbmzWN8M2/wHtyTSCnUZu
=IJEh
-----END PGP SIGNATURE-----

Ivo van Doorn

author

2009-08-30 13:30 UTC #2 lore raw

Quoted 6 lines · Benoit PAPILLAULT

Quoted 5 lines · Ivo van Doorn
Not all values of the TX status enumeration were covered during updating of the TX statistics. This could lead to wrong bitrate tuning but also wrong behavior in tools like hostapd.
Signed-off-by Ivo van Doorn <[email protected]>

Quoted 8 lines

I have a pending patch about TX status that changed the meaning of TXDONE_FALLBACK just a bit... just to avoid this change in fact. I should be able to send those patches in few hours.
In my code, you success/failure of the packet is reported with TXDONE_SUCCESS / TXDONE_FAILURE. The TXDONE_FALLBACK is set independently to indicate that a fallback rate table has been used (and this could be the case for either success or failure).

Well that shouldn't block this patch at this time since it is a bugfix which is quite important to get minstrel and hostapd working properly.

Once I have reviewed your patch it can be send upstream immediately or in a single batch together with the rt2800pci driver. ;)

Ivo