[PATCH] ath5k: fix print on warning on ath5k_hw_to_driver_rix()

[PATCH] ath5k: fix print on warning on ath5k_hw_to_driver_rix()

[Luis R. Rodriguez]

This was printing the wrong value on the warning. While at it
lets expand this warning to provide a little more useful
information for debugging such as the band and hardware rate
index when possible and clarify what the warning is actually
printing.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---

I hit this warning and found it would be more useful with this.
I can't reproduce this right now, but I believe what I did was
suspend at home and resume at the office. Anyway this should
help clear things up.

 drivers/net/wireless/ath/ath5k/base.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 5056410..a4e69c5 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -1136,11 +1136,15 @@ ath5k_hw_to_driver_rix(struct ath5k_softc *sc, int hw_rix)
 
 	/* return base rate on errors */
 	if (WARN(hw_rix < 0 || hw_rix >= AR5K_MAX_RATES,
-			"hw_rix out of bounds: %x\n", hw_rix))
+		 "hardware rate index out of bounds: %x"
+		 "(band: %s)\n", hw_rix,
+		 sc->curband->band ? "5 GHz" : "2.4 GHz"))
 		return 0;
 
 	rix = sc->rate_idx[sc->curband->band][hw_rix];
-	if (WARN(rix < 0, "invalid hw_rix: %x\n", hw_rix))
+	if (WARN(rix < 0, "invalid driver rate index: %x "
+		 "(hw_rix: %x band: %s)\n", rix, hw_rix,
+		 sc->curband->band ? "5 GHz" : "2.4 GHz"))
 		rix = 0;
 
 	return rix;
-- 
1.6.3.3

Re: [PATCH] ath5k: fix print on warning on ath5k_hw_to_driver_rix()

[Bob Copeland]

On Tue, Aug 25, 2009 at 2:25 PM, Luis R.
Rodriguez<lrodriguez@atheros.com> wrote:
> This was printing the wrong value on the warning. While at it
> lets expand this warning to provide a little more useful
> information for debugging such as the band and hardware rate
> index when possible and clarify what the warning is actually
> printing.

>        rix = sc->rate_idx[sc->curband->band][hw_rix];
> -       if (WARN(rix < 0, "invalid hw_rix: %x\n", hw_rix))
> +       if (WARN(rix < 0, "invalid driver rate index: %x "
> +                "(hw_rix: %x band: %s)\n", rix, hw_rix,
> +                sc->curband->band ? "5 GHz" : "2.4 GHz"))

rix will always be -1 here so no real need to print it.

I'm pretty sure band is going to be "the wrong one," because all
instances of this warning I've seen have been valid rate indexes,
like 1 mbit rates when scanning 5 ghz, which could indicate some
race condition with flushing the rx queue on channel changes.  I
haven't yet seen a hw rate we didn't know about.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH] ath5k: fix print on warning on ath5k_hw_to_driver_rix()

[Luis R. Rodriguez]

On Tue, Aug 25, 2009 at 11:45:12AM -0700, Bob Copeland wrote:
> On Tue, Aug 25, 2009 at 2:25 PM, Luis R.
> Rodriguez<lrodriguez@atheros.com> wrote:
> > This was printing the wrong value on the warning. While at it
> > lets expand this warning to provide a little more useful
> > information for debugging such as the band and hardware rate
> > index when possible and clarify what the warning is actually
> > printing.
> 
> >        rix = sc->rate_idx[sc->curband->band][hw_rix];
> > -       if (WARN(rix < 0, "invalid hw_rix: %x\n", hw_rix))
> > +       if (WARN(rix < 0, "invalid driver rate index: %x "
> > +                "(hw_rix: %x band: %s)\n", rix, hw_rix,
> > +                sc->curband->band ? "5 GHz" : "2.4 GHz"))
> 
> rix will always be -1 here so no real need to print it.

OK how about the band info, think that's useful?

> I'm pretty sure band is going to be "the wrong one," because all
> instances of this warning I've seen have been valid rate indexes,
> like 1 mbit rates when scanning 5 ghz, 

I got this on 0x1b which is ATH5K_RATE_CODE_1M, I was suspecting
the same.

> which could indicate some
> race condition with flushing the rx queue on channel changes.

