Hi,

> From: Felipe Balbi, Sent: Monday, May 21, 2018 3:57 PM
> 
> Hi,
> 
> Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com> 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...@renesas.com>
> > ---
> >  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".

Best regards,
Yoshihiro Shimoda

> --
> balbi

Reply via email to