wireless-testing commit eb9d4e8399181357cb6f6625ba7f849987432c6c causes stalls

This commit 'ath5k: Update interrupt masking code' causes stalls under load when built using compat-wireless on 2.6.27. There is no output from dmesg, but it occasionally recovers and moves a few more packets before stalling again. Luis suggested reverting eb9d4e8399181357cb6f6625ba7f849987432c6c which I did. Now it appears to work normally.

I also think this patch should be broken into multiple pieces. There appear to be at least 3 functional changes addressed in this single patch.

Quoted 9 lines

This commit 'ath5k: Update interrupt masking code' causes stalls under load when built using compat-wireless on 2.6.27. There is no output from dmesg, but it occasionally recovers and moves a few more packets before stalling again. Luis suggested reverting eb9d4e8399181357cb6f6625ba7f849987432c6c which I did. Now it appears to work normally.

I also think this patch should be broken into multiple pieces. There appear to be at least 3 functional changes addressed in this single patch.

Agreed, I just tested with the latest wireless-testing and the issue is still present. I reviewed the commit but its a big too large to pin point the exact issue, my AR5414 goes up but then quickly becomes unusable. Nick how about we revert this and you can then split this up into a good number of patches to make the review easier and also to be able to pin point the exact issue better?

The SHA1 remains the same but just in case the title of the patch is:

ath5k: Update interrupt masking code

With it reverted I'm cruising and can ditch MadWifi completely, which is the idea.

Luis

2008/11/1 Luis R. Rodriguez <[email protected]>:

Quoted 27 lines · Tim Gardner

Quoted 9 lines

This commit 'ath5k: Update interrupt masking code' causes stalls under load when built using compat-wireless on 2.6.27. There is no output from dmesg, but it occasionally recovers and moves a few more packets before stalling again. Luis suggested reverting eb9d4e8399181357cb6f6625ba7f849987432c6c which I did. Now it appears to work normally.

I also think this patch should be broken into multiple pieces. There appear to be at least 3 functional changes addressed in this single patch.

Agreed, I just tested with the latest wireless-testing and the issue is still present. I reviewed the commit but its a big too large to pin point the exact issue, my AR5414 goes up but then quickly becomes unusable. Nick how about we revert this and you can then split this up into a good number of patches to make the review easier and also to be able to pin point the exact issue better?

The SHA1 remains the same but just in case the title of the patch is:

ath5k: Update interrupt masking code

With it reverted I'm cruising and can ditch MadWifi completely, which is the idea.

Luis

Can you test the attached patch ?

It works for me (tested on a AR5413)...

makis linux # iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 85.3 KByte (default)
------------------------------------------------------------
[ 4] local 192.168.1.104 port 5001 connected with 192.168.1.10 port 43192

[ ID] Interval       Transfer     Bandwidth
[  4]  0.0-10.6 sec  21.8 MBytes  17.2 Mbits/sec

Quoted 4 lines · Nick Kossifidis

Can you test the attached patch ?

It works for me (tested on a AR5413)...

Quoted 8 lines

makis linux # iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 85.3 KByte (default)
------------------------------------------------------------
[ 4] local 192.168.1.104 port 5001 connected with 192.168.1.10 port 43192

[ ID] Interval       Transfer     Bandwidth
[  4]  0.0-10.6 sec  21.8 MBytes  17.2 Mbits/sec

This patch works for me (I was seeing the same stalls).

I somehow managed to trigger this after reloading the module:

Nov  2 13:12:05 sludge kernel: ------------[ cut here ]------------
Nov  2 13:12:05 sludge kernel: WARNING: at net/mac80211/main.c:236
ieee80211_hw_config+0x82/0x8c [mac80211]()
Nov  2 13:12:05 sludge kernel: Modules linked in: ath5k af_packet