I'll see if I can reproduce somehow.

> I haven't yet seen a hw rate we didn't know about.

So you've seen this lately as well?

  Luis

Re: [PATCH] ath5k: fix print on warning on ath5k_hw_to_driver_rix()

[Bob Copeland]

On Tue, Aug 25, 2009 at 2:58 PM, Luis R.
Rodriguez<lrodriguez@atheros.com> wrote:
> OK how about the band info, think that's useful?

Yeah, band is useful.  We don't have to pretty print it though
(unless you really feel like it), %d would work just as well.

Also WARN_ON_ONCE might be a good idea.

>> which could indicate some
>> race condition with flushing the rx queue on channel changes.
>
> I'll see if I can reproduce somehow.
>
>> I haven't yet seen a hw rate we didn't know about.
>
> So you've seen this lately as well?

I saw it some time ago, but then changed the order of how the
curchan/curband variables were set when we change channels and
haven't seen since.  But kerneloops says a lot of other people
are still hitting it as well :(

Hmm, ath5k_rx_stop probably wants an equivalent to txq_drainq in
there somewhere.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH] ath5k: fix print on warning on ath5k_hw_to_driver_rix()

[Luis R. Rodriguez]

On Tue, Aug 25, 2009 at 12:22:55PM -0700, Bob Copeland wrote:
> On Tue, Aug 25, 2009 at 2:58 PM, Luis R.
> Rodriguez<lrodriguez@atheros.com> wrote:
> > OK how about the band info, think that's useful?
> 
> Yeah, band is useful.  We don't have to pretty print it though
> (unless you really feel like it), %d would work just as well.
> 
> Also WARN_ON_ONCE might be a good idea.

Alright, first let me see if I can reproduce after cleaning
curband crap a little.

> >> which could indicate some
> >> race condition with flushing the rx queue on channel changes.
> >
> > I'll see if I can reproduce somehow.
> >
> >> I haven't yet seen a hw rate we didn't know about.
> >
> > So you've seen this lately as well?
> 
> I saw it some time ago, but then changed the order of how the
> curchan/curband variables were set when we change channels and
> haven't seen since.  But kerneloops says a lot of other people
> are still hitting it as well :(

OK that's good to know too.

> Hmm, ath5k_rx_stop probably wants an equivalent to txq_drainq in
> there somewhere.

True.

First thing I saw upon a quick review was we were relying on our own
curband for RX instead of the cfg80211 hw->conf band. Now granted
that *may* be updated properly, and it seems that may be the case,
but since do not tend to trust drivers here's a removal of all that
crap first. I'll test this first to see if I run into the same warning.
If this doesn't cure it then we are indeed processing old frames on RX
and need to flush 'em out prior to a channel change first.

Good thing is after a few hours I can reproduce at least.

  Luis

From: Luis R. Rodriguez <lrodriguez@atheros.com>
Subject: [PATCH] ath5k: remove curband, curchan, curmode

It still possible for ath5k tasklet to trigger and the
curband to be off. There seems to be some race against it
somewhere. To help aid with this review we simplify the
notion of current band and current channel assumptions
ath5k and only rely on cfg80211's as it already does the
book keeping for us.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath5k/base.c |   87 +++++++++++++++------------------
 drivers/net/wireless/ath/ath5k/base.h |    6 --
 2 files changed, 39 insertions(+), 54 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 5056410..feffc92 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -220,7 +220,8 @@ static struct pci_driver ath5k_pci_driver = {
 static int ath5k_tx(struct ieee80211_hw *hw, struct sk_buff *skb);
 static int ath5k_tx_queue(struct ieee80211_hw *hw, struct sk_buff *skb,
 		struct ath5k_txq *txq);
-static int ath5k_reset(struct ath5k_softc *sc, struct ieee80211_channel *chan);
+static int ath5k_reset(struct ath5k_softc *sc, struct ieee80211_channel *chan,
+		       bool chan_change);
 static int ath5k_reset_wake(struct ath5k_softc *sc);
 static int ath5k_start(struct ieee80211_hw *hw);
 static void ath5k_stop(struct ieee80211_hw *hw);
@@ -293,8 +294,6 @@ static unsigned int ath5k_copy_channels(struct ath5k_hw *ah,
 static int 	ath5k_setup_bands(struct ieee80211_hw *hw);
 static int 	ath5k_chan_set(struct ath5k_softc *sc,
 				struct ieee80211_channel *chan);
-static void	ath5k_setcurmode(struct ath5k_softc *sc,
-				unsigned int mode);
 static void	ath5k_mode_setup(struct ath5k_softc *sc);
 
 /* Descriptor setup */
@@ -759,12 +758,6 @@ ath5k_attach(struct pci_dev *pdev, struct ieee80211_hw *hw)
 		goto err;
 	}
 
-	/* NB: setup here so ath5k_rate_update is happy */
-	if (test_bit(AR5K_MODE_11A, ah->ah_modes))
-		ath5k_setcurmode(sc, AR5K_MODE_11A);
-	else
-		ath5k_setcurmode(sc, AR5K_MODE_11B);
-
 	/*
 	 * Allocate tx+rx descriptors and populate the lists.
 	 */
@@ -1084,7 +1077,7 @@ static int
 ath5k_chan_set(struct ath5k_softc *sc, struct ieee80211_channel *chan)
 {
 	ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "(%u MHz) -> (%u MHz)\n",
-		sc->curchan->center_freq, chan->center_freq);
+		sc->hw->conf.channel->center_freq, chan->center_freq);
 
 	/*
 	 * To switch channels clear any pending DMA operations;
@@ -1092,19 +1085,12 @@ ath5k_chan_set(struct ath5k_softc *sc, struct ieee80211_channel *chan)
 	 * hardware at the new frequency, and then re-enable
 	 * the relevant bits of the h/w.
 	 */
-	return ath5k_reset(sc, chan);
-}
 
