| Message ID | 1554098453-31278-1-git-send-email-hpeter+linux_kernel@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RESEND,V3,1/3] USB: serial: f81232: clear overrun flag | expand |
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) Regards Oliver
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.
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c index 0dcdcb4b2cde..722a184db9fa 100644 --- a/drivers/usb/serial/f81232.c +++ b/drivers/usb/serial/f81232.c @@ -41,12 +41,14 @@ MODULE_DEVICE_TABLE(usb, id_table); #define FIFO_CONTROL_REGISTER (0x02 + SERIAL_BASE_ADDRESS) #define LINE_CONTROL_REGISTER (0x03 + SERIAL_BASE_ADDRESS) #define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS) +#define LINE_STATUS_REGISTER (0x05 + SERIAL_BASE_ADDRESS) #define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS) struct f81232_private { struct mutex lock; u8 modem_control; u8 modem_status; + struct work_struct lsr_work; struct work_struct interrupt_work; struct usb_serial_port *port; }; @@ -282,6 +284,7 @@ static void f81232_read_int_callback(struct urb *urb) static void f81232_process_read_urb(struct urb *urb) { struct usb_serial_port *port = urb->context; + struct f81232_private *priv = usb_get_serial_port_data(port); unsigned char *data = urb->transfer_buffer; char tty_flag; unsigned int i; @@ -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); tty_insert_flip_char(&port->port, 0, TTY_OVERRUN); } @@ -556,9 +560,12 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port) static void f81232_close(struct usb_serial_port *port) { + struct f81232_private *port_priv = usb_get_serial_port_data(port); + f81232_port_disable(port); usb_serial_generic_close(port); usb_kill_urb(port->interrupt_in_urb); + flush_work(&port_priv->lsr_work); } static void f81232_dtr_rts(struct usb_serial_port *port, int on) @@ -603,6 +610,21 @@ static void f81232_interrupt_work(struct work_struct *work) f81232_read_msr(priv->port); } +static void f81232_lsr_worker(struct work_struct *work) +{ + struct f81232_private *priv; + struct usb_serial_port *port; + int status; + u8 tmp; + + priv = container_of(work, struct f81232_private, lsr_work); + port = priv->port; + + status = f81232_get_register(port, LINE_STATUS_REGISTER, &tmp); + if (status) + dev_warn(&port->dev, "read LSR failed: %d\n", status); +} + static int f81232_port_probe(struct usb_serial_port *port) { struct f81232_private *priv; @@ -613,6 +635,7 @@ static int f81232_port_probe(struct usb_serial_port *port) mutex_init(&priv->lock); INIT_WORK(&priv->interrupt_work, f81232_interrupt_work); + INIT_WORK(&priv->lsr_work, f81232_lsr_worker); usb_set_serial_port_data(port, priv); @@ -632,6 +655,16 @@ static int f81232_port_remove(struct usb_serial_port *port) return 0; } +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, }; static struct usb_serial_driver * const serial_drivers[] = {
The F81232 will report data and LSR with bulk like following format: bulk-in data: [LSR(1Byte)+DATA(1Byte)][LSR(1Byte)+DATA(1Byte)]... LSR will auto clear frame/parity/break error flag when reading by H/W, but overrrun will only cleared when reading LSR. So this patch add a worker to read LSR when overrun and flush the worker on close() & suspend(). Cc: Oliver Neukum <oneukum@suse.com> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com> --- v3: 1: Add flush_work(&port_priv->lsr_work) in f81232_suspend(). v2: 1: Add flush_work(&port_priv->lsr_work) in f81232_close(). drivers/usb/serial/f81232.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)