[PATCH 0/4] ath5k: misc fixes

[PATCH 0/4] ath5k: misc fixes

[Bob Copeland]

Hi,

Here are a few bug fixes for ath5k.  They are primarily minor
and/or theoretical so I didn't include stable CCs.

They should not conflict with Bruno's ANI patch.

Bob Copeland (4):
  ath5k: correct channel setting for 2.5 mhz spacing
  ath5k: clean up queue manipulation
  ath5k: fix race condition in tx desc processing
  ath5k: add bounds check to pdadc table

 drivers/net/wireless/ath/ath5k/base.c |   14 ++++++++++++--
 drivers/net/wireless/ath/ath5k/phy.c  |   10 +++++-----
 2 files changed, 17 insertions(+), 7 deletions(-)

[PATCH 1/4] ath5k: correct channel setting for 2.5 mhz spacing

[Bob Copeland]

These channels aren't selectable anyway, but our calculations
for 2.5 mhz frequencies are incorrect.  The value is supposed to
be:

  (frequency - reference) * (10/25)

i.e., divide by 2.5, but we were instead doing:

  (10 * frequency - reference) / 25.

Additionally, the check for (frequency % 5 == 2) had an extra
subtraction that wasn't in madwifi HAL.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 drivers/net/wireless/ath/ath5k/phy.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
index b83ca8a..81bdebd 100644
--- a/drivers/net/wireless/ath/ath5k/phy.c
+++ b/drivers/net/wireless/ath/ath5k/phy.c
@@ -979,7 +979,7 @@ static int ath5k_hw_rf5112_channel(struct ath5k_hw *ah,
 			return -EINVAL;
 
 		data0 = ath5k_hw_bitswap((data0 << 2) & 0xff, 8);
-	} else if ((c - (c % 5)) != 2 || c > 5435) {
+	} else if ((c % 5) != 2 || c > 5435) {
 		if (!(c % 20) && c >= 5120) {
 			data0 = ath5k_hw_bitswap(((c - 4800) / 20 << 2), 8);
 			data2 = ath5k_hw_bitswap(3, 2);
@@ -992,7 +992,7 @@ static int ath5k_hw_rf5112_channel(struct ath5k_hw *ah,
 		} else
 			return -EINVAL;
 	} else {
-		data0 = ath5k_hw_bitswap((10 * (c - 2) - 4800) / 25 + 1, 8);
+		data0 = ath5k_hw_bitswap((10 * (c - 2 - 4800)) / 25 + 1, 8);
 		data2 = ath5k_hw_bitswap(0, 2);
 	}
 
@@ -1020,7 +1020,7 @@ static int ath5k_hw_rf2425_channel(struct ath5k_hw *ah,
 		data0 = ath5k_hw_bitswap((c - 2272), 8);
 		data2 = 0;
 	/* ? 5GHz ? */
-	} else if ((c - (c % 5)) != 2 || c > 5435) {
+	} else if ((c % 5) != 2 || c > 5435) {
 		if (!(c % 20) && c < 5120)
 			data0 = ath5k_hw_bitswap(((c - 4800) / 20 << 2), 8);
 		else if (!(c % 10))
@@ -1031,7 +1031,7 @@ static int ath5k_hw_rf2425_channel(struct ath5k_hw *ah,
 			return -EINVAL;
 		data2 = ath5k_hw_bitswap(1, 2);
 	} else {
-		data0 = ath5k_hw_bitswap((10 * (c - 2) - 4800) / 25 + 1, 8);
+		data0 = ath5k_hw_bitswap((10 * (c - 2 - 4800)) / 25 + 1, 8);
 		data2 = ath5k_hw_bitswap(0, 2);
 	}
 
-- 
1.6.3.3

Re: [PATCH 1/4] ath5k: correct channel setting for 2.5 mhz spacing

[Bruno Randolf]

On Thursday 08 April 2010 12:55:56 Bob Copeland wrote:
> These channels aren't selectable anyway, but our calculations
> for 2.5 mhz frequencies are incorrect.  The value is supposed to
> be:
> 
>   (frequency - reference) * (10/25)
> 
> i.e., divide by 2.5, but we were instead doing:
> 
>   (10 * frequency - reference) / 25.
> 
> Additionally, the check for (frequency % 5 == 2) had an extra
> subtraction that wasn't in madwifi HAL.
> 
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
>  drivers/net/wireless/ath/ath5k/phy.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath5k/phy.c
> b/drivers/net/wireless/ath/ath5k/phy.c index b83ca8a..81bdebd 100644
> --- a/drivers/net/wireless/ath/ath5k/phy.c
> +++ b/drivers/net/wireless/ath/ath5k/phy.c
> @@ -979,7 +979,7 @@ static int ath5k_hw_rf5112_channel(struct ath5k_hw *ah,
>  			return -EINVAL;
> 
>  		data0 = ath5k_hw_bitswap((data0 << 2) & 0xff, 8);
> -	} else if ((c - (c % 5)) != 2 || c > 5435) {
> +	} else if ((c % 5) != 2 || c > 5435) {
>  		if (!(c % 20) && c >= 5120) {
>  			data0 = ath5k_hw_bitswap(((c - 4800) / 20 << 2), 8);
>  			data2 = ath5k_hw_bitswap(3, 2);
> @@ -992,7 +992,7 @@ static int ath5k_hw_rf5112_channel(struct ath5k_hw *ah,
>  		} else
>  			return -EINVAL;
>  	} else {
> -		data0 = ath5k_hw_bitswap((10 * (c - 2) - 4800) / 25 + 1, 8);
> +		data0 = ath5k_hw_bitswap((10 * (c - 2 - 4800)) / 25 + 1, 8);
>  		data2 = ath5k_hw_bitswap(0, 2);
>  	}
> 
> @@ -1020,7 +1020,7 @@ static int ath5k_hw_rf2425_channel(struct ath5k_hw
> *ah, data0 = ath5k_hw_bitswap((c - 2272), 8);
>  		data2 = 0;
>  	/* ? 5GHz ? */
> -	} else if ((c - (c % 5)) != 2 || c > 5435) {
> +	} else if ((c % 5) != 2 || c > 5435) {
>  		if (!(c % 20) && c < 5120)
>  			data0 = ath5k_hw_bitswap(((c - 4800) / 20 << 2), 8);
>  		else if (!(c % 10))
> @@ -1031,7 +1031,7 @@ static int ath5k_hw_rf2425_channel(struct ath5k_hw
> *ah, return -EINVAL;
>  		data2 = ath5k_hw_bitswap(1, 2);
>  	} else {
> -		data0 = ath5k_hw_bitswap((10 * (c - 2) - 4800) / 25 + 1, 8);
> +		data0 = ath5k_hw_bitswap((10 * (c - 2 - 4800)) / 25 + 1, 8);
>  		data2 = ath5k_hw_bitswap(0, 2);
>  	}

for whatever it's worth :-)

Acked-by: Bruno Randolf <br1@einfach.org>

[PATCH 2/4] ath5k: clean up queue manipulation

[Bob Copeland]

Review spotted a couple of strange invocations to
ieee80211_wake_queues that could potentially cause problems:

 - queues are awakened in the calibration tasklet before
   phy calibration, and then again after calibration

 - queues are awakened inside reset when we're trying to
   drain the ath5k transmit queues, and again after
   reset is completed (in callers to ath5k_reset_wake).

In both cases the first wake is unnecessary, so remove it.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 drivers/net/wireless/ath/ath5k/base.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index a1c0dcb..1e10439 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -1633,7 +1633,6 @@ ath5k_txq_cleanup(struct ath5k_softc *sc)
 					sc->txqs[i].link);
 			}
 	}
-	ieee80211_wake_queues(sc->hw); /* XXX move to callers */
 
 	for (i = 0; i < ARRAY_SIZE(sc->txqs); i++)
 		if (sc->txqs[i].setup)
@@ -2761,7 +2760,7 @@ ath5k_tasklet_calibrate(unsigned long data)
 		 * to load new gain values.
 		 */
 		ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "calibration, resetting\n");