-static void
-ath5k_setcurmode(struct ath5k_softc *sc, unsigned int mode)
-{
-	sc->curmode = mode;
+	ath5k_hw_set_imr(sc->ah, 0);
+	ath5k_txq_cleanup(sc);
+	ath5k_rx_stop(sc);
 
-	if (mode == AR5K_MODE_11A) {
-		sc->curband = &sc->sbands[IEEE80211_BAND_5GHZ];
-	} else {
-		sc->curband = &sc->sbands[IEEE80211_BAND_2GHZ];
-	}
+	return ath5k_reset(sc, chan, true);
 }
 
 static void
@@ -1136,11 +1122,15 @@ ath5k_hw_to_driver_rix(struct ath5k_softc *sc, int hw_rix)
 
 	/* return base rate on errors */
 	if (WARN(hw_rix < 0 || hw_rix >= AR5K_MAX_RATES,
-			"hw_rix out of bounds: %x\n", hw_rix))
+		 "hardware rate index out of bounds: %x"
+		 "on freq: %d\n", hw_rix,
+		 sc->hw->conf.channel->center_freq))
 		return 0;
 
-	rix = sc->rate_idx[sc->curband->band][hw_rix];
-	if (WARN(rix < 0, "invalid hw_rix: %x\n", hw_rix))
+	rix = sc->rate_idx[sc->hw->conf.channel->band][hw_rix];
+	if (WARN(rix < 0, "invalid driver rate index mapped for "
+		 "hw_rix: %x on freq: %d\n", hw_rix,
+		 sc->hw->conf.channel->center_freq))
 		rix = 0;
 
 	return rix;
