Messages in this thread | | | Subject | Re: [RESEND PATCH V3 1/3] USB: serial: f81232: clear overrun flag | From | "Ji-Ze Hong (Peter Hong)" <> | Date | Mon, 1 Apr 2019 17:01:29 +0800 |
| |
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
| |