Bisected: ath9k: fix powersave frame filtering/buffering in AP mode broke STA association.

DISCUSSION Ben Greear 2 participants 12 messages May 5, 2011 – May 6, 2011

Subsystems (2)

mac80211 Qualcomm Atheros ATH9K wireless driver

lore

Share & tools

lore

Ben Greear

author

2011-05-05 20:44 UTC #0 lore raw

I bisected the problem I was having with ath9k STA configured for WPA not being able to authenticate. I'm using nohwcrypt=1, which may be why no one else sees it.

According to bisect, this is the first bad commit:

commit 5519541d5a5f19893546883547e2f0f2e5934df7

Author: Felix Fietkau <[email protected]>
Date:   Sun Apr 17 23:28:09 2011 +0200

ath9k: fix powersave frame filtering/buffering in AP mode

This patch fixes a long standing issue of pending packets in the queue being sent (and retransmitted many times) to sleeping stations.
This was made worse by aggregation through driver-internal retransmitting of A-MDPU subframes.
Previously the hardware tx filter was cleared unconditionally for every single packet - with this patch it uses the IEEE80211_TX_CTL_CLEAR_PS_FILT for unaggregated frames.
A sta_notify driver op is added to stop aggregation for stations when they enter powersave mode. Subframes stay buffered inside the driver, to ensure that the BlockAck window keeps a sane state.
Since the driver uses software aggregation, the clearing of the tx filter needs to be handled by the driver instead of mac80211 for aggregated frames.

Signed-off-by Felix Fietkau <[email protected]>

Signed-off-by John W. Linville <[email protected]>

I'll be happy to test patches, it seems very easy to reproduce on my setup.

Thanks,
Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Felix Fietkau

2011-05-05 21:03 UTC #1 lore raw

Quoted 25 lines · Ben Greear

I bisected the problem I was having with ath9k STA configured for WPA not being able to authenticate. I'm using nohwcrypt=1, which may be why no one else sees it.
According to bisect, this is the first bad commit:
commit 5519541d5a5f19893546883547e2f0f2e5934df7
Author: Felix Fietkau<[email protected]>
Date:   Sun Apr 17 23:28:09 2011 +0200
ath9k: fix powersave frame filtering/buffering in AP mode
This patch fixes a long standing issue of pending packets in the queue being sent (and retransmitted many times) to sleeping stations.
This was made worse by aggregation through driver-internal retransmitting of A-MDPU subframes.
Previously the hardware tx filter was cleared unconditionally for every single packet - with this patch it uses the IEEE80211_TX_CTL_CLEAR_PS_FILT for unaggregated frames.
A sta_notify driver op is added to stop aggregation for stations when they enter powersave mode. Subframes stay buffered inside the driver, to ensure that the BlockAck window keeps a sane state.
Since the driver uses software aggregation, the clearing of the tx filter needs to be handled by the driver instead of mac80211 for aggregated frames.

Quoted 6 lines

Signed-off-by Felix Fietkau<[email protected]>
Signed-off-by John W. Linville<[email protected]>
I'll be happy to test patches, it seems very easy to reproduce on my setup.

How about this?

net/mac80211/tx.c +3 -0