sha256_generic aes_i586 aes_generic cbc loop i915 drm binfmt_misc acpi_cpufreq fan container nls_utf8 hfsplus dm_crypt dm_mod kvm_intel kvm fuse sbp2 snd_hda_intel snd_pcm_oss snd_pcm snd_mixer_oss appletouch hid_apple snd_seq_dummy snd_seq_oss arc4 ecb snd_seq_midi usbhid snd_rawmidi snd_seq_midi_event mac80211 snd_seq ohci1394 snd_timer snd_seq_device sr_mod ieee1394 sky2 cfg80211 sg cdrom rtc ehci_hcd uhci_hcd thermal bitrev crc32 snd snd_page_alloc battery ac processor button evdev unix [last unloaded: ath5k]

Nov  2 13:12:05 sludge kernel: Pid: 9748, comm: ath5k_pci Tainted: G

W 2.6.28-rc2-wl #19
[...]

But I think that was just a coincidence with the hardware getting stuck in a bad state -- after reboot it was fine. If not, this at least papers over the warning :)

drivers/net/wireless/ath5k/base.c +0 -0

index a298e8a..0308165 100644

Quoted 46 lines · Bob Copeland

Quoted 4 lines · Nick Kossifidis

Can you test the attached patch ?

It works for me (tested on a AR5413)...

Quoted 8 lines

makis linux # iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 85.3 KByte (default)
------------------------------------------------------------
[ 4] local 192.168.1.104 port 5001 connected with 192.168.1.10 port 43192

[ ID] Interval       Transfer     Bandwidth
[  4]  0.0-10.6 sec  21.8 MBytes  17.2 Mbits/sec

This patch works for me (I was seeing the same stalls).

I somehow managed to trigger this after reloading the module:

Nov  2 13:12:05 sludge kernel: ------------[ cut here ]------------
Nov  2 13:12:05 sludge kernel: WARNING: at net/mac80211/main.c:236
ieee80211_hw_config+0x82/0x8c [mac80211]()
Nov  2 13:12:05 sludge kernel: Modules linked in: ath5k af_packet

sha256_generic aes_i586 aes_generic cbc loop i915 drm binfmt_misc acpi_cpufreq fan container nls_utf8 hfsplus dm_crypt dm_mod kvm_intel kvm fuse sbp2 snd_hda_intel snd_pcm_oss snd_pcm snd_mixer_oss appletouch hid_apple snd_seq_dummy snd_seq_oss arc4 ecb snd_seq_midi usbhid snd_rawmidi snd_seq_midi_event mac80211 snd_seq ohci1394 snd_timer snd_seq_device sr_mod ieee1394 sky2 cfg80211 sg cdrom rtc ehci_hcd uhci_hcd thermal bitrev crc32 snd snd_page_alloc battery ac processor button evdev unix [last unloaded: ath5k]

Nov  2 13:12:05 sludge kernel: Pid: 9748, comm: ath5k_pci Tainted: G

W 2.6.28-rc2-wl #19
[...]

But I think that was just a coincidence with the hardware getting stuck in a bad state -- after reboot it was fine. If not, this at least papers over the warning :)

drivers/net/wireless/ath5k/base.c +0 -0

index a298e8a..0308165 100644
@@ -2796,7 +2796,8 @@ ath5k_config(struct ieee80211_hw *hw, u32 changed)
       sc->bintval = conf->beacon_int;
       sc->power_level = conf->power_level;
 

drivers/net/wireless/ath5k/base.c ath5k_config(struct ieee80211_hw *hw, u32 changed) · line 2801

Quoted 4 lines

drivers/net/wireless/ath5k/base.c +2 -1

-       return ath5k_chan_set(sc, conf->channel);
+       ath5k_chan_set(sc, conf->channel);
+       return 0;
 }

Not sure I agree with the WARN_ON() if the driver's mac80211 config() callback fails. In our case when we tune to a different channel we have to clear any DMA operations first and then we reset the chip. Reseting the chip can fail for whatever strange hw issue cases. The patch fixes the complaint but is the complaint sane?

Quoted 47 lines · Luis R. Rodriguez

