[PATCH 0/4] USB: serial: closing-wait fixes and cleanups

[PATCH 0/4] USB: serial: closing-wait fixes and cleanups

[Johan Hovold]

The port drain_delay parameter is used to add a time-based delay when
closing the port in order to allow the transmit FIFO to drain in cases
where we don't know how to tell if the FIFO is empty.

This series removes a redundant time-based delay which is no longer
needed, and documents the reason for two other uses where such a delay
is needed to let the transmitter shift register clear. As it turns out,
this is really only needed for one of the two device types handled by
the ti_usb_3410_5052 driver.

Johan


Johan Hovold (4):
  USB: serial: f81232: drop time-based drain delay
  USB: serial: io_ti: document reason for drain delay
  USB: serial: ti_usb_3410_5052: reduce drain delay to one char
  USB: serial: ti_usb_3410_5052: drop drain delay for 3410

 drivers/usb/serial/f81232.c           |  1 -
 drivers/usb/serial/io_ti.c            |  4 ++++
 drivers/usb/serial/ti_usb_3410_5052.c | 21 ++++++++++++++++++---
 3 files changed, 22 insertions(+), 4 deletions(-)

-- 
2.26.3

[PATCH 1/4] USB: serial: f81232: drop time-based drain delay

[Johan Hovold]

The f81232 driver now waits for the transmit FIFO to drain during close
so there is no need to keep the time-based drain delay, which would add
up to two seconds on every close for low line speeds.

Fixes: 98405f81036d ("USB: serial: f81232: add tx_empty function")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/f81232.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index b4b847dce4bc..a7a7af8d05bf 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -948,7 +948,6 @@ static int f81232_port_probe(struct usb_serial_port *port)
 
 	usb_set_serial_port_data(port, priv);
 
-	port->port.drain_delay = 256;
 	priv->port = port;
 
 	return 0;
-- 
2.26.3

[PATCH 2/4] USB: serial: io_ti: document reason for drain delay

[Johan Hovold]

Document that the device line-status register doesn't tell when the
transmitter shift register has emptied and that this is why the
one-character drain delay is needed.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/io_ti.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index 75325c2b295e..17720670e06c 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -2590,6 +2590,10 @@ static int edge_port_probe(struct usb_serial_port *port)
 	if (ret)
 		goto err;
 
+	/*
+	 * The LSR does not tell when the transmitter shift register has
+	 * emptied so add a one-character drain delay.
+	 */
 	port->port.drain_delay = 1;
 
 	return 0;
-- 
2.26.3

[PATCH 3/4] USB: serial: ti_usb_3410_5052: reduce drain delay to one char

[Johan Hovold]

The three-character drain delay was added by commit f1175daa5312 ("USB:
ti_usb_3410_5052: kill custom closing_wait") when removing the custom
closing-wait implementation, which used a fixed 20 ms poll period and
drain delay.

This was likely a bit too conservative as a one-character timeout (e.g.
33 ms at 300 bps) should be enough to compensate for the lack of a
transmitter empty bit in the TUSB5052 line-status register.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 03839289d6c0..8ed64115987f 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -610,7 +610,11 @@ static int ti_port_probe(struct usb_serial_port *port)
 
 	usb_set_serial_port_data(port, tport);
 
-	port->port.drain_delay = 3;
+	/*
+	 * The TUSB5052 LSR does not tell when the transmitter shift register
+	 * has emptied so add a one-character drain delay.
+	 */
+	port->port.drain_delay = 1;
 
 	return 0;
 }
-- 
2.26.3

[PATCH 4/4] USB: serial: ti_usb_3410_5052: drop drain delay for 3410

[Johan Hovold]

Unlike the TUSB5052, the TUSB3410 has an LSR TEMT bit to tell if both
the transmitter data and shift registers are empty.

Make sure to check also the shift register on TUSB3410 when waiting for
the transmit buffer to drain during close and drop the time-based
one-char delay which is otherwise needed (e.g. 90 ms at 110 bps).

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 8ed64115987f..d9bffb2de8bf 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -121,6 +121,7 @@
 #define TI_LSR_ERROR			0x0F
 #define TI_LSR_RX_FULL			0x10
 #define TI_LSR_TX_EMPTY			0x20
+#define TI_LSR_TX_EMPTY_BOTH		0x40
 
 /* Line control */
 #define TI_LCR_BREAK			0x40
@@ -614,7 +615,8 @@ static int ti_port_probe(struct usb_serial_port *port)
 	 * The TUSB5052 LSR does not tell when the transmitter shift register
 	 * has emptied so add a one-character drain delay.
 	 */
-	port->port.drain_delay = 1;
+	if (!tport->tp_tdev->td_is_3410)
+		port->port.drain_delay = 1;
 
 	return 0;
 }
@@ -851,11 +853,20 @@ static int ti_chars_in_buffer(struct tty_struct *tty)
 static bool ti_tx_empty(struct usb_serial_port *port)
 {
 	struct ti_port *tport = usb_get_serial_port_data(port);
+	u8 lsr, mask;
 	int ret;
-	u8 lsr;
+
+	/*
+	 * TUSB5052 does not have the TEMT bit to tell if the shift register
+	 * is empty.
+	 */
+	if (tport->tp_tdev->td_is_3410)
+		mask = TI_LSR_TX_EMPTY_BOTH;
+	else
+		mask = TI_LSR_TX_EMPTY;
 
 	ret = ti_get_lsr(tport, &lsr);
-	if (!ret && !(lsr & TI_LSR_TX_EMPTY))
+	if (!ret && !(lsr & mask))
 		return false;
 
 	return true;
-- 
2.26.3

Re: [PATCH 0/4] USB: serial: closing-wait fixes and cleanups

[Greg KH]

On Mon, Apr 12, 2021 at 11:38:11AM +0200, Johan Hovold wrote:
> The port drain_delay parameter is used to add a time-based delay when
> closing the port in order to allow the transmit FIFO to drain in cases
> where we don't know how to tell if the FIFO is empty.
> 
> This series removes a redundant time-based delay which is no longer
> needed, and documents the reason for two other uses where such a delay
> is needed to let the transmitter shift register clear. As it turns out,
> this is really only needed for one of the two device types handled by
> the ti_usb_3410_5052 driver.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>