[bug report] usb: gadget: u_serial: process RX in workqueue instead of tasklet

[bug report] usb: gadget: u_serial: process RX in workqueue instead of tasklet

[Dan Carpenter]

Hello Michał Mirosław,

This is a semi-automatic email about new static checker warnings.

The patch 8b4c62aef6f6: "usb: gadget: u_serial: process RX in
workqueue instead of tasklet" from Dec 16, 2018, leads to the
following Smatch complaint:

    drivers/usb/gadget/function/u_serial.c:438 gs_rx_push()
    error: we previously assumed 'tty' could be null (see line 373)

drivers/usb/gadget/function/u_serial.c
   372			/* leave data queued if tty was rx throttled */
   373			if (tty && tty_throttled(tty))
                            ^^^^^^^^^^^^^^^^^^^^^^^^
Other checks for NULL

   374				break;
   375	
   376			switch (req->status) {
   377			case -ESHUTDOWN:
   378				disconnect = true;
   379				pr_vdebug("ttyGS%d: shutdown\n", port->port_num);
   380				break;
   381	
   382			default:
   383				/* presumably a transient fault */
   384				pr_warn("ttyGS%d: unexpected RX status %d\n",
   385					port->port_num, req->status);
   386				/* FALLTHROUGH */
   387			case 0:
   388				/* normal completion */
   389				break;
   390			}
   391	
   392			/* push data to (open) tty */
   393			if (req->actual && tty) {
   394				char		*packet = req->buf;
   395				unsigned	size = req->actual;
   396				unsigned	n;
   397				int		count;
   398	
   399				/* we may have pushed part of this packet already... */
   400				n = port->n_read;
   401				if (n) {
   402					packet += n;
   403					size -= n;
   404				}
   405	
   406				count = tty_insert_flip_string(&port->port, packet,
   407						size);
   408				if (count)
   409					do_push = true;
   410				if (count != size) {
   411					/* stop pushing; TTY layer can't handle more */
   412					port->n_read += count;
   413					pr_vdebug("ttyGS%d: rx block %d/%d\n",
   414						  port->port_num, count, req->actual);
   415					break;
   416				}
   417				port->n_read = 0;
   418			}
   419	
   420			list_move(&req->list, &port->read_pool);
   421			port->read_started--;
   422		}
   423	
   424		/* Push from tty to ldisc; this is handled by a workqueue,
   425		 * so we won't get callbacks and can hold port_lock
   426		 */
   427		if (do_push)
   428			tty_flip_buffer_push(&port->port);
   429	
   430	
   431		/* We want our data queue to become empty ASAP, keeping data
   432		 * in the tty and ldisc (not here).  If we couldn't push any
   433		 * this time around, RX may be starved, so wait until next jiffy.
   434		 *
   435		 * We may leave non-empty queue only when there is a tty, and
   436		 * either it is throttled or there is no more room in flip buffer.
   437		 */
   438		if (!list_empty(queue) && !tty_throttled(tty))
                                          ^^^^^^^^^^^^^^^^^^^
in the original code there was check for NULL here but the patch removed
it.

   439			schedule_delayed_work(&port->push, 1);
   440	

regards,
dan carpenter

Re: [bug report] usb: gadget: u_serial: process RX in workqueue instead of tasklet

[mirq-linux]

On Mon, Jun 24, 2019 at 03:32:58PM +0300, Dan Carpenter wrote:
> Hello Michał Mirosław,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch 8b4c62aef6f6: "usb: gadget: u_serial: process RX in
> workqueue instead of tasklet" from Dec 16, 2018, leads to the
> following Smatch complaint:
[...] 
>    431		/* We want our data queue to become empty ASAP, keeping data
>    432		 * in the tty and ldisc (not here).  If we couldn't push any
>    433		 * this time around, RX may be starved, so wait until next jiffy.
>    434		 *
>    435		 * We may leave non-empty queue only when there is a tty, and
>    436		 * either it is throttled or there is no more room in flip buffer.
>    437		 */
>    438		if (!list_empty(queue) && !tty_throttled(tty))
>                                           ^^^^^^^^^^^^^^^^^^^
> in the original code there was check for NULL here but the patch removed
> it.
> 
>    439			schedule_delayed_work(&port->push, 1);
>    440	

Hi Dan,

The code is correct and explained in the comment above it - while() loop
above can be exited before emptying the queue only when tty != NULL.

Best Regards,
Michał Mirosław

Re: [bug report] usb: gadget: u_serial: process RX in workqueue instead of tasklet

[Dan Carpenter]

On Mon, Jun 24, 2019 at 03:41:54PM +0200, mirq-linux@rere.qmqm.pl wrote:
> On Mon, Jun 24, 2019 at 03:32:58PM +0300, Dan Carpenter wrote:
> > Hello Michał Mirosław,
> > 
> > This is a semi-automatic email about new static checker warnings.
> > 
> > The patch 8b4c62aef6f6: "usb: gadget: u_serial: process RX in
> > workqueue instead of tasklet" from Dec 16, 2018, leads to the
> > following Smatch complaint:
> [...] 
> >    431		/* We want our data queue to become empty ASAP, keeping data
> >    432		 * in the tty and ldisc (not here).  If we couldn't push any
> >    433		 * this time around, RX may be starved, so wait until next jiffy.
> >    434		 *
> >    435		 * We may leave non-empty queue only when there is a tty, and
> >    436		 * either it is throttled or there is no more room in flip buffer.
> >    437		 */
> >    438		if (!list_empty(queue) && !tty_throttled(tty))
> >                                           ^^^^^^^^^^^^^^^^^^^
> > in the original code there was check for NULL here but the patch removed
> > it.
> > 
> >    439			schedule_delayed_work(&port->push, 1);
> >    440	
> 
> Hi Dan,
> 
> The code is correct and explained in the comment above it - while() loop
> above can be exited before emptying the queue only when tty != NULL.
> 


:(  Sorry...  Smatch isn't supposed to generate those warnings.  I don't
know exactly what happened.  I thought it was because of some recent
code changes I had made but now I think it it's because my build system
had a disk failure...  Anyway, I can't reproduce the warning any more.

Thanks for looking at this.

regards,
dan carpenter