When signal is noisy, hardware can use all RX buffers and since the last
entry in the list is self-linked, it overwrites the entry until we link
new buffers.
Ensure that we don't free this last one until we are 100% sure that it
is not used by the hardware anymore to not cause memory curruption as
can be seen below.
This is done by checking next buffer in the list. Even after that we
know that the hardware refetched the new link and proceeded further
(the next buffer is ready) we can finally free the overwritten buffer.
We discard it since the status in its descriptor is overwritten (OR-ed
by new status) too.
=============================================================================
BUG kmalloc-4096: Poison overwritten
-----------------------------------------------------------------------------
INFO: 0xffff810067419060-0xffff810067419667. First byte 0x8 instead of 0x6b
INFO: Allocated in dev_alloc_skb+0x18/0x30 age=1118 cpu=1 pid=0
INFO: Freed in skb_release_data+0x85/0xd0 age=1105 cpu=1 pid=3718
INFO: Slab 0xffffe200019d0600 objects=7 used=0 fp=0xffff810067419048 flags=0x40000000000020c3
INFO: Object 0xffff810067419048 @offset=4168 fp=0xffff81006741c120
Bytes b4 0xffff810067419038: 4f 0b 02 00 01 00 00 00 5a 5a 5a 5a 5a 5a 5a 5a O.......ZZZZZZZZ
Object 0xffff810067419048: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
Object 0xffff810067419058: 6b 6b 6b 6b 6b 6b 6b 6b 08 42 30 00 00 0b 6b 80 kkkkkkkk.B0...k.
Object 0xffff810067419068: f0 5d 00 4f 62 08 a3 64 00 0c 42 16 52 e4 f0 5a 360].Ob.243d..B.R344360Z
Object 0xffff810067419078: 68 81 00 00 7b a5 b4 be 7d 3b 8f 53 cd d5 de 12 h...{245264276};.S315325336.
Object 0xffff810067419088: 96 10 0b 89 48 54 23 41 0f 4e 2d b9 37 c3 cb 29 ....HT#A.N-2717303313)
Object 0xffff810067419098: d1 e0 de 14 8a 57 2a cc 3b 44 0d 78 7a 19 12 15 321340336..W*314;D.xz...
Object 0xffff8100674190a8: a9 ec d4 35 a8 10 ec 8c 40 a7 06 0a 51 a7 48 bb [email protected]
Object 0xffff8100674190b8: 3e cf a1 c7 38 60 63 3f 51 15 c7 20 eb ba 65 30 >ϡ3078`c?Q.307.353272e0
Redzone 0xffff81006741a048: bb bb bb bb bb bb bb bb 273273273273273273273273
Padding 0xffff81006741a088: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ
Pid: 3297, comm: ath5k_pci Not tainted 2.6.26-rc8-mm1_64 #427
Call Trace:
[<ffffffff802a7306>] print_trailer+0xf6/0x150
[<ffffffff802a7485>] check_bytes_and_report+0x125/0x180
[<ffffffff802a75dc>] check_object+0xac/0x260
[<ffffffff802a9308>] __slab_alloc+0x368/0x6d0
[<ffffffff80544f82>] ? wireless_send_event+0x142/0x310
[<ffffffff804b1bd4>] ? __alloc_skb+0x44/0x150
[<ffffffff80544f82>] ? wireless_send_event+0x142/0x310
[<ffffffff802aa853>] __kmalloc_track_caller+0xc3/0xf0
[<ffffffff804b1bfe>] __alloc_skb+0x6e/0x150
[... stack snipped]
FIX kmalloc-4096: Restoring 0xffff810067419060-0xffff810067419667=0x6b
FIX kmalloc-4096: Marking all objects used
Signed-off-by: Jiri Slaby <[email protected]>
Cc: Nick Kossifidis <[email protected]>
Cc: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath5k/base.c | 32 +++++++++++++++++++++++++-------
drivers/net/wireless/ath5k/base.h | 2 +-
2 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index 12a9443..e9ec284 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -1683,20 +1683,21 @@ ath5k_tasklet_rx(unsigned long data)
struct ath5k_rx_status rs = {};
struct sk_buff *skb;
struct ath5k_softc *sc = (void *)data;
- struct ath5k_buf *bf;
+ struct ath5k_buf *bf, *bf_last;
struct ath5k_desc *ds;
int ret;
int hdrlen;
int pad;
spin_lock(&sc->rxbuflock);
+ if (list_empty(&sc->rxbuf)) {
+ 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;
- if (unlikely(list_empty(&sc->rxbuf))) {
- ATH5K_WARN(sc, "empty rx buf pool\n");
- break;
- }
bf = list_first_entry(&sc->rxbuf, struct ath5k_buf, list);
BUG_ON(bf->skb == NULL);
skb = bf->skb;
@@ -1706,8 +1707,24 @@ ath5k_tasklet_rx(unsigned long data)
pci_dma_sync_single_for_cpu(sc->pdev, sc->desc_daddr,
sc->desc_len, PCI_DMA_FROMDEVICE);
- if (unlikely(ds->ds_link == bf->daddr)) /* this is the end */
- break;
+ /*
+ * 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;
+ }
ret = sc->ah->ah_proc_rx_desc(sc->ah, ds, &rs);
if (unlikely(ret == -EINPROGRESS))
@@ -1817,6 +1834,7 @@ accept:
next:
list_move_tail(&bf->list, &sc->rxbuf);
} while (ath5k_rxbuf_setup(sc, bf) == 0);
+unlock:
spin_unlock(&sc->rxbuflock);
}
diff --git a/drivers/net/wireless/ath5k/base.h b/drivers/net/wireless/ath5k/base.h
index 47f414b..d7e03e6 100644
--- a/drivers/net/wireless/ath5k/base.h
+++ b/drivers/net/wireless/ath5k/base.h
@@ -56,7 +56,7 @@
struct ath5k_buf {
struct list_head list;
- unsigned int flags; /* tx descriptor flags */
+ 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 */
--
1.5.6.2
Don't forget to kill tasklets on stop to not panic if they
fire after freeing some structures.
Signed-off-by: Jiri Slaby <[email protected]>
Cc: Nick Kossifidis <[email protected]>
Cc: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath5k/base.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index e9ec284..4d9ff97 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -2343,6 +2343,9 @@ ath5k_stop_hw(struct ath5k_softc *sc)
mutex_unlock(&sc->lock);
del_timer_sync(&sc->calib_tim);
+ tasklet_kill(&sc->rxtq);
+ tasklet_kill(&sc->txtq);
+ tasklet_kill(&sc->restq);
return ret;
}
--
1.5.6.2
Make sure that the irq is not in progress after stop. This means
two things:
- ensure the intr setting register is set by flushing posted values
- call synchronize_irq() after that
Also flush stop tx write, inform callers of the tx stop about still
pending transfers (unsuccessful stop) and finally don't wait another
3ms in ath5k_rx_stop, since ath5k_hw_stop_rx_dma ensures transfer to
be finished.
Make sure all writes will be ordered in respect to locks by mmiowb().
Signed-off-by: Jiri Slaby <[email protected]>
Cc: Nick Kossifidis <[email protected]>
Cc: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath5k/base.c | 13 +++++++++++--
drivers/net/wireless/ath5k/hw.c | 4 ++++
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index 4d9ff97..4874a6f 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -43,7 +43,9 @@
#include <linux/version.h>
#include <linux/module.h>
#include <linux/delay.h>
+#include <linux/hardirq.h>
#include <linux/if.h>
+#include <linux/io.h>
#include <linux/netdevice.h>
#include <linux/cache.h>
#include <linux/pci.h>
@@ -1250,6 +1252,7 @@ ath5k_txbuf_setup(struct ath5k_softc *sc, struct ath5k_buf *bf)
txq->link = &ds->ds_link;
ath5k_hw_tx_start(ah, txq->qnum);
+ mmiowb();
spin_unlock_bh(&txq->lock);
return 0;
@@ -1584,7 +1587,6 @@ ath5k_rx_stop(struct ath5k_softc *sc)
ath5k_hw_stop_pcu_recv(ah); /* disable PCU */
ath5k_hw_set_rx_filter(ah, 0); /* clear recv filter */
ath5k_hw_stop_rx_dma(ah); /* disable DMA engine */
- mdelay(3); /* 3ms is long enough for 1 frame */
ath5k_debug_printrxbuffs(sc, ah);
@@ -2259,6 +2261,7 @@ ath5k_init(struct ath5k_softc *sc)
ret = 0;
done:
+ mmiowb();
mutex_unlock(&sc->lock);
return ret;
}
@@ -2291,6 +2294,7 @@ ath5k_stop_locked(struct ath5k_softc *sc)
if (!test_bit(ATH_STAT_INVALID, sc->status)) {
ath5k_led_off(sc);
ath5k_hw_set_intr(ah, 0);
+ synchronize_irq(sc->pdev->irq);
}
ath5k_txq_cleanup(sc);
if (!test_bit(ATH_STAT_INVALID, sc->status)) {
@@ -2340,6 +2344,7 @@ ath5k_stop_hw(struct ath5k_softc *sc)
}
}
ath5k_txbuf_free(sc, sc->bbuf);
+ mmiowb();
mutex_unlock(&sc->lock);
del_timer_sync(&sc->calib_tim);
@@ -2805,6 +2810,7 @@ ath5k_config_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
/* XXX: assoc id is set to 0 for now, mac80211 doesn't have
* a clean way of letting us retrieve this yet. */
ath5k_hw_set_associd(ah, ah->ah_bssid, 0);
+ mmiowb();
}
mutex_unlock(&sc->lock);
@@ -2981,6 +2987,7 @@ ath5k_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
}
unlock:
+ mmiowb();
mutex_unlock(&sc->lock);
return ret;
}
@@ -3054,8 +3061,10 @@ ath5k_beacon_update(struct ieee80211_hw *hw, struct sk_buff *skb)
ret = ath5k_beacon_setup(sc, sc->bbuf);
if (ret)
sc->bbuf->skb = NULL;
- else
+ else {
ath5k_beacon_config(sc);
+ mmiowb();
+ }
end:
mutex_unlock(&sc->lock);
diff --git a/drivers/net/wireless/ath5k/hw.c b/drivers/net/wireless/ath5k/hw.c
index c6d12c5..7ca87a5 100644
--- a/drivers/net/wireless/ath5k/hw.c
+++ b/drivers/net/wireless/ath5k/hw.c
@@ -1440,6 +1440,7 @@ int ath5k_hw_stop_tx_dma(struct ath5k_hw *ah, unsigned int queue)
/* Stop queue */
ath5k_hw_reg_write(ah, tx_queue, AR5K_CR);
+ ath5k_hw_reg_read(ah, AR5K_CR);
} else {
/*
* Schedule TX disable and wait until queue is empty
@@ -1456,6 +1457,8 @@ int ath5k_hw_stop_tx_dma(struct ath5k_hw *ah, unsigned int queue)
/* Clear register */
ath5k_hw_reg_write(ah, 0, AR5K_QCU_TXD);
+ if (pending)
+ return -EBUSY;
}
/* TODO: Check for success else return error */
@@ -1716,6 +1719,7 @@ enum ath5k_int ath5k_hw_set_intr(struct ath5k_hw *ah, enum ath5k_int new_mask)
/* ..re-enable interrupts */
ath5k_hw_reg_write(ah, AR5K_IER_ENABLE, AR5K_IER);
+ ath5k_hw_reg_read(ah, AR5K_IER);
return old_mask;
}
--
1.5.6.2
- free and re-request irq since it might have changed during suspend
- disable and enable msi
- don't set D0 state of the device, it's already done by the PCI layer
- do restore_state before enable_device, it's safer
- check ath5k_init return value
Signed-off-by: Jiri Slaby <[email protected]>
Cc: Nick Kossifidis <[email protected]>
Cc: Luis R. Rodriguez <[email protected]>
Cc: Jesse Barnes <[email protected]>
---
drivers/net/wireless/ath5k/base.c | 26 +++++++++++++++++++++-----
1 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index 713ee99..d58a883 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -593,6 +593,9 @@ ath5k_pci_suspend(struct pci_dev *pdev, pm_message_t state)
ath5k_led_off(sc);
ath5k_stop_hw(sc);
+
+ free_irq(pdev->irq, sc);
+ pci_disable_msi(pdev);
pci_save_state(pdev);
pci_disable_device(pdev);
pci_set_power_state(pdev, PCI_D3hot);
@@ -608,15 +611,12 @@ ath5k_pci_resume(struct pci_dev *pdev)
struct ath5k_hw *ah = sc->ah;
int i, err;
- err = pci_set_power_state(pdev, PCI_D0);
- if (err)
- return err;
+ pci_restore_state(pdev);
err = pci_enable_device(pdev);
if (err)
return err;
- pci_restore_state(pdev);
/*
* Suspend/Resume resets the PCI configuration space, so we have to
* re-disable the RETRY_TIMEOUT register (0x41) to keep
@@ -624,7 +624,17 @@ ath5k_pci_resume(struct pci_dev *pdev)
*/
pci_write_config_byte(pdev, 0x41, 0);
- ath5k_init(sc);
+ pci_enable_msi(pdev);
+
+ err = request_irq(pdev->irq, ath5k_intr, IRQF_SHARED, "ath", sc);
+ if (err) {
+ ATH5K_ERR(sc, "request_irq failed\n");
+ goto err_msi;
+ }
+
+ err = ath5k_init(sc);
+ if (err)
+ goto err_irq;
ath5k_led_enable(sc);
/*
@@ -638,6 +648,12 @@ ath5k_pci_resume(struct pci_dev *pdev)
ath5k_hw_reset_key(ah, i);
return 0;
+err_irq:
+ free_irq(pdev->irq, sc);
+err_msi:
+ pci_disable_msi(pdev);
+ pci_disable_device(pdev);
+ return err;
}
#endif /* CONFIG_PM */
--
1.5.6.2
Don't sync
- coherent mapping (descriptors)
- before unmap, it's useless
- (wrongly anyway -- for_cpu) beacon skb, it's just mapped,
so by the device yet
Signed-off-by: Jiri Slaby <[email protected]>
Cc: Nick Kossifidis <[email protected]>
Cc: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath5k/base.c | 11 -----------
1 files changed, 0 insertions(+), 11 deletions(-)
diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index 4874a6f..713ee99 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -1705,10 +1705,6 @@ ath5k_tasklet_rx(unsigned long data)
skb = bf->skb;
ds = bf->desc;
- /* TODO only one segment */
- pci_dma_sync_single_for_cpu(sc->pdev, sc->desc_daddr,
- sc->desc_len, PCI_DMA_FROMDEVICE);
-
/*
* last buffer must not be freed to ensure proper hardware
* function. When the hardware finishes also a packet next to
@@ -1772,8 +1768,6 @@ ath5k_tasklet_rx(unsigned long data)
goto next;
}
accept:
- pci_dma_sync_single_for_cpu(sc->pdev, bf->skbaddr,
- rs.rs_datalen, PCI_DMA_FROMDEVICE);
pci_unmap_single(sc->pdev, bf->skbaddr, sc->rxbufsize,
PCI_DMA_FROMDEVICE);
bf->skb = NULL;
@@ -1861,9 +1855,6 @@ ath5k_tx_processq(struct ath5k_softc *sc, struct ath5k_txq *txq)
list_for_each_entry_safe(bf, bf0, &txq->q, list) {
ds = bf->desc;
- /* TODO only one segment */
- pci_dma_sync_single_for_cpu(sc->pdev, sc->desc_daddr,
- sc->desc_len, PCI_DMA_FROMDEVICE);
ret = sc->ah->ah_proc_tx_desc(sc->ah, ds, &ts);
if (unlikely(ret == -EINPROGRESS))
break;
@@ -2036,8 +2027,6 @@ ath5k_beacon_send(struct ath5k_softc *sc)
ATH5K_WARN(sc, "beacon queue %u didn't stop?\n", sc->bhalq);
/* NB: hw still stops DMA, so proceed */
}
- pci_dma_sync_single_for_cpu(sc->pdev, bf->skbaddr, bf->skb->len,
- PCI_DMA_TODEVICE);
ath5k_hw_put_tx_buf(ah, sc->bhalq, bf->daddr);
ath5k_hw_tx_start(ah, sc->bhalq);
--
1.5.6.2
2008/7/15 Jiri Slaby <[email protected]>:> When signal is noisy, hardware can use all RX buffers and since the last> entry in the list is self-linked, it overwrites the entry until we link> new buffers.>> Ensure that we don't free this last one until we are 100% sure that it> is not used by the hardware anymore to not cause memory curruption as> can be seen below.>> This is done by checking next buffer in the list. Even after that we> know that the hardware refetched the new link and proceeded further> (the next buffer is ready) we can finally free the overwritten buffer.>> We discard it since the status in its descriptor is overwritten (OR-ed> by new status) too.>> =============================================================================> BUG kmalloc-4096: Poison overwritten> ----------------------------------------------------------------------------->> INFO: 0xffff810067419060-0xffff810067419667. First byte 0x8 instead of 0x6b> INFO: Allocated in dev_alloc_skb+0x18/0x30 age=1118 cpu=1 pid=0> INFO: Freed in skb_release_data+0x85/0xd0 age=1105 cpu=1 pid=3718> INFO: Slab 0xffffe200019d0600 objects=7 used=0 fp=0xffff810067419048 flags=0x40000000000020c3> INFO: Object 0xffff810067419048 @offset=4168 fp=0xffff81006741c120>> Bytes b4 0xffff810067419038: 4f 0b 02 00 01 00 00 00 5a 5a 5a 5a 5a 5a 5a 5a O.......ZZZZZZZZ> Object 0xffff810067419048: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk> Object 0xffff810067419058: 6b 6b 6b 6b 6b 6b 6b 6b 08 42 30 00 00 0b 6b 80 kkkkkkkk.B0...k.> Object 0xffff810067419068: f0 5d 00 4f 62 08 a3 64 00 0c 42 16 52 e4 f0 5a 360].Ob.243d..B.R344360Z> Object 0xffff810067419078: 68 81 00 00 7b a5 b4 be 7d 3b 8f 53 cd d5 de 12 h...{245264276};.S315325336.> Object 0xffff810067419088: 96 10 0b 89 48 54 23 41 0f 4e 2d b9 37 c3 cb 29 ....HT#A.N-2717303313)> Object 0xffff810067419098: d1 e0 de 14 8a 57 2a cc 3b 44 0d 78 7a 19 12 15 321340336..W*314;D.xz...> Object 0xffff8100674190a8: a9 ec d4 35 a8 10 ec 8c 40 a7 06 0a 51 a7 48 bb [email protected]> Object 0xffff8100674190b8: 3e cf a1 c7 38 60 63 3f 51 15 c7 20 eb ba 65 30 >ϡ3078`c?Q.307.353272e0> Redzone 0xffff81006741a048: bb bb bb bb bb bb bb bb 273273273273273273273273> Padding 0xffff81006741a088: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ> Pid: 3297, comm: ath5k_pci Not tainted 2.6.26-rc8-mm1_64 #427>> Call Trace:> [<ffffffff802a7306>] print_trailer+0xf6/0x150> [<ffffffff802a7485>] check_bytes_and_report+0x125/0x180> [<ffffffff802a75dc>] check_object+0xac/0x260> [<ffffffff802a9308>] __slab_alloc+0x368/0x6d0> [<ffffffff80544f82>] ? wireless_send_event+0x142/0x310> [<ffffffff804b1bd4>] ? __alloc_skb+0x44/0x150> [<ffffffff80544f82>] ? wireless_send_event+0x142/0x310> [<ffffffff802aa853>] __kmalloc_track_caller+0xc3/0xf0> [<ffffffff804b1bfe>] __alloc_skb+0x6e/0x150> [... stack snipped]>> FIX kmalloc-4096: Restoring 0xffff810067419060-0xffff810067419667=0x6b>> FIX kmalloc-4096: Marking all objects used>> Signed-off-by: Jiri Slaby <[email protected]>> Cc: Nick Kossifidis <[email protected]>> Cc: Luis R. Rodriguez <[email protected]>> ---> drivers/net/wireless/ath5k/base.c | 32 +++++++++++++++++++++++++-------> drivers/net/wireless/ath5k/base.h | 2 +-> 2 files changed, 26 insertions(+), 8 deletions(-)>> diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c> index 12a9443..e9ec284 100644> --- a/drivers/net/wireless/ath5k/base.c> +++ b/drivers/net/wireless/ath5k/base.c> @@ -1683,20 +1683,21 @@ ath5k_tasklet_rx(unsigned long data)> struct ath5k_rx_status rs = {};> struct sk_buff *skb;> struct ath5k_softc *sc = (void *)data;> - struct ath5k_buf *bf;> + struct ath5k_buf *bf, *bf_last;> struct ath5k_desc *ds;> int ret;> int hdrlen;> int pad;>> spin_lock(&sc->rxbuflock);> + if (list_empty(&sc->rxbuf)) {> + 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;>> - if (unlikely(list_empty(&sc->rxbuf))) {> - ATH5K_WARN(sc, "empty rx buf pool\n");> - break;> - }> bf = list_first_entry(&sc->rxbuf, struct ath5k_buf, list);> BUG_ON(bf->skb == NULL);> skb = bf->skb;> @@ -1706,8 +1707,24 @@ ath5k_tasklet_rx(unsigned long data)> pci_dma_sync_single_for_cpu(sc->pdev, sc->desc_daddr,> sc->desc_len, PCI_DMA_FROMDEVICE);>> - if (unlikely(ds->ds_link == bf->daddr)) /* this is the end */> - break;> + /*> + * 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;> + }>> ret = sc->ah->ah_proc_rx_desc(sc->ah, ds, &rs);> if (unlikely(ret == -EINPROGRESS))> @@ -1817,6 +1834,7 @@ accept:> next:> list_move_tail(&bf->list, &sc->rxbuf);> } while (ath5k_rxbuf_setup(sc, bf) == 0);> +unlock:> spin_unlock(&sc->rxbuflock);> }>> diff --git a/drivers/net/wireless/ath5k/base.h b/drivers/net/wireless/ath5k/base.h> index 47f414b..d7e03e6 100644> --- a/drivers/net/wireless/ath5k/base.h> +++ b/drivers/net/wireless/ath5k/base.h> @@ -56,7 +56,7 @@>> struct ath5k_buf {> struct list_head list;> - unsigned int flags; /* tx descriptor flags */> + 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 */> --> 1.5.6.2>>
Nice catch ;-)
Acked-by: Nick Kossifidis <[email protected]>
-- GPG ID: 0xD21DB2DBAs you read this post global entropy rises. Have Fun ;-)Nick????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
2008/7/15 Jiri Slaby <[email protected]>:
> Don't forget to kill tasklets on stop to not panic if they
> fire after freeing some structures.
>
> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: Nick Kossifidis <[email protected]>
> Cc: Luis R. Rodriguez <[email protected]>
> ---
> drivers/net/wireless/ath5k/base.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
> index e9ec284..4d9ff97 100644
> --- a/drivers/net/wireless/ath5k/base.c
> +++ b/drivers/net/wireless/ath5k/base.c
> @@ -2343,6 +2343,9 @@ ath5k_stop_hw(struct ath5k_softc *sc)
> mutex_unlock(&sc->lock);
>
> del_timer_sync(&sc->calib_tim);
> + tasklet_kill(&sc->rxtq);
> + tasklet_kill(&sc->txtq);
> + tasklet_kill(&sc->restq);
>
> return ret;
> }
> --
> 1.5.6.2
>
>
Acked-by: Nick Kossifidis <[email protected]>
--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick
2008/7/15 Jiri Slaby <[email protected]>:
> Make sure that the irq is not in progress after stop. This means
> two things:
> - ensure the intr setting register is set by flushing posted values
> - call synchronize_irq() after that
>
> Also flush stop tx write, inform callers of the tx stop about still
> pending transfers (unsuccessful stop) and finally don't wait another
> 3ms in ath5k_rx_stop, since ath5k_hw_stop_rx_dma ensures transfer to
> be finished.
>
> Make sure all writes will be ordered in respect to locks by mmiowb().
>
> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: Nick Kossifidis <[email protected]>
> Cc: Luis R. Rodriguez <[email protected]>
> ---
> drivers/net/wireless/ath5k/base.c | 13 +++++++++++--
> drivers/net/wireless/ath5k/hw.c | 4 ++++
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
> index 4d9ff97..4874a6f 100644
> --- a/drivers/net/wireless/ath5k/base.c
> +++ b/drivers/net/wireless/ath5k/base.c
> @@ -43,7 +43,9 @@
> #include <linux/version.h>
> #include <linux/module.h>
> #include <linux/delay.h>
> +#include <linux/hardirq.h>
> #include <linux/if.h>
> +#include <linux/io.h>
> #include <linux/netdevice.h>
> #include <linux/cache.h>
> #include <linux/pci.h>
> @@ -1250,6 +1252,7 @@ ath5k_txbuf_setup(struct ath5k_softc *sc, struct ath5k_buf *bf)
>
> txq->link = &ds->ds_link;
> ath5k_hw_tx_start(ah, txq->qnum);
> + mmiowb();
> spin_unlock_bh(&txq->lock);
>
> return 0;
> @@ -1584,7 +1587,6 @@ ath5k_rx_stop(struct ath5k_softc *sc)
> ath5k_hw_stop_pcu_recv(ah); /* disable PCU */
> ath5k_hw_set_rx_filter(ah, 0); /* clear recv filter */
> ath5k_hw_stop_rx_dma(ah); /* disable DMA engine */
> - mdelay(3); /* 3ms is long enough for 1 frame */
>
> ath5k_debug_printrxbuffs(sc, ah);
>
> @@ -2259,6 +2261,7 @@ ath5k_init(struct ath5k_softc *sc)
>
> ret = 0;
> done:
> + mmiowb();
> mutex_unlock(&sc->lock);
> return ret;
> }
> @@ -2291,6 +2294,7 @@ ath5k_stop_locked(struct ath5k_softc *sc)
> if (!test_bit(ATH_STAT_INVALID, sc->status)) {
> ath5k_led_off(sc);
> ath5k_hw_set_intr(ah, 0);
> + synchronize_irq(sc->pdev->irq);
> }
> ath5k_txq_cleanup(sc);
> if (!test_bit(ATH_STAT_INVALID, sc->status)) {
> @@ -2340,6 +2344,7 @@ ath5k_stop_hw(struct ath5k_softc *sc)
> }
> }
> ath5k_txbuf_free(sc, sc->bbuf);
> + mmiowb();
> mutex_unlock(&sc->lock);
>
> del_timer_sync(&sc->calib_tim);
> @@ -2805,6 +2810,7 @@ ath5k_config_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> /* XXX: assoc id is set to 0 for now, mac80211 doesn't have
> * a clean way of letting us retrieve this yet. */
> ath5k_hw_set_associd(ah, ah->ah_bssid, 0);
> + mmiowb();
> }
> mutex_unlock(&sc->lock);
>
> @@ -2981,6 +2987,7 @@ ath5k_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> }
>
> unlock:
> + mmiowb();
> mutex_unlock(&sc->lock);
> return ret;
> }
> @@ -3054,8 +3061,10 @@ ath5k_beacon_update(struct ieee80211_hw *hw, struct sk_buff *skb)
> ret = ath5k_beacon_setup(sc, sc->bbuf);
> if (ret)
> sc->bbuf->skb = NULL;
> - else
> + else {
> ath5k_beacon_config(sc);
> + mmiowb();
> + }
>
> end:
> mutex_unlock(&sc->lock);
> diff --git a/drivers/net/wireless/ath5k/hw.c b/drivers/net/wireless/ath5k/hw.c
> index c6d12c5..7ca87a5 100644
> --- a/drivers/net/wireless/ath5k/hw.c
> +++ b/drivers/net/wireless/ath5k/hw.c
> @@ -1440,6 +1440,7 @@ int ath5k_hw_stop_tx_dma(struct ath5k_hw *ah, unsigned int queue)
>
> /* Stop queue */
> ath5k_hw_reg_write(ah, tx_queue, AR5K_CR);
> + ath5k_hw_reg_read(ah, AR5K_CR);
> } else {
> /*
> * Schedule TX disable and wait until queue is empty
> @@ -1456,6 +1457,8 @@ int ath5k_hw_stop_tx_dma(struct ath5k_hw *ah, unsigned int queue)
>
> /* Clear register */
> ath5k_hw_reg_write(ah, 0, AR5K_QCU_TXD);
> + if (pending)
> + return -EBUSY;
> }
>
> /* TODO: Check for success else return error */
> @@ -1716,6 +1719,7 @@ enum ath5k_int ath5k_hw_set_intr(struct ath5k_hw *ah, enum ath5k_int new_mask)
>
> /* ..re-enable interrupts */
> ath5k_hw_reg_write(ah, AR5K_IER_ENABLE, AR5K_IER);
> + ath5k_hw_reg_read(ah, AR5K_IER);
>
> return old_mask;
> }
> --
> 1.5.6.2
>
>
Acked-by: Nick Kossifidis <[email protected]>
--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick
2008/7/15 Jiri Slaby <[email protected]>:
> Don't sync
> - coherent mapping (descriptors)
> - before unmap, it's useless
> - (wrongly anyway -- for_cpu) beacon skb, it's just mapped,
> so by the device yet
>
> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: Nick Kossifidis <[email protected]>
> Cc: Luis R. Rodriguez <[email protected]>
> ---
> drivers/net/wireless/ath5k/base.c | 11 -----------
> 1 files changed, 0 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
> index 4874a6f..713ee99 100644
> --- a/drivers/net/wireless/ath5k/base.c
> +++ b/drivers/net/wireless/ath5k/base.c
> @@ -1705,10 +1705,6 @@ ath5k_tasklet_rx(unsigned long data)
> skb = bf->skb;
> ds = bf->desc;
>
> - /* TODO only one segment */
> - pci_dma_sync_single_for_cpu(sc->pdev, sc->desc_daddr,
> - sc->desc_len, PCI_DMA_FROMDEVICE);
> -
> /*
> * last buffer must not be freed to ensure proper hardware
> * function. When the hardware finishes also a packet next to
> @@ -1772,8 +1768,6 @@ ath5k_tasklet_rx(unsigned long data)
> goto next;
> }
> accept:
> - pci_dma_sync_single_for_cpu(sc->pdev, bf->skbaddr,
> - rs.rs_datalen, PCI_DMA_FROMDEVICE);
> pci_unmap_single(sc->pdev, bf->skbaddr, sc->rxbufsize,
> PCI_DMA_FROMDEVICE);
> bf->skb = NULL;
> @@ -1861,9 +1855,6 @@ ath5k_tx_processq(struct ath5k_softc *sc, struct ath5k_txq *txq)
> list_for_each_entry_safe(bf, bf0, &txq->q, list) {
> ds = bf->desc;
>
> - /* TODO only one segment */
> - pci_dma_sync_single_for_cpu(sc->pdev, sc->desc_daddr,
> - sc->desc_len, PCI_DMA_FROMDEVICE);
> ret = sc->ah->ah_proc_tx_desc(sc->ah, ds, &ts);
> if (unlikely(ret == -EINPROGRESS))
> break;
> @@ -2036,8 +2027,6 @@ ath5k_beacon_send(struct ath5k_softc *sc)
> ATH5K_WARN(sc, "beacon queue %u didn't stop?\n", sc->bhalq);
> /* NB: hw still stops DMA, so proceed */
> }
> - pci_dma_sync_single_for_cpu(sc->pdev, bf->skbaddr, bf->skb->len,
> - PCI_DMA_TODEVICE);
>
> ath5k_hw_put_tx_buf(ah, sc->bhalq, bf->daddr);
> ath5k_hw_tx_start(ah, sc->bhalq);
> --
> 1.5.6.2
>
>
Acked-by: Nick Kossifidis <[email protected]>
--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick
2008/7/15 Jiri Slaby <[email protected]>:
> - free and re-request irq since it might have changed during suspend
> - disable and enable msi
> - don't set D0 state of the device, it's already done by the PCI layer
> - do restore_state before enable_device, it's safer
> - check ath5k_init return value
>
> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: Nick Kossifidis <[email protected]>
> Cc: Luis R. Rodriguez <[email protected]>
> Cc: Jesse Barnes <[email protected]>
> ---
> drivers/net/wireless/ath5k/base.c | 26 +++++++++++++++++++++-----
> 1 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
> index 713ee99..d58a883 100644
> --- a/drivers/net/wireless/ath5k/base.c
> +++ b/drivers/net/wireless/ath5k/base.c
> @@ -593,6 +593,9 @@ ath5k_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> ath5k_led_off(sc);
>
> ath5k_stop_hw(sc);
> +
> + free_irq(pdev->irq, sc);
> + pci_disable_msi(pdev);
> pci_save_state(pdev);
> pci_disable_device(pdev);
> pci_set_power_state(pdev, PCI_D3hot);
> @@ -608,15 +611,12 @@ ath5k_pci_resume(struct pci_dev *pdev)
> struct ath5k_hw *ah = sc->ah;
> int i, err;
>
> - err = pci_set_power_state(pdev, PCI_D0);
> - if (err)
> - return err;
> + pci_restore_state(pdev);
>
> err = pci_enable_device(pdev);
> if (err)
> return err;
>
> - pci_restore_state(pdev);
> /*
> * Suspend/Resume resets the PCI configuration space, so we have to
> * re-disable the RETRY_TIMEOUT register (0x41) to keep
> @@ -624,7 +624,17 @@ ath5k_pci_resume(struct pci_dev *pdev)
> */
> pci_write_config_byte(pdev, 0x41, 0);
>
> - ath5k_init(sc);
> + pci_enable_msi(pdev);
> +
> + err = request_irq(pdev->irq, ath5k_intr, IRQF_SHARED, "ath", sc);
> + if (err) {
> + ATH5K_ERR(sc, "request_irq failed\n");
> + goto err_msi;
> + }
> +
> + err = ath5k_init(sc);
> + if (err)
> + goto err_irq;
> ath5k_led_enable(sc);
>
> /*
> @@ -638,6 +648,12 @@ ath5k_pci_resume(struct pci_dev *pdev)
> ath5k_hw_reset_key(ah, i);
>
> return 0;
> +err_irq:
> + free_irq(pdev->irq, sc);
> +err_msi:
> + pci_disable_msi(pdev);
> + pci_disable_device(pdev);
> + return err;
> }
> #endif /* CONFIG_PM */
>
> --
> 1.5.6.2
>
>
It's ok for now, but have in mind that on my upcoming patch series i'm
disabling msi (commented out) since it results no interrupts on pci-e
cards (seems there is a bug in kernel's msi implementation).
Acked-by: Nick Kossifidis <[email protected]>
--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick
On Wednesday, July 16, 2008 9:15 am Nick Kossifidis wrote:
> It's ok for now, but have in mind that on my upcoming patch series i'm
> disabling msi (commented out) since it results no interrupts on pci-e
> cards (seems there is a bug in kernel's msi implementation).
Hm, would be good to get details here. MSI is being used by other drivers
successfully...
Jesse
On Wed, 2008-07-16 at 09:31 -0700, Jesse Barnes wrote:
> On Wednesday, July 16, 2008 9:15 am Nick Kossifidis wrote:
> > It's ok for now, but have in mind that on my upcoming patch series i'm
> > disabling msi (commented out) since it results no interrupts on pci-e
> > cards (seems there is a bug in kernel's msi implementation).
>
> Hm, would be good to get details here. MSI is being used by other drivers
> successfully...
That's true, but no driver uses the same interrupt handler with and
without MSI. Either it's different handlers or the handler checks if
MSI is enabled and does something differently.
Checks is any interrupts are pending are different for MSI. It may be
not hard to do (it's probably easier than for real interrupts), but it
has to be figured out. Somebody has to do it right. Until MSI is
supported by the interrupt handler, it should not be enabled by the
driver, or we get a non-functioning driver for AR5006.
I see that MSI is enabled in wireless-testing, but I think we should try
to avoid this mistake when the commit goes upstream. I'm not sure what
the procedure there, but it would be nice to keep the tree bisectable,
i.e. avoid known regressions in any revision. Maybe an amended commit
should go upstream, and we'll drop the original patch when rebasing to
the upstream branch?
--
Regards,
Pavel Roskin
On Wednesday, July 16, 2008 10:35 am Pavel Roskin wrote:
> On Wed, 2008-07-16 at 09:31 -0700, Jesse Barnes wrote:
> > On Wednesday, July 16, 2008 9:15 am Nick Kossifidis wrote:
> > > It's ok for now, but have in mind that on my upcoming patch series i'm
> > > disabling msi (commented out) since it results no interrupts on pci-e
> > > cards (seems there is a bug in kernel's msi implementation).
> >
> > Hm, would be good to get details here. MSI is being used by other
> > drivers successfully...
>
> That's true, but no driver uses the same interrupt handler with and
> without MSI. Either it's different handlers or the handler checks if
> MSI is enabled and does something differently.
Yeah, using MSI on your device can mean that you have to make some changes
(like on Intel graphics you have to disable regular interrupts or bad things
happen), but you can still share the handler or at least most of it in some
cases.
> Checks is any interrupts are pending are different for MSI. It may be
> not hard to do (it's probably easier than for real interrupts), but it
> has to be figured out. Somebody has to do it right. Until MSI is
> supported by the interrupt handler, it should not be enabled by the
> driver, or we get a non-functioning driver for AR5006.
Sure, that's fine. I just wanted to make sure that there weren't some weird
generic MSI problems.
Thanks,
Jesse
2008/7/16 Jesse Barnes <[email protected]>:
> On Wednesday, July 16, 2008 10:35 am Pavel Roskin wrote:
>> On Wed, 2008-07-16 at 09:31 -0700, Jesse Barnes wrote:
>> > On Wednesday, July 16, 2008 9:15 am Nick Kossifidis wrote:
>> > > It's ok for now, but have in mind that on my upcoming patch series i'm
>> > > disabling msi (commented out) since it results no interrupts on pci-e
>> > > cards (seems there is a bug in kernel's msi implementation).
>> >
>> > Hm, would be good to get details here. MSI is being used by other
>> > drivers successfully...
>>
>> That's true, but no driver uses the same interrupt handler with and
>> without MSI. Either it's different handlers or the handler checks if
>> MSI is enabled and does something differently.
>
> Yeah, using MSI on your device can mean that you have to make some changes
> (like on Intel graphics you have to disable regular interrupts or bad things
> happen), but you can still share the handler or at least most of it in some
> cases.
>
Is there documentation for this somewhere ? Msi documentation
(MSI-HOWTO.txt) doesn't say anything about a different interrupt
handler etc.
>> Checks is any interrupts are pending are different for MSI. It may be
>> not hard to do (it's probably easier than for real interrupts), but it
>> has to be figured out. Somebody has to do it right. Until MSI is
>> supported by the interrupt handler, it should not be enabled by the
>> driver, or we get a non-functioning driver for AR5006.
>
> Sure, that's fine. I just wanted to make sure that there weren't some weird
> generic MSI problems.
>
I was referring to this post...
http://lkml.org/lkml/2008/6/24/150
--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick
On Wednesday, July 16, 2008 12:27 pm Nick Kossifidis wrote:
> 2008/7/16 Jesse Barnes <[email protected]>:
> > On Wednesday, July 16, 2008 10:35 am Pavel Roskin wrote:
> >> On Wed, 2008-07-16 at 09:31 -0700, Jesse Barnes wrote:
> >> > On Wednesday, July 16, 2008 9:15 am Nick Kossifidis wrote:
> >> > > It's ok for now, but have in mind that on my upcoming patch series
> >> > > i'm disabling msi (commented out) since it results no interrupts on
> >> > > pci-e cards (seems there is a bug in kernel's msi implementation).
> >> >
> >> > Hm, would be good to get details here. MSI is being used by other
> >> > drivers successfully...
> >>
> >> That's true, but no driver uses the same interrupt handler with and
> >> without MSI. Either it's different handlers or the handler checks if
> >> MSI is enabled and does something differently.
> >
> > Yeah, using MSI on your device can mean that you have to make some
> > changes (like on Intel graphics you have to disable regular interrupts or
> > bad things happen), but you can still share the handler or at least most
> > of it in some cases.
>
> Is there documentation for this somewhere ? Msi documentation
> (MSI-HOWTO.txt) doesn't say anything about a different interrupt
> handler etc.
Matthew just rewrote MSI-HOWTO as part of his rework of the MSI code. It
might be more helpful now (see
http://kerneltrap.org/mailarchive/linux-kernel/2008/7/1).
>
> >> Checks is any interrupts are pending are different for MSI. It may be
> >> not hard to do (it's probably easier than for real interrupts), but it
> >> has to be figured out. Somebody has to do it right. Until MSI is
> >> supported by the interrupt handler, it should not be enabled by the
> >> driver, or we get a non-functioning driver for AR5006.
> >
> > Sure, that's fine. I just wanted to make sure that there weren't some
> > weird generic MSI problems.
>
> I was referring to this post...
> http://lkml.org/lkml/2008/6/24/150
Yeah, that's a real bug on x86; I don't know how widespread its effects are
though. I'll ping Ingo & Thomas again.
Jesse