[PATCH] rt2x00: Fix TX status reporting

[PATCH] rt2x00: Fix TX status reporting

[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 <IvDoorn@gmail.com>
---
 drivers/net/wireless/rt2x00/rt2x00dev.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c 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);
 
 	/*
-	 * 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++;
 	}
 
-- 
1.6.4.1

Re: [rt2x00-users] [PATCH] rt2x00: Fix TX status reporting

[Benoit PAPILLAULT]

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

Ivo van Doorn a écrit :
> 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 <IvDoorn@gmail.com>
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

> --- drivers/net/wireless/rt2x00/rt2x00dev.c |   28
> ++++++++++++++++------------ 1 files changed, 16 insertions(+), 12
> deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c
> 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);
>
> /* -     * 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++; }
>

-----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-----

Re: [rt2x00-users] [PATCH] rt2x00: Fix TX status reporting

[Ivo van Doorn]

On Saturday 29 August 2009, Benoit PAPILLAULT wrote:
> Ivo van Doorn a écrit :
> > 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 <IvDoorn@gmail.com>
> 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