@@ -1742,6 +1732,8 @@ static void
 ath5k_tasklet_rx(unsigned long data)
 {
 	struct ieee80211_rx_status rxs = {};
+	struct ieee80211_rate *bitrate;
+	struct ieee80211_supported_band *sband;
 	struct ath5k_rx_status rs = {};
 	struct sk_buff *skb, *next_skb;
 	dma_addr_t next_skb_addr;
@@ -1751,6 +1743,7 @@ ath5k_tasklet_rx(unsigned long data)
 	int ret;
 	int hdrlen;
 	int padsize;
+	int cur_band;
 
 	spin_lock(&sc->rxbuflock);
 	if (list_empty(&sc->rxbuf)) {
@@ -1864,8 +1857,8 @@ accept:
 		rxs.mactime = ath5k_extend_tsf(sc->ah, rs.rs_tstamp);
 		rxs.flag |= RX_FLAG_TSFT;
 
-		rxs.freq = sc->curchan->center_freq;
-		rxs.band = sc->curband->band;
+		rxs.freq = sc->hw->conf.channel->center_freq;
+		rxs.band = sc->hw->conf.channel->band;
 
 		rxs.noise = sc->ah->ah_noise_floor;
 		rxs.signal = rxs.noise + rs.rs_rssi;
@@ -1885,8 +1878,11 @@ accept:
 		rxs.rate_idx = ath5k_hw_to_driver_rix(sc, rs.rs_rate);
 		rxs.flag |= ath5k_rx_decrypted(sc, ds, skb, &rs);
 
-		if (rxs.rate_idx >= 0 && rs.rs_rate ==
-		    sc->curband->bitrates[rxs.rate_idx].hw_value_short)
+		cur_band = sc->hw->conf.channel->band;
+		sband = sc->hw->wiphy->bands[cur_band];
+		bitrate = &sband->bitrates[rxs.rate_idx];
+
+		if (rxs.rate_idx >= 0 && rs.rs_rate == bitrate->hw_value_short)
 			rxs.flag |= RX_FLAG_SHORTPRE;
 
 		ath5k_debug_dump_skb(sc, skb, "RX  ", 0);
@@ -2328,6 +2324,11 @@ static void ath5k_tasklet_beacon(unsigned long data)
 	}
 }
 
+static int
+ath5k_reset_init(struct ath5k_softc *sc)
+{
+	return ath5k_reset(sc, sc->hw->conf.channel, false);
+}
 
 /********************\
 * Interrupt handling *
@@ -2356,12 +2357,10 @@ ath5k_init(struct ath5k_softc *sc)
 	 * be followed by initialization of the appropriate bits
 	 * and then setup of the interrupt mask.
 	 */
-	sc->curchan = sc->hw->conf.channel;
-	sc->curband = &sc->sbands[sc->curchan->band];
 	sc->imask = AR5K_INT_RXOK | AR5K_INT_RXERR | AR5K_INT_RXEOL |
 		AR5K_INT_RXORN | AR5K_INT_TXDESC | AR5K_INT_TXEOL |
 		AR5K_INT_FATAL | AR5K_INT_GLOBAL | AR5K_INT_SWI;
-	ret = ath5k_reset(sc, NULL);
+	ret = ath5k_reset_init(sc);
 	if (ret)
 		goto done;
 
