Bug 11749

Summary: Ath5k driver has too many interrupts per second at idle
Product: Drivers Reporter: Yi Yang (yang.y.yi)
Component: network-wirelessAssignee: Luis Chamberlain (mcgrof)
Status: CLOSED CODE_FIX    
Severity: high CC: jirislaby, martin.xu, mcgrof, me
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 2.6.26 and later Subsystem:
Regression: --- Bisected commit-id:
Attachments: Disable MIB interrupts on ath5k for 2.6.27
Fix handling of FIF_BCN_PRBRESP_PROMISC
stop hw when bringing an interface down
Add bss_info_change() to ath5k, disable radio when not associated
disable beacon filter when disassocated

Description Yi Yang 2008-10-12 20:26:21 UTC
Latest working kernel version: None
Earliest failing kernel version: Unknow
Distribution: all
Hardware Environment: Atheros wireless chipset
Software Environment: Fedora
Problem Description: ath5k driver has many inperrupts per second although the system is idle.

I used AspierOne to test it, interrupts are so many that CPU's C3 residence time is only 20 milliseconds or less, there isn't any valid traffic on my wireless network except beacon frame.

I tested X61 in the same wireless environment, number of interrupt is only 1 per second, X61 has the latest Intel wireless chipset and 2.6.27 kernel was running.

So i think ath5k and ath9k should can do as Intel wireless driver does, otherwise, CPU can't stay at C3 with longer time because of wireless interrupts.

Steps to reproduce:
1. Enable wireless on platform with Atheros wireless chipset.
2. watch cat /proc/interrupts
Comment 1 Luis Chamberlain 2008-10-13 12:07:24 UTC
Created attachment 18286 [details]
Disable MIB interrupts on ath5k for 2.6.27

ANI is not yet used in ath5k
Comment 2 Luis Chamberlain 2008-10-13 12:07:44 UTC
Please try this patch.
Comment 3 Martin Xu 2008-10-13 20:16:59 UTC
I have tried the patch, it can not fix the issue.

I think the issue may come from the filter, at default filter receives all the multiple packages even station does not associate with any AP. 

We also track the bug a moblin bugzilla. see

http://bugzilla.moblin.org/show_bug.cgi?id=80
Comment 4 Luis Chamberlain 2008-10-13 20:35:33 UTC
What?? You receive frames intended to go to another AP????
Comment 5 Yi Yang 2008-10-13 20:46:23 UTC
(In reply to comment #2)
> Please try this patch.
> 
This patch can't fix this issue, the result is still so.
Comment 6 Bob Copeland 2008-10-16 12:48:20 UTC
Re #3, AFAICT the moblin.org bugzilla is talking about beacons, not data frames.  The filter is currently set to all-1s in ath5k_hw_set_associd so I wouldn't expect seeing data frames from other APs in sta mode.
Comment 7 Yi Yang 2008-10-16 19:19:58 UTC
Sorry, the above statements about Intel wireless driver is incorrect, number of interrupt per second should be 60 or more when it is in state "Not Associated", but that will be 1 to 5 after association is successful and AP is connected.

But for ath5k, that value doesn't change before and after association is done, it is about 40.

So now the problem is why there are so many interrupts before association is done, why does it not change after association is done.

Why does wireless driver respond all the beacon frames? Why "iwconfig wlan0 sens 90" can't work? Do we have an effective way to ignore beacon frames from those APs to which we don't want to connect?
Comment 8 Luis Chamberlain 2008-10-16 19:41:12 UTC
Intel's wireless cards have a dedicated CPU as part of their hardware and they use firmware which runs on it. We don't have a CPU and we don't have firmware which is why it was *very* difficult for us to open up our drivers.

So you should see more load on the CPU on our hardware, its just the nature of our hardware. You however will see no proprietary firmware :)
Comment 9 Bob Copeland 2008-10-24 04:55:59 UTC
Created attachment 18424 [details]
Fix handling of FIF_BCN_PRBRESP_PROMISC

