[PATCH 0/6] USB: serial: mxuport: improve tty operation support

[PATCH 0/6] USB: serial: mxuport: improve tty operation support

[Crescent Hsieh]

This series improves the mxuport driver's handling of several tty
operations.

The first patch converts the driver's control reads to
usb_control_msg_recv(), which handles DMA-safe temporary buffers and
validates the returned transfer length.

The second patch initializes the cached modem status when opening a
port, preventing TIOCMGET from temporarily reporting stale input-line
state.

Patches three and four add optional stop and start callbacks to the USB
serial core and implement them in mxuport. This prevents the driver from
submitting new write data while tty output is stopped.

Patch five adds device-specific TCFLSH handling so that flush requests
also purge the corresponding device buffers and USB serial write FIFO.

The final patch adds a wait_until_sent callback. It retains the generic
USB-serial queue polling and adds a baud-dependent delay before
reporting transmit completion.

Crescent Hsieh (6):
  USB: serial: mxuport: use usb_control_msg_recv()
  USB: serial: mxuport: initialize modem status on open
  USB: serial: add stop and start callbacks
  USB: serial: mxuport: handle output flow control
  USB: serial: mxuport: handle TCFLSH buffer purge
  USB: serial: mxuport: wait for UART transmit completion

 drivers/usb/serial/mxuport.c    | 287 +++++++++++++++++++++++---------
 drivers/usb/serial/usb-serial.c |  18 ++
 include/linux/usb/serial.h      |   2 +
 3 files changed, 231 insertions(+), 76 deletions(-)

-- 
2.53.0

[PATCH 1/6] USB: serial: mxuport: use usb_control_msg_recv()

[Crescent Hsieh]

mxuport_recv_ctrl_urb() uses usb_control_msg(), so its callers need to
provide DMA-safe buffers. It also checks the returned length itself.

Use usb_control_msg_recv() instead. This helper provides the DMA-safe
buffer and reports an error when the device returns an unexpected
length.

The three existing control reads all expect four bytes. Use stack
buffers for them and update the callers to handle zero as success. This
also keeps the original USB error when reading the firmware version
instead of replacing it with -EIO.

Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>
---
 drivers/usb/serial/mxuport.c | 100 +++++++++++------------------------
 1 file changed, 32 insertions(+), 68 deletions(-)

diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
index 319704e64528..fa75f844050d 100644
--- a/drivers/usb/serial/mxuport.c
+++ b/drivers/usb/serial/mxuport.c
@@ -311,32 +311,20 @@ static void mxuport_write_rollback(struct usb_serial_port *port, int count)
 }
 
 /* Read the given buffer in from the control pipe. */
-static int mxuport_recv_ctrl_urb(struct usb_serial *serial,
-				 u8 request, u16 value, u16 index,
-				 u8 *data, size_t size)
+static int mxuport_recv_ctrl(struct usb_serial *serial, u8 request,
+			     u16 value, u16 index, void *data, u16 size)
 {
 	int status;
 
-	status = usb_control_msg(serial->dev,
-				 usb_rcvctrlpipe(serial->dev, 0),
-				 request,
-				 (USB_DIR_IN | USB_TYPE_VENDOR |
-				  USB_RECIP_DEVICE), value, index,
-				 data, size,
-				 USB_CTRL_GET_TIMEOUT);
-	if (status < 0) {
+	status = usb_control_msg_recv(serial->dev, 0, request,
+				      USB_DIR_IN | USB_TYPE_VENDOR |
+				      USB_RECIP_DEVICE, value, index,
+				      data, size, USB_CTRL_GET_TIMEOUT,
+				      GFP_KERNEL);
+	if (status)
 		dev_err(&serial->interface->dev,
-			"%s - usb_control_msg failed (%d)\n",
+			"%s - usb_control_msg_recv failed (%d)\n",
 			__func__, status);
-		return status;
-	}
-
-	if (status != size) {
-		dev_err(&serial->interface->dev,
-			"%s - short read (%d / %zd)\n",
-			__func__, status, size);
-		return -EIO;
-	}
 
 	return status;
 }
