[PATCH/RFC] usb: gadget: function: printer: avoid wrong list handling in printer_write()

[Felipe Balbi <balbi@kernel.org>]

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);