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

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

[Yoshihiro Shimoda]

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

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

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

[Felipe Balbi]

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?

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

[Yoshihiro Shimoda]

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

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

[Felipe Balbi]

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

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

[Yoshihiro Shimoda]

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

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

[Felipe Balbi]

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

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

[Yoshihiro Shimoda]

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