@@ -682,29 +670,19 @@ static void mxuport_process_read_urb(struct urb *urb)
 static bool mxuport_tx_empty(struct usb_serial_port *port)
 {
 	struct usb_serial *serial = port->serial;
-	bool is_empty = true;
+	u8 buf[sizeof(u32)];
 	u32 txlen;
-	u8 *len_buf;
 	int err;
 
-	len_buf = kzalloc(4, GFP_KERNEL);
-	if (!len_buf)
-		goto out;
-
-	err = mxuport_recv_ctrl_urb(serial, RQ_VENDOR_GET_OUTQUEUE, 0,
-				    port->port_number, len_buf, 4);
-	if (err < 0)
-		goto out;
+	err = mxuport_recv_ctrl(serial, RQ_VENDOR_GET_OUTQUEUE, 0,
+				port->port_number, buf, sizeof(buf));
+	if (err)
+		return true;
 
-	txlen = get_unaligned_be32(len_buf);
+	txlen = get_unaligned_be32(buf);
 	dev_dbg(&port->dev, "%s - tx len = %u\n", __func__, txlen);
 
-	if (txlen != 0)
-		is_empty = false;
-
-out:
-	kfree(len_buf);
-	return is_empty;
+	return txlen == 0;
 }
 
 static int mxuport_set_mcr(struct usb_serial_port *port, u8 mcr_state)
@@ -1229,51 +1207,37 @@ static void mxuport_get_fw_name(unsigned long features, u16 productid,
 
 static int mxuport_fw_running_from_sram(struct usb_serial *serial, bool *from_sram)
 {
-	int err;
+	u8 buf[sizeof(u32)];
 	u32 reg;
-	u8 *buf;
-
-	buf = kzalloc_objs(*buf, sizeof(reg));
-	if (!buf)
-		return -ENOMEM;
+	int err;
 
-	err = mxuport_recv_ctrl_urb(serial, RQ_VENDOR_GET_SYS_REG,
-				    MX_SYS_REG_REMAP_OFF, 0, buf, sizeof(reg));
-	if (err < 0)
-		goto out;
+	err = mxuport_recv_ctrl(serial, RQ_VENDOR_GET_SYS_REG,
+				MX_SYS_REG_REMAP_OFF, 0, buf, sizeof(buf));
+	if (err)
+		return err;
 
 	reg = get_unaligned_be32(buf);
 	*from_sram = (((reg >> 24) & MX_PWR_REMAP_MASK) == MX_REMAP_TO_SRAM);
-	err = 0;
-out:
-	kfree(buf);
-	return err;
+
+	return 0;
 }
 
 /* Get the version of the firmware currently running. */
 static int mxuport_get_fw_version(struct usb_serial *serial,
 				  struct mxuport_fw_version *version)
 {
-	u8 *ver_buf;
+	u8 buf[4];
 	int err;
 
-	ver_buf = kzalloc(4, GFP_KERNEL);
-	if (!ver_buf)
-		return -ENOMEM;
-
 	/* Get firmware version from SDRAM */
-	err = mxuport_recv_ctrl_urb(serial, RQ_VENDOR_GET_VERSION, 0, 0,
-				    ver_buf, 4);
-	if (err != 4) {
-		err = -EIO;
-		goto out;
-	}
+	err = mxuport_recv_ctrl(serial, RQ_VENDOR_GET_VERSION, 0, 0,
+				buf, sizeof(buf));
+	if (err)
+		return err;
 
-	mxuport_compose_fw_version(version, ver_buf[0], ver_buf[1], ver_buf[2]);
-	err = 0;
-out:
-	kfree(ver_buf);
-	return err;
+	mxuport_compose_fw_version(version, buf[0], buf[1], buf[2]);
+
+	return 0;
 }
 
 /* Given a firmware blob, download it to the device. */
-- 
2.53.0

Re: [PATCH 1/6] USB: serial: mxuport: use usb_control_msg_recv()

[sashiko-bot]

> mxuport_recv_ctrl_urb() uses usb_control_msg(), so its callers need to
> provide DMA-safe buffers. It also checks the returned length itself.
> 
> Use usb_control_msg_recv() instead. This helper provides the DMA-safe
> buffer and reports an error when the device returns an unexpected
> length.
> 
> The three existing control reads all expect four bytes. Use stack
> buffers for them and update the callers to handle zero as success. This
> also keeps the original USB error when reading the firmware version
> instead of replacing it with -EIO.
> 
> Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016100351.681390-1-crescentcy.hsieh@moxa.com?part=1

[PATCH 2/6] USB: serial: mxuport: initialize modem status on open

[Crescent Hsieh]

The cached modem status is currently cleared whenever a port is opened,
regardless of the current modem-status inputs. Consequently, TIOCMGET
may report asserted input signals as inactive immediately after open.

Read the one-byte MSR with RQ_VENDOR_GET_MSR after setting up the port.
Ignore the transient delta bits and cache only the current modem-line
state.

If the request fails, keep the cached state clear and allow the port to
open, preserving the existing fallback behaviour.

Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>
---
 drivers/usb/serial/mxuport.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
index fa75f844050d..3f1989afde30 100644
--- a/drivers/usb/serial/mxuport.c
+++ b/drivers/usb/serial/mxuport.c
@@ -1452,6 +1452,7 @@ static int mxuport_open(struct tty_struct *tty, struct usb_serial_port *port)
 	struct mxuport_port *mxport = usb_get_serial_port_data(port);
 	struct usb_serial *serial = port->serial;
 	unsigned long flags;
+	u8 msr_state;
 	int err;
 
 	/* Set receive host (enable) */
@@ -1477,13 +1478,19 @@ static int mxuport_open(struct tty_struct *tty, struct usb_serial_port *port)
 	mxport->hold_reason &= ~MX_WAIT_FOR_SEND_NEXT;
 	spin_unlock_irqrestore(&port->lock, flags);
 
-	/*
-	 * TODO: use RQ_VENDOR_GET_MSR, once we know what it
-	 * returns.
-	 */
-	mxport->msr_state = 0;
+	err = mxuport_recv_ctrl(serial, RQ_VENDOR_GET_MSR, 0,
+				port->port_number, &msr_state,
+				sizeof(msr_state));
+	if (err)
+		msr_state = 0;
+	else
+		msr_state &= ~UART_MSR_ANY_DELTA;
 
-	return err;
+	spin_lock_irqsave(&mxport->spinlock, flags);
+	mxport->msr_state = msr_state;
+	spin_unlock_irqrestore(&mxport->spinlock, flags);
+
+	return 0;
 }
 
 static void mxuport_close(struct usb_serial_port *port)