@@ -2581,8 +2580,8 @@ ath5k_tasklet_calibrate(unsigned long data)
 	ieee80211_stop_queues(sc->hw);
 
 	ATH5K_DBG(sc, ATH5K_DEBUG_CALIBRATE, "channel %u/%x\n",
-		ieee80211_frequency_to_channel(sc->curchan->center_freq),
-		sc->curchan->hw_value);
+		ieee80211_frequency_to_channel(sc->hw->conf.channel->center_freq),
+		sc->hw->conf.channel->hw_value);
 
 	if (ath5k_hw_gainf_calibrate(ah) == AR5K_RFGAIN_NEED_CHANGE) {
 		/*
@@ -2592,10 +2591,10 @@ ath5k_tasklet_calibrate(unsigned long data)
 		ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "calibration, resetting\n");
 		ath5k_reset_wake(sc);
 	}
-	if (ath5k_hw_phy_calibrate(ah, sc->curchan))
+	if (ath5k_hw_phy_calibrate(ah, sc->hw->conf.channel))
 		ATH5K_ERR(sc, "calibration of channel %u failed\n",
 			ieee80211_frequency_to_channel(
-				sc->curchan->center_freq));
+				sc->hw->conf.channel->center_freq));
 
 	ah->ah_swi_mask = 0;
 
@@ -2680,26 +2679,18 @@ drop_packet:
 }
 
 /*
- * Reset the hardware.  If chan is not NULL, then also pause rx/tx
- * and change to the given channel.
+ * Reset the hardware.
  */
 static int
-ath5k_reset(struct ath5k_softc *sc, struct ieee80211_channel *chan)
+ath5k_reset(struct ath5k_softc *sc, struct ieee80211_channel *chan,
+	    bool chan_change)
 {
 	struct ath5k_hw *ah = sc->ah;
 	int ret;
 
 	ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "resetting\n");
 
-	if (chan) {
-		ath5k_hw_set_imr(ah, 0);
-		ath5k_txq_cleanup(sc);
-		ath5k_rx_stop(sc);
-
-		sc->curchan = chan;
-		sc->curband = &sc->sbands[chan->band];
-	}
-	ret = ath5k_hw_reset(ah, sc->opmode, sc->curchan, chan != NULL);
+	ret = ath5k_hw_reset(ah, sc->opmode, chan, chan_change);
 	if (ret) {
 		ATH5K_ERR(sc, "can't reset hardware (%d)\n", ret);
 		goto err;
@@ -2735,7 +2726,7 @@ ath5k_reset_wake(struct ath5k_softc *sc)
 {
 	int ret;
 
-	ret = ath5k_reset(sc, sc->curchan);
+	ret = ath5k_reset_init(sc);
 	if (!ret)
 		ieee80211_wake_queues(sc->hw);
 
diff --git a/drivers/net/wireless/ath/ath5k/base.h b/drivers/net/wireless/ath/ath5k/base.h
index a28c42f..1b88814 100644
--- a/drivers/net/wireless/ath/ath5k/base.h
+++ b/drivers/net/wireless/ath/ath5k/base.h
@@ -128,8 +128,6 @@ struct ath5k_softc {
 	enum nl80211_iftype	opmode;
 	struct ath5k_hw		*ah;		/* Atheros HW */
 
-	struct ieee80211_supported_band		*curband;
-
 #ifdef CONFIG_ATH5K_DEBUG
 	struct ath5k_dbg_info	debug;		/* debug info */
 #endif /* CONFIG_ATH5K_DEBUG */
@@ -147,11 +145,7 @@ struct ath5k_softc {
 #define ATH_STAT_STARTED	4		/* opened & irqs enabled */
 
 	unsigned int		filter_flags;	/* HW flags, AR5K_RX_FILTER_* */
-	unsigned int		curmode;	/* current phy mode */
-	struct ieee80211_channel *curchan;	/* current h/w channel */
-
 	struct ieee80211_vif *vif;
-
 	enum ath5k_int		imask;		/* interrupt mask copy */
 
 	DECLARE_BITMAP(keymap, AR5K_KEYCACHE_SIZE); /* key use bit map */
-- 
1.6.3.3

Re: [PATCH] ath5k: fix print on warning on ath5k_hw_to_driver_rix()

[Luis R. Rodriguez]

On Tue, Aug 25, 2009 at 4:38 PM, Luis R.
Rodriguez<lrodriguez@atheros.com> wrote:
> On Tue, Aug 25, 2009 at 12:22:55PM -0700, Bob Copeland wrote:
>> On Tue, Aug 25, 2009 at 2:58 PM, Luis R.
>> Rodriguez<lrodriguez@atheros.com> wrote:
>> > OK how about the band info, think that's useful?
>>
>> Yeah, band is useful.  We don't have to pretty print it though
>> (unless you really feel like it), %d would work just as well.
>>
>> Also WARN_ON_ONCE might be a good idea.
>
> Alright, first let me see if I can reproduce after cleaning
> curband crap a little.
>
>> >> which could indicate some
>> >> race condition with flushing the rx queue on channel changes.
>> >
>> > I'll see if I can reproduce somehow.
>> >
>> >> I haven't yet seen a hw rate we didn't know about.
>> >
>> > So you've seen this lately as well?
>>
>> I saw it some time ago, but then changed the order of how the
>> curchan/curband variables were set when we change channels and
>> haven't seen since.  But kerneloops says a lot of other people
>> are still hitting it as well :(
>
> OK that's good to know too.
>
>> Hmm, ath5k_rx_stop probably wants an equivalent to txq_drainq in
>> there somewhere.
>
> True.
>
> First thing I saw upon a quick review was we were relying on our own
> curband for RX instead of the cfg80211 hw->conf band. Now granted
> that *may* be updated properly, and it seems that may be the case,
> but since do not tend to trust drivers here's a removal of all that
> crap first. I'll test this first to see if I run into the same warning.
> If this doesn't cure it then we are indeed processing old frames on RX
> and need to flush 'em out prior to a channel change first.
>
> Good thing is after a few hours I can reproduce at least.

Whah, the warn is still there.

[ 7281.492622] ------------[ cut here ]------------
[ 7281.492655] WARNING: at drivers/net/wireless/ath/ath5k/base.c:1133
ath5k_tasklet_rx+0x57b/0x720 [ath5k]()
[ 7281.492663] Hardware name: 7660A14
[ 7281.492668] invalid driver rate index mapped for hw_rix: 1b on freq: 5540

  Luis

Re: [PATCH] ath5k: fix print on warning on ath5k_hw_to_driver_rix()

[Bob Copeland]

On Tue, Aug 25, 2009 at 7:38 PM, Luis R. Rodriguez<lrodriguez@atheros.com>
> First thing I saw upon a quick review was we were relying on our own
> curband for RX instead of the cfg80211 hw->conf band. Now granted
> that *may* be updated properly, and it seems that may be the case,

Looks good at first glance, except it conflicts with changes I
recently posted.  I'm all for getting rid of driver copies of stuff
in the stack.

> -       /* NB: setup here so ath5k_rate_update is happy */
> -       if (test_bit(AR5K_MODE_11A, ah->ah_modes))
> -               ath5k_setcurmode(sc, AR5K_MODE_11A);
> -       else
> -               ath5k_setcurmode(sc, AR5K_MODE_11B);
> -

I take it ath5k_rate_update didn't really care?

>  ath5k_chan_set(struct ath5k_softc *sc, struct ieee80211_channel *chan)
>  {
>        ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "(%u MHz) -> (%u MHz)\n",
> -               sc->curchan->center_freq, chan->center_freq);
> +               sc->hw->conf.channel->center_freq, chan->center_freq);

So, does hw->conf.channel change before we're told to change channel,
or after?

> -       if (chan) {
> -               ath5k_hw_set_imr(ah, 0);
> -               ath5k_txq_cleanup(sc);
> -               ath5k_rx_stop(sc);
> -
> -               sc->curchan = chan;
> -               sc->curband = &sc->sbands[chan->band];
> -       }

Unless I missed something I think we still want:

    if (chan_change)
        ath5k_chan_set(...);

> -       ret = ath5k_hw_reset(ah, sc->opmode, sc->curchan, chan != NULL);
> +       ret = ath5k_hw_reset(ah, sc->opmode, chan, chan_change);

There's just no synchronization of this stuff, not too surprising there
are races.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH] ath5k: fix print on warning on ath5k_hw_to_driver_rix()

[Luis R. Rodriguez]

On Wed, Aug 26, 2009 at 07:08:55AM -0700, Bob Copeland wrote:
> On Tue, Aug 25, 2009 at 7:38 PM, Luis R. Rodriguez<lrodriguez@atheros.com>
> > First thing I saw upon a quick review was we were relying on our own
> > curband for RX instead of the cfg80211 hw->conf band. Now granted
> > that *may* be updated properly, and it seems that may be the case,
> 
> Looks good at first glance, except it conflicts with changes I
> recently posted.  I'm all for getting rid of driver copies of stuff
> in the stack.

I can rebase.

> > -       /* NB: setup here so ath5k_rate_update is happy */
> > -       if (test_bit(AR5K_MODE_11A, ah->ah_modes))
> > -               ath5k_setcurmode(sc, AR5K_MODE_11A);
> > -       else
> > -               ath5k_setcurmode(sc, AR5K_MODE_11B);
> > -
> 
> I take it ath5k_rate_update didn't really care?

Well I nuked ath5k_setcurmode() and didn't see anything relying on that
stuff.

> >  ath5k_chan_set(struct ath5k_softc *sc, struct ieee80211_channel *chan)
> >  {
> >        ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "(%u MHz) -> (%u MHz)\n",
> > -               sc->curchan->center_freq, chan->center_freq);
> > +               sc->hw->conf.channel->center_freq, chan->center_freq);
> 
> So, does hw->conf.channel change before we're told to change channel,
> or after?

Oh bummer, good thing you asked as I simply assumed it would be set *after*.
Turns out its done prior to the drv_config() call.

        if (chan != local->hw.conf.channel ||
            channel_type != local->hw.conf.channel_type) {
                local->hw.conf.channel = chan;
                local->hw.conf.channel_type = channel_type;
                changed |= IEEE80211_CONF_CHANGE_CHANNEL;
        }

	...

        if (changed && local->open_count) {
                ret = drv_config(local, changed);
		...
	}

I suppose we can just remove that printk ? :)

> > -       if (chan) {
> > -               ath5k_hw_set_imr(ah, 0);
> > -               ath5k_txq_cleanup(sc);
> > -               ath5k_rx_stop(sc);
> > -
> > -               sc->curchan = chan;
> > -               sc->curband = &sc->sbands[chan->band];
> > -       }
> 
> Unless I missed something I think we still want:
> 
>     if (chan_change)
>         ath5k_chan_set(...);

Not sure I follow. Let me explain the logic here a little better.
ath5k_reset() had a check to see if we switched channels. The check
is above, and I moved it ath5k_chan_set() called by the config callback.
Reason for this is channel change *only* occurs through the config callback
so we can be certain no other path will call reset with a channel change
request.

> > -       ret = ath5k_hw_reset(ah, sc->opmode, sc->curchan, chan != NULL);
> > +       ret = ath5k_hw_reset(ah, sc->opmode, chan, chan_change);
> 
> There's just no synchronization of this stuff, not too surprising there
> are races.

config calls for reset seem to be with sc->lock. ath9k uses a mutex to
protect races between mac80211 callback calls, ath5k seems to use the
sc->lock *sometimes*, a good review of that may help but for channel
change this seems protected unless I missed something. Since channel
change goes through the config callback and since the callback protects
through sc->lock I can't see how we'd race against changing channels.

  Luis

Re: [PATCH] ath5k: fix print on warning on ath5k_hw_to_driver_rix()

[Bob Copeland]

On Wed, Aug 26, 2009 at 12:29 PM, Luis R.
Rodriguez<lrodriguez@atheros.com> wrote:

> Not sure I follow. Let me explain the logic here a little better.
> ath5k_reset() had a check to see if we switched channels. The check
> is above, and I moved it ath5k_chan_set() called by the config callback.
> Reason for this is channel change *only* occurs through the config callback
> so we can be certain no other path will call reset with a channel change
> request.

But previously we did all this stuff for _every_ reset except the first
one, now we only do it for channel change resets.  It may make sense your
way, but I'd rather see that as a separate patch rather than part of a
cleanup.  (Also, chan_change is used to skip a bunch of phy register writes
that we only need to do once, not sure if that changes here).

>> There's just no synchronization of this stuff, not too surprising there
>> are races.
>
> config calls for reset seem to be with sc->lock. ath9k uses a mutex to
> protect races between mac80211 callback calls, ath5k seems to use the
> sc->lock *sometimes*, a good review of that may help but for channel
> change this seems protected unless I missed something. Since channel
> change goes through the config callback and since the callback protects
> through sc->lock I can't see how we'd race against changing channels.

rx_tasklet doesn't (can't) take the mutex though.  Here's the race:

cpu 1                       cpu 2
                            1mbit packet received, ack intr and queue it
client changes to 5ghz band
hw->conf.channel = 36
                            ath5k_rx_tasklet runs
                            hmm, we got 1mbit packet on channel 36, wtf
ath5k_chan_change()

This is still racy with curband variables of course, but IMO the
proposed change actually makes the race wider (in current code, we only
update the band after we disable the interrupts, but the tasklet can
still run on queued packets).

The right thing to do is either process or drop all of those packets
with interrupts disabled before updating the band/channel (rx_drainq),
or protect that channel info with a spinlock.   Or perhaps we can
figure it out directly from the rx descriptor -- I looked at this
briefly and didn't think there was enough info there, but didn't spend
much time on it.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH] ath5k: fix print on warning on ath5k_hw_to_driver_rix()