@@ -1270,6 +1270,9 @@ ieee80211_tx_prepare(struct ieee80211_su
 		info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
 	else if (test_and_clear_sta_flags(tx->sta, WLAN_STA_CLEAR_PS_FILT))
 		info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
+	else if (sdata->vif.type != NL80211_IFTYPE_AP &&
+	         sdata->vif.type != NL80211_IFTYPE_AP_VLAN)
+		info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
 
 	hdrlen = ieee80211_hdrlen(hdr->frame_control);
 	if (skb->len > hdrlen + sizeof(rfc1042_header) + 2) {

Ben Greear

author

2011-05-05 21:15 UTC #2 lore raw

Quoted 12 lines · Felix Fietkau

Quoted 6 lines · Ben Greear
I bisected the problem I was having with ath9k STA configured for WPA not being able to authenticate. I'm using nohwcrypt=1, which may be why no one else sees it.
I'll be happy to test patches, it seems very easy to reproduce on my setup.
How about this?

net/mac80211/tx.c ieee80211_tx_prepare(struct ieee80211_su · line 1278

Quoted 9 lines

net/mac80211/tx.c +3 -0

@@ -1270,6 +1270,9 @@ ieee80211_tx_prepare(struct ieee80211_su
  		info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
  	else if (test_and_clear_sta_flags(tx->sta, WLAN_STA_CLEAR_PS_FILT))
  		info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
+	else if (sdata->vif.type != NL80211_IFTYPE_AP&&
+	         sdata->vif.type != NL80211_IFTYPE_AP_VLAN)
+		info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
 
  	hdrlen = ieee80211_hdrlen(hdr->frame_control);
  	if (skb->len>  hdrlen + sizeof(rfc1042_header) + 2) {

Yes, that seems to fix the problem.

Thanks for the quick fix!

Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Ben Greear

author

2011-05-05 21:43 UTC #3 lore raw

Quoted 20 lines · Ben Greear

Quoted 19 lines · Felix Fietkau
Quoted 6 lines · Ben Greear
I bisected the problem I was having with ath9k STA configured for WPA not being able to authenticate. I'm using nohwcrypt=1, which may be why no one else sees it.
I'll be happy to test patches, it seems very easy to reproduce on my setup.
How about this?
net/mac80211/tx.c +3 -0
@@ -1270,6 +1270,9 @@ ieee80211_tx_prepare(struct ieee80211_su
 info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
 else if (test_and_clear_sta_flags(tx->sta, WLAN_STA_CLEAR_PS_FILT))
 info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
+ else if (sdata->vif.type != NL80211_IFTYPE_AP&&
+ sdata->vif.type != NL80211_IFTYPE_AP_VLAN)
+ info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
 

Quoted 6 lines

net/mac80211/tx.c ieee80211_tx_prepare(struct ieee80211_su · line 1278
Quoted 2 lines
net/mac80211/tx.c +0 -0
 hdrlen = ieee80211_hdrlen(hdr->frame_control);
 if (skb->len> hdrlen + sizeof(rfc1042_header) + 2) {
Yes, that seems to fix the problem.
Thanks for the quick fix!

Well, that appeared to fix the problem on -rc1, but when I go back to -rc6 and apply that patch, it still doesn't work.

Might be some other issue though...

Are you able to get STA to associate & athenticate on latest wireless-testing?

Thanks,
Ben

Quoted 3 lines

Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Felix Fietkau

2011-05-05 21:54 UTC #4 lore raw

Quoted 24 lines · Ben Greear

Quoted 23 lines · Ben Greear
Quoted 21 lines · Felix Fietkau
Quoted 6 lines · Ben Greear
I bisected the problem I was having with ath9k STA configured for WPA not being able to authenticate. I'm using nohwcrypt=1, which may be why no one else sees it.
I'll be happy to test patches, it seems very easy to reproduce on my setup.
How about this?
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1270,6 +1270,9 @@ ieee80211_tx_prepare(struct ieee80211_su info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
else if (test_and_clear_sta_flags(tx->sta, WLAN_STA_CLEAR_PS_FILT)) info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
+ else if (sdata->vif.type != NL80211_IFTYPE_AP&&
+ sdata->vif.type != NL80211_IFTYPE_AP_VLAN)
+ info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
hdrlen = ieee80211_hdrlen(hdr->frame_control); if (skb->len> hdrlen + sizeof(rfc1042_header) + 2) {

Quoted 8 lines

Quoted 3 lines
Yes, that seems to fix the problem.
Thanks for the quick fix!
Well, that appeared to fix the problem on -rc1, but when I go back to -rc6 and apply that patch, it still doesn't work.
Might be some other issue though...

Odd. Please try bisecting by applying the patch for each test and reverting it before running git bisect again.

Quoted 1 line

Are you able to get STA to associate& athenticate on latest wireless-testing?

I'll try it by making a new wireless-testing based compat-wireless.

- Felix

Ben Greear

author

2011-05-06 00:16 UTC #5 lore raw

Quoted 27 lines · Felix Fietkau

Quoted 26 lines · Ben Greear
Quoted 25 lines · Ben Greear
Quoted 12 lines · Felix Fietkau
Quoted 6 lines · Ben Greear
I bisected the problem I was having with ath9k STA configured for WPA not being able to authenticate. I'm using nohwcrypt=1, which may be why no one else sees it.
I'll be happy to test patches, it seems very easy to reproduce on my setup.
How about this?
net/mac80211/tx.c ieee80211_tx_prepare(struct ieee80211_su · line 1278
Quoted 9 lines
net/mac80211/tx.c +3 -0
@@ -1270,6 +1270,9 @@ ieee80211_tx_prepare(struct ieee80211_su
 info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
 else if (test_and_clear_sta_flags(tx->sta, WLAN_STA_CLEAR_PS_FILT))
 info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
+ else if (sdata->vif.type != NL80211_IFTYPE_AP&&
+ sdata->vif.type != NL80211_IFTYPE_AP_VLAN)
+ info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
 
 hdrlen = ieee80211_hdrlen(hdr->frame_control);
 if (skb->len> hdrlen + sizeof(rfc1042_header) + 2) {
Yes, that seems to fix the problem.

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
Thanks for the quick fix!
Well, that appeared to fix the problem on -rc1, but when I go back to -rc6 and apply that patch, it still doesn't work.
Might be some other issue though...
Odd. Please try bisecting by applying the patch for each test and reverting it before running git bisect again.

Unless I made a mistake, it's this patch that causes the next problem:

[greearb@build-32 linux.wt]$ git bisect bad 93ae2dd2230393566738a5f211ffbaa33b056d56 is the first bad commit commit 93ae2dd2230393566738a5f211ffbaa33b056d56

Author: Felix Fietkau <[email protected]>
Date:   Sun Apr 17 23:28:10 2011 +0200

ath9k: assign keycache slots to unencrypted stations

Makes me wonder if I'm the only one crazy enough to actually use wireless testing :P

Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Felix Fietkau

2011-05-06 00:44 UTC #6 lore raw

Quoted 42 lines · Ben Greear

Quoted 27 lines · Felix Fietkau
Quoted 26 lines · Ben Greear
Quoted 25 lines · Ben Greear
Quoted 19 lines · Felix Fietkau
Quoted 6 lines · Ben Greear
I bisected the problem I was having with ath9k STA configured for WPA not being able to authenticate. I'm using nohwcrypt=1, which may be why no one else sees it.
I'll be happy to test patches, it seems very easy to reproduce on my setup.
How about this?
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1270,6 +1270,9 @@ ieee80211_tx_prepare(struct ieee80211_su info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
else if (test_and_clear_sta_flags(tx->sta, WLAN_STA_CLEAR_PS_FILT)) info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
+ else if (sdata->vif.type != NL80211_IFTYPE_AP&&
+ sdata->vif.type != NL80211_IFTYPE_AP_VLAN)
+ info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
Quoted 2 lines
hdrlen = ieee80211_hdrlen(hdr->frame_control); if (skb->len> hdrlen + sizeof(rfc1042_header) + 2) {
Yes, that seems to fix the problem.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Thanks for the quick fix!
Well, that appeared to fix the problem on -rc1, but when I go back to -rc6 and apply that patch, it still doesn't work.
Might be some other issue though...
Odd. Please try bisecting by applying the patch for each test and reverting it before running git bisect again.
Unless I made a mistake, it's this patch that causes the next problem:
[greearb@build-32 linux.wt]$ git bisect bad 93ae2dd2230393566738a5f211ffbaa33b056d56 is the first bad commit commit 93ae2dd2230393566738a5f211ffbaa33b056d56

Quoted 7 lines

Author: Felix Fietkau<[email protected]>
Date:   Sun Apr 17 23:28:10 2011 +0200
ath9k: assign keycache slots to unencrypted stations
Makes me wonder if I'm the only one crazy enough to actually use wireless testing :P

I use compat-wireless based on wireless-testing all the time, but in my tests, I did not encounter the issues that you're reporting, no idea why.

This should fix the second issue:

drivers/net/wireless/ath/ath9k/main.c +4 -0

@@ -1722,6 +1722,10 @@ static int ath9k_sta_add(struct ieee8021
 	struct ath_node *an = (struct ath_node *) sta->drv_priv;
 	struct ieee80211_key_conf ps_key = { };
 
+	if (vif->opmode != NL80211_IFTYPE_AP &&
+	    vif->opmode != NL80211_IFTYPE_AP_VLAN)
+		return 0;
+
 	ath_node_attach(sc, sta);
 	an->ps_key = ath_key_config(common, vif, sta, &ps_key);

Ben Greear

author

2011-05-06 16:29 UTC #7 lore raw

Quoted 5 lines · Felix Fietkau

This should fix the second issue:

drivers/net/wireless/ath/ath9k/main.c static int ath9k_sta_add(struct ieee8021 · line 1730

Quoted 9 lines

drivers/net/wireless/ath/ath9k/main.c +4 -0

@@ -1722,6 +1722,10 @@ static int ath9k_sta_add(struct ieee8021
  	struct ath_node *an = (struct ath_node *) sta->drv_priv;
  	struct ieee80211_key_conf ps_key = { };
 
+	if (vif->opmode != NL80211_IFTYPE_AP&&
+	    vif->opmode != NL80211_IFTYPE_AP_VLAN)
+		return 0;
+
  	ath_node_attach(sc, sta);
  	an->ps_key = ath_key_config(common, vif, sta,&ps_key);

This doesn't compile..appears there is no vif->opmode member?

Also, if you are trying to compare the hardware opmode, I'm not sure that would be correct since you can be in AP mode and still have stations. Maybe I'm just confused though...

Thanks,
Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Felix Fietkau

2011-05-06 16:51 UTC #8 lore raw

Quoted 17 lines · Ben Greear

Quoted 8 lines · Felix Fietkau
This should fix the second issue:
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1722,6 +1722,10 @@ static int ath9k_sta_add(struct ieee8021
   	struct ath_node *an = (struct ath_node *) sta->drv_priv;
   	struct ieee80211_key_conf ps_key = { };
Quoted 6 lines
+ if (vif->opmode != NL80211_IFTYPE_AP&&
+ vif->opmode != NL80211_IFTYPE_AP_VLAN)
+ return 0;
+
   	ath_node_attach(sc, sta);
   	an->ps_key = ath_key_config(common, vif, sta,&ps_key);

Quoted 5 lines

This doesn't compile..appears there is no vif->opmode member?
Also, if you are trying to compare the hardware opmode, I'm not sure that would be correct since you can be in AP mode and still have stations. Maybe I'm just confused though...

I sent the wrong version of the patch, forgot to refresh it. Here's the correct one:

drivers/net/wireless/ath/ath9k/main.c +4 -0

@@ -1722,6 +1722,10 @@ static int ath9k_sta_add(struct ieee8021
 	struct ath_node *an = (struct ath_node *) sta->drv_priv;
 	struct ieee80211_key_conf ps_key = { };
 
+	if (vif->type != NL80211_IFTYPE_AP &&
+	    vif->type != NL80211_IFTYPE_AP_VLAN)
+		return 0;
+
 	ath_node_attach(sc, sta);
 	an->ps_key = ath_key_config(common, vif, sta, &ps_key);

Ben Greear

author

2011-05-06 16:54 UTC #9 lore raw

Quoted 29 lines · Felix Fietkau

Quoted 17 lines · Ben Greear
Quoted 14 lines · Felix Fietkau
This should fix the second issue:
  --- a/drivers/net/wireless/ath/ath9k/main.c
  +++ b/drivers/net/wireless/ath/ath9k/main.c
  @@ -1722,6 +1722,10 @@ static int ath9k_sta_add(struct ieee8021
    	struct ath_node *an = (struct ath_node *) sta->drv_priv;
    	struct ieee80211_key_conf ps_key = { };

  +	if (vif->opmode != NL80211_IFTYPE_AP&&
  +	    vif->opmode != NL80211_IFTYPE_AP_VLAN)
  +		return 0;
  +
    	ath_node_attach(sc, sta);
    	an->ps_key = ath_key_config(common, vif, sta,&ps_key);
Quoted 5 lines
This doesn't compile..appears there is no vif->opmode member?
Also, if you are trying to compare the hardware opmode, I'm not sure that would be correct since you can be in AP mode and still have stations. Maybe I'm just confused though...
I sent the wrong version of the patch, forgot to refresh it. Here's the correct one:

drivers/net/wireless/ath/ath9k/main.c static int ath9k_sta_add(struct ieee8021 · line 1731

Quoted 10 lines

drivers/net/wireless/ath/ath9k/main.c +4 -0

@@ -1722,6 +1722,10 @@ static int ath9k_sta_add(struct ieee8021
  	struct ath_node *an = (struct ath_node *) sta->drv_priv;
  	struct ieee80211_key_conf ps_key = { };
 
+	if (vif->type != NL80211_IFTYPE_AP&&
+	    vif->type != NL80211_IFTYPE_AP_VLAN)
+		return 0;
+
  	ath_node_attach(sc, sta);
  	an->ps_key = ath_key_config(common, vif, sta,&ps_key);

I think maybe you want that logic inserted below the ath_node_attach?

I tried it as you have it above, guessing that is what you meant, and it crashed with null pointer exception....

Compiling with the logic below node_attach now...

Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Ben Greear

author

2011-05-06 16:58 UTC #10 lore raw

In reply to Felix Fietkau

Quoted 29 lines · Felix Fietkau

Quoted 17 lines · Ben Greear
Quoted 14 lines · Felix Fietkau
This should fix the second issue:
  --- a/drivers/net/wireless/ath/ath9k/main.c
  +++ b/drivers/net/wireless/ath/ath9k/main.c
  @@ -1722,6 +1722,10 @@ static int ath9k_sta_add(struct ieee8021
    	struct ath_node *an = (struct ath_node *) sta->drv_priv;
    	struct ieee80211_key_conf ps_key = { };

  +	if (vif->opmode != NL80211_IFTYPE_AP&&
  +	    vif->opmode != NL80211_IFTYPE_AP_VLAN)
  +		return 0;
  +
    	ath_node_attach(sc, sta);
    	an->ps_key = ath_key_config(common, vif, sta,&ps_key);
Quoted 5 lines
This doesn't compile..appears there is no vif->opmode member?
Also, if you are trying to compare the hardware opmode, I'm not sure that would be correct since you can be in AP mode and still have stations. Maybe I'm just confused though...
I sent the wrong version of the patch, forgot to refresh it. Here's the correct one:

drivers/net/wireless/ath/ath9k/main.c static int ath9k_sta_add(struct ieee8021 · line 1731

Quoted 10 lines

drivers/net/wireless/ath/ath9k/main.c +4 -0

@@ -1722,6 +1722,10 @@ static int ath9k_sta_add(struct ieee8021
  	struct ath_node *an = (struct ath_node *) sta->drv_priv;
  	struct ieee80211_key_conf ps_key = { };
 
+	if (vif->type != NL80211_IFTYPE_AP&&
+	    vif->type != NL80211_IFTYPE_AP_VLAN)
+		return 0;
+
  	ath_node_attach(sc, sta);
  	an->ps_key = ath_key_config(common, vif, sta,&ps_key);

This patch appears to fix things. I'll let you send it proper for submittal since you have a better idea of what it's doing:

drivers/net/wireless/ath/ath9k/main.c +5 -0

index 3de115d..3d999b7 100644
@@ -1778,6 +1778,11 @@ static int ath9k_sta_add(struct ieee80211_hw *hw,
        struct ieee80211_key_conf ps_key = { };
 
        ath_node_attach(sc, sta);
+
+       if (vif->type != NL80211_IFTYPE_AP &&
+           vif->type != NL80211_IFTYPE_AP_VLAN)
+               return 0;
+
        an->ps_key = ath_key_config(common, vif, sta, &ps_key);
 
        return 0;

Thanks,
Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Felix Fietkau

2011-05-06 17:04 UTC #11 lore raw

Quoted 42 lines · Ben Greear

Quoted 32 lines · Felix Fietkau
Quoted 17 lines · Ben Greear
Quoted 6 lines · Felix Fietkau
   This should fix the second issue:

   --- a/drivers/net/wireless/ath/ath9k/main.c
   +++ b/drivers/net/wireless/ath/ath9k/main.c
   @@ -1722,6 +1722,10 @@ static int ath9k_sta_add(struct ieee8021
     	struct ath_node *an = (struct ath_node *) sta->drv_priv;
Quoted 8 lines
     	struct ieee80211_key_conf ps_key = { };

   +	if (vif->opmode != NL80211_IFTYPE_AP&&
   +	    vif->opmode != NL80211_IFTYPE_AP_VLAN)
   +		return 0;
   +
     	ath_node_attach(sc, sta);
     	an->ps_key = ath_key_config(common, vif, sta,&ps_key);
Quoted 5 lines
This doesn't compile..appears there is no vif->opmode member?
Also, if you are trying to compare the hardware opmode, I'm not sure that would be correct since you can be in AP mode and still have stations. Maybe I'm just confused though...
I sent the wrong version of the patch, forgot to refresh it. Here's the correct one:
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1722,6 +1722,10 @@ static int ath9k_sta_add(struct ieee8021
   	struct ath_node *an = (struct ath_node *) sta->drv_priv;
   	struct ieee80211_key_conf ps_key = { };
Quoted 7 lines
+ if (vif->type != NL80211_IFTYPE_AP&&
+ vif->type != NL80211_IFTYPE_AP_VLAN)
+ return 0;
+
   	ath_node_attach(sc, sta);
   	an->ps_key = ath_key_config(common, vif, sta,&ps_key);

Quoted 2 lines

This patch appears to fix things. I'll let you send it proper for submittal since you have a better idea of what it's doing:

OK, thanks. I'll combine this with another ath9k patch to replace the rejected mac80211 PS filter patch and submit that for 2.6.39.

- Felix