-- 
2.53.0

Re: [PATCH 2/6] USB: serial: mxuport: initialize modem status on open

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: serial: mxuport: TOCTOU race drops MSR events during open
--

commit e30f4f38ee9a11d678d89a99a4f6221fd7d285d1
Author: Crescent Hsieh <crescentcy.hsieh@moxa.com>

USB: serial: mxuport: initialize modem status on open

The cached modem status is currently cleared whenever a port is opened,
regardless of the current modem-status inputs. This commit fetches the
modem status register via a vendor request during port open so that
TIOCMGET reports the correct initial input signals.

> diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
> index fa75f844050dc..3f1989afde30e 100644
> --- a/drivers/usb/serial/mxuport.c
> +++ b/drivers/usb/serial/mxuport.c

[ ... ]

> @@ -1477,13 +1478,19 @@ static int mxuport_open(struct tty_struct *tty, struct usb_serial_port *port)
>  	mxport->hold_reason &= ~MX_WAIT_FOR_SEND_NEXT;
>  	spin_unlock_irqrestore(&port->lock, flags);
>  
> -	/*
> -	 * TODO: use RQ_VENDOR_GET_MSR, once we know what it
> -	 * returns.
> -	 */
> -	mxport->msr_state = 0;
> +	err = mxuport_recv_ctrl(serial, RQ_VENDOR_GET_MSR, 0,
> +				port->port_number, &msr_state,
> +				sizeof(msr_state));

[Severity: Medium]
Does this introduce a race condition where modem status events can be
dropped?

If a physical modem status line changes immediately after this
RQ_VENDOR_GET_MSR request completes, but before the core layer marks the
port as initialized, it appears the completion handler drops the incoming
hardware events.

Looking at mxuport_process_read_urb_demux_event():

    if (tty_port_initialized(&demux_port->port)) {
        ch = data + HEADER_SIZE;
        rcv_event = get_unaligned_be16(data + 2);
        mxuport_process_read_urb_event(demux_port, ch, rcv_event);
    } else {
        dev_dbg(&demux_port->dev, "%s - event for closed port\n", __func__);
    }

Since tty_port_initialized() is not set to true until after mxuport_open()
returns to the core layer, any state transition in this window is silently
dropped. Can this leave the cached msr_state permanently desynchronized
from the actual hardware state until a subsequent event occurs?

> +	if (err)
> +		msr_state = 0;
> +	else
> +		msr_state &= ~UART_MSR_ANY_DELTA;
>  
> -	return err;
> +	spin_lock_irqsave(&mxport->spinlock, flags);
> +	mxport->msr_state = msr_state;
> +	spin_unlock_irqrestore(&mxport->spinlock, flags);
> +
> +	return 0;
>  }
>  
>  static void mxuport_close(struct usb_serial_port *port)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016100351.681390-1-crescentcy.hsieh@moxa.com?part=2

[PATCH 3/6] USB: serial: add stop and start callbacks

[Crescent Hsieh]

The tty layer uses the stop and start operations to suspend and resume
output, but the USB serial core does not currently provide them. This
prevents USB serial subdrivers from handling output flow control through
the normal tty path.

Add tty stop and start operations and forward them to optional USB
serial driver callbacks. Drivers which do not provide the callbacks
retain the existing behaviour.

Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>
---
 drivers/usb/serial/usb-serial.c | 18 ++++++++++++++++++
 include/linux/usb/serial.h      |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 4a19621f08ba..73bb0cb0be6c 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -429,6 +429,22 @@ static void serial_unthrottle(struct tty_struct *tty)
 		port->serial->type->unthrottle(tty);
 }
 