Quoted 46 lines · Bob Copeland

Quoted 4 lines · Nick Kossifidis

Can you test the attached patch ?

It works for me (tested on a AR5413)...

Quoted 8 lines

makis linux # iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 85.3 KByte (default)
------------------------------------------------------------
[ 4] local 192.168.1.104 port 5001 connected with 192.168.1.10 port 43192

[ ID] Interval       Transfer     Bandwidth
[  4]  0.0-10.6 sec  21.8 MBytes  17.2 Mbits/sec

This patch works for me (I was seeing the same stalls).

I somehow managed to trigger this after reloading the module:

Nov  2 13:12:05 sludge kernel: ------------[ cut here ]------------
Nov  2 13:12:05 sludge kernel: WARNING: at net/mac80211/main.c:236
ieee80211_hw_config+0x82/0x8c [mac80211]()
Nov  2 13:12:05 sludge kernel: Modules linked in: ath5k af_packet

sha256_generic aes_i586 aes_generic cbc loop i915 drm binfmt_misc acpi_cpufreq fan container nls_utf8 hfsplus dm_crypt dm_mod kvm_intel kvm fuse sbp2 snd_hda_intel snd_pcm_oss snd_pcm snd_mixer_oss appletouch hid_apple snd_seq_dummy snd_seq_oss arc4 ecb snd_seq_midi usbhid snd_rawmidi snd_seq_midi_event mac80211 snd_seq ohci1394 snd_timer snd_seq_device sr_mod ieee1394 sky2 cfg80211 sg cdrom rtc ehci_hcd uhci_hcd thermal bitrev crc32 snd snd_page_alloc battery ac processor button evdev unix [last unloaded: ath5k]

Nov  2 13:12:05 sludge kernel: Pid: 9748, comm: ath5k_pci Tainted: G

W 2.6.28-rc2-wl #19
[...]

But I think that was just a coincidence with the hardware getting stuck in a bad state -- after reboot it was fine. If not, this at least papers over the warning :)

drivers/net/wireless/ath5k/base.c +0 -0

index a298e8a..0308165 100644
@@ -2796,7 +2796,8 @@ ath5k_config(struct ieee80211_hw *hw, u32 changed)
       sc->bintval = conf->beacon_int;
       sc->power_level = conf->power_level;
 

Quoted 10 lines

drivers/net/wireless/ath5k/base.c ath5k_config(struct ieee80211_hw *hw, u32 changed) · line 2801

Quoted 4 lines

drivers/net/wireless/ath5k/base.c +2 -1

-       return ath5k_chan_set(sc, conf->channel);
+       ath5k_chan_set(sc, conf->channel);
+       return 0;
 }

Not sure I agree with the WARN_ON() if the driver's mac80211 config() callback fails. In our case when we tune to a different channel we have to clear any DMA operations first and then we reset the chip. Reseting the chip can fail for whatever strange hw issue cases. The patch fixes the complaint but is the complaint sane?

What's mac80211 to do when it fails?

Quoted 7 lines

Quoted 5 lines

Not sure I agree with the WARN_ON() if the driver's mac80211 config() callback fails. In our case when we tune to a different channel we have to clear any DMA operations first and then we reset the chip. Reseting the chip can fail for whatever strange hw issue cases. The patch fixes the complaint but is the complaint sane?

What's mac80211 to do when it fails?

How about a shiny new nl80211 event?

Quoted 6 lines · Luis R. Rodriguez

Quoted 6 lines

Quoted 5 lines

Not sure I agree with the WARN_ON() if the driver's mac80211 config() callback fails. In our case when we tune to a different channel we have to clear any DMA operations first and then we reset the chip. Reseting the chip can fail for whatever strange hw issue cases. The patch fixes the complaint but is the complaint sane?

Quoted 3 lines

Quoted 1 line

What's mac80211 to do when it fails?

How about a shiny new nl80211 event?

And wtf would userspace do?

Quoted 8 lines · Johannes Berg

