| Message ID | 1526632641-30086-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Yoshihiro Shimoda <yoshihiro.shimoda.uh@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.uh@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?
Hi, > From: Felipe Balbi, Sent: Monday, May 21, 2018 3:57 PM > > Hi, > > Yoshihiro Shimoda <yoshihiro.shimoda.uh@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.uh@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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> writes: > Hi, > >> From: Felipe Balbi, Sent: Monday, May 21, 2018 3:57 PM >> >> Hi, >> >> Yoshihiro Shimoda <yoshihiro.shimoda.uh@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.uh@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". 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);
Hi, > From: Felipe Balbi, Sent: Monday, May 21, 2018 5:05 PM <snip> > 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); > > -- Thank you very much for your patch! This could resolve the issue. So, should I submit this your patch as your author? Best regards, Yoshihiro Shimoda > balbi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> writes: > Hi, > >> From: Felipe Balbi, Sent: Monday, May 21, 2018 5:05 PM > <snip> >> 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); >> >> -- > > Thank you very much for your patch! This could resolve the issue. > So, should I submit this your patch as your author? you can send it with your authorship, it's totally fine :-) You can also add my: Acked-by: Felipe Balbi <felipe.balbi@linux.intel.com> thanks
Hi, > From: Felipe Balbi, Sent: Monday, May 21, 2018 7:19 PM > > Hi, > > Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> writes: > > Hi, > > > >> From: Felipe Balbi, Sent: Monday, May 21, 2018 5:05 PM > > <snip> > >> 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); > >> > >> -- > > > > Thank you very much for your patch! This could resolve the issue. > > So, should I submit this your patch as your author? > > you can send it with your authorship, it's totally fine :-) I got it :) > You can also add my: > > Acked-by: Felipe Balbi <felipe.balbi@linux.intel.com> Thank you for your Acked-by! I'll submit v2 patch soon. Best regards, Yoshihiro Shimoda > thanks > > -- > balbi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c index d359efe..01de45e 100644 --- a/drivers/usb/gadget/function/f_printer.c +++ b/drivers/usb/gadget/function/f_printer.c @@ -640,10 +640,14 @@ static void tx_complete(struct usb_ep *ep, struct usb_request *req) spin_unlock_irqrestore(&dev->lock, flags); mutex_unlock(&dev->lock_printer_io); return -EAGAIN; + } else if (list_empty(&req->list)) { + /* + * Since usb_ep_queue() is possible to call complete, + * we should check the req->list here, and then add + * it into dev->tx_reqs_active. + */ + list_add(&req->list, &dev->tx_reqs_active); } - - list_add(&req->list, &dev->tx_reqs_active); - } spin_unlock_irqrestore(&dev->lock, flags);
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@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. drivers/usb/gadget/function/f_printer.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)