+static void serial_stop(struct tty_struct *tty)
+{
+	struct usb_serial_port *port = tty->driver_data;
+
+	if (port->serial->type->stop)
+		port->serial->type->stop(tty);
+}
+
+static void serial_start(struct tty_struct *tty)
+{
+	struct usb_serial_port *port = tty->driver_data;
+
+	if (port->serial->type->start)
+		port->serial->type->start(tty);
+}
+
 static int serial_get_serial(struct tty_struct *tty, struct serial_struct *ss)
 {
 	struct usb_serial_port *port = tty->driver_data;
@@ -1351,6 +1367,8 @@ static const struct tty_operations serial_ops = {
 	.set_termios =		serial_set_termios,
 	.throttle =		serial_throttle,
 	.unthrottle =		serial_unthrottle,
+	.stop =			serial_stop,
+	.start =		serial_start,
 	.break_ctl =		serial_break,
 	.chars_in_buffer =	serial_chars_in_buffer,
 	.wait_until_sent =	serial_wait_until_sent,
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index 5866f19d3c7b..633bdde530c5 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -285,6 +285,8 @@ struct usb_serial_driver {
 	bool (*tx_empty)(struct usb_serial_port *port);
 	void (*throttle)(struct tty_struct *tty);
 	void (*unthrottle)(struct tty_struct *tty);
+	void (*stop)(struct tty_struct *tty);
+	void (*start)(struct tty_struct *tty);
 	int  (*tiocmget)(struct tty_struct *tty);
 	int  (*tiocmset)(struct tty_struct *tty,
 			 unsigned int set, unsigned int clear);
-- 
2.53.0

Re: [PATCH 3/6] USB: serial: add stop and start callbacks

[sashiko-bot]

> The tty layer uses the stop and start operations to suspend and resume
> output, but the USB serial core does not currently provide them. This
> prevents USB serial subdrivers from handling output flow control through
> the normal tty path.
> 
> Add tty stop and start operations and forward them to optional USB
> serial driver callbacks. Drivers which do not provide the callbacks
> retain the existing behaviour.
> 
> Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016100351.681390-1-crescentcy.hsieh@moxa.com?part=3

[PATCH 4/6] USB: serial: mxuport: handle output flow control

[Crescent Hsieh]

Use the USB serial stop and start callbacks to prevent mxuport from
submitting buffered data while tty output is stopped.

Record the stopped state in each port's hold_reason and prevent further
URB submissions while it is set. Keep this state separate from the
SEND_NEXT hold so that transmission resumes only after both conditions
have been cleared.

Clear the tty stop state when opening a port so that a previous close
does not leave the port stopped. This only stops new submissions; URBs
already in flight complete normally.

Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>
---
 drivers/usb/serial/mxuport.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
index 3f1989afde30..326a5ce18c6a 100644
--- a/drivers/usb/serial/mxuport.c
+++ b/drivers/usb/serial/mxuport.c
@@ -265,7 +265,32 @@ static bool mxuport_write_ready(struct usb_serial_port *port)
 {
 	struct mxuport_port *mxport = usb_get_serial_port_data(port);
 
-	return !(mxport->hold_reason & MX_WAIT_FOR_SEND_NEXT);
+	return !(mxport->hold_reason & (MX_WAIT_FOR_START_TX |
+					MX_WAIT_FOR_SEND_NEXT));
+}
+
+static void mxuport_stop(struct tty_struct *tty)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct mxuport_port *mxport = usb_get_serial_port_data(port);
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	mxport->hold_reason |= MX_WAIT_FOR_START_TX;
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static void mxuport_start(struct tty_struct *tty)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct mxuport_port *mxport = usb_get_serial_port_data(port);
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	mxport->hold_reason &= ~MX_WAIT_FOR_START_TX;
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	usb_serial_generic_write_start(port, GFP_ATOMIC);
 }
 
 /*
@@ -1475,7 +1500,7 @@ static int mxuport_open(struct tty_struct *tty, struct usb_serial_port *port)
 
 	spin_lock_irqsave(&port->lock, flags);
 	mxport->sent_payload = 0;
-	mxport->hold_reason &= ~MX_WAIT_FOR_SEND_NEXT;
+	mxport->hold_reason &= ~(MX_WAIT_FOR_START_TX | MX_WAIT_FOR_SEND_NEXT);
 	spin_unlock_irqrestore(&port->lock, flags);
 
 	err = mxuport_recv_ctrl(serial, RQ_VENDOR_GET_MSR, 0,
@@ -1574,6 +1599,8 @@ static struct usb_serial_driver mxuport_device = {
 	.get_icount		= usb_serial_generic_get_icount,
 	.throttle		= mxuport_throttle,
 	.unthrottle		= mxuport_unthrottle,
+	.stop			= mxuport_stop,
+	.start			= mxuport_start,
 	.tiocmget		= mxuport_tiocmget,
 	.tiocmset		= mxuport_tiocmset,
 	.dtr_rts		= mxuport_dtr_rts,
-- 
2.53.0

Re: [PATCH 4/6] USB: serial: mxuport: handle output flow control

[sashiko-bot]

> Use the USB serial stop and start callbacks to prevent mxuport from
> submitting buffered data while tty output is stopped.
> 
> Record the stopped state in each port's hold_reason and prevent further
> URB submissions while it is set. Keep this state separate from the
> SEND_NEXT hold so that transmission resumes only after both conditions
> have been cleared.
> 
> Clear the tty stop state when opening a port so that a previous close
> does not leave the port stopped. This only stops new submissions; URBs
> already in flight complete normally.
> 
> Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016100351.681390-1-crescentcy.hsieh@moxa.com?part=4

[PATCH 5/6] USB: serial: mxuport: handle TCFLSH buffer purge

[Crescent Hsieh]

Generic tty flush handling does not purge the receive and transmit
buffers in mxuport firmware or clear the USB serial write FIFO.

Handle input, output, and combined flush requests by issuing the
corresponding firmware purge request.

For input flushes, retain the line-discipline input flush and
unthrottle behavior.

For output flushes, stop new write submissions while clearing the
write FIFO and purging the device. Reset SEND_NEXT accounting after a
successful purge. Submitted URBs are left to complete.

Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>
---
 drivers/usb/serial/mxuport.c | 76 +++++++++++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
index 326a5ce18c6a..9d304fb3aa02 100644
--- a/drivers/usb/serial/mxuport.c
+++ b/drivers/usb/serial/mxuport.c
@@ -148,6 +148,10 @@ static const u16 mxuport_mux50u_fw_ver_offsets[] = {
 
 #define RQ_VENDOR_GET_MSR		0x86 /* Get modem status register */
 
+/* Definitions for purge request */
+#define MX_PURGE_TX			0x04
+#define MX_PURGE_RX			0x08
+
 /* Definitions for UPort event type */
 #define UPORT_EVENT_NONE		0 /* None */
 #define UPORT_EVENT_TXBUF_THRESHOLD	1 /* Tx buffer threshold */
@@ -201,11 +205,12 @@ static const u16 mxuport_mux50u_fw_ver_offsets[] = {
 #define MX_WAIT_FOR_UNTHROTTLE		0x0020
 #define MX_WAIT_FOR_LOW_WATER		0x0040
 #define MX_WAIT_FOR_SEND_NEXT		0x0080
+#define MX_WAIT_FOR_PURGE		0x0100
 
 /* This structure holds all of the local port information */
 struct mxuport_port {
 	u32 sent_payload;
-	u8 hold_reason;
+	u16 hold_reason;
 	u8 mcr_state;		/* Last MCR state */
 	u8 msr_state;		/* Last MSR state */
 	struct serial_rs485 rs485;
@@ -266,7 +271,8 @@ static bool mxuport_write_ready(struct usb_serial_port *port)
 	struct mxuport_port *mxport = usb_get_serial_port_data(port);
 
 	return !(mxport->hold_reason & (MX_WAIT_FOR_START_TX |
-					MX_WAIT_FOR_SEND_NEXT));
+					MX_WAIT_FOR_SEND_NEXT |
+					MX_WAIT_FOR_PURGE));
 }
 
 static void mxuport_stop(struct tty_struct *tty)
@@ -952,6 +958,70 @@ static int mxuport_set_rs485_config(struct usb_serial_port *port,
 	return 0;
 }
 
+static void mxuport_tx_flush_start(struct usb_serial_port *port)
+{
+	struct mxuport_port *mxport = usb_get_serial_port_data(port);
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	mxport->hold_reason |= MX_WAIT_FOR_PURGE;
+	kfifo_reset_out(&port->write_fifo);
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static void mxuport_tx_flush_done(struct usb_serial_port *port, bool purged)
+{
+	struct mxuport_port *mxport = usb_get_serial_port_data(port);
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	if (purged) {
+		mxport->sent_payload = 0;
+		mxport->hold_reason &= ~MX_WAIT_FOR_SEND_NEXT;
+	}
+	mxport->hold_reason &= ~MX_WAIT_FOR_PURGE;
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	usb_serial_generic_write_start(port, GFP_KERNEL);
+}
+
+static int mxuport_flush_buffer(struct tty_struct *tty,
+				unsigned long arg)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	int err, ret;
+	u16 mask;
+
+	switch (arg) {
+	case TCIFLUSH:
+		mask = MX_PURGE_RX;
+		break;
+	case TCOFLUSH:
+		mask = MX_PURGE_TX;
+		break;
+	case TCIOFLUSH:
+		mask = MX_PURGE_RX | MX_PURGE_TX;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (mask & MX_PURGE_TX)
+		mxuport_tx_flush_start(port);
+
+	err = mxuport_send_ctrl_urb(port->serial, RQ_VENDOR_PURGE, mask,
+				    port->port_number);
+	ret = tty_perform_flush(tty, arg);
+
+	if (mask & MX_PURGE_TX)
+		mxuport_tx_flush_done(port, !err);
+
+	if (err)
+		return err;
+
+	return ret;
+}
+
 static int mxuport_ioctl(struct tty_struct *tty, unsigned int cmd,
 			 unsigned long arg)
 {
@@ -959,6 +1029,8 @@ static int mxuport_ioctl(struct tty_struct *tty, unsigned int cmd,
 	void __user *argp = (void __user *)arg;
 
 	switch (cmd) {
+	case TCFLSH:
+		return mxuport_flush_buffer(tty, arg);
 	case TIOCGRS485:
 		return mxuport_get_rs485_config(port, argp);
 	case TIOCSRS485:
-- 
2.53.0

Re: [PATCH 5/6] USB: serial: mxuport: handle TCFLSH buffer purge

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tty: mxuport: TCFLSH interception causes AB-BA deadlock and job control bypass
- [Medium] usb: serial: mxuport: sent_payload accounting desynchronized by in-flight bulk URBs during purge
--

commit 50a2e5dd5f4f9804e6a3238f55deca09791f0018
Author: Crescent Hsieh <crescentcy.hsieh@moxa.com>

USB: serial: mxuport: handle TCFLSH buffer purge

Generic tty flush handling does not purge the receive and transmit
buffers in mxuport firmware or clear the USB serial write FIFO. This
commit handles input, output, and combined flush requests by issuing
the corresponding firmware purge request.

> diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
> index 326a5ce18c6a0..9d304fb3aa02f 100644
> --- a/drivers/usb/serial/mxuport.c
> +++ b/drivers/usb/serial/mxuport.c
[ ... ]
> @@ -952,6 +958,70 @@ static int mxuport_set_rs485_config(struct usb_serial_port *port,
>  	return 0;
>  }
>  
> +static void mxuport_tx_flush_start(struct usb_serial_port *port)
> +{
> +	struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&port->lock, flags);
> +	mxport->hold_reason |= MX_WAIT_FOR_PURGE;
> +	kfifo_reset_out(&port->write_fifo);
> +	spin_unlock_irqrestore(&port->lock, flags);
> +}
> +
> +static void mxuport_tx_flush_done(struct usb_serial_port *port, bool purged)
> +{
> +	struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&port->lock, flags);
> +	if (purged) {
> +		mxport->sent_payload = 0;
> +		mxport->hold_reason &= ~MX_WAIT_FOR_SEND_NEXT;
> +	}

[Severity: Medium]
Since kfifo_reset_out() empties the software FIFO but does not unlink or
kill already-submitted URBs, is it possible for a late-arriving bulk URB to
be received by the device after the device has processed the RQ_VENDOR_PURGE
control message?

If the host controller schedules the bulk and control transfers such that an
in-flight bulk URB arrives post-purge, resetting sent_payload to 0 here
could permanently desynchronize the driver's flow control accounting. Could
this lead to miscalculating the UPORT_REQUEST_SEND_NEXT threshold, resulting
in over-transmission and an overflow of the device's internal RX buffer?

> +	mxport->hold_reason &= ~MX_WAIT_FOR_PURGE;
> +	spin_unlock_irqrestore(&port->lock, flags);
> +
> +	usb_serial_generic_write_start(port, GFP_KERNEL);
> +}
> +
> +static int mxuport_flush_buffer(struct tty_struct *tty,
> +				unsigned long arg)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +	int err, ret;
> +	u16 mask;
> +
> +	switch (arg) {
> +	case TCIFLUSH:
> +		mask = MX_PURGE_RX;
> +		break;
> +	case TCOFLUSH:
> +		mask = MX_PURGE_TX;
> +		break;
> +	case TCIOFLUSH:
> +		mask = MX_PURGE_RX | MX_PURGE_TX;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (mask & MX_PURGE_TX)
> +		mxuport_tx_flush_start(port);
> +
> +	err = mxuport_send_ctrl_urb(port->serial, RQ_VENDOR_PURGE, mask,
> +				    port->port_number);
> +	ret = tty_perform_flush(tty, arg);

[Severity: High]
Does calling tty_perform_flush() from within this driver's ioctl handler
create an AB-BA deadlock between tport->mutex and ldisc_sem?

When ioctl(TCFLSH) is called, tty_ioctl() delegates to serial_ioctl() which
acquires tport->mutex, and then calls mxuport_ioctl() and eventually this
function. tty_perform_flush() acquires ldisc_sem (read).

Concurrently, another thread could call a TCSETS ioctl, which acquires
ldisc_sem (read) in tty_mode_ioctl() and then tries to acquire tport->mutex
via serial_set_termios(). If a writer (like tty_set_ldisc) is waiting for
ldisc_sem, the read lock will block, completing the deadlock.

Additionally, because the hardware buffer is purged here before
tty_perform_flush() calls tty_check_change(), does this bypass POSIX job
control, allowing background processes to evade SIGTTOU restrictions?

> +
> +	if (mask & MX_PURGE_TX)
> +		mxuport_tx_flush_done(port, !err);
> +
> +	if (err)
> +		return err;
> +
> +	return ret;
> +}
> +
>  static int mxuport_ioctl(struct tty_struct *tty, unsigned int cmd,
>  			 unsigned long arg)
>  {
> @@ -959,6 +1029,8 @@ static int mxuport_ioctl(struct tty_struct *tty, unsigned int cmd,
>  	void __user *argp = (void __user *)arg;
>  
>  	switch (cmd) {
> +	case TCFLSH:
> +		return mxuport_flush_buffer(tty, arg);
>  	case TIOCGRS485:
>  		return mxuport_get_rs485_config(port, argp);
>  	case TIOCSRS485:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016100351.681390-1-crescentcy.hsieh@moxa.com?part=5

[PATCH 6/6] USB: serial: mxuport: wait for UART transmit completion

[Crescent Hsieh]

The RQ_VENDOR_GET_OUTQUEUE response only includes the firmware transmit
queue on G1, so up to 128 bytes can remain in the UART FIFO after the
reported count reaches zero. For G2 and Platform UART firmware, the
response includes the UART FIFO count, but a frame may still be in the
transmit shifter.

The generic USB-serial wait stops when the reported count reaches zero
and can therefore return before UART transmission has completed.

Add a device-specific wait_until_sent callback which retains the generic
queue polling and then waits for the remaining UART transmit stages. Use
the active termios baud rate and frame size to wait 129 frame times for
G1 and one frame time for G2 and Platform UART devices. Keep the generic
polling and supplemental delay within the caller's timeout, and allow
both waits to be interrupted by a signal.

Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>
---
 drivers/usb/serial/mxuport.c | 65 ++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
index 9d304fb3aa02..53ab284f82aa 100644
--- a/drivers/usb/serial/mxuport.c
+++ b/drivers/usb/serial/mxuport.c
@@ -25,6 +25,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/firmware.h>
+#include <linux/hrtimer.h>
 #include <linux/jiffies.h>
 #include <linux/serial.h>
 #include <linux/serial_reg.h>
@@ -207,6 +208,11 @@ static const u16 mxuport_mux50u_fw_ver_offsets[] = {
 #define MX_WAIT_FOR_SEND_NEXT		0x0080
 #define MX_WAIT_FOR_PURGE		0x0100
 
+/* G1 queue count excludes its 128-byte UART FIFO and transmit shifter. */
+#define MX_UPORT_G1_DRAIN_FRAMES	129
+/* MUX50U queue count includes its UART FIFO but may exclude the shifter. */
+#define MX_MUX50U_DRAIN_FRAMES		1
+
 /* This structure holds all of the local port information */
 struct mxuport_port {
 	u32 sent_payload;
@@ -716,6 +722,64 @@ static bool mxuport_tx_empty(struct usb_serial_port *port)
 	return txlen == 0;
 }
 
+static void mxuport_wait_until_sent(struct tty_struct *tty, long timeout)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct usb_serial *serial = port->serial;
+	unsigned long features = (unsigned long)usb_get_serial_data(serial);
+	unsigned long deadline = 0;
+	unsigned int frame_bits;
+	unsigned int frames;
+	unsigned int baud;
+	ktime_t expires;
+	u64 delay_ns;
+
+	switch (MX_FAMILY(features)) {
+	case MX_UPORT_G1:
+		frames = MX_UPORT_G1_DRAIN_FRAMES;
+		break;
+	case MX_UPORT_G2:
+	case MX_PLATFORM_UART:
+		frames = MX_MUX50U_DRAIN_FRAMES;
+		break;
+	default:
+		usb_serial_generic_wait_until_sent(tty, timeout);
+		return;
+	}
+
+	baud = tty_get_baud_rate(tty);
+	if (!baud)
+		baud = 9600;
+
+	frame_bits = tty_get_frame_size(tty->termios.c_cflag);
+	if (timeout)
+		deadline = jiffies + timeout;
+
+	usb_serial_generic_wait_until_sent(tty, timeout);
+	if (signal_pending(current))
+		return;
+
+	if (timeout && time_after_eq(jiffies, deadline))
+		return;
+
+	delay_ns = DIV_ROUND_UP_ULL((u64)frames * frame_bits * NSEC_PER_SEC,
+				    baud);
+	if (timeout)
+		delay_ns = min_t(u64, delay_ns,
+				 jiffies_to_nsecs(deadline - jiffies));
+
+	dev_dbg(&port->dev,
+		"%s - waiting %llu us after transmit queue drained\n",
+		__func__, DIV_ROUND_UP_ULL(delay_ns, NSEC_PER_USEC));
+
+	expires = ktime_add_ns(ktime_get(), delay_ns);
+	while (!signal_pending(current)) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (!schedule_hrtimeout(&expires, HRTIMER_MODE_ABS))
+			break;
+	}
+}
+
 static int mxuport_set_mcr(struct usb_serial_port *port, u8 mcr_state)
 {
 	struct usb_serial *serial = port->serial;
@@ -1666,6 +1730,7 @@ static struct usb_serial_driver mxuport_device = {
 	.set_termios		= mxuport_set_termios,
 	.break_ctl		= mxuport_break_ctl,
 	.tx_empty		= mxuport_tx_empty,
+	.wait_until_sent	= mxuport_wait_until_sent,
 	.ioctl			= mxuport_ioctl,
 	.tiocmiwait		= usb_serial_generic_tiocmiwait,
 	.get_icount		= usb_serial_generic_get_icount,
-- 
2.53.0

Re: [PATCH 6/6] USB: serial: mxuport: wait for UART transmit completion

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] usb: serial: mxuport: missing tty_io_error check after generic wait
- [Low] usb: serial: mxuport: TOCTOU race on jiffies causes integer underflow
--

commit 4adf9d00bbc773d24e34eebaf0a7427eedbc5bef
Author: Crescent Hsieh <crescentcy.hsieh@moxa.com>
USB: serial: mxuport: wait for UART transmit completion

This patch introduces a device-specific wait_until_sent callback for the
mxuport driver. It supplements the generic queue polling with an additional
wait for remaining UART transmit stages based on the active termios baud
rate and frame size.

> diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
> index 9d304fb3aa02f..53ab284f82aad 100644
> --- a/drivers/usb/serial/mxuport.c
> +++ b/drivers/usb/serial/mxuport.c

[ ... ]

> @@ -716,6 +722,64 @@ static bool mxuport_tx_empty(struct usb_serial_port *port)
>  	return txlen == 0;
>  }
>  
> +static void mxuport_wait_until_sent(struct tty_struct *tty, long timeout)
> +{
> +	struct usb_serial_port *port = tty->driver_data;

[ ... ]

> +	frame_bits = tty_get_frame_size(tty->termios.c_cflag);
> +	if (timeout)
> +		deadline = jiffies + timeout;
> +
> +	usb_serial_generic_wait_until_sent(tty, timeout);
> +	if (signal_pending(current))
> +		return;

[Severity: Medium]
The generic wait function usb_serial_generic_wait_until_sent() can break out
early when tty_io_error(tty) is true, such as during a device disconnect.

Because mxuport_wait_until_sent() does not check tty_io_error() or device
disconnection after the generic wait returns, does it proceed unconditionally
to sleep for the hardware drain delay?

If the device is unplugged, this could cause close() or tcsetattr() to block
pointlessly for a long duration, up to around 25 seconds per port at 50 baud.

> +
> +	if (timeout && time_after_eq(jiffies, deadline))
> +		return;
> +
> +	delay_ns = DIV_ROUND_UP_ULL((u64)frames * frame_bits * NSEC_PER_SEC,
> +				    baud);
> +	if (timeout)
> +		delay_ns = min_t(u64, delay_ns,
> +				 jiffies_to_nsecs(deadline - jiffies));

[Severity: Low]
Can the unprotected double-read of jiffies cause an integer underflow here?

If the timer tick advances jiffies past deadline exactly between the
time_after_eq(jiffies, deadline) check and the deadline - jiffies subtraction
below it, the result of the subtraction will be negative. This negative value
will underflow to a massive unsigned value.

Consequently, min_t() would fail to bound the wait to the remaining timeout,
instead selecting the full delay_ns and artificially extending the sleep
duration. Would capturing jiffies into a local variable before the check
prevent this race?

> +
> +	dev_dbg(&port->dev,
> +		"%s - waiting %llu us after transmit queue drained\n",
> +		__func__, DIV_ROUND_UP_ULL(delay_ns, NSEC_PER_USEC));

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016100351.681390-1-crescentcy.hsieh@moxa.com?part=6