[Luis R. Rodriguez]

On Wed, Aug 26, 2009 at 10:03:09AM -0700, Bob Copeland wrote:
> On Wed, Aug 26, 2009 at 12:29 PM, Luis R.
> Rodriguez<lrodriguez@atheros.com> wrote:
> 
> > Not sure I follow. Let me explain the logic here a little better.
> > ath5k_reset() had a check to see if we switched channels. The check
> > is above, and I moved it ath5k_chan_set() called by the config callback.
> > Reason for this is channel change *only* occurs through the config callback
> > so we can be certain no other path will call reset with a channel change
> > request.
> 
> But previously we did all this stuff for _every_ reset except the first
> one, 

Actually we did it in every ath5k_reset() if and only if chan was not NULL.
We called ath5k_reset() from 3 places:

  * Init
  * Channel change
  * Reset tasklet for hw issues

chan was only null upon init as you point out, so you are right in that
I overlooked that we currently do this also upon hw issues.

> now we only do it for channel change resets.  It may make sense your
> way, but I'd rather see that as a separate patch rather than part of a
> cleanup.

Absolutely, I overlooked this, thanks for picking that up.

> (Also, chan_change is used to skip a bunch of phy register writes
> that we only need to do once, not sure if that changes here).

Yeah since I also used ath5k_reset_init() on the reset hw issue tasklet and
that does pass false for channel_change this patch would have changed that
as well, so I'll fix that.