-		ath5k_reset_wake(sc);
+		ath5k_reset(sc, sc->curchan);
 	}
 	if (ath5k_hw_phy_calibrate(ah, sc->curchan))
 		ATH5K_ERR(sc, "calibration of channel %u failed\n",
-- 
1.6.3.3

Re: [PATCH 2/4] ath5k: clean up queue manipulation

[Bruno Randolf]

On Thursday 08 April 2010 12:55:57 Bob Copeland wrote:
> Review spotted a couple of strange invocations to
> ieee80211_wake_queues that could potentially cause problems:
> 
>  - queues are awakened in the calibration tasklet before
>    phy calibration, and then again after calibration
> 
>  - queues are awakened inside reset when we're trying to
>    drain the ath5k transmit queues, and again after
>    reset is completed (in callers to ath5k_reset_wake).
> 
> In both cases the first wake is unnecessary, so remove it.
> 
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
>  drivers/net/wireless/ath/ath5k/base.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath5k/base.c
> b/drivers/net/wireless/ath/ath5k/base.c index a1c0dcb..1e10439 100644
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -1633,7 +1633,6 @@ ath5k_txq_cleanup(struct ath5k_softc *sc)
>  					sc->txqs[i].link);
>  			}
>  	}
> -	ieee80211_wake_queues(sc->hw); /* XXX move to callers */
> 
>  	for (i = 0; i < ARRAY_SIZE(sc->txqs); i++)
>  		if (sc->txqs[i].setup)
> @@ -2761,7 +2760,7 @@ ath5k_tasklet_calibrate(unsigned long data)
>  		 * to load new gain values.
>  		 */
>  		ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "calibration, resetting\n");
> -		ath5k_reset_wake(sc);
> +		ath5k_reset(sc, sc->curchan);
>  	}
>  	if (ath5k_hw_phy_calibrate(ah, sc->curchan))
>  		ATH5K_ERR(sc, "calibration of channel %u failed\n",

for whatever it's worth :-)

Acked-by: Bruno Randolf <br1@einfach.org>

[PATCH 3/4] ath5k: fix race condition in tx desc processing

[Bob Copeland]

As pointed out by Benoit Papillault, there is a potential
race condition between the host and the hardware in reading
the next link in the transmit descriptor list:

cpu0              hw
                  tx for buf completed
                  raise tx_ok interrupt
process buf
buf->ds_link = 0
                  read buf->ds_link

This change checks txdp before processing a descriptor
(if there are any subsequent descriptors) to see if
hardware moved on.  We'll then process this descriptor on
the next tasklet.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 drivers/net/wireless/ath/ath5k/base.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 1e10439..f90c679 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -2080,6 +2080,17 @@ ath5k_tx_processq(struct ath5k_softc *sc, struct ath5k_txq *txq)
 	list_for_each_entry_safe(bf, bf0, &txq->q, list) {
 		ds = bf->desc;
 
+		/*
+		 * It's possible that the hardware can say the buffer is
+		 * completed when it hasn't yet loaded the ds_link from
+		 * host memory and moved on.  If there are more TX
+		 * descriptors in the queue, wait for TXDP to change
+		 * before processing this one.
+		 */
+		if (ath5k_hw_get_txdp(sc->ah, txq->qnum) == bf->daddr &&
+		    !list_is_last(&bf->list, &txq->q))
+			break;
+
 		ret = sc->ah->ah_proc_tx_desc(sc->ah, ds, &ts);
 		if (unlikely(ret == -EINPROGRESS))
 			break;
-- 
1.6.3.3

Re: [PATCH 3/4] ath5k: fix race condition in tx desc processing

[Bruno Randolf]

On Thursday 08 April 2010 12:55:58 Bob Copeland wrote:
> As pointed out by Benoit Papillault, there is a potential
> race condition between the host and the hardware in reading
> the next link in the transmit descriptor list:
> 
> cpu0              hw
>                   tx for buf completed
>                   raise tx_ok interrupt
> process buf
> buf->ds_link = 0
>                   read buf->ds_link
> 
> This change checks txdp before processing a descriptor
> (if there are any subsequent descriptors) to see if
> hardware moved on.  We'll then process this descriptor on
> the next tasklet.
> 
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
>  drivers/net/wireless/ath/ath5k/base.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath5k/base.c
> b/drivers/net/wireless/ath/ath5k/base.c index 1e10439..f90c679 100644
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -2080,6 +2080,17 @@ ath5k_tx_processq(struct ath5k_softc *sc, struct
> ath5k_txq *txq) list_for_each_entry_safe(bf, bf0, &txq->q, list) {
>  		ds = bf->desc;
> 
> +		/*
> +		 * It's possible that the hardware can say the buffer is
> +		 * completed when it hasn't yet loaded the ds_link from
> +		 * host memory and moved on.  If there are more TX
> +		 * descriptors in the queue, wait for TXDP to change
> +		 * before processing this one.
> +		 */
> +		if (ath5k_hw_get_txdp(sc->ah, txq->qnum) == bf->daddr &&
> +		    !list_is_last(&bf->list, &txq->q))
> +			break;
> +
>  		ret = sc->ah->ah_proc_tx_desc(sc->ah, ds, &ts);
>  		if (unlikely(ret == -EINPROGRESS))
>  			break;

for whatever it's worth :-)

Acked-by: Bruno Randolf <br1@einfach.org>

[PATCH 4/4] ath5k: add bounds check to pdadc table

[Bob Copeland]

We check the bounds on pdadc once when correcting for
negative curves but not when we later copy values from
from the pdadc_tmp array, leading to a potential overrun.

Although we shouldn't hit this case in practice, let's
be consistent.

Reported-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 drivers/net/wireless/ath/ath5k/phy.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
index 81bdebd..65ac50b 100644
--- a/drivers/net/wireless/ath/ath5k/phy.c
+++ b/drivers/net/wireless/ath/ath5k/phy.c
@@ -2560,7 +2560,7 @@ ath5k_combine_pwr_to_pdadc_curves(struct ath5k_hw *ah,
 		max_idx = (pdadc_n < table_size) ? pdadc_n : table_size;
 
 		/* Fill pdadc_out table */
-		while (pdadc_0 < max_idx)
+		while (pdadc_0 < max_idx && pdadc_i < 128)
 			pdadc_out[pdadc_i++] = pdadc_tmp[pdadc_0++];
 
 		/* Need to extrapolate above this pdgain? */
-- 
1.6.3.3

Re: [PATCH 4/4] ath5k: add bounds check to pdadc table

[Bruno Randolf]

On Thursday 08 April 2010 12:55:59 Bob Copeland wrote:
> We check the bounds on pdadc once when correcting for
> negative curves but not when we later copy values from
> from the pdadc_tmp array, leading to a potential overrun.
> 
> Although we shouldn't hit this case in practice, let's
> be consistent.
> 
> Reported-by: Dan Carpenter <error27@gmail.com>
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
>  drivers/net/wireless/ath/ath5k/phy.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath5k/phy.c
> b/drivers/net/wireless/ath/ath5k/phy.c index 81bdebd..65ac50b 100644
> --- a/drivers/net/wireless/ath/ath5k/phy.c
> +++ b/drivers/net/wireless/ath/ath5k/phy.c
> @@ -2560,7 +2560,7 @@ ath5k_combine_pwr_to_pdadc_curves(struct ath5k_hw
> *ah, max_idx = (pdadc_n < table_size) ? pdadc_n : table_size;
> 
>  		/* Fill pdadc_out table */
> -		while (pdadc_0 < max_idx)
> +		while (pdadc_0 < max_idx && pdadc_i < 128)
>  			pdadc_out[pdadc_i++] = pdadc_tmp[pdadc_0++];
> 
>  		/* Need to extrapolate above this pdgain? */

for whatever it's worth :-)

Acked-by: Bruno Randolf <br1@einfach.org>