2013-04-07 22:04:18

by Felix Fietkau

[permalink] [raw]
Subject: [PATCH 1/7] ath9k_hw: clean up RF Bank6 handling on AR5416/AR91xx

There are two sets of initvals for this RF bank, one with TPC support and
one without.
The TPC one always gets used, so remove the other one to avoid confusion.

Signed-off-by: Felix Fietkau <[email protected]>
---
drivers/net/wireless/ath/ath9k/ar5008_phy.c | 31 ++++++++++-------------------
drivers/net/wireless/ath/ath9k/ar9002_hw.c | 11 +++-------
drivers/net/wireless/ath/ath9k/hw.h | 3 ---
3 files changed, 13 insertions(+), 32 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar5008_phy.c b/drivers/net/wireless/ath/ath9k/ar5008_phy.c
index fd69376..93f8f96 100644
--- a/drivers/net/wireless/ath/ath9k/ar5008_phy.c
+++ b/drivers/net/wireless/ath/ath9k/ar5008_phy.c
@@ -485,9 +485,7 @@ static int ar5008_hw_rf_alloc_ext_banks(struct ath_hw *ah)
ATH_ALLOC_BANK(ah->analogBank2Data, ah->iniBank2.ia_rows);
ATH_ALLOC_BANK(ah->analogBank3Data, ah->iniBank3.ia_rows);
ATH_ALLOC_BANK(ah->analogBank6Data, ah->iniBank6.ia_rows);
- ATH_ALLOC_BANK(ah->analogBank6TPCData, ah->iniBank6TPC.ia_rows);
ATH_ALLOC_BANK(ah->analogBank7Data, ah->iniBank7.ia_rows);
- ATH_ALLOC_BANK(ah->bank6Temp, ah->iniBank6.ia_rows);

return 0;
#undef ATH_ALLOC_BANK
@@ -517,6 +515,7 @@ static bool ar5008_hw_set_rf_regs(struct ath_hw *ah,
u32 ob5GHz = 0, db5GHz = 0;
u32 ob2GHz = 0, db2GHz = 0;
int regWrites = 0;
+ int i;

/*
* Software does not need to program bank data
@@ -541,13 +540,9 @@ static bool ar5008_hw_set_rf_regs(struct ath_hw *ah,
/* Setup Bank 6 Write */
ar5008_rf_bank_setup(ah->analogBank3Data, &ah->iniBank3,
modesIndex);
- {
- int i;
- for (i = 0; i < ah->iniBank6TPC.ia_rows; i++) {
- ah->analogBank6Data[i] =
- INI_RA(&ah->iniBank6TPC, i, modesIndex);
- }
- }
+
+ for (i = 0; i < ah->iniBank6.ia_rows; i++)
+ ah->analogBank6Data[i] = INI_RA(&ah->iniBank6, i, modesIndex);

/* Only the 5 or 2 GHz OB/DB need to be set for a mode */
if (eepMinorRev >= 2) {
@@ -572,18 +567,12 @@ static bool ar5008_hw_set_rf_regs(struct ath_hw *ah,
ar5008_rf_bank_setup(ah->analogBank7Data, &ah->iniBank7, 1);

/* Write Analog registers */
- REG_WRITE_RF_ARRAY(&ah->iniBank0, ah->analogBank0Data,
- regWrites);
- REG_WRITE_RF_ARRAY(&ah->iniBank1, ah->analogBank1Data,
- regWrites);
- REG_WRITE_RF_ARRAY(&ah->iniBank2, ah->analogBank2Data,
- regWrites);
- REG_WRITE_RF_ARRAY(&ah->iniBank3, ah->analogBank3Data,
- regWrites);
- REG_WRITE_RF_ARRAY(&ah->iniBank6TPC, ah->analogBank6Data,
- regWrites);
- REG_WRITE_RF_ARRAY(&ah->iniBank7, ah->analogBank7Data,
- regWrites);
+ REG_WRITE_RF_ARRAY(&ah->iniBank0, ah->analogBank0Data, regWrites);
+ REG_WRITE_RF_ARRAY(&ah->iniBank1, ah->analogBank1Data, regWrites);
+ REG_WRITE_RF_ARRAY(&ah->iniBank2, ah->analogBank2Data, regWrites);
+ REG_WRITE_RF_ARRAY(&ah->iniBank3, ah->analogBank3Data, regWrites);
+ REG_WRITE_RF_ARRAY(&ah->iniBank6, ah->analogBank6Data, regWrites);
+ REG_WRITE_RF_ARRAY(&ah->iniBank7, ah->analogBank7Data, regWrites);

return true;
}
diff --git a/drivers/net/wireless/ath/ath9k/ar9002_hw.c b/drivers/net/wireless/ath/ath9k/ar9002_hw.c
index f053d97..a4654d3 100644
--- a/drivers/net/wireless/ath/ath9k/ar9002_hw.c
+++ b/drivers/net/wireless/ath/ath9k/ar9002_hw.c
@@ -67,12 +67,10 @@ static int ar9002_hw_init_mode_regs(struct ath_hw *ah)
} else if (AR_SREV_9100_OR_LATER(ah)) {
INIT_INI_ARRAY(&ah->iniModes, ar5416Modes_9100);
INIT_INI_ARRAY(&ah->iniCommon, ar5416Common_9100);
- INIT_INI_ARRAY(&ah->iniBank6, ar5416Bank6_9100);
INIT_INI_ARRAY(&ah->iniAddac, ar5416Addac_9100);
} else {
INIT_INI_ARRAY(&ah->iniModes, ar5416Modes);
INIT_INI_ARRAY(&ah->iniCommon, ar5416Common);
- INIT_INI_ARRAY(&ah->iniBank6TPC, ar5416Bank6TPC);
INIT_INI_ARRAY(&ah->iniAddac, ar5416Addac);
}

@@ -86,14 +84,11 @@ static int ar9002_hw_init_mode_regs(struct ath_hw *ah)
INIT_INI_ARRAY(&ah->iniBank3, ar5416Bank3);
INIT_INI_ARRAY(&ah->iniBank7, ar5416Bank7);

- /* Common for AR5416, AR9160 */
- if (!AR_SREV_9100(ah))
- INIT_INI_ARRAY(&ah->iniBank6, ar5416Bank6);
-
/* Common for AR913x, AR9160 */
if (!AR_SREV_5416(ah))
- INIT_INI_ARRAY(&ah->iniBank6TPC,
- ar5416Bank6TPC_9100);
+ INIT_INI_ARRAY(&ah->iniBank6, ar5416Bank6TPC_9100);
+ else
+ INIT_INI_ARRAY(&ah->iniBank6, ar5416Bank6TPC);
}

