Here are various fixes for ath5k. Patch 1 is the result of a script to
compare our initval tables against the ones in the open source HALs.
Patches 2 and 3 are self-explanatory. Patches 4 and 5 may help make a
use-after-free bug more rare (though it isn't fully solved - both need
some testing so are 2.6.31 material).
Bob Copeland (5):
ath5k: fix initvals errors
ath5k: use tasklet_hi_schedule for beacon queue
ath5k: use bool for modparams
ath5k: use rx hw descriptor pointer for self-linked check
ath5k: manipulate rxlink and descriptor address under rxbuf lock
drivers/net/wireless/ath/ath5k/base.c | 35 +++++++---------------------
drivers/net/wireless/ath/ath5k/base.h | 1 -
drivers/net/wireless/ath/ath5k/dma.c | 2 -
drivers/net/wireless/ath/ath5k/initvals.c | 8 ++----
4 files changed, 12 insertions(+), 34 deletions(-)
Hi Bob,
On Wed, Apr 15, 2009 at 12:57 PM, Bob Copeland <[email protected]> wro=
te:
> This patch simplifies the code used to detect when the
> self-linked DMA buffer is still in use by hardware, by
> checking the hardware's rxdp register instead of looking
> at the software buffer list.
Just an observation: Each synchronous read over PCI will take at
minimum 0.9us to return. Accessing tables in the host's cache
hierarchy/memory will likely be much faster - possibly why it is done
like this already.
Is this a worthwhile tradeoff?
Many thanks,
Daniel
> Signed-off-by: Bob Copeland <[email protected]>
> ---
> =A0drivers/net/wireless/ath/ath5k/base.c | =A0 24 ++++---------------=
-----
> =A0drivers/net/wireless/ath/ath5k/base.h | =A0 =A01 -
> =A0drivers/net/wireless/ath/ath5k/dma.c =A0| =A0 =A02 --
> =A03 files changed, 4 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wire=
less/ath/ath5k/base.c
> index 3bde18f..1a6e72f 100644
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -1780,7 +1780,7 @@ ath5k_tasklet_rx(unsigned long data)
> =A0 =A0 =A0 =A0struct sk_buff *skb, *next_skb;
> =A0 =A0 =A0 =A0dma_addr_t next_skb_addr;
> =A0 =A0 =A0 =A0struct ath5k_softc *sc =3D (void *)data;
> - =A0 =A0 =A0 struct ath5k_buf *bf, *bf_last;
> + =A0 =A0 =A0 struct ath5k_buf *bf;
> =A0 =A0 =A0 =A0struct ath5k_desc *ds;
> =A0 =A0 =A0 =A0int ret;
> =A0 =A0 =A0 =A0int hdrlen;
> @@ -1791,7 +1791,6 @@ ath5k_tasklet_rx(unsigned long data)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ATH5K_WARN(sc, "empty rx buf pool\n");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto unlock;
> =A0 =A0 =A0 =A0}
> - =A0 =A0 =A0 bf_last =3D list_entry(sc->rxbuf.prev, struct ath5k_buf=
, list);
> =A0 =A0 =A0 =A0do {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0rxs.flag =3D 0;
>
> @@ -1800,24 +1799,9 @@ ath5k_tasklet_rx(unsigned long data)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0skb =3D bf->skb;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ds =3D bf->desc;
>
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* last buffer must not be freed to e=
nsure proper hardware
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* function. When the hardware finish=
es also a packet next to
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* it, we are sure, it doesn't use it=
anymore and we can go on.
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (bf_last =3D=3D bf)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bf->flags |=3D 1;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (bf->flags) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct ath5k_buf *bf_ne=
xt =3D list_entry(bf->list.next,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 struct ath5k_buf, list);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D sc->ah->ah_proc=
_rx_desc(sc->ah, bf_next->desc,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 &rs);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ret)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bf->flags &=3D ~1;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* skip the overwritten=
one (even status is martian) */
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto next;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* bail if HW is still using self-linke=
d descriptor */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ath5k_hw_get_rxdp(sc->ah) =3D=3D bf=
->daddr)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D sc->ah->ah_proc_rx_desc(sc->ah=
, ds, &rs);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (unlikely(ret =3D=3D -EINPROGRESS))
> diff --git a/drivers/net/wireless/ath/ath5k/base.h b/drivers/net/wire=
less/ath/ath5k/base.h
> index 8229561..852b2c1 100644
> --- a/drivers/net/wireless/ath/ath5k/base.h
> +++ b/drivers/net/wireless/ath/ath5k/base.h
> @@ -56,7 +56,6 @@
>
> =A0struct ath5k_buf {
> =A0 =A0 =A0 =A0struct list_head =A0 =A0 =A0 =A0list;
> - =A0 =A0 =A0 unsigned int =A0 =A0 =A0 =A0 =A0 =A0flags; =A0/* rx des=
criptor flags */
> =A0 =A0 =A0 =A0struct ath5k_desc =A0 =A0 =A0 *desc; =A0/* virtual add=
r of desc */
> =A0 =A0 =A0 =A0dma_addr_t =A0 =A0 =A0 =A0 =A0 =A0 =A0daddr; =A0/* phy=
sical addr of desc */
> =A0 =A0 =A0 =A0struct sk_buff =A0 =A0 =A0 =A0 =A0*skb; =A0 /* skbuff =
for buf */
> diff --git a/drivers/net/wireless/ath/ath5k/dma.c b/drivers/net/wirel=
ess/ath/ath5k/dma.c
> index b65b4fe..941b511 100644
> --- a/drivers/net/wireless/ath/ath5k/dma.c
> +++ b/drivers/net/wireless/ath/ath5k/dma.c
> @@ -80,8 +80,6 @@ int ath5k_hw_stop_rx_dma(struct ath5k_hw *ah)
> =A0* ath5k_hw_get_rxdp - Get RX Descriptor's address
> =A0*
> =A0* @ah: The &struct ath5k_hw
> - *
> - * XXX: Is RXDP read and clear ?
> =A0*/
> =A0u32 ath5k_hw_get_rxdp(struct ath5k_hw *ah)
> =A0{
--=20
Daniel J Blueman
On Wed, Apr 15, 2009 at 01:20:09PM +0100, Daniel J Blueman wrote:
> Just an observation: Each synchronous read over PCI will take at
> minimum 0.9us to return. Accessing tables in the host's cache
> hierarchy/memory will likely be much faster - possibly why it is done
> like this already.
>
> Is this a worthwhile tradeoff?
This is a good point -- but I think in this case, it is worth it
from a correctness rather than performance standpoint.
See http://lkml.org/lkml/2009/3/8/99 for why I think the current
code doesn't handle every case.
FWIW, madwifi does this too. We could eliminate it if we dropped the
self-linked buffer thing (but then we'd need to handle EOL interrupts).
--
Bob Copeland %% http://www.bobcopeland.com
Current code uses int types, but both modparams are boolean values.
Changes-licensed-under: 3-Clause-BSD
Signed-off-by: Bob Copeland <[email protected]>
---
drivers/net/wireless/ath/ath5k/base.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index ef8523e..3bde18f 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -61,11 +61,11 @@
static int ath5k_calinterval = 10; /* Calibrate PHY every 10 secs (TODO: Fixme) */
static int modparam_nohwcrypt;
-module_param_named(nohwcrypt, modparam_nohwcrypt, int, 0444);
+module_param_named(nohwcrypt, modparam_nohwcrypt, bool, S_IRUGO);
MODULE_PARM_DESC(nohwcrypt, "Disable hardware encryption.");
static int modparam_all_channels;
-module_param_named(all_channels, modparam_all_channels, int, 0444);
+module_param_named(all_channels, modparam_all_channels, bool, S_IRUGO);
MODULE_PARM_DESC(all_channels, "Expose all channels the device can use.");
--
1.6.0.6
2009/4/15 Bob Copeland <[email protected]>:
> Grabbing an ath5k_buf then dropping the lock is racy because the
> referenced descriptor can be obtained in another thread and released
> before the buffer is handed to the hardware. =C2=A0Likewise, manipula=
ting
> sc->rxlink without the lock can lead to having multiple self-linked
> hardware descriptors.
>
> Changes-licensed-under: 3-Clause-BSD
>
> Signed-off-by: Bob Copeland <[email protected]>
> ---
Acked-by: Nick Kossifidis <[email protected]>
--=20
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick
2009/4/15 Bob Copeland <[email protected]>:
> This patch simplifies the code used to detect when the
> self-linked DMA buffer is still in use by hardware, by
> checking the hardware's rxdp register instead of looking
> at the software buffer list.
>
> Signed-off-by: Bob Copeland <[email protected]>
> ---
Acked-by: Nick Kossifidis <[email protected]>
--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick
2009/4/15 Bob Copeland <[email protected]>:
> This patch corrects a few errors in the initvals tables to match thos=
e
> in the HAL tables. =C2=A0Namely, remove a couple of repetitions, fix =
some
> turbo mode errors, and correct a register for the CCK rate power tabl=
e.
>
> Changes-licensed-under: ISC
>
> Signed-off-by: Bob Copeland <[email protected]>
> ---
Acked-by: Nick Kossifidis <[email protected]>
--=20
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick
2009/4/15 Bob Copeland <[email protected]>:
> For embedded platforms, beacon transmission can be starved when
> flooded with data packets. =C2=A0Prioritize beacons by giving the bea=
con
> queue the first shot when the isr completes.
>
> Changes-licensed-under: 3-Clause-BSD
>
> Signed-off-by: Bob Copeland <[email protected]>
Acked-by: Nick Kossifidis <[email protected]>
--=20
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick
Grabbing an ath5k_buf then dropping the lock is racy because the
referenced descriptor can be obtained in another thread and released
before the buffer is handed to the hardware. Likewise, manipulating
sc->rxlink without the lock can lead to having multiple self-linked
hardware descriptors.
Changes-licensed-under: 3-Clause-BSD
Signed-off-by: Bob Copeland <[email protected]>
---
drivers/net/wireless/ath/ath5k/base.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 1a6e72f..c8c658b 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -1618,9 +1618,8 @@ ath5k_rx_start(struct ath5k_softc *sc)
ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "cachelsz %u rxbufsize %u\n",
sc->cachelsz, sc->rxbufsize);
- sc->rxlink = NULL;
-
spin_lock_bh(&sc->rxbuflock);
+ sc->rxlink = NULL;
list_for_each_entry(bf, &sc->rxbuf, list) {
ret = ath5k_rxbuf_setup(sc, bf);
if (ret != 0) {
@@ -1629,9 +1628,9 @@ ath5k_rx_start(struct ath5k_softc *sc)
}
}
bf = list_first_entry(&sc->rxbuf, struct ath5k_buf, list);
+ ath5k_hw_set_rxdp(ah, bf->daddr);
spin_unlock_bh(&sc->rxbuflock);
- ath5k_hw_set_rxdp(ah, bf->daddr);
ath5k_hw_start_rx_dma(ah); /* enable recv descriptors */
ath5k_mode_setup(sc); /* set filters, etc. */
ath5k_hw_start_rx_pcu(ah); /* re-enable PCU/DMA engine */
--
1.6.0.6
2009/4/15 Bob Copeland <[email protected]>:
> Current code uses int types, but both modparams are boolean values.
>
> Changes-licensed-under: 3-Clause-BSD
>
> Signed-off-by: Bob Copeland <[email protected]>
> ---
Acked-by: Nick Kossifidis <[email protected]>
--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick
This patch simplifies the code used to detect when the
self-linked DMA buffer is still in use by hardware, by
checking the hardware's rxdp register instead of looking
at the software buffer list.
Signed-off-by: Bob Copeland <[email protected]>
---
drivers/net/wireless/ath/ath5k/base.c | 24 ++++--------------------
drivers/net/wireless/ath/ath5k/base.h | 1 -
drivers/net/wireless/ath/ath5k/dma.c | 2 --
3 files changed, 4 insertions(+), 23 deletions(-)
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 3bde18f..1a6e72f 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -1780,7 +1780,7 @@ ath5k_tasklet_rx(unsigned long data)
struct sk_buff *skb, *next_skb;
dma_addr_t next_skb_addr;
struct ath5k_softc *sc = (void *)data;
- struct ath5k_buf *bf, *bf_last;
+ struct ath5k_buf *bf;
struct ath5k_desc *ds;
int ret;
int hdrlen;
@@ -1791,7 +1791,6 @@ ath5k_tasklet_rx(unsigned long data)
ATH5K_WARN(sc, "empty rx buf pool\n");
goto unlock;
}
- bf_last = list_entry(sc->rxbuf.prev, struct ath5k_buf, list);
do {
rxs.flag = 0;
@@ -1800,24 +1799,9 @@ ath5k_tasklet_rx(unsigned long data)
skb = bf->skb;
ds = bf->desc;
- /*
- * last buffer must not be freed to ensure proper hardware
- * function. When the hardware finishes also a packet next to
- * it, we are sure, it doesn't use it anymore and we can go on.
- */
- if (bf_last == bf)
- bf->flags |= 1;
- if (bf->flags) {
- struct ath5k_buf *bf_next = list_entry(bf->list.next,
- struct ath5k_buf, list);
- ret = sc->ah->ah_proc_rx_desc(sc->ah, bf_next->desc,
- &rs);
- if (ret)
- break;
- bf->flags &= ~1;
- /* skip the overwritten one (even status is martian) */
- goto next;
- }
+ /* bail if HW is still using self-linked descriptor */
+ if (ath5k_hw_get_rxdp(sc->ah) == bf->daddr)
+ break;
ret = sc->ah->ah_proc_rx_desc(sc->ah, ds, &rs);
if (unlikely(ret == -EINPROGRESS))
diff --git a/drivers/net/wireless/ath/ath5k/base.h b/drivers/net/wireless/ath/ath5k/base.h
index 8229561..852b2c1 100644
--- a/drivers/net/wireless/ath/ath5k/base.h
+++ b/drivers/net/wireless/ath/ath5k/base.h
@@ -56,7 +56,6 @@
struct ath5k_buf {
struct list_head list;
- unsigned int flags; /* rx descriptor flags */
struct ath5k_desc *desc; /* virtual addr of desc */
dma_addr_t daddr; /* physical addr of desc */
struct sk_buff *skb; /* skbuff for buf */
diff --git a/drivers/net/wireless/ath/ath5k/dma.c b/drivers/net/wireless/ath/ath5k/dma.c
index b65b4fe..941b511 100644
--- a/drivers/net/wireless/ath/ath5k/dma.c
+++ b/drivers/net/wireless/ath/ath5k/dma.c
@@ -80,8 +80,6 @@ int ath5k_hw_stop_rx_dma(struct ath5k_hw *ah)
* ath5k_hw_get_rxdp - Get RX Descriptor's address
*
* @ah: The &struct ath5k_hw
- *
- * XXX: Is RXDP read and clear ?
*/
u32 ath5k_hw_get_rxdp(struct ath5k_hw *ah)
{
--
1.6.0.6
For embedded platforms, beacon transmission can be starved when
flooded with data packets. Prioritize beacons by giving the beacon
queue the first shot when the isr completes.
Changes-licensed-under: 3-Clause-BSD
Signed-off-by: Bob Copeland <[email protected]>
---
drivers/net/wireless/ath/ath5k/base.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index ff6d4f8..ef8523e 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -2496,7 +2496,7 @@ ath5k_intr(int irq, void *dev_id)
tasklet_schedule(&sc->restq);
} else {
if (status & AR5K_INT_SWBA) {
- tasklet_schedule(&sc->beacontq);
+ tasklet_hi_schedule(&sc->beacontq);
}
if (status & AR5K_INT_RXEOL) {
/*
--
1.6.0.6
This patch corrects a few errors in the initvals tables to match those
in the HAL tables. Namely, remove a couple of repetitions, fix some
turbo mode errors, and correct a register for the CCK rate power table.
Changes-licensed-under: ISC
Signed-off-by: Bob Copeland <[email protected]>
---
drivers/net/wireless/ath/ath5k/initvals.c | 8 +++-----
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/ath/ath5k/initvals.c b/drivers/net/wireless/ath/ath5k/initvals.c
index 61fb621..18eb519 100644
--- a/drivers/net/wireless/ath/ath5k/initvals.c
+++ b/drivers/net/wireless/ath/ath5k/initvals.c
@@ -537,8 +537,6 @@ static const struct ath5k_ini ar5212_ini_common_start[] = {
{ AR5K_DCU_TX_FILTER_1(15), 0x00000000 },
{ AR5K_DCU_TX_FILTER_CLR, 0x00000000 },
{ AR5K_DCU_TX_FILTER_SET, 0x00000000 },
- { AR5K_DCU_TX_FILTER_CLR, 0x00000000 },
- { AR5K_DCU_TX_FILTER_SET, 0x00000000 },
{ AR5K_STA_ID1, 0x00000000 },
{ AR5K_BSS_ID0, 0x00000000 },
{ AR5K_BSS_ID1, 0x00000000 },
@@ -669,7 +667,7 @@ static const struct ath5k_ini ar5212_ini_common_start[] = {
/*{ AR5K_PHY(650), 0x000001b5 },*/
{ AR5K_PHY(651), 0x00000000 },
{ AR5K_PHY_TXPOWER_RATE3, 0x20202020 },
- { AR5K_PHY_TXPOWER_RATE2, 0x20202020 },
+ { AR5K_PHY_TXPOWER_RATE4, 0x20202020 },
/*{ AR5K_PHY(655), 0x13c889af },*/
{ AR5K_PHY(656), 0x38490a20 },
{ AR5K_PHY(657), 0x00007bb6 },
@@ -718,7 +716,7 @@ static const struct ath5k_ini_mode ar5212_ini_mode_start[] = {
{ AR5K_PHY_SETTLING,
{ 0x1372161c, 0x13721c25, 0x13721722, 0x137216a2, 0x13721c25 } },
{ AR5K_PHY_AGCCTL,
- { 0x00009d10, 0x00009d10, 0x00009d18, 0x00009d18, 0x00009d18 } },
+ { 0x00009d10, 0x00009d10, 0x00009d18, 0x00009d18, 0x00009d10 } },
{ AR5K_PHY_NF,
{ 0x0001ce00, 0x0001ce00, 0x0001ce00, 0x0001ce00, 0x0001ce00 } },
{ AR5K_PHY_WEAK_OFDM_HIGH_THR,
@@ -799,7 +797,7 @@ static const struct ath5k_ini_mode rf5112_ini_mode_end[] = {
{ AR5K_PHY_DESIRED_SIZE,
{ 0x0de8b4e0, 0x0de8b4e0, 0x0de8b4e0, 0x0de8b4e0, 0x0de8b4e0 } },
{ AR5K_PHY_SIG,
- { 0x7e800d2e, 0x7e800d2e, 0x7ee80d2e, 0x7ee80d2e, 0x7ee80d2e } },
+ { 0x7e800d2e, 0x7e800d2e, 0x7ee80d2e, 0x7ee80d2e, 0x7e800d2e } },
{ AR5K_PHY_AGCCOARSE,
{ 0x3137665e, 0x3137665e, 0x3137665e, 0x3137665e, 0x3137665e } },
{ AR5K_PHY_WEAK_OFDM_LOW_THR,
--
1.6.0.6