Re: [RESEND PATCH V3 1/3] USB: serial: f81232: clear overrun flag

[Ji-Ze Hong (Peter Hong)]

Oliver Neukum æ 2019/4/1 äå 04:34 åé:
> On Mo, 2019-04-01 at 14:00 +0800,  Ji-Ze Hong (Peter Hong)  wrote:
>
> Hi,
> I am afraid there is a race condiion in this code.
>
>
> > @@ -315,6 +318,7 @@ static void f81232_process_read_urb(struct urb *urb)
> >   
> >   			if (lsr & UART_LSR_OE) {
> >   				port->icount.overrun++;
> > +				schedule_work(&priv->lsr_work);
>
> Unconditionally scheduled
>
> >   				tty_insert_flip_char(&port->port, 0,
> >   						TTY_OVERRUN);
> >   			}
>
> [..]
> > +static int f81232_suspend(struct usb_serial *serial, pm_message_t message)
> > +{
> > +	struct f81232_private *port_priv;
> > +
> > +	port_priv = usb_get_serial_port_data(serial->port[0]);
> > +	flush_work(&port_priv->lsr_work);
> > +
> > +	return 0;
> > +}
> > +
> >   static struct usb_serial_driver f81232_device = {
> >   	.driver = {
> >   		.owner =	THIS_MODULE,
> > @@ -655,6 +688,7 @@ static struct usb_serial_driver f81232_device = {
> >   	.read_int_callback =	f81232_read_int_callback,
> >   	.port_probe =		f81232_port_probe,
> >   	.port_remove =		f81232_port_remove,
> > +	.suspend =		f81232_suspend,
>
> Please have a look at:
> int usb_serial_suspend(struct usb_interface *intf, pm_message_t message)
> {
>          struct usb_serial *serial = usb_get_intfdata(intf);
>          int i, r = 0;
>
>
>          serial->suspending = 1;
>
>
>          /*
>           * serial->type->suspend() MUST return 0 in system sleep context,
>           * otherwise, the resume callback has to recover device from
>           * previous suspend failure.
>           */
>          if (serial->type->suspend) {
>                  r = serial->type->suspend(serial, message);
>                  if (r < 0) {
>                          serial->suspending = 0;
>                          goto err_out;
>                  }
>          }
>
>
>          for (i = 0; i < serial->num_ports; ++i)
>                  usb_serial_port_poison_urbs(serial->port[i]);
> err_out:
>          return r;
> }
> EXPORT_SYMBOL(usb_serial_suspend);
>
> As you can see, the suspend method is called first and then the URBs
> are poisoned. That means that after you have flushed the work, it may
> be submitted again. The fix would be to test the 'suspending' flag
> before you schedule work (and recheck the need to schedule it
> during resume)

Thanks for report the race condition issue. It's seems the same bug
in f81534.c. I'll try to fix it on f81232.c then fix f81534.c too.

--
With Best Regards,
Peter Hong