On Thu, 6 Jun 2019 17:34:16 +0200, Lorenzo Bianconi wrote:
> When the device is disconnected while passing traffic it is possible
> to receive out of order urbs causing a memory leak since the skb liked
> to the current tx urb is not removed. Fix the issue deallocating the skb
> cleaning up the tx ring. Moreover this patch fixes the following kernel
> warning
Ugh if we don't have ordering guarantees then the entire "ring" scheme
no longer works :( Should we move to URB queues, I don't remember now,
but there seem to had been a better way to manage URBs :S
> [ 57.480771] usb 1-1: USB disconnect, device number 2
> [ 57.483451] ------------[ cut here ]------------
> [ 57.483462] TX urb mismatch
> [ 57.483481] WARNING: CPU: 1 PID: 32 at
> drivers/net/wireless/mediatek/mt7601u/dma.c:245 mt7601u_complete_tx+0x165/00
> [ 57.483483] Modules linked in:
> [ 57.483496] CPU: 1 PID: 32 Comm: kworker/1:1 Not tainted 5.2.0-rc1+ #72
> [ 57.483498] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
> 1.12.0-2.fc30 04/01/2014
> [ 57.483502] Workqueue: usb_hub_wq hub_event
> [ 57.483507] RIP: 0010:mt7601u_complete_tx+0x165/0x1e0
> [ 57.483510] Code: 8b b5 10 04 00 00 8b 8d 14 04 00 00 eb 8b 80 3d b1 cb e1
> 00 00 75 9e 48 c7 c7 a4 ea 05 82 c6 05 f
> [ 57.483513] RSP: 0000:ffffc900000a0d28 EFLAGS: 00010092
> [ 57.483516] RAX: 000000000000000f RBX: ffff88802c0a62c0 RCX:
> ffffc900000a0c2c
> [ 57.483518] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
> ffffffff810a8371
> [ 57.483520] RBP: ffff88803ced6858 R08: 0000000000000000 R09:
> 0000000000000001
> [ 57.483540] R10: 0000000000000002 R11: 0000000000000000 R12:
> 0000000000000046
> [ 57.483542] R13: ffff88802c0a6c88 R14: ffff88803baab540 R15:
> ffff88803a0cc078
> [ 57.483548] FS: 0000000000000000(0000) GS:ffff88803eb00000(0000)
> knlGS:0000000000000000
> [ 57.483550] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 57.483552] CR2: 000055e7f6780100 CR3: 0000000028c86000 CR4:
> 00000000000006a0
> [ 57.483554] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
> [ 57.483556] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> 0000000000000400
> [ 57.483559] Call Trace:
> [ 57.483561] <IRQ>
> [ 57.483565] __usb_hcd_giveback_urb+0x77/0xe0
> [ 57.483570] xhci_giveback_urb_in_irq.isra.0+0x8b/0x140
> [ 57.483574] handle_cmd_completion+0xf5b/0x12c0
> [ 57.483577] xhci_irq+0x1f6/0x1810
> [ 57.483581] ? lockdep_hardirqs_on+0x9e/0x180
> [ 57.483584] ? _raw_spin_unlock_irq+0x24/0x30
> [ 57.483588] __handle_irq_event_percpu+0x3a/0x260
> [ 57.483592] handle_irq_event_percpu+0x1c/0x60
> [ 57.483595] handle_irq_event+0x2f/0x4c
> [ 57.483599] handle_edge_irq+0x7e/0x1a0
> [ 57.483603] handle_irq+0x17/0x20
> [ 57.483607] do_IRQ+0x54/0x110
> [ 57.483610] common_interrupt+0xf/0xf
> [ 57.483612] </IRQ>
>
> Fixes: c869f77d6abb ("add mt7601u driver")
> Signed-off-by: Lorenzo Bianconi <lore...@kernel.org>
> ---
> drivers/net/wireless/mediatek/mt7601u/dma.c | 24 +++++++++++++++------
> drivers/net/wireless/mediatek/mt7601u/tx.c | 4 ++--
> 2 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.c
> b/drivers/net/wireless/mediatek/mt7601u/dma.c
> index e7703990b291..bbf1deed7f3b 100644
> --- a/drivers/net/wireless/mediatek/mt7601u/dma.c
> +++ b/drivers/net/wireless/mediatek/mt7601u/dma.c
> @@ -238,14 +238,25 @@ static void mt7601u_complete_tx(struct urb *urb)
> struct sk_buff *skb;
> unsigned long flags;
>
> - spin_lock_irqsave(&dev->tx_lock, flags);
> + switch (urb->status) {
> + case -ECONNRESET:
> + case -ESHUTDOWN:
> + case -ENOENT:
> + return;
> + default:
> + dev_err_ratelimited(dev->dev, "tx urb failed: %d\n",
> + urb->status);
> + /* fall through */
> + case 0:
> + break;
> + }
>
> - if (mt7601u_urb_has_error(urb))
> - dev_err(dev->dev, "Error: TX urb failed:%d\n", urb->status);
> + spin_lock_irqsave(&dev->tx_lock, flags);
> if (WARN_ONCE(q->e[q->start].urb != urb, "TX urb mismatch"))
> goto out;
>
> skb = q->e[q->start].skb;
> + q->e[q->start].skb = NULL;
> trace_mt_tx_dma_done(dev, skb);
>
> __skb_queue_tail(&dev->tx_skb_done, skb);
> @@ -446,10 +457,10 @@ static void mt7601u_free_tx_queue(struct
> mt7601u_tx_queue *q)
> {
> int i;
>
> - WARN_ON(q->used);
> -
> for (i = 0; i < q->entries; i++) {
> usb_poison_urb(q->e[i].urb);
> + if (q->e[i].skb)
> + mt7601u_tx_status(q->dev, q->e[i].skb);
Perhaps a separate patch?
> usb_free_urb(q->e[i].urb);
> }
> }
> @@ -463,6 +474,7 @@ static void mt7601u_free_tx(struct mt7601u_dev *dev)
>
> for (i = 0; i < __MT_EP_OUT_MAX; i++)
> mt7601u_free_tx_queue(&dev->tx_q[i]);
> + tasklet_kill(&dev->tx_tasklet);
> }
>
> static int mt7601u_alloc_tx_queue(struct mt7601u_dev *dev,
> @@ -528,6 +540,4 @@ void mt7601u_dma_cleanup(struct mt7601u_dev *dev)
>
> mt7601u_free_rx(dev);
> mt7601u_free_tx(dev);
> -
> - tasklet_kill(&dev->tx_tasklet);
> }
> diff --git a/drivers/net/wireless/mediatek/mt7601u/tx.c
> b/drivers/net/wireless/mediatek/mt7601u/tx.c
> index 3600e911a63e..4d81c45722fb 100644
> --- a/drivers/net/wireless/mediatek/mt7601u/tx.c
> +++ b/drivers/net/wireless/mediatek/mt7601u/tx.c
> @@ -117,9 +117,9 @@ void mt7601u_tx_status(struct mt7601u_dev *dev, struct
> sk_buff *skb)
> info->status.rates[0].idx = -1;
> info->flags |= IEEE80211_TX_STAT_ACK;
>
> - spin_lock(&dev->mac_lock);
> + spin_lock_bh(&dev->mac_lock);
> ieee80211_tx_status(dev->hw, skb);
> - spin_unlock(&dev->mac_lock);
> + spin_unlock_bh(&dev->mac_lock);
> }
>
> static int mt7601u_skb_rooms(struct mt7601u_dev *dev, struct sk_buff *skb)
