2012-04-06 18:48:16

by Lorenzo Bianconi

Subject: [PATCH] mac80211: fix an issue in ieee80211_tx_info count field management

Hi,

I noticed a possible issue in the status count field management of the
ieee80211_tx_info data structure. In particular, when the AGGR
processing is employed,
status.rates[].count is set just for the first frame and not for
others belonging to the same burst, leading to wrong statistic data in
the mac80211 debug file system.

Regards

Lorenzo

Signed-off-by: Lorenzo Bianconi <[email protected]>
---
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -355,7 +355,13 @@
int rtap_len;

for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
- if (info->status.rates[i].idx < 0) {
+ if ((info->flags & IEEE80211_TX_CTL_AMPDU) &&
+ !(info->flags & IEEE80211_TX_STAT_AMPDU)) {
+ /* just the first aggr frame carry status info */
+ info->status.rates[i].idx = -1;
+ info->status.rates[i].count = 0;
+ break;
+ } else if (info->status.rates[i].idx < 0) {
break;
} else if (i >= hw->max_report_rates) {
/* the HW cannot have attempted that rate */
--

UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep

2012-04-12 07:25:03

by Felix Fietkau

Subject: Re: [PATCH] mac80211: fix an issue in ieee80211_tx_info count field management

On 2012-04-12 6:03 AM, Johannes Berg wrote:
> On Fri, 2012-04-06 at 20:48 +0200, Lorenzo Bianconi wrote:
>> Hi,
>>
>> I noticed a possible issue in the status count field management of the
>> ieee80211_tx_info data structure. In particular, when the AGGR
>> processing is employed,
>> status.rates[].count is set just for the first frame and not for
>> others belonging to the same burst, leading to wrong statistic data in
>> the mac80211 debug file system.
>>
>> Regards
>>
>> Lorenzo
>>
>> Signed-off-by: Lorenzo Bianconi <[email protected]>
>> ---
>> --- a/net/mac80211/status.c
>> +++ b/net/mac80211/status.c
>> @@ -355,7 +355,13 @@
>> int rtap_len;
>>
>> for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
>> - if (info->status.rates[i].idx < 0) {
>> + if ((info->flags & IEEE80211_TX_CTL_AMPDU) &&
>> + !(info->flags & IEEE80211_TX_STAT_AMPDU)) {
>> + /* just the first aggr frame carry status info */
>> + info->status.rates[i].idx = -1;
>> + info->status.rates[i].count = 0;
>> + break;
>> + } else if (info->status.rates[i].idx < 0) {
>
> Is that really the way all drivers do it? I know ath9k seems to do it
> that way, but I don't think all drivers do. ath9k is probably the
> exception rather than the rule, at least today
At least minstrel_ht expects it this way. If I understand iwlwifi
correctly, it reports tx status in a similar way - on completion in
iwlagn_rx_reply_compressed_ba(), the first subframe carries the
information for an entire A-MPDU, the other subframes are not touched.

- Felix

2012-04-12 04:04:39

by Johannes Berg

Subject: Re: [PATCH] mac80211: fix an issue in ieee80211_tx_info count field management

On Fri, 2012-04-06 at 20:48 +0200, Lorenzo Bianconi wrote:
> Hi,
>
> I noticed a possible issue in the status count field management of the
> ieee80211_tx_info data structure. In particular, when the AGGR
> processing is employed,
> status.rates[].count is set just for the first frame and not for
> others belonging to the same burst, leading to wrong statistic data in
> the mac80211 debug file system.
>
> Regards
>
> Lorenzo
>
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
> --- a/net/mac80211/status.c
> +++ b/net/mac80211/status.c
> @@ -355,7 +355,13 @@
> int rtap_len;
>
> for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
> - if (info->status.rates[i].idx < 0) {
> + if ((info->flags & IEEE80211_TX_CTL_AMPDU) &&
> + !(info->flags & IEEE80211_TX_STAT_AMPDU)) {
> + /* just the first aggr frame carry status info */
> + info->status.rates[i].idx = -1;
> + info->status.rates[i].count = 0;
> + break;
> + } else if (info->status.rates[i].idx < 0) {

Is that really the way all drivers do it? I know ath9k seems to do it
that way, but I don't think all drivers do. ath9k is probably the
exception rather than the rule, at least today

johannes