On Thu, 6 Jun 2019 14:26:12 +0200, Lorenzo Bianconi wrote:
> Do not schedule rx_tasklet when the usb dongle is disconnected. This
> patch fixes the common kernel warning reported when the device is
> removed.
>
> [ 24.921354] usb 3-14: USB disconnect, device number 7
> [ 24.921593] ------------[ cut here ]------------
> [ 24.921594] RX urb mismatch
> [ 24.921675] WARNING: CPU: 4 PID: 163 at drivers/net/wireless/mediatek/mt7601u/dma.c:200 mt7601u_complete_rx+0xcb/0xd0 [mt7601u]
> [ 24.921769] CPU: 4 PID: 163 Comm: kworker/4:2 Tainted: G OE 4.19.31-041931-generic #201903231635
> [ 24.921770] Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z97 Extreme4, BIOS P1.30 05/23/2014
> [ 24.921782] Workqueue: usb_hub_wq hub_event
> [ 24.921797] RIP: 0010:mt7601u_complete_rx+0xcb/0xd0 [mt7601u]
> [ 24.921800] RSP: 0018:ffff9bd9cfd03d08 EFLAGS: 00010086
> [ 24.921802] RAX: 0000000000000000 RBX: ffff9bd9bf043540 RCX: 0000000000000006
> [ 24.921803] RDX: 0000000000000007 RSI: 0000000000000096 RDI: ffff9bd9cfd16420
> [ 24.921804] RBP: ffff9bd9cfd03d28 R08: 0000000000000002 R09: 00000000000003a8
> [ 24.921805] R10: 0000002f485fca34 R11: 0000000000000000 R12: ffff9bd9bf043c1c
> [ 24.921806] R13: ffff9bd9c62fa3c0 R14: 0000000000000082 R15: 0000000000000000
> [ 24.921807] FS: 0000000000000000(0000) GS:ffff9bd9cfd00000(0000) knlGS:0000000000000000
> [ 24.921808] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 24.921808] CR2: 00007fb2648b0000 CR3: 0000000142c0a004 CR4: 00000000001606e0
> [ 24.921809] Call Trace:
> [ 24.921812] <IRQ>
> [ 24.921819] __usb_hcd_giveback_urb+0x8b/0x140
> [ 24.921821] usb_hcd_giveback_urb+0xca/0xe0
> [ 24.921828] xhci_giveback_urb_in_irq.isra.42+0x82/0xf0
> [ 24.921834] handle_cmd_completion+0xe02/0x10d0
> [ 24.921837] xhci_irq+0x274/0x4a0
> [ 24.921838] xhci_msi_irq+0x11/0x20
> [ 24.921851] __handle_irq_event_percpu+0x44/0x190
> [ 24.921856] handle_irq_event_percpu+0x32/0x80
> [ 24.921861] handle_irq_event+0x3b/0x5a
> [ 24.921867] handle_edge_irq+0x80/0x190
> [ 24.921874] handle_irq+0x20/0x30
> [ 24.921889] do_IRQ+0x4e/0xe0
> [ 24.921891] common_interrupt+0xf/0xf
> [ 24.921892] </IRQ>
> [ 24.921900] RIP: 0010:usb_hcd_flush_endpoint+0x78/0x180
> [ 24.921354] usb 3-14: USB disconnect, device number 7
Is this a new thing? I def tested unplugging the dongle under
traffic, but that must had been in 3.19 days :S
> Fixes: c869f77d6abb ("add mt7601u driver")
> Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> ---
> I will post a patch to fix tx side as well
> ---
> drivers/net/wireless/mediatek/mt7601u/dma.c | 33 ++++++++++-----------
> 1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.c b/drivers/net/wireless/mediatek/mt7601u/dma.c
> index f7edeffb2b19..e7703990b291 100644
> --- a/drivers/net/wireless/mediatek/mt7601u/dma.c
> +++ b/drivers/net/wireless/mediatek/mt7601u/dma.c
> @@ -193,10 +193,20 @@ static void mt7601u_complete_rx(struct urb *urb)
> struct mt7601u_rx_queue *q = &dev->rx_q;
> unsigned long flags;
>
> - spin_lock_irqsave(&dev->rx_lock, flags);
> + switch (urb->status) {
> + case -ECONNRESET:
> + case -ESHUTDOWN:
> + case -ENOENT:
> + return;
So we assume this is non-recoverable? Everything will fail after?
Because pending is incremented linearly :S That's why there is a
warning here.
> + default:
> + dev_err_ratelimited(dev->dev, "rx urb failed: %d\n",
> + urb->status);
> + /* fall through */
> + case 0:
> + break;
> + }
>
> - if (mt7601u_urb_has_error(urb))
> - dev_err(dev->dev, "Error: RX urb failed:%d\n", urb->status);
> + spin_lock_irqsave(&dev->rx_lock, flags);
> if (WARN_ONCE(q->e[q->end].urb != urb, "RX urb mismatch"))
> goto out;
>
> @@ -363,19 +373,10 @@ int mt7601u_dma_enqueue_tx(struct mt7601u_dev *dev, struct sk_buff *skb,
> static void mt7601u_kill_rx(struct mt7601u_dev *dev)
> {
> int i;
> - unsigned long flags;
> -
> - spin_lock_irqsave(&dev->rx_lock, flags);
>
> - for (i = 0; i < dev->rx_q.entries; i++) {
> - int next = dev->rx_q.end;
> -
> - spin_unlock_irqrestore(&dev->rx_lock, flags);
> - usb_poison_urb(dev->rx_q.e[next].urb);
> - spin_lock_irqsave(&dev->rx_lock, flags);
> - }
Why is there no need to take the lock? Admittedly it's not clear what
this lock is protecting here :P Perhaps a separate patch to remove the
unnecessary locking with an explanation?
> - spin_unlock_irqrestore(&dev->rx_lock, flags);
> + for (i = 0; i < dev->rx_q.entries; i++)
> + usb_poison_urb(dev->rx_q.e[i].urb);
> + tasklet_kill(&dev->rx_tasklet);
> }
>
> static int mt7601u_submit_rx_buf(struct mt7601u_dev *dev,
> @@ -525,8 +526,6 @@ void mt7601u_dma_cleanup(struct mt7601u_dev *dev)
> {
> mt7601u_kill_rx(dev);
>
> - tasklet_kill(&dev->rx_tasklet);
Why the move? Looks a bit unnecessary..
> mt7601u_free_rx(dev);
> mt7601u_free_tx(dev);
>
