Hi,
Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx> writes:
> Hi,
>
>> From: Felipe Balbi, Sent: Monday, May 21, 2018 3:57 PM
>>
>> Hi,
>>
>> Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx> writes:
>> > The usb_ep_queue() in printer_write() is possible to call req->complete().
>> > In that case, since tx_complete() calls list_add(&req->list, &dev->tx_reqs),
>> > printer_write() should not call list_add(&req->list, &dev->tx_reqs_active)
>> > because the transfer has already finished. So, this patch checks
>> > the condition of req->list before adding the list in printer_write().
>> >
>> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
>> > ---
>> > This issue can be caused by renesas_usbhs udc driver. I'm not sure
>> > this patch is acceptable or not. So, I marked RFC on this patch.
>>
>> can you explain this a little more? How do you trigger the problem?
>
> Sure. If printer_write() called usb_ep_queue() with 63 bytes or less data,
> the renesas_usbhs udc driver transfers data as PIO. In this case, the udc
> driver calls usb_gadget_giveback_reuqest() in .queue ops (usbhsg_ep_queue())
> immediately. Then, printer_write() calls list_add(&req->list, &dev->tx_reqs_active); wrongly.
> After that, if we do rmmod g_printer, WARN_ON(!list_empty(&dev->tx_reqs_active); happens in
> printer_func_unbind() because the list entry is not removed.
>
> < Reference: calltrace (very long though...) >
> usb_ep_queue(...); at f_printer.c / printer_write()
> 1-> usbhsg_ep_queue(); at renesas_usbhs/mod_gadget.c
> 2-> usbhsg_queue_push();
> 3-> usbhs_pkt_start(); at renesas_usbhs/fifo.c
> 4-> usbhsf_pkt_handler();
> 5-> func() = usbhsf_dma_prepare_push();
> 6-> goto usbhsf_pio_prepare_push; // Because len is 63
> 7-> usbhsf_pio_prepare_push();
> 8-> usbhsf_pio_try_push();
> 5-> done() = usbhsg_queue_done(); at renesas_usbhs/mod_gadget.c
> 6-> __usbsg_queue_pop();
> 7-> usb_gadget_giveback_reuqest();
> 8-> tx_complete(); at f_printer.c
> 9-> list_del_init(&req->list);
> 9-> list_add(&req->list, &dev->tx_reqs);
> list_add(&req->list, &dev->tx_reqs_active); // Even if the transaction already finished, this driver is possible to add the list to "active".
seems like it would be better to just move this like before
usb_ep_queue():
modified drivers/usb/gadget/function/f_printer.c
@@ -631,19 +631,19 @@ printer_write(struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
return -EAGAIN;
}
+ list_add(&req->list, &dev->tx_reqs_active);
+
/* here, we unlock, and only unlock, to avoid deadlock. */
spin_unlock(&dev->lock);
value = usb_ep_queue(dev->in_ep, req, GFP_ATOMIC);
spin_lock(&dev->lock);
if (value) {
+ list_del(&req->list);
list_add(&req->list, &dev->tx_reqs);
spin_unlock_irqrestore(&dev->lock, flags);
mutex_unlock(&dev->lock_printer_io);
return -EAGAIN;
}
-
- list_add(&req->list, &dev->tx_reqs_active);
-
}
spin_unlock_irqrestore(&dev->lock, flags);
--
balbi
Attachment:
signature.asc
Description: PGP signature
