>
> On Thu, 6 Jun 2019 23:10:15 +0200, Lorenzo Bianconi wrote:
> > On Thu, Jun 6, 2019 at 8:49 PM Jakub Kicinski <kubakici@xxxxx> wrote:
> > >
> > > 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
> > >
> >
> > actually I have observed these issues on tx/rx side just during device
> > disconnection while passing traffic.
> > I guess we can assume a proper urb ordering during normal operation
> > (and so tx/rx ring works fine).
>
> Ah, phew, okay. That's fine then.
>
> > Btw what do you mean with 'URB queues'? :)
>
> I think it may have been URB anchors. I remember looking at carl9170
> and liking how the DMAs operated there.
Uhm, interesting. AFAIK urb anchors are used to cease I/O operations
but maybe they can be used even for other purposes (it will be
interesting even for mt76).
I guess this can be a future improvement, do you agree? Do I need to
resubmit this patch?
Regards,
Lorenzo
>
> > > > [ 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 <lorenzo@xxxxxxxxxx>
> > > > ---
> > > > 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?
> >
> > As I did for rx side, if we do not schedule the tx tasklet when the
> > device has been disconnected I guess we need this chuck in the same
> > patch. What do you think?
>
> Yeah, I see.
