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: <[email protected]>
Signed-off-by: Bj?rn Smedman <[email protected]>
---
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,
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: <[email protected]>
> Signed-off-by: Bj?rn Smedman <[email protected]>
> ---
> 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
2010/10/10 Björn Smedman <[email protected]>:
> 2010/10/10 Felix Fietkau <[email protected]>:
>> 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
2010/10/10 Felix Fietkau <[email protected]>:
> 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