* drivers/net/wireless/p54/p54pci.c: Adjusted for deblobbed sources. From: Christian Lamparter Date: Sun, 17 Jan 2010 22:19:25 +0000 (+0100) Subject: p54pci: move tx cleanup into tasklet X-Git-Tag: master-2010-01-19~9 X-Git-Url: http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Flinville%2Fwireless-next-2.6.git;a=commitdiff_plain;h=d713804c6032b95cd3035014e16fadebb9655c6f p54pci: move tx cleanup into tasklet This patch moves the tx cleanup routines out of the critical interrupt context and into the (previously known as rx) tasklet. The main goal of this operation is to remove the extensive usage of spin_lock_irqsaves in the generic p54common library. The next step would be to modify p54usb to do the rx processing inside a tasklet (just like usbnet). Signed-off-by: Christian Lamparter Signed-off-by: John W. Linville --- diff --git a/drivers/net/wireless/p54/p54pci.c b/drivers/net/wireless/p54/p54pci.c index 4bf4c21..48cae48 100644 --- a/drivers/net/wireless/p54/p54pci.c +++ b/drivers/net/wireless/p54/p54pci.c @@ -234,25 +234,26 @@ static void p54p_check_rx_ring(struct ieee80211_hw *dev, u32 *index, p54p_refill_rx_ring(dev, ring_index, ring, ring_limit, rx_buf); } -/* caller must hold priv->lock */ static void p54p_check_tx_ring(struct ieee80211_hw *dev, u32 *index, int ring_index, struct p54p_desc *ring, u32 ring_limit, - void **tx_buf) + struct sk_buff **tx_buf) { + unsigned long flags; struct p54p_priv *priv = dev->priv; struct p54p_ring_control *ring_control = priv->ring_control; struct p54p_desc *desc; + struct sk_buff *skb; u32 idx, i; i = (*index) % ring_limit; (*index) = idx = le32_to_cpu(ring_control->device_idx[ring_index]); idx %= ring_limit; + spin_lock_irqsave(&priv->lock, flags); while (i != idx) { desc = &ring[i]; - if (tx_buf[i]) - if (FREE_AFTER_TX((struct sk_buff *) tx_buf[i])) - p54_free_skb(dev, tx_buf[i]); + + skb = tx_buf[i]; tx_buf[i] = NULL; pci_unmap_single(priv->pdev, le32_to_cpu(desc->host_addr), @@ -263,17 +264,32 @@ static void p54p_check_tx_ring(struct ieee80211_hw *dev, u32 *index, desc->len = 0; desc->flags = 0; + if (skb && FREE_AFTER_TX(skb)) { + spin_unlock_irqrestore(&priv->lock, flags); + p54_free_skb(dev, skb); + spin_lock_irqsave(&priv->lock, flags); + } + i++; i %= ring_limit; } + spin_unlock_irqrestore(&priv->lock, flags); } -static void p54p_rx_tasklet(unsigned long dev_id) +static void p54p_tasklet(unsigned long dev_id) { struct ieee80211_hw *dev = (struct ieee80211_hw *)dev_id; struct p54p_priv *priv = dev->priv; struct p54p_ring_control *ring_control = priv->ring_control; + p54p_check_tx_ring(dev, &priv->tx_idx_mgmt, 3, ring_control->tx_mgmt, + ARRAY_SIZE(ring_control->tx_mgmt), + priv->tx_buf_mgmt); + + p54p_check_tx_ring(dev, &priv->tx_idx_data, 1, ring_control->tx_data, + ARRAY_SIZE(ring_control->tx_data), + priv->tx_buf_data); + p54p_check_rx_ring(dev, &priv->rx_idx_mgmt, 2, ring_control->rx_mgmt, ARRAY_SIZE(ring_control->rx_mgmt), priv->rx_buf_mgmt); @@ -288,38 +304,24 @@ static irqreturn_t p54p_interrupt(int irq, void *dev_id) { struct ieee80211_hw *dev = dev_id; struct p54p_priv *priv = dev->priv; - struct p54p_ring_control *ring_control = priv->ring_control; __le32 reg; spin_lock(&priv->lock); reg = P54P_READ(int_ident); if (unlikely(reg == cpu_to_le32(0xFFFFFFFF))) { - spin_unlock(&priv->lock); - return IRQ_HANDLED; + goto out; } - P54P_WRITE(int_ack, reg); reg &= P54P_READ(int_enable); - if (reg & cpu_to_le32(ISL38XX_INT_IDENT_UPDATE)) { - p54p_check_tx_ring(dev, &priv->tx_idx_mgmt, - 3, ring_control->tx_mgmt, - ARRAY_SIZE(ring_control->tx_mgmt), - priv->tx_buf_mgmt); - - p54p_check_tx_ring(dev, &priv->tx_idx_data, - 1, ring_control->tx_data, - ARRAY_SIZE(ring_control->tx_data), - priv->tx_buf_data); - - tasklet_schedule(&priv->rx_tasklet); - - } else if (reg & cpu_to_le32(ISL38XX_INT_IDENT_INIT)) + if (reg & cpu_to_le32(ISL38XX_INT_IDENT_UPDATE)) + tasklet_schedule(&priv->tasklet); + else if (reg & cpu_to_le32(ISL38XX_INT_IDENT_INIT)) complete(&priv->boot_comp); +out: spin_unlock(&priv->lock); - return reg ? IRQ_HANDLED : IRQ_NONE; } @@ -368,7 +370,7 @@ static void p54p_stop(struct ieee80211_hw *dev) unsigned int i; struct p54p_desc *desc; - tasklet_kill(&priv->rx_tasklet); + tasklet_kill(&priv->tasklet); P54P_WRITE(int_enable, cpu_to_le32(0)); P54P_READ(int_enable); @@ -559,7 +561,7 @@ static int __devinit p54p_probe(struct pci_dev *pdev, priv->common.tx = p54p_tx; spin_lock_init(&priv->lock); - tasklet_init(&priv->rx_tasklet, p54p_rx_tasklet, (unsigned long)dev); + tasklet_init(&priv->tasklet, p54p_tasklet, (unsigned long)dev); err = reject_firmware(&priv->firmware, "/*(DEBLOBBED)*/pci", &priv->pdev->dev); diff --git a/drivers/net/wireless/p54/p54pci.h b/drivers/net/wireless/p54/p54pci.h index fbb6839..2feead6 100644 --- a/drivers/net/wireless/p54/p54pci.h +++ b/drivers/net/wireless/p54/p54pci.h @@ -92,7 +92,7 @@ struct p54p_priv { struct p54_common common; struct pci_dev *pdev; struct p54p_csr __iomem *map; - struct tasklet_struct rx_tasklet; + struct tasklet_struct tasklet; const struct firmware *firmware; spinlock_t lock; struct p54p_ring_control *ring_control; @@ -101,8 +101,8 @@ struct p54p_priv { u32 rx_idx_mgmt, tx_idx_mgmt; struct sk_buff *rx_buf_data[8]; struct sk_buff *rx_buf_mgmt[4]; - void *tx_buf_data[32]; - void *tx_buf_mgmt[4]; + struct sk_buff *tx_buf_data[32]; + struct sk_buff *tx_buf_mgmt[4]; struct completion boot_comp; }; From: Christian Lamparter Date: Fri, 22 Jan 2010 07:01:11 +0000 (+0100) Subject: p54pci: revise tx locking X-Git-Tag: master-2010-01-22~1 X-Git-Url: http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Flinville%2Fwireless-next-2.6.git;a=commitdiff_plain;h=b92f7d30830a319148df2943b7565989494e5ad1 p54pci: revise tx locking This patch continues the effort which began with: "[PATCH] p54pci: move tx cleanup into tasklet". Thanks to these changes, p54pci's interrupt & tx cleanup routines can be made lock-less. Signed-off-by: Christian Lamparter Signed-off-by: John W. Linville --- diff --git a/drivers/net/wireless/p54/p54pci.c b/drivers/net/wireless/p54/p54pci.c index 48cae48..bda29c0 100644 --- a/drivers/net/wireless/p54/p54pci.c +++ b/drivers/net/wireless/p54/p54pci.c @@ -238,7 +238,6 @@ static void p54p_check_tx_ring(struct ieee80211_hw *dev, u32 *index, int ring_index, struct p54p_desc *ring, u32 ring_limit, struct sk_buff **tx_buf) { - unsigned long flags; struct p54p_priv *priv = dev->priv; struct p54p_ring_control *ring_control = priv->ring_control; struct p54p_desc *desc; @@ -249,7 +248,6 @@ static void p54p_check_tx_ring(struct ieee80211_hw *dev, u32 *index, (*index) = idx = le32_to_cpu(ring_control->device_idx[ring_index]); idx %= ring_limit; - spin_lock_irqsave(&priv->lock, flags); while (i != idx) { desc = &ring[i]; @@ -264,16 +262,12 @@ static void p54p_check_tx_ring(struct ieee80211_hw *dev, u32 *index, desc->len = 0; desc->flags = 0; - if (skb && FREE_AFTER_TX(skb)) { - spin_unlock_irqrestore(&priv->lock, flags); + if (skb && FREE_AFTER_TX(skb)) p54_free_skb(dev, skb); - spin_lock_irqsave(&priv->lock, flags); - } i++; i %= ring_limit; } - spin_unlock_irqrestore(&priv->lock, flags); } static void p54p_tasklet(unsigned long dev_id) @@ -306,7 +300,6 @@ static irqreturn_t p54p_interrupt(int irq, void *dev_id) struct p54p_priv *priv = dev->priv; __le32 reg; - spin_lock(&priv->lock); reg = P54P_READ(int_ident); if (unlikely(reg == cpu_to_le32(0xFFFFFFFF))) { goto out; @@ -321,15 +314,14 @@ static irqreturn_t p54p_interrupt(int irq, void *dev_id) complete(&priv->boot_comp); out: - spin_unlock(&priv->lock); return reg ? IRQ_HANDLED : IRQ_NONE; } static void p54p_tx(struct ieee80211_hw *dev, struct sk_buff *skb) { + unsigned long flags; struct p54p_priv *priv = dev->priv; struct p54p_ring_control *ring_control = priv->ring_control; - unsigned long flags; struct p54p_desc *desc; dma_addr_t mapping; u32 device_idx, idx, i; @@ -370,14 +362,14 @@ static void p54p_stop(struct ieee80211_hw *dev) unsigned int i; struct p54p_desc *desc; - tasklet_kill(&priv->tasklet); - P54P_WRITE(int_enable, cpu_to_le32(0)); P54P_READ(int_enable); udelay(10); free_irq(priv->pdev->irq, dev); + tasklet_kill(&priv->tasklet); + P54P_WRITE(dev_int, cpu_to_le32(ISL38XX_DEV_INT_RESET)); for (i = 0; i < ARRAY_SIZE(priv->rx_buf_data); i++) { From: Quintin Pitts Date: Fri, 9 Apr 2010 19:37:38 +0000 (+0200) Subject: p54pci: prevent stuck rx-ring on slow system X-Git-Tag: master-2010-04-12~9 X-Git-Url: http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Flinville%2Fwireless-next-2.6.git;a=commitdiff_plain;h=5988f385b4cffa9ca72c5be0188e5f4c9ef46d82 p54pci: prevent stuck rx-ring on slow system This patch fixes an old problem, which - under certain circumstances - could cause the device to become unresponsive. most of p54pci's rx-ring management is implemented in just two distinct standalone functions. p54p_check_rx_ring takes care of processing incoming data, while p54p_refill_rx_ring tries to replenish all depleted communication buffers. This has always worked fine on my fast machine, but now I know there is a hidden race... The most likely candidate here is ring_control->device_idx. Quintin Pitts had already analyzed the culprit and posted a patch back in Oct 2009. But sadly, no one's picked up on this. ( https://patchwork.kernel.org/patch/53079/ [2 & 3] ). This patch does the same way, except that it also prioritize rx data processing, simply because tx routines *can* wait. Reported-by: Sean Young Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=11386 Reported-by: Quintin Pitts Signed-off-by: Quintin Pitts Signed-off-by: Christian Lamparter Signed-off-by: John W. Linville --- diff --git a/drivers/net/wireless/p54/p54pci.c b/drivers/net/wireless/p54/p54pci.c index ed4bdff..aa29663 100644 --- a/drivers/net/wireless/p54/p54pci.c +++ b/drivers/net/wireless/p54/p54pci.c @@ -131,7 +131,7 @@ static int p54p_upload_firmware(struct ieee80211_hw *dev) static void p54p_refill_rx_ring(struct ieee80211_hw *dev, int ring_index, struct p54p_desc *ring, u32 ring_limit, - struct sk_buff **rx_buf) + struct sk_buff **rx_buf, u32 index) { struct p54p_priv *priv = dev->priv; struct p54p_ring_control *ring_control = priv->ring_control; @@ -139,7 +139,7 @@ static void p54p_refill_rx_ring(struct ieee80211_hw *dev, idx = le32_to_cpu(ring_control->host_idx[ring_index]); limit = idx; - limit -= le32_to_cpu(ring_control->device_idx[ring_index]); + limit -= le32_to_cpu(index); limit = ring_limit - limit; i = idx % ring_limit; @@ -231,7 +231,7 @@ static void p54p_check_rx_ring(struct ieee80211_hw *dev, u32 *index, i %= ring_limit; } - p54p_refill_rx_ring(dev, ring_index, ring, ring_limit, rx_buf); + p54p_refill_rx_ring(dev, ring_index, ring, ring_limit, rx_buf, *index); } static void p54p_check_tx_ring(struct ieee80211_hw *dev, u32 *index, @@ -276,14 +276,6 @@ static void p54p_tasklet(unsigned long dev_id) struct p54p_priv *priv = dev->priv; struct p54p_ring_control *ring_control = priv->ring_control; - p54p_check_tx_ring(dev, &priv->tx_idx_mgmt, 3, ring_control->tx_mgmt, - ARRAY_SIZE(ring_control->tx_mgmt), - priv->tx_buf_mgmt); - - p54p_check_tx_ring(dev, &priv->tx_idx_data, 1, ring_control->tx_data, - ARRAY_SIZE(ring_control->tx_data), - priv->tx_buf_data); - p54p_check_rx_ring(dev, &priv->rx_idx_mgmt, 2, ring_control->rx_mgmt, ARRAY_SIZE(ring_control->rx_mgmt), priv->rx_buf_mgmt); @@ -292,6 +284,14 @@ static void p54p_tasklet(unsigned long dev_id) wmb(); P54P_WRITE(dev_int, cpu_to_le32(ISL38XX_DEV_INT_UPDATE)); + + p54p_check_tx_ring(dev, &priv->tx_idx_mgmt, 3, ring_control->tx_mgmt, + ARRAY_SIZE(ring_control->tx_mgmt), + priv->tx_buf_mgmt); + + p54p_check_tx_ring(dev, &priv->tx_idx_data, 1, ring_control->tx_data, + ARRAY_SIZE(ring_control->tx_data), + priv->tx_buf_data); } static irqreturn_t p54p_interrupt(int irq, void *dev_id) @@ -444,10 +444,10 @@ static int p54p_open(struct ieee80211_hw *dev) priv->rx_idx_mgmt = priv->tx_idx_mgmt = 0; p54p_refill_rx_ring(dev, 0, priv->ring_control->rx_data, - ARRAY_SIZE(priv->ring_control->rx_data), priv->rx_buf_data); + ARRAY_SIZE(priv->ring_control->rx_data), priv->rx_buf_data, 0); p54p_refill_rx_ring(dev, 2, priv->ring_control->rx_mgmt, - ARRAY_SIZE(priv->ring_control->rx_mgmt), priv->rx_buf_mgmt); + ARRAY_SIZE(priv->ring_control->rx_mgmt), priv->rx_buf_mgmt, 0); P54P_WRITE(ring_control_base, cpu_to_le32(priv->ring_control_dma)); P54P_READ(ring_control_base); From: Christian Lamparter Date: Thu, 15 Apr 2010 12:17:07 +0000 (+0200) Subject: p54pci: fix serious sparse warning X-Git-Tag: master-2010-04-16~117 X-Git-Url: http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Flinville%2Fwireless-next-2.6.git;a=commitdiff_plain;h=103823db62ffca028c7a214c80266519d2ea7d8d p54pci: fix serious sparse warning This patch fixes a bug which was just recently introduced by ("p54pci: prevent stuck rx-ring on slow system"). make M=drivers/net/wireless/p54 C=2 CF=-D__CHECK_ENDIAN__ CHECK drivers/net/wireless/p54/p54pci.c drivers/net/wireless/p54/p54pci.c:143:11: warning: cast to restricted __le32 CC [M] drivers/net/wireless/p54/p54pci.o Reported-by: Johannes Berg Signed-off-by: Christian Lamparter Signed-off-by: John W. Linville --- diff --git a/drivers/net/wireless/p54/p54pci.c b/drivers/net/wireless/p54/p54pci.c index aa29663..0a516c8 100644 --- a/drivers/net/wireless/p54/p54pci.c +++ b/drivers/net/wireless/p54/p54pci.c @@ -139,7 +139,7 @@ static void p54p_refill_rx_ring(struct ieee80211_hw *dev, idx = le32_to_cpu(ring_control->host_idx[ring_index]); limit = idx; - limit -= le32_to_cpu(index); + limit -= index; limit = ring_limit - limit; i = idx % ring_limit; Subject: [PATCH 2/2] p54pci: fix regression from prevent stuck rx-ring on slow system From: Christian Lamparter Date: Thu, 22 Apr 2010 19:52:43 +0200 To: linux-wireless@vger.kernel.org CC: linville@tuxdriver.com, hdegoede@redhat.com From: Hans de Goede This patch fixes a recently introduced use-after-free regression from "p54pci: prevent stuck rx-ring on slow system". Hans de Goede reported a use-after-free regression: > >BUG: unable to handle kernel paging request at 6b6b6b6b > >IP: [] p54p_check_tx_ring+0x84/0xb1 [p54pci] > >*pde = 00000000 > >Oops: 0000 [#1] SMP > >EIP: 0060:[] EFLAGS: 00010286 CPU: 0 > >EIP is at p54p_check_tx_ring+0x84/0xb1 [p54pci] > >EAX: 6b6b6b6b EBX: df10b170 ECX: 00000003 EDX: 00000001 > >ESI: dc471500 EDI: d8acaeb0 EBP: c098be9c ESP: c098be84 > > DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 > >Process swapper (pid: 0, ti=c098a000 task=c09ccfe0 task.ti=c098a000) > >Call Trace: > > [] ? p54p_tasklet+0xaa/0xb5 [p54pci] > > [] ? tasklet_action+0x78/0xcb > > [] ? __do_softirq+0xbc/0x173 Quote from comment #17: "The problem is the innocent looking moving of the tx processing to after the rx processing in the tasklet. Quoting from the changelog: This patch does it the same way, except that it also prioritize rx data processing, simply because tx routines *can* wait. This is causing an issue with us referencing already freed memory, because some skb's we transmit, we immediately receive back, such as those for reading the eeprom (*) and getting stats. What can happen because of the moving of the tx processing to after the rx processing is that when the tasklet first runs after doing a special skb tx (such as eeprom) we've already received the answer to it. Then the rx processing ends up calling p54_find_and_unlink_skb to find the matching tx skb for the just received special rx skb and frees the tx skb. Then after the processing of the rx skb answer, and thus freeing the tx skb, we go process the completed tx ring entires, and then dereference the free-ed skb, to see if it should free free-ed by p54p_check_tx_ring()." Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=583623 Bug-Identified-by: Hans de Goede Signed-off-by: Hans de Goede Signed-off-by: Christian Lamparter --- diff --git a/drivers/net/wireless/p54/p54pci.c b/drivers/net/wireless/p54/p54pci.c index ca42ccb..07c4528 100644 --- a/drivers/net/wireless/p54/p54pci.c +++ b/drivers/net/wireless/p54/p54pci.c @@ -277,6 +277,14 @@ static void p54p_tasklet(unsigned long dev_id) struct p54p_priv *priv = dev->priv; struct p54p_ring_control *ring_control = priv->ring_control; + p54p_check_tx_ring(dev, &priv->tx_idx_mgmt, 3, ring_control->tx_mgmt, + ARRAY_SIZE(ring_control->tx_mgmt), + priv->tx_buf_mgmt); + + p54p_check_tx_ring(dev, &priv->tx_idx_data, 1, ring_control->tx_data, + ARRAY_SIZE(ring_control->tx_data), + priv->tx_buf_data); + p54p_check_rx_ring(dev, &priv->rx_idx_mgmt, 2, ring_control->rx_mgmt, ARRAY_SIZE(ring_control->rx_mgmt), priv->rx_buf_mgmt); @@ -285,14 +293,6 @@ static void p54p_tasklet(unsigned long dev_id) wmb(); P54P_WRITE(dev_int, cpu_to_le32(ISL38XX_DEV_INT_UPDATE)); - - p54p_check_tx_ring(dev, &priv->tx_idx_mgmt, 3, ring_control->tx_mgmt, - ARRAY_SIZE(ring_control->tx_mgmt), - priv->tx_buf_mgmt); - - p54p_check_tx_ring(dev, &priv->tx_idx_data, 1, ring_control->tx_data, - ARRAY_SIZE(ring_control->tx_data), - priv->tx_buf_data); } static irqreturn_t p54p_interrupt(int irq, void *dev_id)