> >> There's just no synchronization of this stuff, not too surprising there
> >> are races.
> >
> > config calls for reset seem to be with sc->lock. ath9k uses a mutex to
> > protect races between mac80211 callback calls, ath5k seems to use the
> > sc->lock *sometimes*, a good review of that may help but for channel
> > change this seems protected unless I missed something. Since channel
> > change goes through the config callback and since the callback protects
> > through sc->lock I can't see how we'd race against changing channels.
> 
> rx_tasklet doesn't (can't) take the mutex though.  Here's the race:
> 
> cpu 1                       cpu 2
>                             1mbit packet received, ack intr and queue it
> client changes to 5ghz band
> hw->conf.channel = 36
>                             ath5k_rx_tasklet runs
>                             hmm, we got 1mbit packet on channel 36, wtf
> ath5k_chan_change()
> 
> This is still racy with curband variables of course, but IMO the
> proposed change actually makes the race wider (in current code, we only
> update the band after we disable the interrupts, but the tasklet can
> still run on queued packets).

I don't want to bother looking ath ath5k solutions to the race, I rather
we try to fix this if possible with a generic solution on mac80211. I suspect
ath5k is not the only one with this possible race.

> The right thing to do is either process or drop all of those packets

Would be nice if we did not have to, those packets can be very valuable
considering we now support spreading our scan over time.

