[PATCH 3/3] ath9k: built-in rate control A-MPDU fix

[PATCH 3/3] ath9k: built-in rate control A-MPDU fix

[Björn Smedman]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1023 bytes --]

This patch attempts to ensure that ath9k's built-in rate control algorithm 
does not rely on the value of the ampdu_len and ampdu_ack_len tx status 
fields unless the IEEE80211_TX_STAT_AMPDU flag is set.

This patch has not been tested.

Cc: <stable@kernel.org>
Signed-off-by: Björn Smedman <bjorn.smedman@venatech.se>
---
diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index ce1cd6d..13f9e88 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -1375,6 +1375,12 @@ static void ath_tx_status(void *priv, struct ieee80211_supported_band *sband,
 	if (tx_info->flags & IEEE80211_TX_STAT_TX_FILTERED)
 		return;
 
+	if (!(tx_info->flags & IEEE80211_TX_STAT_AMPDU)) {
+		tx_info->status.ampdu_ack_len =
+			(tx_info->flags & IEEE80211_TX_STAT_ACK ? 1 : 0);
+		tx_info->status.ampdu_len = 1;
+	}
+
 	/*
 	 * If an underrun error is seen assume it as an excessive retry only
 	 * if max frame trigger level has been reached (2 KB for singel stream,

Re: [PATCH 3/3] ath9k: built-in rate control A-MPDU fix

[Felix Fietkau]

On 2010-10-10 10:51 PM, Björn Smedman wrote:
> This patch attempts to ensure that ath9k's built-in rate control algorithm 
> does not rely on the value of the ampdu_len and ampdu_ack_len tx status 
> fields unless the IEEE80211_TX_STAT_AMPDU flag is set.
> 
> This patch has not been tested.
> 
> Cc: <stable@kernel.org>
> Signed-off-by: Björn Smedman <bjorn.smedman@venatech.se>
> ---
> diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
> index ce1cd6d..13f9e88 100644
> --- a/drivers/net/wireless/ath/ath9k/rc.c
> +++ b/drivers/net/wireless/ath/ath9k/rc.c
> @@ -1375,6 +1375,12 @@ static void ath_tx_status(void *priv, struct ieee80211_supported_band *sband,
>  	if (tx_info->flags & IEEE80211_TX_STAT_TX_FILTERED)
>  		return;
>  
> +	if (!(tx_info->flags & IEEE80211_TX_STAT_AMPDU)) {
> +		tx_info->status.ampdu_ack_len =
> +			(tx_info->flags & IEEE80211_TX_STAT_ACK ? 1 : 0);
> +		tx_info->status.ampdu_len = 1;
> +	}
> +
Shouldn't mac80211 do this instead?

- Felix

Re: [PATCH 3/3] ath9k: built-in rate control A-MPDU fix

[Björn Smedman]

2010/10/10 Felix Fietkau <nbd@openwrt.org>:
> On 2010-10-10 10:51 PM, Björn Smedman wrote:
>> --- a/drivers/net/wireless/ath/ath9k/rc.c
>> +++ b/drivers/net/wireless/ath/ath9k/rc.c
>> @@ -1375,6 +1375,12 @@ static void ath_tx_status(void *priv, struct ieee80211_supported_band *sband,
>>       if (tx_info->flags & IEEE80211_TX_STAT_TX_FILTERED)
>>               return;
>>
>> +     if (!(tx_info->flags & IEEE80211_TX_STAT_AMPDU)) {
>> +             tx_info->status.ampdu_ack_len =
>> +                     (tx_info->flags & IEEE80211_TX_STAT_ACK ? 1 : 0);
>> +             tx_info->status.ampdu_len = 1;
>> +     }
>> +
> Shouldn't mac80211 do this instead?

I guess you could move this into mac80211 but that would change the
suggested contract: (1) the ampdu_len and ampdu_ack_len fields of the
tx status structure must be set correctly by the driver if the
IEEE80211_TX_STAT_AMPDU flag is set, and (2) the value of these fields
must not be relied upon by rate control if the flag is not set. I
personally think this is more clean than having rate control
algorithms trust the value of ampdu_len and ampdu_ack_len even when
the frame has clearly not been aggregated.

If the interface was correct in principle I think I could be persuaded
with a simple renaming of the fields, but in this case the interface
is fundamentally broken (e.g. in the case of an aggregate in which all
frames failed to be ACKed and are to be software retried - no rate
control feedback is possible).

For that reason I think we should push the temporary/ugly code out
into drivers and rate control plugins and not try to "tidy up" the
wrong solution in mac80211. But that's just my two cents of course.

/Björn

Re: [PATCH 3/3] ath9k: built-in rate control A-MPDU fix

[Luis R. Rodriguez]

2010/10/10 Björn Smedman <bjorn.smedman@venatech.se>:
> 2010/10/10 Felix Fietkau <nbd@openwrt.org>:
>> On 2010-10-10 10:51 PM, Björn Smedman wrote:
>>> --- a/drivers/net/wireless/ath/ath9k/rc.c
>>> +++ b/drivers/net/wireless/ath/ath9k/rc.c
>>> @@ -1375,6 +1375,12 @@ static void ath_tx_status(void *priv, struct ieee80211_supported_band *sband,
>>>       if (tx_info->flags & IEEE80211_TX_STAT_TX_FILTERED)
>>>               return;
>>>
>>> +     if (!(tx_info->flags & IEEE80211_TX_STAT_AMPDU)) {
>>> +             tx_info->status.ampdu_ack_len =
>>> +                     (tx_info->flags & IEEE80211_TX_STAT_ACK ? 1 : 0);
>>> +             tx_info->status.ampdu_len = 1;
>>> +     }
>>> +
>> Shouldn't mac80211 do this instead?
>
> I guess you could move this into mac80211 but that would change the
> suggested contract: (1) the ampdu_len and ampdu_ack_len fields of the
> tx status structure must be set correctly by the driver if the
> IEEE80211_TX_STAT_AMPDU flag is set, and (2) the value of these fields
> must not be relied upon by rate control if the flag is not set. I
> personally think this is more clean than having rate control
> algorithms trust the value of ampdu_len and ampdu_ack_len even when
> the frame has clearly not been aggregated.
>
> If the interface was correct in principle I think I could be persuaded
> with a simple renaming of the fields, but in this case the interface
> is fundamentally broken (e.g. in the case of an aggregate in which all
> frames failed to be ACKed and are to be software retried - no rate
> control feedback is possible).
>
> For that reason I think we should push the temporary/ugly code out
> into drivers and rate control plugins and not try to "tidy up" the
> wrong solution in mac80211. But that's just my two cents of course.

what's the status of this patch

 Luis