[V6,1/3] USB: serial: f81232: clear overrun flag

Commit Message

Ji-Ze Hong (Peter Hong) April 3, 2019, 8:40 a.m. UTC

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 <[e-mail]>
Signed-off-by: Ji-Ze Hong (Peter Hong) <[e-mail]>
---
V6:
	1: Add deferred_lsr_work_needed to re-trigger when f81232_resume()

v5:
	1: Source code base revert to v3 and remove all v4 changes.
	2: Add serial->suspending check in f81232_process_read_urb()
	   before schedule_work(&priv->lsr_work) to avoid race condition.

v4:
	1: Add serial->suspending check in f81232_lsr_worker() to avoid
	   re-trigger
	2: Add port_priv-lsr_work_resched to re-trigger LSR worker

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 | 55 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Comments

Oliver Neukum April 3, 2019, 10:36 a.m. UTC | #1

On Mi, 2019-04-03 at 16:40 +0800,  Ji-Ze Hong (Peter Hong)  wrote:
> 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 <[e-mail]>
> Signed-off-by: Ji-Ze Hong (Peter Hong) <[e-mail]>
Reviewed-by: Oliver Neukum <[e-mail]>

Johan Hovold April 15, 2019, 10:20 a.m. UTC | #2

On Wed, Apr 03, 2019 at 04:40:30PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> 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 <[e-mail]>
> Signed-off-by: Ji-Ze Hong (Peter Hong) <[e-mail]>
> ---
> V6:
> 	1: Add deferred_lsr_work_needed to re-trigger when f81232_resume()
> 
> v5:
> 	1: Source code base revert to v3 and remove all v4 changes.
> 	2: Add serial->suspending check in f81232_process_read_urb()
> 	   before schedule_work(&priv->lsr_work) to avoid race condition.
> 
> v4:
> 	1: Add serial->suspending check in f81232_lsr_worker() to avoid
> 	   re-trigger
> 	2: Add port_priv-lsr_work_resched to re-trigger LSR worker
> 
> 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 | 55 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)

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

Note that usb-serial core doesn't manage the interrupt urb for you, so
there's already a bug in this driver which should be fixed in a
preparatory patch (i.e. resubmit the interrupt urb on resume).

And if you stop the bulk-in and interrupt urbs explicitly here, you can
safely flush the lsr work without any races afterwards.

Also note that the interrupt work is currently never flushed on suspend
or close either... Another thing to fix first.

There's also a layering violation here, since you're accessing the child
port data from a parent interface driver callback. The port driver could
have been unbound and you'd get a NULL deref here. Iterating over the
interface's ports, and checking if it's been opened (as you need to do
on resume anyway) might be enough though.

Johan

Patch

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 0dcdcb4b2cde..600e1f4d94e0 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -41,12 +41,15 @@  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;
+	bool deferred_lsr_work_needed;
+	struct work_struct lsr_work;
 	struct work_struct interrupt_work;
 	struct usb_serial_port *port;
 };
@@ -282,6 +285,8 @@  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 usb_serial *serial = port->serial;
+	struct f81232_private *priv = usb_get_serial_port_data(port);
 	unsigned char *data = urb->transfer_buffer;
 	char tty_flag;
 	unsigned int i;
@@ -315,6 +320,12 @@  static void f81232_process_read_urb(struct urb *urb)
 
 			if (lsr & UART_LSR_OE) {
 				port->icount.overrun++;
+
+				if (!serial->suspending)
+					schedule_work(&priv->lsr_work);
+				else
+					priv->deferred_lsr_work_needed = true;
+
 				tty_insert_flip_char(&port->port, 0,
 						TTY_OVERRUN);
 			}
@@ -556,9 +567,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 +617,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 +642,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 +662,29 @@  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 int f81232_resume(struct usb_serial *serial)
+{
+	struct f81232_private *port_priv;
+
+	port_priv = usb_get_serial_port_data(serial->port[0]);
+	if (port_priv->deferred_lsr_work_needed) {
+		port_priv->deferred_lsr_work_needed = false;
+		schedule_work(&port_priv->lsr_work);
+	}
+
+	return usb_serial_generic_resume(serial);
+}
+
 static struct usb_serial_driver f81232_device = {
 	.driver = {
 		.owner =	THIS_MODULE,
@@ -655,6 +708,8 @@  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,
+	.resume =		f81232_resume,
 };
 
 static struct usb_serial_driver * const serial_drivers[] = {