[PATCH 0/2] ath5k: reset path fixes

[PATCH 0/2] ath5k: reset path fixes

[Sergey Ryazanov]

Couple of patches for the reset code path to address the
"invalid hw_rix" warning and to make reset more robust.

Patch 1 helps to avoid warning by fixing the order of rx disable and
channel update actions.

Patch 2 fixes the root cause, which is interrupt disable/enable race
between tasklet and reset function.

The second patch contains a more generic fix, which is enough to
prevent the warning. But I think that the first patch should be applied
too, just in case. If no one has any objections.

Sergey Ryazanov (2):
  ath5k: channel change fix
  ath5k: fix reset race

 drivers/net/wireless/ath/ath5k/ath5k.h |  1 +
 drivers/net/wireless/ath/ath5k/base.c  | 31 ++++++++++++++++++++++++++++---
 drivers/net/wireless/ath/ath5k/reset.c | 24 ------------------------
 3 files changed, 29 insertions(+), 27 deletions(-)

-- 
2.0.4

[PATCH 1/2] ath5k: channel change fix

[Sergey Ryazanov]

ath5k updates the channel pointer and after that it stops the Rx logic
and apply channel to HW. In case of channel switch, such sequence
creates a small window when a frame, which is received on the old
channel is considered as a frame received on the new one.

The most notable consequence of this situation occurs during the switch
from 2 GHz band (CCK+OFDM) to the 5GHz band (OFDM-only). Frame received
with CCK rate, e.g. beacon received at the 1mbps, causes the following
warning:

  WARNING: at ath5k/base.c:589 ath5k_tasklet_rx+0x318/0x6ec [ath5k]()
  invalid hw_rix: 1a
  [..]
  Call Trace:
  [<802656a8>] show_stack+0x48/0x70
  [<802dd92c>] warn_slowpath_common+0x88/0xbc
  [<802dd98c>] warn_slowpath_fmt+0x2c/0x38
  [<81b51be8>] ath5k_tasklet_rx+0x318/0x6ec [ath5k]
  [<8028ac64>] tasklet_action+0x8c/0xf0
  [<80075804>] __do_softirq+0x180/0x32c
  [<80196ce8>] irq_exit+0x54/0x70
  [<80041848>] ret_from_irq+0x0/0x4
  [<80182fdc>] ioread32+0x4/0xc
  [<81b4c42c>] ath5k_hw_set_sleep_clock+0x2ec/0x474 [ath5k]
  [<81b4cf28>] ath5k_hw_reset+0x50/0xeb8 [ath5k]
  [<81b50900>] ath5k_reset+0xd4/0x310 [ath5k]
  [<81b557e8>] ath5k_config+0x4c/0x104 [ath5k]
  [<80d01770>] ieee80211_hw_config+0x2f4/0x35c [mac80211]
  [<80d09aa8>] ieee80211_scan_work+0x2e4/0x414 [mac80211]
  [<8022c3f4>] process_one_work+0x28c/0x400
  [<802df8f8>] worker_thread+0x258/0x3c0
  [<801b5710>] kthread+0xe0/0xec
  [<800418a8>] ret_from_kernel_thread+0x14/0x1c

The easiest way to reproduce this warning is to run scan with dualband
NIC in noisy environments, when the channel 11 runs multiple APs. In my
tests if the APs num >= 12, the warning appears in the first few
seconds of scanning.

In order to fix this, the Rx disable code moved to a higher level and
placed before the channel pointer update. This is also makes the code a
bit more symmetrical, since we disable and enable the Rx in the same
function.

In fact, at the pointer update time new frames should not appear,
because interrupt generation at this point should already be disabled.
The next patch should address this issue.