/* iniAddac needs to be modified for these chips */
diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index 30e62d9..e463c9a 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -852,9 +852,7 @@ struct ath_hw {
u32 *analogBank2Data;
u32 *analogBank3Data;
u32 *analogBank6Data;
- u32 *analogBank6TPCData;
u32 *analogBank7Data;
- u32 *bank6Temp;

int coverage_class;
u32 slottime;
@@ -891,7 +889,6 @@ struct ath_hw {
struct ar5416IniArray iniBank2;
struct ar5416IniArray iniBank3;
struct ar5416IniArray iniBank6;
- struct ar5416IniArray iniBank6TPC;
struct ar5416IniArray iniBank7;
struct ar5416IniArray iniAddac;
struct ar5416IniArray iniPcieSerdes;
--
1.8.0.2



2013-04-07 22:04:19

by Felix Fietkau

[permalink] [raw]
Subject: [PATCH 5/7] ath9k: fix handling of broken descriptors

As the comment in ath_get_next_rx_buf indicates, if a descriptor with
the done bit set follows one with the done bit cleared, both descriptors
should be discarded, however the driver is not doing that yet.

To fix this, use the rs->rs_more flag as an indicator that the following
frame should be discarded. This also helps with the split buffer case:
if the first part of the frame is discarded, the following parts need to
be discarded as well, since they contain no valid header or usable data.

Signed-off-by: Felix Fietkau <[email protected]>
---
drivers/net/wireless/ath/ath9k/ath9k.h | 1 +
drivers/net/wireless/ath/ath9k/recv.c | 24 +++++++++++++++++++-----
2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index a56b241..86d3572 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -311,6 +311,7 @@ struct ath_rx_edma {
struct ath_rx {
u8 defant;
u8 rxotherant;
+ bool discard_next;
u32 *rxlink;
u32 num_pkts;
unsigned int rxfilter;
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 1bc8030..0c6dc28 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -723,6 +723,13 @@ static struct ath_buf *ath_get_next_rx_buf(struct ath_softc *sc,
ret = ath9k_hw_rxprocdesc(ah, tds, &trs);
if (ret == -EINPROGRESS)
return NULL;
+
+ /*
+ * mark descriptor as zero-length and set the 'more'
+ * flag to ensure that both buffers get discarded
+ */
+ rs->rs_datalen = 0;
+ rs->rs_more = true;
}

list_del(&bf->list);
@@ -929,14 +936,20 @@ static void ath9k_process_rssi(struct ath_common *common,
* up the frame up to let mac80211 handle the actual error case, be it no
* decryption key or real decryption error. This let us keep statistics there.
*/
-static int ath9k_rx_skb_preprocess(struct ath_common *common,
- struct ieee80211_hw *hw,
+static int ath9k_rx_skb_preprocess(struct ath_softc *sc,
struct ieee80211_hdr *hdr,
struct ath_rx_status *rx_stats,
struct ieee80211_rx_status *rx_status,
bool *decrypt_error)
{
- struct ath_hw *ah = common->ah;
+ struct ieee80211_hw *hw = sc->hw;
+ struct ath_hw *ah = sc->sc_ah;
+ struct ath_common *common = ath9k_hw_common(ah);
+ bool discard_current = sc->rx.discard_next;
+
+ sc->rx.discard_next = rx_stats->rs_more;
+ if (discard_current)
+ return -EINVAL;

/*
* everything but the rate is checked here, the rate check is done
@@ -962,6 +975,7 @@ static int ath9k_rx_skb_preprocess(struct ath_common *common,
if (rx_stats->rs_moreaggr)
rx_status->flag |= RX_FLAG_NO_SIGNAL_VAL;

+ sc->rx.discard_next = false;
return 0;
}

@@ -1236,8 +1250,8 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
}
}

- retval = ath9k_rx_skb_preprocess(common, hw, hdr, &rs,
- rxs, &decrypt_error);
+ retval = ath9k_rx_skb_preprocess(sc, hdr, &rs, rxs,
+ &decrypt_error);
if (retval)
goto requeue_drop_frag;

--
1.8.0.2


2013-04-07 22:04:18

by Felix Fietkau

[permalink] [raw]
Subject: [PATCH 7/7] ath9k: implement buffer holding handling for EDMA FIFO

Inside one FIFO slot queue, EDMA chipsets have the same link pointer
re-read race condition as older chipsets, so the same buffer holding
logic needs to be used in order to avoid use-after-free bugs.
Unlike on older chips, it can be skipped for the end of the queue.

Signed-off-by: Felix Fietkau <[email protected]>
---
drivers/net/wireless/ath/ath9k/xmit.c | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 94b2ee1..5bc5802 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -516,8 +516,7 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
* not a holding desc.
*/
INIT_LIST_HEAD(&bf_head);
- if ((sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_EDMA) ||
- bf_next != NULL || !bf_last->bf_stale)
+ if (bf_next != NULL || !bf_last->bf_stale)
list_move_tail(&bf->list, &bf_head);

if (!txpending || (tid->state & AGGR_CLEANUP)) {
@@ -537,8 +536,7 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
!txfail);
} else {
/* retry the un-acked ones */
- if (!(sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_EDMA) &&
- bf->bf_next == NULL && bf_last->bf_stale) {
+ if (bf->bf_next == NULL && bf_last->bf_stale) {
struct ath_buf *tbf;

tbf = ath_clone_txbuf(sc, bf_last);
@@ -2264,6 +2262,7 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
struct ath_txq *txq;
struct ath_buf *bf, *lastbf;
struct list_head bf_head;
+ struct list_head *fifo_list;
int status;

for (;;) {
@@ -2291,20 +2290,24 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)

TX_STAT_INC(txq->axq_qnum, txprocdesc);

- if (list_empty(&txq->txq_fifo[txq->txq_tailidx])) {
+ fifo_list = &txq->txq_fifo[txq->txq_tailidx];
+ if (list_empty(fifo_list)) {
ath_txq_unlock(sc, txq);
return;
}

- bf = list_first_entry(&txq->txq_fifo[txq->txq_tailidx],
- struct ath_buf, list);
+ bf = list_first_entry(fifo_list, struct ath_buf, list);
+ if (bf->bf_stale) {
+ list_del(&bf->list);
+ ath_tx_return_buffer(sc, bf);
+ bf = list_first_entry(fifo_list, struct ath_buf, list);
+ }
+
lastbf = bf->bf_lastbf;

INIT_LIST_HEAD(&bf_head);
- list_cut_position(&bf_head, &txq->txq_fifo[txq->txq_tailidx],
- &lastbf->list);
-
- if (list_empty(&txq->txq_fifo[txq->txq_tailidx])) {
+ if (list_is_last(&lastbf->list, fifo_list)) {
+ list_splice_tail_init(fifo_list, &bf_head);
INCR(txq->txq_tailidx, ATH_TXFIFO_DEPTH);

if (!list_empty(&txq->axq_q)) {
@@ -2315,6 +2318,11 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
list_splice_tail_init(&txq->axq_q, &bf_q);
ath_tx_txqaddbuf(sc, txq, &bf_q, true);
}
+ } else {
+ lastbf->bf_stale = true;
+ if (bf != lastbf)
+ list_cut_position(&bf_head, fifo_list,
+ lastbf->list.prev);
}

ath_tx_process_buffer(sc, txq, &ts, bf, &bf_head);
--
1.8.0.2


2013-04-07 22:34:36

by Felix Fietkau

[permalink] [raw]
Subject: Re: [PATCH 5/7] ath9k: fix handling of broken descriptors

On 2013-04-08 12:24 AM, Adrian Chadd wrote:
> Hm!
>
> On 7 April 2013 15:04, Felix Fietkau <[email protected]> wrote:
>> As the comment in ath_get_next_rx_buf indicates, if a descriptor with
>> the done bit set follows one with the done bit cleared, both descriptors
>> should be discarded, however the driver is not doing that yet.
>>
>> To fix this, use the rs->rs_more flag as an indicator that the following
>> frame should be discarded. This also helps with the split buffer case:
>> if the first part of the frame is discarded, the following parts need to
>> be discarded as well, since they contain no valid header or usable data.
>
> Have you seen this happen?
>
> I've added code to this in FreeBSD (based on the reference driver and
> ath9k doing it) and I've never, ever seen it trigger.
>
> I went digging through the EV database and I found a bug (long since
> fixed) where the code was adjusting the skb size and then pushing it
> back onto the RX queue with an incorrect size. If it allocated a 2kiB
> SKU and then reset the size to 4KiB, hilarity would ensue.
>
> This is why I'm asking. If it happens in the real world, fine. :-)
I didn't see the done bit related corruption, but I fixed the code so
that I could reuse it for the patch after it (which is based on real
world observations).

- Felix

2013-04-07 22:04:18

by Felix Fietkau

[permalink] [raw]
Subject: [PATCH 6/7] ath9k: detect more kinds of invalid descriptors

If AR_CRCErr, AR_PHYErr, AR_DecryptCRCErr or AR_MichaelErr is indicated
in the rx status word, but AR_RxFrameOK is also set, the descriptor
contents are typically invalid. This can show up as a warning about
invalid MCS rates in a frame. Even with those checks in place, a
descriptor with invalid MCS rates can still sometimes make it through to
the driver (mostly on older hardware like AR91xx).

Detect such errors in the last descriptor of a frame and discard the
whole frame if present.

Signed-off-by: Felix Fietkau <[email protected]>
---
drivers/net/wireless/ath/ath9k/mac.c | 8 ++++++++
drivers/net/wireless/ath/ath9k/mac.h | 1 +
drivers/net/wireless/ath/ath9k/recv.c | 2 ++
3 files changed, 11 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c
index 811007e..498fee0 100644
--- a/drivers/net/wireless/ath/ath9k/mac.c
+++ b/drivers/net/wireless/ath/ath9k/mac.c
@@ -615,6 +615,14 @@ int ath9k_hw_rxprocdesc(struct ath_hw *ah, struct ath_desc *ds,
rs->rs_status |= ATH9K_RXERR_DECRYPT;
else if (ads.ds_rxstatus8 & AR_MichaelErr)
rs->rs_status |= ATH9K_RXERR_MIC;
+ } else {
+ if (ads.ds_rxstatus8 &
+ (AR_CRCErr | AR_PHYErr | AR_DecryptCRCErr | AR_MichaelErr))
+ rs->rs_status |= ATH9K_RXERR_CORRUPT_DESC;
+
+ /* Only up to MCS16 supported, everything above is invalid */
+ if (rs->rs_rate >= 0x90)
+ rs->rs_status |= ATH9K_RXERR_CORRUPT_DESC;
}

if (ads.ds_rxstatus8 & AR_KeyMiss)
diff --git a/drivers/net/wireless/ath/ath9k/mac.h b/drivers/net/wireless/ath/ath9k/mac.h
index 1ff8170..5865f92 100644
--- a/drivers/net/wireless/ath/ath9k/mac.h
+++ b/drivers/net/wireless/ath/ath9k/mac.h
@@ -183,6 +183,7 @@ struct ath_htc_rx_status {
#define ATH9K_RXERR_DECRYPT 0x08
#define ATH9K_RXERR_MIC 0x10
#define ATH9K_RXERR_KEYMISS 0x20
+#define ATH9K_RXERR_CORRUPT_DESC 0x40

#define ATH9K_RX_MORE 0x01
#define ATH9K_RX_MORE_AGGR 0x02
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 0c6dc28..703c82c 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -1312,6 +1312,8 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
sc->rx.frag = skb;
goto requeue;
}
+ if (rs.rs_status & ATH9K_RXERR_CORRUPT_DESC)
+ goto requeue_drop_frag;

if (sc->rx.frag) {
int space = skb->len - skb_tailroom(hdr_skb);
--
1.8.0.2


2013-04-07 22:04:20

by Felix Fietkau

[permalink] [raw]
Subject: [PATCH 2/7] ath9k_hw: make various ar5416/ar91xx rf banks const

Banks 0-3,7 are neither modified at run time, nor SREV dependent.

Signed-off-by: Felix Fietkau <[email protected]>
---
drivers/net/wireless/ath/ath9k/ar5008_phy.c | 75 ++++++++---------------------
drivers/net/wireless/ath/ath9k/ar9002_hw.c | 6 ---
drivers/net/wireless/ath/ath9k/calib.h | 6 +++
drivers/net/wireless/ath/ath9k/hw.h | 10 ----
4 files changed, 26 insertions(+), 71 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar5008_phy.c b/drivers/net/wireless/ath/ath9k/ar5008_phy.c
index 93f8f96..391da5a 100644
--- a/drivers/net/wireless/ath/ath9k/ar5008_phy.c
+++ b/drivers/net/wireless/ath/ath9k/ar5008_phy.c
@@ -18,6 +18,7 @@
#include "hw-ops.h"
#include "../regd.h"
#include "ar9002_phy.h"
+#include "ar5008_initvals.h"

/* All code below is for AR5008, AR9001, AR9002 */

@@ -43,23 +44,16 @@ static const int m2ThreshLowExt_off = 127;
static const int m1ThreshExt_off = 127;
static const int m2ThreshExt_off = 127;

+static const struct ar5416IniArray bank0 = STATIC_INI_ARRAY(ar5416Bank0);
+static const struct ar5416IniArray bank1 = STATIC_INI_ARRAY(ar5416Bank1);
+static const struct ar5416IniArray bank2 = STATIC_INI_ARRAY(ar5416Bank2);
+static const struct ar5416IniArray bank3 = STATIC_INI_ARRAY(ar5416Bank3);
+static const struct ar5416IniArray bank7 = STATIC_INI_ARRAY(ar5416Bank7);

-static void ar5008_rf_bank_setup(u32 *bank, struct ar5416IniArray *array,
- int col)
-{
- int i;
-
- for (i = 0; i < array->ia_rows; i++)
- bank[i] = INI_RA(array, i, col);
-}
-
-
-#define REG_WRITE_RF_ARRAY(iniarray, regData, regWr) \
- ar5008_write_rf_array(ah, iniarray, regData, &(regWr))
-
-static void ar5008_write_rf_array(struct ath_hw *ah, struct ar5416IniArray *array,
- u32 *data, unsigned int *writecnt)
+static void ar5008_write_bank6(struct ath_hw *ah, unsigned int *writecnt)
{
+ struct ar5416IniArray *array = &ah->iniBank6;
+ u32 *data = ah->analogBank6Data;
int r;

ENABLE_REGWRITE_BUFFER(ah);
@@ -165,7 +159,7 @@ static void ar5008_hw_force_bias(struct ath_hw *ah, u16 synth_freq)
ar5008_hw_phy_modify_rx_buffer(ah->analogBank6Data, tmp_reg, 3, 181, 3);

/* write Bank 6 with new params */
- REG_WRITE_RF_ARRAY(&ah->iniBank6, ah->analogBank6Data, reg_writes);
+ ar5008_write_bank6(ah, &reg_writes);
}

/**
@@ -469,29 +463,16 @@ static void ar5008_hw_spur_mitigate(struct ath_hw *ah,
*/
static int ar5008_hw_rf_alloc_ext_banks(struct ath_hw *ah)
{
-#define ATH_ALLOC_BANK(bank, size) do { \
- bank = devm_kzalloc(ah->dev, sizeof(u32) * size, GFP_KERNEL); \
- if (!bank) \
- goto error; \
- } while (0);
-
- struct ath_common *common = ath9k_hw_common(ah);
+ int size = ah->iniBank6.ia_rows * sizeof(u32);

if (AR_SREV_9280_20_OR_LATER(ah))
return 0;

- ATH_ALLOC_BANK(ah->analogBank0Data, ah->iniBank0.ia_rows);
- ATH_ALLOC_BANK(ah->analogBank1Data, ah->iniBank1.ia_rows);
- ATH_ALLOC_BANK(ah->analogBank2Data, ah->iniBank2.ia_rows);
- ATH_ALLOC_BANK(ah->analogBank3Data, ah->iniBank3.ia_rows);
- ATH_ALLOC_BANK(ah->analogBank6Data, ah->iniBank6.ia_rows);
- ATH_ALLOC_BANK(ah->analogBank7Data, ah->iniBank7.ia_rows);
+ ah->analogBank6Data = devm_kzalloc(ah->dev, size, GFP_KERNEL);
+ if (!ah->analogBank6Data)
+ return -ENOMEM;

return 0;
-#undef ATH_ALLOC_BANK
-error:
- ath_err(common, "Cannot allocate RF banks\n");
- return -ENOMEM;
}


@@ -528,19 +509,6 @@ static bool ar5008_hw_set_rf_regs(struct ath_hw *ah,
/* Setup rf parameters */
eepMinorRev = ah->eep_ops->get_eeprom(ah, EEP_MINOR_REV);

- /* Setup Bank 0 Write */
- ar5008_rf_bank_setup(ah->analogBank0Data, &ah->iniBank0, 1);
-
- /* Setup Bank 1 Write */
- ar5008_rf_bank_setup(ah->analogBank1Data, &ah->iniBank1, 1);
-
- /* Setup Bank 2 Write */
- ar5008_rf_bank_setup(ah->analogBank2Data, &ah->iniBank2, 1);
-
- /* Setup Bank 6 Write */
- ar5008_rf_bank_setup(ah->analogBank3Data, &ah->iniBank3,
- modesIndex);
-
for (i = 0; i < ah->iniBank6.ia_rows; i++)
ah->analogBank6Data[i] = INI_RA(&ah->iniBank6, i, modesIndex);

@@ -563,16 +531,13 @@ static bool ar5008_hw_set_rf_regs(struct ath_hw *ah,
}
}

- /* Setup Bank 7 Setup */
- ar5008_rf_bank_setup(ah->analogBank7Data, &ah->iniBank7, 1);
-
/* Write Analog registers */
- REG_WRITE_RF_ARRAY(&ah->iniBank0, ah->analogBank0Data, regWrites);
- REG_WRITE_RF_ARRAY(&ah->iniBank1, ah->analogBank1Data, regWrites);
- REG_WRITE_RF_ARRAY(&ah->iniBank2, ah->analogBank2Data, regWrites);
- REG_WRITE_RF_ARRAY(&ah->iniBank3, ah->analogBank3Data, regWrites);
- REG_WRITE_RF_ARRAY(&ah->iniBank6, ah->analogBank6Data, regWrites);
- REG_WRITE_RF_ARRAY(&ah->iniBank7, ah->analogBank7Data, regWrites);
+ REG_WRITE_ARRAY(&bank0, 1, regWrites);
+ REG_WRITE_ARRAY(&bank1, 1, regWrites);
+ REG_WRITE_ARRAY(&bank2, 1, regWrites);
+ REG_WRITE_ARRAY(&bank3, modesIndex, regWrites);
+ ar5008_write_bank6(ah, &regWrites);
+ REG_WRITE_ARRAY(&bank7, 1, regWrites);

return true;
}
diff --git a/drivers/net/wireless/ath/ath9k/ar9002_hw.c b/drivers/net/wireless/ath/ath9k/ar9002_hw.c
index a4654d3..830daa1 100644
--- a/drivers/net/wireless/ath/ath9k/ar9002_hw.c
+++ b/drivers/net/wireless/ath/ath9k/ar9002_hw.c
@@ -78,12 +78,6 @@ static int ar9002_hw_init_mode_regs(struct ath_hw *ah)
/* Common for AR5416, AR913x, AR9160 */
INIT_INI_ARRAY(&ah->iniBB_RfGain, ar5416BB_RfGain);

- INIT_INI_ARRAY(&ah->iniBank0, ar5416Bank0);
- INIT_INI_ARRAY(&ah->iniBank1, ar5416Bank1);
- INIT_INI_ARRAY(&ah->iniBank2, ar5416Bank2);
- INIT_INI_ARRAY(&ah->iniBank3, ar5416Bank3);
- INIT_INI_ARRAY(&ah->iniBank7, ar5416Bank7);
-
/* Common for AR913x, AR9160 */
if (!AR_SREV_5416(ah))
INIT_INI_ARRAY(&ah->iniBank6, ar5416Bank6TPC_9100);
diff --git a/drivers/net/wireless/ath/ath9k/calib.h b/drivers/net/wireless/ath/ath9k/calib.h
index 60dcb6c..3d70b8c 100644
--- a/drivers/net/wireless/ath/ath9k/calib.h
+++ b/drivers/net/wireless/ath/ath9k/calib.h
@@ -33,6 +33,12 @@ struct ar5416IniArray {
u32 ia_columns;
};

+#define STATIC_INI_ARRAY(array) { \
+ .ia_array = (u32 *)(array), \
+ .ia_rows = ARRAY_SIZE(array), \
+ .ia_columns = ARRAY_SIZE(array[0]), \
+ }
+
#define INIT_INI_ARRAY(iniarray, array) do { \
(iniarray)->ia_array = (u32 *)(array); \
(iniarray)->ia_rows = ARRAY_SIZE(array); \
diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index e463c9a..ae30343 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -847,12 +847,7 @@ struct ath_hw {
struct ath_hw_ops ops;

/* Used to program the radio on non single-chip devices */
- u32 *analogBank0Data;
- u32 *analogBank1Data;
- u32 *analogBank2Data;
- u32 *analogBank3Data;
u32 *analogBank6Data;
- u32 *analogBank7Data;

int coverage_class;
u32 slottime;
@@ -883,13 +878,8 @@ struct ath_hw {

struct ar5416IniArray iniModes;
struct ar5416IniArray iniCommon;
- struct ar5416IniArray iniBank0;
struct ar5416IniArray iniBB_RfGain;
- struct ar5416IniArray iniBank1;
- struct ar5416IniArray iniBank2;
- struct ar5416IniArray iniBank3;
struct ar5416IniArray iniBank6;
- struct ar5416IniArray iniBank7;
struct ar5416IniArray iniAddac;
struct ar5416IniArray iniPcieSerdes;
#ifdef CONFIG_PM_SLEEP
--
1.8.0.2


2013-04-07 22:04:24

by Felix Fietkau

[permalink] [raw]
Subject: [PATCH 3/7] ath9k_common: remove ath9k_cmn_padpos

It is equivalent to ieee80211_hdrlen

Signed-off-by: Felix Fietkau <[email protected]>
---
drivers/net/wireless/ath/ath9k/common.c | 14 --------------
drivers/net/wireless/ath/ath9k/common.h | 1 -
drivers/net/wireless/ath/ath9k/htc_drv_beacon.c | 2 +-
drivers/net/wireless/ath/ath9k/htc_drv_main.c | 2 +-
drivers/net/wireless/ath/ath9k/htc_drv_txrx.c | 10 ++++------
drivers/net/wireless/ath/ath9k/recv.c | 2 +-
drivers/net/wireless/ath/ath9k/xmit.c | 4 ++--
7 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/common.c b/drivers/net/wireless/ath/ath9k/common.c
index 905f1b3..27f1eaf 100644
--- a/drivers/net/wireless/ath/ath9k/common.c
+++ b/drivers/net/wireless/ath/ath9k/common.c
@@ -27,20 +27,6 @@ MODULE_AUTHOR("Atheros Communications");
MODULE_DESCRIPTION("Shared library for Atheros wireless 802.11n LAN cards.");
MODULE_LICENSE("Dual BSD/GPL");

-int ath9k_cmn_padpos(__le16 frame_control)
-{
- int padpos = 24;
- if (ieee80211_has_a4(frame_control)) {
- padpos += ETH_ALEN;
- }
- if (ieee80211_is_data_qos(frame_control)) {
- padpos += IEEE80211_QOS_CTL_LEN;
- }
-
- return padpos;
-}
-EXPORT_SYMBOL(ath9k_cmn_padpos);
-
int ath9k_cmn_get_hw_crypto_keytype(struct sk_buff *skb)
{
struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
diff --git a/drivers/net/wireless/ath/ath9k/common.h b/drivers/net/wireless/ath/ath9k/common.h
index 6102476..207d069 100644
--- a/drivers/net/wireless/ath/ath9k/common.h
+++ b/drivers/net/wireless/ath/ath9k/common.h
@@ -42,7 +42,6 @@
#define ATH_EP_RND(x, mul) \
(((x) + ((mul)/2)) / (mul))

-int ath9k_cmn_padpos(__le16 frame_control);
int ath9k_cmn_get_hw_crypto_keytype(struct sk_buff *skb);
void ath9k_cmn_update_ichannel(struct ath9k_channel *ichan,
struct ieee80211_channel *chan,
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_beacon.c b/drivers/net/wireless/ath/ath9k/htc_drv_beacon.c
index d0ce1f5..f13f458 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_beacon.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_beacon.c
@@ -308,7 +308,7 @@ static void ath9k_htc_send_buffered(struct ath9k_htc_priv *priv,
while(skb) {
hdr = (struct ieee80211_hdr *) skb->data;

- padpos = ath9k_cmn_padpos(hdr->frame_control);
+ padpos = ieee80211_hdrlen(hdr->frame_control);
padsize = padpos & 3;
if (padsize && skb->len > padpos) {
if (skb_headroom(skb) < padsize) {
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
index a8016d7..3381939 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
@@ -866,7 +866,7 @@ static void ath9k_htc_tx(struct ieee80211_hw *hw,
hdr = (struct ieee80211_hdr *) skb->data;

/* Add the padding after the header if this is not already done */
- padpos = ath9k_cmn_padpos(hdr->frame_control);
+ padpos = ieee80211_hdrlen(hdr->frame_control);
padsize = padpos & 3;
if (padsize && skb->len > padpos) {
if (skb_headroom(skb) < padsize) {
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
index bd8251c..5c33a46 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
@@ -966,7 +966,7 @@ static bool ath9k_rx_prepare(struct ath9k_htc_priv *priv,
struct sk_buff *skb = rxbuf->skb;
struct ath_common *common = ath9k_hw_common(priv->ah);
struct ath_htc_rx_status *rxstatus;
- int hdrlen, padpos, padsize;
+ int hdrlen, padsize;
int last_rssi = ATH_RSSI_DUMMY_MARKER;
__le16 fc;

@@ -996,11 +996,9 @@ static bool ath9k_rx_prepare(struct ath9k_htc_priv *priv,
fc = hdr->frame_control;
hdrlen = ieee80211_get_hdrlen_from_skb(skb);

- padpos = ath9k_cmn_padpos(fc);
-
- padsize = padpos & 3;
- if (padsize && skb->len >= padpos+padsize+FCS_LEN) {
- memmove(skb->data + padsize, skb->data, padpos);
+ padsize = hdrlen & 3;
+ if (padsize && skb->len >= hdrlen+padsize+FCS_LEN) {
+ memmove(skb->data + padsize, skb->data, hdrlen);
skb_pull(skb, padsize);
}

diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index ee156e5..a617ef9 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -981,7 +981,7 @@ static void ath9k_rx_skb_postprocess(struct ath_common *common,
hdr = (struct ieee80211_hdr *) skb->data;
hdrlen = ieee80211_get_hdrlen_from_skb(skb);
fc = hdr->frame_control;
- padpos = ath9k_cmn_padpos(hdr->frame_control);
+ padpos = ieee80211_hdrlen(fc);

/* The MAC header is padded to have 32-bit boundary if the
* packet payload is non-zero. The general calculation for
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 89a6441..94b2ee1 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -1971,7 +1971,7 @@ int ath_tx_start(struct ieee80211_hw *hw, struct sk_buff *skb,
}

/* Add the padding after the header if this is not already done */
- padpos = ath9k_cmn_padpos(hdr->frame_control);
+ padpos = ieee80211_hdrlen(hdr->frame_control);
padsize = padpos & 3;
if (padsize && skb->len > padpos) {
if (skb_headroom(skb) < padsize)
@@ -2033,7 +2033,7 @@ static void ath_tx_complete(struct ath_softc *sc, struct sk_buff *skb,
/* Frame was ACKed */
tx_info->flags |= IEEE80211_TX_STAT_ACK;

- padpos = ath9k_cmn_padpos(hdr->frame_control);
+ padpos = ieee80211_hdrlen(hdr->frame_control);
padsize = padpos & 3;
if (padsize && skb->len>padpos+padsize) {
/*
--
1.8.0.2


2013-04-07 22:04:19

by Felix Fietkau

[permalink] [raw]
Subject: [PATCH 4/7] ath9k: improve dma map failure handling

Instead of leaving the buffer without skb and breaking out of the loop
(which could leak the rx buffer), use the common error path.

Signed-off-by: Felix Fietkau <[email protected]>
---
drivers/net/wireless/ath/ath9k/recv.c | 30 +++++++++++++-----------------
1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index a617ef9..1bc8030 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -1162,6 +1162,7 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
u64 tsf = 0;
u32 tsf_lower = 0;
unsigned long flags;
+ dma_addr_t new_buf_addr;

if (edma)
dma_type = DMA_BIDIRECTIONAL;
@@ -1257,10 +1258,20 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
goto requeue_drop_frag;
}

+ /* We will now give hardware our shiny new allocated skb */
+ new_buf_addr = dma_map_single(sc->dev, requeue_skb->data,
+ common->rx_bufsize, dma_type);
+ if (unlikely(dma_mapping_error(sc->dev, new_buf_addr))) {
+ dev_kfree_skb_any(requeue_skb);
+ goto requeue_drop_frag;
+ }
+
+ bf->bf_mpdu = requeue_skb;
+ bf->bf_buf_addr = new_buf_addr;
+
/* Unmap the frame */
dma_unmap_single(sc->dev, bf->bf_buf_addr,
- common->rx_bufsize,
- dma_type);
+ common->rx_bufsize, dma_type);

skb_put(skb, rs.rs_datalen + ah->caps.rx_status_len);
if (ah->caps.rx_status_len)
@@ -1270,21 +1281,6 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
ath9k_rx_skb_postprocess(common, hdr_skb, &rs,
rxs, decrypt_error);

- /* We will now give hardware our shiny new allocated skb */
- bf->bf_mpdu = requeue_skb;
- bf->bf_buf_addr = dma_map_single(sc->dev, requeue_skb->data,
- common->rx_bufsize,
- dma_type);
- if (unlikely(dma_mapping_error(sc->dev,
- bf->bf_buf_addr))) {
- dev_kfree_skb_any(requeue_skb);
- bf->bf_mpdu = NULL;
- bf->bf_buf_addr = 0;
- ath_err(common, "dma_mapping_error() on RX\n");
- ieee80211_rx(hw, skb);
- break;
- }
-
if (rs.rs_more) {
RX_STAT_INC(rx_frags);
/*
--
1.8.0.2


2013-04-07 22:24:10

by Adrian Chadd

[permalink] [raw]
Subject: Re: [PATCH 5/7] ath9k: fix handling of broken descriptors

Hm!

On 7 April 2013 15:04, Felix Fietkau <[email protected]> wrote:
> As the comment in ath_get_next_rx_buf indicates, if a descriptor with
> the done bit set follows one with the done bit cleared, both descriptors
> should be discarded, however the driver is not doing that yet.
>
> To fix this, use the rs->rs_more flag as an indicator that the following
> frame should be discarded. This also helps with the split buffer case:
> if the first part of the frame is discarded, the following parts need to
> be discarded as well, since they contain no valid header or usable data.

Have you seen this happen?

I've added code to this in FreeBSD (based on the reference driver and
ath9k doing it) and I've never, ever seen it trigger.

I went digging through the EV database and I found a bug (long since
fixed) where the code was adjusting the skb size and then pushing it
back onto the RX queue with an incorrect size. If it allocated a 2kiB
SKU and then reset the size to 4KiB, hilarity would ensue.

This is why I'm asking. If it happens in the real world, fine. :-)

Thanks,


adrian

2013-04-07 22:21:51

by Adrian Chadd

[permalink] [raw]
Subject: Re: [PATCH 7/7] ath9k: implement buffer holding handling for EDMA FIFO

For what it's worth, I've verified this DMA behaviour with the MAC
team and tested a similar fix for FreeBSD's EDMA code.

(They were confused as to why were doing this in the first place,
until I pointed out how we do CABQ.)

I'm happy to provide more background into the issue if people care.

Thanks,



Adrian

On 7 April 2013 15:04, Felix Fietkau <[email protected]> wrote:
> Inside one FIFO slot queue, EDMA chipsets have the same link pointer
> re-read race condition as older chipsets, so the same buffer holding
> logic needs to be used in order to avoid use-after-free bugs.
> Unlike on older chips, it can be skipped for the end of the queue.
>
> Signed-off-by: Felix Fietkau <[email protected]>
> ---
> drivers/net/wireless/ath/ath9k/xmit.c | 30 +++++++++++++++++++-----------
> 1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> index 94b2ee1..5bc5802 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -516,8 +516,7 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
> * not a holding desc.
> */
> INIT_LIST_HEAD(&bf_head);
> - if ((sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_EDMA) ||
> - bf_next != NULL || !bf_last->bf_stale)
> + if (bf_next != NULL || !bf_last->bf_stale)
> list_move_tail(&bf->list, &bf_head);
>
> if (!txpending || (tid->state & AGGR_CLEANUP)) {
> @@ -537,8 +536,7 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
> !txfail);
> } else {
> /* retry the un-acked ones */
> - if (!(sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_EDMA) &&
> - bf->bf_next == NULL && bf_last->bf_stale) {
> + if (bf->bf_next == NULL && bf_last->bf_stale) {
> struct ath_buf *tbf;
>
> tbf = ath_clone_txbuf(sc, bf_last);
> @@ -2264,6 +2262,7 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
> struct ath_txq *txq;
> struct ath_buf *bf, *lastbf;
> struct list_head bf_head;
> + struct list_head *fifo_list;
> int status;
>
> for (;;) {
> @@ -2291,20 +2290,24 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
>
> TX_STAT_INC(txq->axq_qnum, txprocdesc);
>
> - if (list_empty(&txq->txq_fifo[txq->txq_tailidx])) {
> + fifo_list = &txq->txq_fifo[txq->txq_tailidx];
> + if (list_empty(fifo_list)) {
> ath_txq_unlock(sc, txq);
> return;
> }
>
> - bf = list_first_entry(&txq->txq_fifo[txq->txq_tailidx],
> - struct ath_buf, list);
> + bf = list_first_entry(fifo_list, struct ath_buf, list);
> + if (bf->bf_stale) {
> + list_del(&bf->list);
> + ath_tx_return_buffer(sc, bf);
> + bf = list_first_entry(fifo_list, struct ath_buf, list);
> + }
> +
> lastbf = bf->bf_lastbf;
>
> INIT_LIST_HEAD(&bf_head);
> - list_cut_position(&bf_head, &txq->txq_fifo[txq->txq_tailidx],
> - &lastbf->list);
> -
> - if (list_empty(&txq->txq_fifo[txq->txq_tailidx])) {
> + if (list_is_last(&lastbf->list, fifo_list)) {
> + list_splice_tail_init(fifo_list, &bf_head);
> INCR(txq->txq_tailidx, ATH_TXFIFO_DEPTH);
>
> if (!list_empty(&txq->axq_q)) {
> @@ -2315,6 +2318,11 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
> list_splice_tail_init(&txq->axq_q, &bf_q);
> ath_tx_txqaddbuf(sc, txq, &bf_q, true);
> }
> + } else {
> + lastbf->bf_stale = true;
> + if (bf != lastbf)
> + list_cut_position(&bf_head, fifo_list,
> + lastbf->list.prev);
> }
>
> ath_tx_process_buffer(sc, txq, &ts, bf, &bf_head);
> --
> 1.8.0.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html