Quoted 6 lines · Luis R. Rodriguez

Quoted 6 lines

Quoted 5 lines

Not sure I agree with the WARN_ON() if the driver's mac80211 config() callback fails. In our case when we tune to a different channel we have to clear any DMA operations first and then we reset the chip. Reseting the chip can fail for whatever strange hw issue cases. The patch fixes the complaint but is the complaint sane?

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

What's mac80211 to do when it fails?

How about a shiny new nl80211 event?

And wtf would userspace do?

If multiple configuration attempts fail perhaps stop trying, and inform the user?

Quoted 12 lines · Luis R. Rodriguez

Quoted 10 lines · Johannes Berg

Quoted 8 lines · Luis R. Rodriguez

Quoted 7 lines

Quoted 5 lines

Not sure I agree with the WARN_ON() if the driver's mac80211 config() callback fails. In our case when we tune to a different channel we have to clear any DMA operations first and then we reset the chip. Reseting the chip can fail for whatever strange hw issue cases. The patch fixes the complaint but is the complaint sane?

What's mac80211 to do when it fails?

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

How about a shiny new nl80211 event?

And wtf would userspace do?

If multiple configuration attempts fail perhaps stop trying, and inform the user?

But you're saying it's "normal" to get this failure,

so wouldn't it
always do that sooner or later and always say your hw is broken?

Also,
that's a bad thing to do in userspace, imho.

Why can it fail here
anyway?

hw borked is one obvious case, but it shouldn't happen enough for this to be a problem yet.

Quoted 1 line

But you're saying it's "normal" to get this failure,

No, I'm just sayings its possible, right now mac80211 assumes its not and if it does its because we somehow lied to mac80211 of our capabilities.

Quoted 2 lines

so wouldn't it
always do that sooner or later and always say your hw is broken?

Nope

Quoted 2 lines

Also,
that's a bad thing to do in userspace, imho.

What should we do with these rare failures then?

Quoted 2 lines

hw borked is one obvious case, but it shouldn't happen enough for this to be a problem yet.

This I agree with. It is rare, its just possible, right now mac80211 assumes it never will.

Quoted 5 lines · Luis R. Rodriguez

Quoted 1 line

But you're saying it's "normal" to get this failure,

No, I'm just sayings its possible, right now mac80211 assumes its not and if it does its because we somehow lied to mac80211 of our capabilities.

Well, yes, sort of.

Quoted 4 lines

Quoted 2 lines

so wouldn't it
always do that sooner or later and always say your hw is broken?

Nope

Why not?

Quoted 4 lines

Quoted 2 lines

Also,
that's a bad thing to do in userspace, imho.

What should we do with these rare failures then?

print an error message? ignore them? try again?

Quoted 5 lines

Quoted 2 lines

hw borked is one obvious case, but it shouldn't happen enough for this to be a problem yet.

This I agree with. It is rare, its just possible, right now mac80211 assumes it never will.

It's _always_ assumed that by ignoring the return value, now it's just noisy about it because clearly it doesn't like when the driver fails to do what it wants since then the hw and sw states get out of sync. I really don't see what to do other than retry maybe, but that might well be done in the driver instead.

Quoted 3 lines

Quoted 1 line

What should we do with these rare failures then?

print an error message? ignore them? try again?

I'd prefer a simple error print than a WARN_ON().

Quoted 11 lines

Quoted 5 lines

Quoted 2 lines

hw borked is one obvious case, but it shouldn't happen enough for this to be a problem yet.

This I agree with. It is rare, its just possible, right now mac80211 assumes it never will.

It's _always_ assumed that by ignoring the return value, now it's just noisy about it because clearly it doesn't like when the driver fails to do what it wants since then the hw and sw states get out of sync. I really don't see what to do other than retry maybe, but that might well be done in the driver instead.

