[PATCH] ath9k: Fix rate table choice

[PATCH] ath9k: Fix rate table choice

[Benoit Papillault]

ath9k rate control choose the rate table based on the current channel
(if it is ht40 or not). However, it happens that at this stage, the
channel is still ht20 even if the peer sta capabilities would later
allow ht40. So let's use peer sta capabilities directly.

Signed-off-by: Benoit Papillault <benoit.papillault@free.fr>
---
 drivers/net/wireless/ath/ath9k/rc.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index 1196884..5ce9da5 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -1341,10 +1341,8 @@ static void ath_rate_update(void *priv, struct ieee80211_supported_band *sband,
 		if (sc->sc_ah->opmode != NL80211_IFTYPE_STATION)
 			return;
 
-		if (sc->hw->conf.channel_type == NL80211_CHAN_HT40MINUS ||
-		    sc->hw->conf.channel_type == NL80211_CHAN_HT40PLUS)
-			oper_cw40 = true;
-
+		oper_cw40 = (sta->ht_cap.cap &
+			     IEEE80211_HT_CAP_SUP_WIDTH_20_40) ? true : false;
 		oper_sgi40 = (sta->ht_cap.cap & IEEE80211_HT_CAP_SGI_40) ?
 			true : false;
 
-- 
1.6.3.3

Re: [PATCH] ath9k: Fix rate table choice

[Luis R. Rodriguez]

On Thu, Feb 4, 2010 at 1:06 PM, Benoit Papillault
<benoit.papillault@free.fr> wrote:
> ath9k rate control choose the rate table based on the current channel
> (if it is ht40 or not). However, it happens that at this stage, the
> channel is still ht20 even if the peer sta capabilities would later
> allow ht40. So let's use peer sta capabilities directly.

Can you clarify this commit log entry a little more, I don't think I
get it yet. Also, is this a stable fix? If so, please Cc:
stable@kernel.org on the commit log entry.

  Luis

Re: [PATCH] ath9k: Fix rate table choice

[Benoit PAPILLAULT]

Luis R. Rodriguez a écrit :
> On Thu, Feb 4, 2010 at 1:06 PM, Benoit Papillault
> <benoit.papillault@free.fr> wrote:
>   
>> ath9k rate control choose the rate table based on the current channel
>> (if it is ht40 or not). However, it happens that at this stage, the
>> channel is still ht20 even if the peer sta capabilities would later
>> allow ht40. So let's use peer sta capabilities directly.
>>     
>
> Can you clarify this commit log entry a little more, I don't think I
> get it yet. Also, is this a stable fix? If so, please Cc:
> stable@kernel.org on the commit log entry.
>
>   Luis
>   
I think it could be Cc:stable@kernel.org, but I'll leave the decision up 
to you.

In fact, I was using ath9k as a STA connected to an HT40 AP. However, 
only HT20 was displayed in ath9k/phy0/rcstat.
After digging for a while, i found that the current channel 
(sc->hw->conf.channel_type) was indeed an HT20 (or no-ht) channel!

In fact, association is handled as a queued work (from what I 
understood). As such, it uses tmp_channel (see mac80211/work.c). In the 
enable_ht() [mac80211/mlme.c], we update oper_channel_type before 
calling ieee80211_hw_config to call the hw_config driver callback that 
upates conf.channel_type. However, since we are using queued work, 
ieee80211_hw_config will use tmp_channel instead. Then enable_ht calls 
rate_control_rate_update that calls ath9k rate control callbacks ... 
where the channel is ... not HT40! I guess that when the queued work is 
done, the channel is switched again to ht40.

I would appreciate Johannes point of view here...

After applying this patch, ath9k/phy0/rcstat shows both ht20 and ht40 rates.

Regards,
Benoit

[PATCH] ath9k: Fix rate table choice

[Sujith]

Benoit Papillault wrote:
> ath9k rate control choose the rate table based on the current channel
> (if it is ht40 or not). However, it happens that at this stage, the
> channel is still ht20 even if the peer sta capabilities would later
> allow ht40. So let's use peer sta capabilities directly.
> 
> Signed-off-by: Benoit Papillault <benoit.papillault@free.fr>
> ---
>  drivers/net/wireless/ath/ath9k/rc.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
> index 1196884..5ce9da5 100644
> --- a/drivers/net/wireless/ath/ath9k/rc.c
> +++ b/drivers/net/wireless/ath/ath9k/rc.c
> @@ -1341,10 +1341,8 @@ static void ath_rate_update(void *priv, struct ieee80211_supported_band *sband,
>                 if (sc->sc_ah->opmode != NL80211_IFTYPE_STATION)
>                         return;
> 
> -               if (sc->hw->conf.channel_type == NL80211_CHAN_HT40MINUS ||
> -                   sc->hw->conf.channel_type == NL80211_CHAN_HT40PLUS)
> -                       oper_cw40 = true;
> -
> +               oper_cw40 = (sta->ht_cap.cap &
> +                            IEEE80211_HT_CAP_SUP_WIDTH_20_40) ? true : false;
>                 oper_sgi40 = (sta->ht_cap.cap & IEEE80211_HT_CAP_SGI_40) ?
>                         true : false;
> 

This is wrong, the AP can certainly support HT40/HT20 and switch between them
dynamically - this patch changes how ath9k reacts to channel switching.

The AP can switch its operating channel to HT20 - but the HT40 supported bit
will _still_ be set. Can you capture the association sequence and check the
HT_INFO IE ( which is different from HT CAP) ?

Sujith

Re: [PATCH] ath9k: Fix rate table choice

[Benoit PAPILLAULT]

Sujith a écrit :
> Benoit Papillault wrote:
>   
>> ath9k rate control choose the rate table based on the current channel
>> (if it is ht40 or not). However, it happens that at this stage, the
>> channel is still ht20 even if the peer sta capabilities would later
>> allow ht40. So let's use peer sta capabilities directly.
>>
>> Signed-off-by: Benoit Papillault <benoit.papillault@free.fr>
>> ---
>>  drivers/net/wireless/ath/ath9k/rc.c |    6 ++----
>>  1 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
>> index 1196884..5ce9da5 100644
>> --- a/drivers/net/wireless/ath/ath9k/rc.c
>> +++ b/drivers/net/wireless/ath/ath9k/rc.c
>> @@ -1341,10 +1341,8 @@ static void ath_rate_update(void *priv, struct ieee80211_supported_band *sband,
>>                 if (sc->sc_ah->opmode != NL80211_IFTYPE_STATION)
>>                         return;
>>
>> -               if (sc->hw->conf.channel_type == NL80211_CHAN_HT40MINUS ||
>> -                   sc->hw->conf.channel_type == NL80211_CHAN_HT40PLUS)
>> -                       oper_cw40 = true;
>> -
>> +               oper_cw40 = (sta->ht_cap.cap &
>> +                            IEEE80211_HT_CAP_SUP_WIDTH_20_40) ? true : false;
>>                 oper_sgi40 = (sta->ht_cap.cap & IEEE80211_HT_CAP_SGI_40) ?
>>                         true : false;
>>
>>     
>
> This is wrong, the AP can certainly support HT40/HT20 and switch between them
> dynamically - this patch changes how ath9k reacts to channel switching.
>   
Sure, but when the channel is switched, ath9k rate control is not called 
AFAIK. So nothing is updated.
ath_rate_update is called by rate_control_rate_update, which is only 
called by enable_ht. enable_ht is called as association time and on RX 
beacon. However, since the content of RX beacon is not changed, 
rate_control_rate_update will not be called again. That's what I show in 
my logs...
> The AP can switch its operating channel to HT20 - but the HT40 supported bit
> will _still_ be set. Can you capture the association sequence and check the
> HT_INFO IE ( which is different from HT CAP) ?
>
> Sujith
>   
Will try to do it. I see your point :
- current code never enables ht40 rate table, but the rate table if 
updated if the AP switch to an ht20 operating channel.
- my patch correctly enables ht40 rate table, but is not updated if the 
AP switch to an ht20 operating channel.

Maybe local->oper_channel_type could be passed directly to 
rate_control_rate_update?
Or as Johannes suggest, we could always select the ht40 rate table and 
adapt in ath_get_rate according to hw.conf.channel_type dynamically?

Regards,
Benoit

Re: [PATCH] ath9k: Fix rate table choice

[Sujith]

Benoit PAPILLAULT wrote:
> Sure, but when the channel is switched, ath9k rate control is not called
> AFAIK. So nothing is updated.
> ath_rate_update is called by rate_control_rate_update, which is only
> called by enable_ht. enable_ht is called as association time and on RX
> beacon. However, since the content of RX beacon is not changed,
> rate_control_rate_update will not be called again. That's what I show in
> my logs...

Can you post the ath9k logs with debug=0x200 ?

> Will try to do it. I see your point :
> - current code never enables ht40 rate table, but the rate table if
> updated if the AP switch to an ht20 operating channel.

Hm, if the channel type is HT40, then ath9k would enable HT40 in RC.

Sujith