> with interrupts disabled before updating the band/channel (rx_drainq),

That would be nice, so prior to channel change we need to ensure we:

  * Disable interrupts
  * Disable RX
  * Process all pending frames in RX queue

It is the last part that it seems we are missing. Running tasklet_schedule()
will ensure we can schedule the tasklet but it does not ensure we will
wait until it has run at least once. Johill pointed out tasklet_disable()
followed by a tasklet_enable() would do it, but seems rather odd.

But do we need this?

In the rx tasklet we need band to access the right rate table -- this is
how we got into this conversation but if you think about it you also need
to ensure your harware hasn't already flipped to another band otherwise
you can start sending mac80211 frames for a different band. This certainly
matters for rate control but haven't reviewed all the details of where
inconsistancy between the hw->conf.channel and the skbs received by
mac80211 are.

Processing RXd frames prior to switching channels seems like a good idea
regardless.

>From what I see Atheros RX descriptors do not have the frequency on which the
frame was received on so you do need some sort of house keeping.
mac80211/cfg80211 does this for us so I'd like to try to avoid doing more
on drivers if possible.

Since cfg80211 modifies hw->conf.channel prior to calling drv_config() it
is possible for the driver to not reliably use this variable either right now.

So how about a generic rx_drain() callback for mac80211 drivers and document
that on it we need disable RXing frames, disable interrupts for RX,
and then either have the driver drop pending frames (worst case) or process them.
mac80211 could then use this prior to drv_config() if we are changing channels.

> or protect that channel info with a spinlock.   Or perhaps we can
> figure it out directly from the rx descriptor -- I looked at this
> briefly and didn't think there was enough info there, but didn't spend
> much time on it.

I looked too and did not find it. I can't find a reason to have it in
hardware descriptors though, given that you could just drain rx prior to
a switch.

Only argument I can see which may affect draining RX prior to channel
change is it could delay the time it takes for you to switch. Either way
this seems just a lot cleaner to support.

Thoughts?

  Luis