At any rate I'm not convinced ath5k's handling of FIF_BCN_PRBRESP_PROMISC is correct, since it doesn't set the associd back to broadcast, and it enables beacons in STA mode regardless of whether the flag is set.  Could you try the attached patch which makes it behave similarly to ath9k?
Comment 10 Yi Yang 2008-10-30 01:56:24 UTC
Sorry, i missed this.

This patch is very good, it can reduce many interrupts, it is valid both before and after association is done, so strongly suggest to submit it to upstream kernel driver.

With this patch, number of interrupts of ath5k per second is 2~4  before association is done, after that number of interrupts of ath5k per second is 0.
Comment 11 Bob Copeland 2008-10-31 04:56:26 UTC
Ok, patch without the broadcast associd (since it didn't seem to matter) was posted on linux-wireless and committed as ba7273afa2db548ba556d6b2325c36d9c91fad95 in the wireless-testing tree.

Here's the thread:
http://marc.info/?l=linux-wireless&m=122528356211367&w=2
Comment 12 Bob Copeland 2008-11-07 07:24:35 UTC
As Luis discovered, the patch alone won't work - it needs help from the beacon miss interrupt or else all beacons are simply discarded and consequently we will lose associations without probe responses.
Comment 13 Luis Chamberlain 2008-11-07 14:57:31 UTC
And additionally in order to fix this we need to rely on the beacon miss interrupt but in order to use that we need to inform mac80211 about it which requires adding a small API for it so mac80211 doesn't *always* expect beacons.
Comment 14 martin xu 2008-11-08 20:03:30 UTC
Can we just let station ignore/filter all the beacons when it is not associated with any AP. That can resolve the the main issue of the bug.  
Comment 15 Luis Chamberlain 2008-11-09 00:14:07 UTC
Created attachment 18742 [details]
stop hw when bringing an interface down

Good point. Please try this patch.

I think this should work as we call remove_interface() callback of the driver from mac80211 when we call ieee80211_stop() the net_device's stop(). This occurs for IBSS, STA and Mesh type interfaces.

ath5k_stop_locked() is also correct as its under the sc->lock.
Comment 16 Luis Chamberlain 2008-11-09 00:17:57 UTC
Well sorry this will just stop the filter/hw/etc when we bring the interface down, which is still correct IMHO. What you want is to disable the beacon filter when you are not connected to an AP??

Well how will you connect to an AP then? Mind you to resolve the issue *properly* we just need to add API to mac80211 to make it posssible for it not to require beacons so we can use the beacon miss interrupt instead of always allowing all the beacons through.

I suppose we can disable the beacon filter if we're not associated *and* not scanning.. Have to think of where to put that. Anyone with ideas?
Comment 17 Bob Copeland 2008-11-09 06:42:37 UTC
I think mac80211 already sets PRBRESP_PROMISC whenever it starts an active scan.
Comment 18 Bob Copeland 2008-11-09 06:46:39 UTC
Regarding the other patch - shouldn't balancing ->start() and ->stop() be enough?  I would worry about calling stop if we haven't called start already (e.g. we never brought the interface up, and then stop() wants to dispose of things only created in start())
Comment 19 martin xu 2008-11-10 06:35:57 UTC
the key issue is to let ath5k driver know the change of the associate state so the beacon filter can be disable/enable.
I tried to use bss_info_changed(), but it can only work when set/disable essid through iwconfig command; and can not handle other scenario. 

Someone know how to info the associate state changing to ath5k driver?
Comment 20 Luis Chamberlain 2008-11-10 10:49:44 UTC
Bob so the stop thing I put in the suggested patch doesn't kfree() anything but just stops the radio and disables the RX filter and turns off the leds so I think it should be safe on first glance.

So the reason we need the beacon filter *on* is mac80211 expects beacons. In this case Yi is asking for us to disable it when we are not associated, I just did it when we bring the interface down though..

Not sure where to poke yet to enable this where we are not associated yet, it would probably come in from mac80211 -- take a look at ieee80211_set_associated() on mlme.c in mac80211.

But I think it'd be nicer to add functionality to mac80211 to allow us to use the beacon miss interrupt as Felix was suggesting, which is what ath9k was using. To do that I think we will need maybe only enable the timer if the beacon miss interrupt is triggered but we would need a way to communicate that to mac80211.
Comment 21 Luis Chamberlain 2008-11-10 11:57:47 UTC
Created attachment 18786 [details]
Add bss_info_change() to ath5k, disable radio when not associated


This adds the bss_info_change() callback to mac80211 and disables the radio, rx filter and leds when not associated. Let me know what you think.

With the other attachment 18742 [details] this also disables the RX filter when the interface is down.
Comment 22 martin xu 2008-11-10 20:27:22 UTC
(In reply to comment #21)
> Created an attachment (id=18786) [details]
> Add bss_info_change() to ath5k, disable radio when not associated
> 
> 
> This adds the bss_info_change() callback to mac80211 and disables the radio,
> rx
> filter and leds when not associated. Let me know what you think.
> 
> With the other attachment 18742 [details] this also disables the RX filter
> when the
> interface is down.
> 

I think the patch has some issues.
1. when disassociate it will power down the device totally, how can the station be powered on when it want to associate again?
2. basing on my observation, bss_info_change() can not notify the associate state changing all the times. (Although I originally thought it should)
see below scenario:
associate to one AP named Guest (iwconfig wlan0 essid Guest)
reassociated to another AP name tt which is not existing. (iwconfig wlan0 tt )
now iwconfig wlan0 you will find that station is not associated. HOWEVER, the changing of the associate state is not reported to ath5k driver. 

In fact I have also worked out a patch basing on bss_info_change(), in the patch I just disable/enable beacon filter. It can work fine except that bss_info_change issue mentioned above. 
Comment 23 martin xu 2008-11-10 20:32:05 UTC
(In reply to comment #20)
> Bob so the stop thing I put in the suggested patch doesn't kfree() anything
> but
> just stops the radio and disables the RX filter and turns off the leds so I
> think it should be safe on first glance.
> 
> So the reason we need the beacon filter *oissn* is mac80211 expects beacons.
> In
> this case Yi is asking for us to disable it when we are not associated, I
> just
> did it when we bring the interface down though..n
> e
To the current ath5k driver,when we bring the interface down, the issue does not exists any more. No interrupts will take place. 
BTW: I am Martin not Yi  :-)
Comment 24 martin xu 2008-11-10 21:40:56 UTC
Created attachment 18794 [details]
disable beacon filter when disassocated 

Please review the patch
Comment 25 Luis Chamberlain 2008-11-11 14:51:45 UTC
I like it, except you just forgot the mutex_unlock(&sc->lock) on bss change callback. With the mutex_unlock() does it work for you?
Comment 26 martin xu 2008-11-11 22:54:57 UTC
I do not think mutex_unlock is useful here. I have verified the patch again, and find that it can work fine.
Comment 27 Yi Yang 2008-11-12 01:03:55 UTC
(In reply to comment #26)
> I do not think mutex_unlock is useful here. I have verified the patch again,
> and find that it can work fine.
> 
This mutex is necessary, sc must be accessed with sc->lock holden.
Comment 28 Bob Copeland 2008-11-13 07:06:17 UTC
Definitely agree it needs balanced locking, also it would be nice if it compiled :)  But the idea is good...
Comment 29 martin xu 2008-11-22 20:52:06 UTC
Yang Yi, can you help us close the bug? Thanks!
Comment 30 Luis Chamberlain 2008-12-01 15:07:08 UTC
commit cb99cf4fc881989464f2ba597cdcd276f80bc08e
Author: Martin Xu <martin.xu@intel.com>
Date:   Mon Nov 24 10:49:27 2008 +0800

    ath5k: disable beacon filter when station is not associated
    
    Ath5k driver has too many interrupts per second at idle
    http://bugzilla.kernel.org/show_bug.cgi?id=11749
    
    Signed-off-by: Martin Xu <martin.xu@intel.com>
    Signed-off-by: John W. Linville <linville@tuxdriver.com>