As can be seen from the patch suggested what some drivers will end up doing is just ignoring failures but I guess that can be up to the drivers to deal with as you are suggesting. I'd be inclined to try to disable the device in case of a few failed resets to be specific with ath5k. Would mac80211 want to be informed of that through the return value? Is the WARN still appropriate?

Quoted 5 lines · Luis R. Rodriguez

Quoted 3 lines

Quoted 1 line

What should we do with these rare failures then?

print an error message? ignore them? try again?

I'd prefer a simple error print than a WARN_ON().

Just so you don't get blamed on kerneloops.org? :)

Quoted 12 lines

Quoted 11 lines

Quoted 5 lines

Quoted 2 lines

hw borked is one obvious case, but it shouldn't happen enough for this to be a problem yet.

This I agree with. It is rare, its just possible, right now mac80211 assumes it never will.

It's _always_ assumed that by ignoring the return value, now it's just noisy about it because clearly it doesn't like when the driver fails to do what it wants since then the hw and sw states get out of sync. I really don't see what to do other than retry maybe, but that might well be done in the driver instead.

Quoted 6 lines

As can be seen from the patch suggested what some drivers will end up doing is just ignoring failures but I guess that can be up to the drivers to deal with as you are suggesting. I'd be inclined to try to disable the device in case of a few failed resets to be specific with ath5k. Would mac80211 want to be informed of that through the return value? Is the WARN still appropriate?

I think the warning is appropriate, yes, since mac80211 has no concept of failing hardware configuration. You're free to add such a concept, but I really don't know what mac80211 would be supposed to do when things fail, just try to stop/start? panic()? Instruct the user to reboot? to swap hardware? See?

Quoted 4 lines · Johannes Berg

Quoted 2 lines · Luis R. Rodriguez

Quoted 2 lines

Quoted 1 line

What should we do with these rare failures then?

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

print an error message? ignore them? try again?

I'd prefer a simple error print than a WARN_ON().

Just so you don't get blamed on kerneloops.org? :)

No because like I said a hw reset can fail and don't think software should be blamed. Right now we do that.

As can be seen from the patch suggested what some drivers will end up doing is just ignoring failures but I guess that can be up to the drivers to deal with as you are suggesting.

Quoted 4 lines

Quoted 2 lines

Also,
that's a bad thing to do in userspace, imho.

What should we do with these rare failures then?

print an error message? ignore them? try again?

Quoted 3 lines · Johannes Berg

Quoted 3 lines

Quoted 2 lines

Also,
that's a bad thing to do in userspace, imho.

Quoted 3 lines

Quoted 1 line

What should we do with these rare failures then?

print an error message? ignore them? try again?

I would say that try few (three?) times and if it still fails, restart firmware and push the same settings again to the firmware. There's very little that mac80211 could do here, driver has to make the decisions in case like this.

2008/11/2 Nick Kossifidis <[email protected]>:

Quoted 26 lines

2008/11/1 Luis R. Rodriguez <[email protected]>:

Quoted 25 lines · Tim Gardner

Quoted 9 lines

This commit 'ath5k: Update interrupt masking code' causes stalls under load when built using compat-wireless on 2.6.27. There is no output from dmesg, but it occasionally recovers and moves a few more packets before stalling again. Luis suggested reverting eb9d4e8399181357cb6f6625ba7f849987432c6c which I did. Now it appears to work normally.

I also think this patch should be broken into multiple pieces. There appear to be at least 3 functional changes addressed in this single patch.

Agreed, I just tested with the latest wireless-testing and the issue is still present. I reviewed the commit but its a big too large to pin point the exact issue, my AR5414 goes up but then quickly becomes unusable. Nick how about we revert this and you can then split this up into a good number of patches to make the review easier and also to be able to pin point the exact issue better?

The SHA1 remains the same but just in case the title of the patch is:

ath5k: Update interrupt masking code

With it reverted I'm cruising and can ditch MadWifi completely, which is the idea.

Quoted 3 lines

Quoted 2 lines

Luis

Can you test the attached patch ?

It works for me (tested on a AR5413)...