Re: [PATCH v1 2/4] usb: serial: mxuport: handle SEND_NEXT tx flow control

[Johan Hovold <johan@kernel.org>]

On Tue, Mar 24, 2026 at 11:50:39AM +0800, Crescent Hsieh wrote:
> Track the transmitted payload size per port and stop queueing more data
> once a bulk-out transfer reaches the device buffer threshold.
> 
> Resume transmission when the device reports UPORT_EVENT_SEND_NEXT, and
> reset the TX flow-control state when the port is opened.
> 
> This prevents the driver from queueing more TX data until the device
> reports that it is ready to accept the next transfer.

This explains what the patch does but says nothing about why you think
this is needed (which is the more important part).

The currently supported devices seems to work fine without this and
introducing this scheme should impact throughput negatively.

> Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>
> ---
>  drivers/usb/serial/mxuport.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
> index 034b506322c2..4d29a431cefd 100644
> --- a/drivers/usb/serial/mxuport.c
> +++ b/drivers/usb/serial/mxuport.c
> @@ -179,6 +179,8 @@
>  
>  /* This structure holds all of the local port information */
>  struct mxuport_port {
> +	u32 sent_payload;
> +	u32 hold_reason;
>  	u8 mcr_state;		/* Last MCR state */
>  	u8 msr_state;		/* Last MSR state */
>  	struct mutex mutex;	/* Protects mcr_state */
> @@ -250,9 +252,13 @@ MODULE_DEVICE_TABLE(usb, mxuport_idtable);
>  static int mxuport_prepare_write_buffer(struct usb_serial_port *port,
>  					void *dest, size_t size)
>  {
> +	struct mxuport_port *mxport = usb_get_serial_port_data(port);
>  	u8 *buf = dest;
>  	int count;
>  
> +	if (mxport->hold_reason & MX_WAIT_FOR_SEND_NEXT)
> +		return 0;

The generic write implementation does not support drivers returning zero
here (and has already made sure that there is data in the fifo) so if
this is at all needed that may need to be addressed.

> +
>  	count = kfifo_out_locked(&port->write_fifo, buf + HEADER_SIZE,
>  				 size - HEADER_SIZE,
>  				 &port->lock);
> @@ -263,6 +269,13 @@ static int mxuport_prepare_write_buffer(struct usb_serial_port *port,
>  	dev_dbg(&port->dev, "%s - size %zd count %d\n", __func__,
>  		size, count);
>  
> +	mxport->sent_payload += count;
> +
> +	if (mxport->sent_payload >= port->bulk_out_size) {
> +		mxport->hold_reason |= MX_WAIT_FOR_SEND_NEXT;
> +		buf[0] |= 0x80;
> +	}
> +
>  	return count + HEADER_SIZE;
>  }
>  
> @@ -484,6 +497,9 @@ static void mxuport_lsr_event(struct usb_serial_port *port, u8 buf[4])
>  static void mxuport_process_read_urb_event(struct usb_serial_port *port,
>  					   u8 buf[4], u32 event)
>  {
> +	struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +	unsigned long flags;
> +
>  	dev_dbg(&port->dev, "%s - receive event : %04x\n", __func__, event);
>  
>  	switch (event) {
> @@ -492,6 +508,13 @@ static void mxuport_process_read_urb_event(struct usb_serial_port *port,
>  		 * Sent as part of the flow control on device buffers.
>  		 * Not currently used.
>  		 */
> +		if (mxport->hold_reason & MX_WAIT_FOR_SEND_NEXT) {
> +			spin_lock_irqsave(&mxport->spinlock, flags);
> +			mxport->hold_reason &= ~MX_WAIT_FOR_SEND_NEXT;
> +			mxport->sent_payload = 0;
> +			usb_serial_generic_write_start(port, GFP_ATOMIC);
> +			spin_unlock_irqrestore(&mxport->spinlock, flags);
> +		}

The locking here looks questionable.

>  		break;
>  	case UPORT_EVENT_MSR:
>  		mxuport_msr_event(port, buf);
> @@ -1318,6 +1341,9 @@ static int mxuport_open(struct tty_struct *tty, struct usb_serial_port *port)
>  	 * returns.
>  	 */
>  	mxport->msr_state = 0;
> +	mxport->sent_payload = 0;
> +	mxport->hold_reason = 0;
> +	kfifo_reset(&port->write_fifo);

The fifo is already cleared on close.

>  
>  	return err;
>  }

Johan