CC: Jiri Slaby <jirislaby@gmail.com>
CC: Nick Kossifidis <mickflemm@gmail.com>
CC: Luis R. Rodriguez <mcgrof@do-not-panic.com>
Reported-by: Christophe Prevotaux <cprevotaux@nltinc.com>
Tested-by: Christophe Prevotaux <cprevotaux@nltinc.com>
Tested-by: Eric Bree <ebree@nltinc.com>
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 drivers/net/wireless/ath/ath5k/base.c  | 24 +++++++++++++++++++++---
 drivers/net/wireless/ath/ath5k/reset.c | 24 ------------------------
 2 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index bc9cb35..34b2f15 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -2858,7 +2858,7 @@ ath5k_reset(struct ath5k_hw *ah, struct ieee80211_channel *chan,
 {
 	struct ath_common *common = ath5k_hw_common(ah);
 	int ret, ani_mode;
-	bool fast;
+	bool fast = chan && modparam_fastchanswitch ? 1 : 0;
 
 	ATH5K_DBG(ah, ATH5K_DEBUG_RESET, "resetting\n");
 
@@ -2876,11 +2876,29 @@ ath5k_reset(struct ath5k_hw *ah, struct ieee80211_channel *chan,
 	 * so we should also free any remaining
 	 * tx buffers */
 	ath5k_drain_tx_buffs(ah);
+
+	/* Stop PCU */
+	ath5k_hw_stop_rx_pcu(ah);
+
+	/* Stop DMA
+	 *
+	 * Note: If DMA didn't stop continue
+	 * since only a reset will fix it.
+	 */
+	ret = ath5k_hw_dma_stop(ah);
+
+	/* RF Bus grant won't work if we have pending
+	 * frames
+	 */
+	if (ret && fast) {
+		ATH5K_DBG(ah, ATH5K_DEBUG_RESET,
+			  "DMA didn't stop, falling back to normal reset\n");
+		fast = false;
+	}
+
 	if (chan)
 		ah->curchan = chan;
 
-	fast = ((chan != NULL) && modparam_fastchanswitch) ? 1 : 0;
-
 	ret = ath5k_hw_reset(ah, ah->opmode, ah->curchan, fast, skip_pcu);
 	if (ret) {
 		ATH5K_ERR(ah, "can't reset hardware (%d)\n", ret);
diff --git a/drivers/net/wireless/ath/ath5k/reset.c b/drivers/net/wireless/ath/ath5k/reset.c
index b9b651e..99e62f9 100644
--- a/drivers/net/wireless/ath/ath5k/reset.c
+++ b/drivers/net/wireless/ath/ath5k/reset.c
@@ -1169,30 +1169,6 @@ ath5k_hw_reset(struct ath5k_hw *ah, enum nl80211_iftype op_mode,
 	if (ah->ah_version == AR5K_AR5212)
 		ath5k_hw_set_sleep_clock(ah, false);
 
-	/*
-	 * Stop PCU
-	 */
-	ath5k_hw_stop_rx_pcu(ah);
-
-	/*
-	 * Stop DMA
-	 *
-	 * Note: If DMA didn't stop continue
-	 * since only a reset will fix it.
-	 */
-	ret = ath5k_hw_dma_stop(ah);
-
-	/* RF Bus grant won't work if we have pending
-	 * frames */
-	if (ret && fast) {
-		ATH5K_DBG(ah, ATH5K_DEBUG_RESET,
-			"DMA didn't stop, falling back to normal reset\n");
-		fast = false;
-		/* Non fatal, just continue with
-		 * normal reset */
-		ret = 0;
-	}
-
 	mode = channel->hw_value;
 	switch (mode) {
 	case AR5K_MODE_11A:
-- 
2.0.4

[PATCH 2/2] ath5k: fix reset race

[Sergey Ryazanov]

To prepare for reset ath5k should finish all asynchronous tasks. At
first, it disables the interrupt generation, then it waits for the
interrupt handler and tasklets completion, and then proceeds to the HW
configuration update. But it does not consider that the interrupt
handler or tasklet re-enables the interrupt generation. And we fall in a
situation when ath5k assumes that interrupts are disabled, but it is
not.

This can lead to different consequences, such as reception of the frame,
when we do not expect it. Under certain circumstances, this can lead to
the following warning:

  WARNING: at ath5k/base.c:589 ath5k_tasklet_rx+0x318/0x6ec [ath5k]()
  invalid hw_rix: 1a
  [..]
  Call Trace:
  [<802656a8>] show_stack+0x48/0x70
  [<802dd92c>] warn_slowpath_common+0x88/0xbc
  [<802dd98c>] warn_slowpath_fmt+0x2c/0x38
  [<81b51be8>] ath5k_tasklet_rx+0x318/0x6ec [ath5k]
  [<8028ac64>] tasklet_action+0x8c/0xf0
  [<80075804>] __do_softirq+0x180/0x32c
  [<80196ce8>] irq_exit+0x54/0x70
  [<80041848>] ret_from_irq+0x0/0x4
  [<80182fdc>] ioread32+0x4/0xc
  [<81b4c42c>] ath5k_hw_set_sleep_clock+0x2ec/0x474 [ath5k]
  [<81b4cf28>] ath5k_hw_reset+0x50/0xeb8 [ath5k]
  [<81b50900>] ath5k_reset+0xd4/0x310 [ath5k]
  [<81b557e8>] ath5k_config+0x4c/0x104 [ath5k]
  [<80d01770>] ieee80211_hw_config+0x2f4/0x35c [mac80211]
  [<80d09aa8>] ieee80211_scan_work+0x2e4/0x414 [mac80211]
  [<8022c3f4>] process_one_work+0x28c/0x400
  [<802df8f8>] worker_thread+0x258/0x3c0
  [<801b5710>] kthread+0xe0/0xec
  [<800418a8>] ret_from_kernel_thread+0x14/0x1c

Fix this issue by adding a new status flag, which forbids to re-enable
the interrupt generation until the HW configuration is completed.

Note: previous patch, which reorders the Rx disable code helps to avoid
the above warning, but not fixes the root cause of unexpected frame
receiving.

CC: Jiri Slaby <jirislaby@gmail.com>
CC: Nick Kossifidis <mickflemm@gmail.com>
CC: Luis R. Rodriguez <mcgrof@do-not-panic.com>
Reported-by: Christophe Prevotaux <cprevotaux@nltinc.com>
Tested-by: Christophe Prevotaux <cprevotaux@nltinc.com>
Tested-by: Eric Bree <ebree@nltinc.com>
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 drivers/net/wireless/ath/ath5k/ath5k.h | 1 +
 drivers/net/wireless/ath/ath5k/base.c  | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
index 1ed7a88..7ca0d6f 100644
--- a/drivers/net/wireless/ath/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath/ath5k/ath5k.h
@@ -1283,6 +1283,7 @@ struct ath5k_hw {
 #define ATH_STAT_PROMISC	1
 #define ATH_STAT_LEDSOFT	2		/* enable LED gpio status */
 #define ATH_STAT_STARTED	3		/* opened & irqs enabled */
+#define ATH_STAT_RESET		4		/* hw reset */
 
 	unsigned int		filter_flags;	/* HW flags, AR5K_RX_FILTER_* */
 	unsigned int		fif_filter_flags; /* Current FIF_* filter flags */
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 34b2f15..41848e1 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -1523,6 +1523,9 @@ ath5k_set_current_imask(struct ath5k_hw *ah)
 	enum ath5k_int imask;
 	unsigned long flags;
 
+	if (test_bit(ATH_STAT_RESET, ah->status))
+		return;
+
 	spin_lock_irqsave(&ah->irqlock, flags);
 	imask = ah->imask;
 	if (ah->rx_pending)
@@ -2862,6 +2865,8 @@ ath5k_reset(struct ath5k_hw *ah, struct ieee80211_channel *chan,
 
 	ATH5K_DBG(ah, ATH5K_DEBUG_RESET, "resetting\n");
 
+	__set_bit(ATH_STAT_RESET, ah->status);
+
 	ath5k_hw_set_imr(ah, 0);
 	synchronize_irq(ah->irq);
 	ath5k_stop_tasklets(ah);
@@ -2952,6 +2957,8 @@ ath5k_reset(struct ath5k_hw *ah, struct ieee80211_channel *chan,
 	 */
 /*	ath5k_chan_change(ah, c); */
 
+	__clear_bit(ATH_STAT_RESET, ah->status);
+
 	ath5k_beacon_config(ah);
 	/* intrs are enabled by ath5k_beacon_config */
 
-- 
2.0.4

Re: [1/2] ath5k: channel change fix

[Kalle Valo]

> ath5k updates the channel pointer and after that it stops the Rx logic
> and apply channel to HW. In case of channel switch, such sequence
> creates a small window when a frame, which is received on the old
> channel is considered as a frame received on the new one.
> 
> The most notable consequence of this situation occurs during the switch
> from 2 GHz band (CCK+OFDM) to the 5GHz band (OFDM-only). Frame received
> with CCK rate, e.g. beacon received at the 1mbps, causes the following
> warning:
> 
>   WARNING: at ath5k/base.c:589 ath5k_tasklet_rx+0x318/0x6ec [ath5k]()
>   invalid hw_rix: 1a
>   [..]
>   Call Trace:
>   [<802656a8>] show_stack+0x48/0x70
>   [<802dd92c>] warn_slowpath_common+0x88/0xbc
>   [<802dd98c>] warn_slowpath_fmt+0x2c/0x38
>   [<81b51be8>] ath5k_tasklet_rx+0x318/0x6ec [ath5k]
>   [<8028ac64>] tasklet_action+0x8c/0xf0
>   [<80075804>] __do_softirq+0x180/0x32c
>   [<80196ce8>] irq_exit+0x54/0x70
>   [<80041848>] ret_from_irq+0x0/0x4
>   [<80182fdc>] ioread32+0x4/0xc
>   [<81b4c42c>] ath5k_hw_set_sleep_clock+0x2ec/0x474 [ath5k]
>   [<81b4cf28>] ath5k_hw_reset+0x50/0xeb8 [ath5k]
>   [<81b50900>] ath5k_reset+0xd4/0x310 [ath5k]
>   [<81b557e8>] ath5k_config+0x4c/0x104 [ath5k]
>   [<80d01770>] ieee80211_hw_config+0x2f4/0x35c [mac80211]
>   [<80d09aa8>] ieee80211_scan_work+0x2e4/0x414 [mac80211]
>   [<8022c3f4>] process_one_work+0x28c/0x400
>   [<802df8f8>] worker_thread+0x258/0x3c0
>   [<801b5710>] kthread+0xe0/0xec
>   [<800418a8>] ret_from_kernel_thread+0x14/0x1c
> 
> The easiest way to reproduce this warning is to run scan with dualband
> NIC in noisy environments, when the channel 11 runs multiple APs. In my
> tests if the APs num >= 12, the warning appears in the first few
> seconds of scanning.
> 
> In order to fix this, the Rx disable code moved to a higher level and
> placed before the channel pointer update. This is also makes the code a
> bit more symmetrical, since we disable and enable the Rx in the same
> function.
> 
> In fact, at the pointer update time new frames should not appear,
> because interrupt generation at this point should already be disabled.
> The next patch should address this issue.
> 
> CC: Jiri Slaby <jirislaby@gmail.com>
> CC: Nick Kossifidis <mickflemm@gmail.com>
> CC: Luis R. Rodriguez <mcgrof@do-not-panic.com>
> Reported-by: Christophe Prevotaux <cprevotaux@nltinc.com>
> Tested-by: Christophe Prevotaux <cprevotaux@nltinc.com>
> Tested-by: Eric Bree <ebree@nltinc.com>
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>

Thanks, 2 patches applied to wireless-drivers-next.git:

4a2f248f9eaf ath5k: channel change fix
ab5e290a8607 ath5k: fix reset race

Kalle Valo