Re: [PATCH v2 RESEND 1/2] serial: sc16is7xx: refill TX FIFO below trigger using fresh TXLVL

[David Laight <david.laight.linux@gmail.com>]

On Wed, 30 Sep 2026 16:56:27 +0200
Paul Mbewe <paultyson.mbewe@ziehl-abegg.de> wrote:

> sc16is7xx_handle_tx() reads TXLVL once and sizes the hardware TX FIFO
> write from that value. TXLVL reports free space in the hardware TX FIFO.
> 
> On this SPI-backed path, hardirq/softirq activity, RT scheduling, and
> waiting for synchronous SPI transfers can delay the refill while the UART
> continues draining. The TXLVL value can therefore become stale before the
> hardware TX FIFO write completes.
> 
> One failing ftrace with the default 8-free-space trigger showed:
> 
>   tx_start       txlvl=9  txlvl_read_us=580
>   tx_pre_write   sent=9   pre_write_us=16
>   tx_segment     sent=9   seg_us=364
>   tx_post_write  txlvl_before=9 sent=9 txlvl_after=12 pending_after=29
>                  post_gap_us=12 post_txlvl_us=130
> 
> The driver read 9 free spaces and wrote 9 bytes to the hardware TX FIFO,
> but the post-write TXLVL read still reported 12 free spaces while 29 bytes
> remained queued in the xmit kfifo. Even allowing for the post-write read
> window, the hardware TX FIFO had not been filled below the 8-free-space
> trigger, so no new threshold crossing was expected.
> 
> The captured failing samples had the same pattern: data remained queued
> in the xmit kfifo while post-write TXLVL remained above the hardware
> trigger. The hardware TX FIFO then drained empty before another refill
> was requested, producing an unintended gap on the wire.
> 
> Fix this by re-reading TXLVL after each hardware TX FIFO write while data
> remains queued in the xmit kfifo. If TXLVL is still at or above the
> trigger, top up the hardware TX FIFO again. Stop when the xmit kfifo is
> empty or a TXLVL read confirms that hardware TX FIFO free space is
> strictly below the trigger.
> 
> Stopping when TXLVL was equal to the trigger still allowed TX gaps in the
> tested workload. Continuing until TXLVL was strictly below the trigger
> eliminated the observed gaps caused by stale-TXLVL under-fill.
> 
> Program the hardware TX trigger explicitly through TLR using the same
> constant as the refill-loop threshold. This prevents the software refill
> condition from diverging from the programmed hardware trigger.
> 
> Tested on SC16IS752 over 1 MHz SPI on an i.MX6ULL single-core
> PREEMPT_RT system, transmitting RS-485 at 115200 baud 8N1 under
> continuous Modbus RTU load.
> 
> Fixes: dfeae619d781 ("serial: sc16is7xx")
> Cc: stable@vger.kernel.org
> Reported-by: Tobias Gannert <tobias.gannert@ziehl-abegg.de>
> Link: https://lore.kernel.org/linux-serial/20260623112225.82386-3-paultyson.mbewe@ziehl-abegg.de/
> Signed-off-by: Paul Mbewe <paultyson.mbewe@ziehl-abegg.de>

Since I made him find the real bug...

Reviewed-by: David Laight <david.laight.linux@gmail.com>

> ---
> Changes in v2:
>   - New patch arising from review of v1 patch 2
>   - Added a fresh TXLVL read after each hardware TX FIFO write
>   - Continue refilling until TXLVL is strictly below the trigger
>   - Explicitly program the hardware trigger from the same constant used
>     by the refill loop
>   - Corrected the root cause from trigger-level margin to stale-TXLVL
>     under-fill
>   - Removed the max310x comparison
> 
>  drivers/tty/serial/sc16is7xx.c | 38 +++++++++++++++++++++++++++-------
>  1 file changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index fa7805d2cde2..1f7e98c2b570 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -216,6 +216,8 @@
>  #define SC16IS7XX_TLR_TX_TRIGGER(words)	((((words) / 4) & 0x0f) << 0)
>  #define SC16IS7XX_TLR_RX_TRIGGER(words)	((((words) / 4) & 0x0f) << 4)
>  
> +#define SC16IS7XX_TX_TRIGGER_LEVEL	8
> +
>  /* IOControl register bits (Only 75x/76x) */
>  #define SC16IS7XX_IOCONTROL_LATCH_BIT	BIT(0)   /* Enable input latching */
>  #define SC16IS7XX_IOCONTROL_MODEM_A_BIT	BIT(1)   /* Enable GPIO[7:4] as modem A pins */
> @@ -647,6 +649,21 @@ static void sc16is7xx_handle_rx(struct uart_port *port, unsigned int rxlen,
>  	tty_flip_buffer_push(&port->state->port);
>  }
>  
> +static unsigned int sc16is7xx_txlvl(struct uart_port *port)
> +{
> +	unsigned int txlvl;
> +
> +	txlvl = sc16is7xx_port_read(port, SC16IS7XX_TXLVL_REG);
> +	if (txlvl > SC16IS7XX_FIFO_SIZE) {
> +		dev_err_ratelimited(port->dev,
> +				    "chip reports %u free bytes in TX FIFO, but it only has %u\n",
> +				    txlvl, SC16IS7XX_FIFO_SIZE);
> +		return 0;
> +	}
> +
> +	return txlvl;
> +}
> +
>  static void sc16is7xx_handle_tx(struct uart_port *port)
>  {
>  	struct tty_port *tport = &port->state->port;
> @@ -668,13 +685,7 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
>  	}
>  
>  	/* Limit to space available in TX FIFO */
> -	txlen = sc16is7xx_port_read(port, SC16IS7XX_TXLVL_REG);
> -	if (txlen > SC16IS7XX_FIFO_SIZE) {
> -		dev_err_ratelimited(port->dev,
> -			"chip reports %d free bytes in TX fifo, but it only has %d",
> -			txlen, SC16IS7XX_FIFO_SIZE);
> -		txlen = 0;
> -	}
> +	txlen = sc16is7xx_txlvl(port);
>  
>  	/* Handle circular buffer wrap-around by sending multiple segments */
>  	while (txlen > 0 && !kfifo_is_empty(&tport->xmit_fifo)) {
> @@ -687,7 +698,14 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
>  
>  		sc16is7xx_fifo_write(port, tail, to_send);
>  		uart_xmit_advance(port, to_send);
> -		txlen -= to_send;
> +
> +		if (kfifo_is_empty(&tport->xmit_fifo))
> +			break;
> +
> +		/* Refill below the trigger to enable the next THRI crossing. */
> +		txlen = sc16is7xx_txlvl(port);
> +		if (txlen < SC16IS7XX_TX_TRIGGER_LEVEL)
> +			break;
>  	}
>  
>  	uart_port_lock_irqsave(port, &flags);
> @@ -1139,6 +1157,10 @@ static int sc16is7xx_startup(struct uart_port *port)
>  			     SC16IS7XX_TCR_RX_RESUME(24) |
>  			     SC16IS7XX_TCR_RX_HALT(48));
>  
> +	/* Sync hardware and software TX trigger levels */
> +	sc16is7xx_port_write(port, SC16IS7XX_TLR_REG,
> +			     SC16IS7XX_TLR_TX_TRIGGER(SC16IS7XX_TX_TRIGGER_LEVEL));
> +
>  	/* Disable TCR/TLR access */
>  	sc16is7xx_port_update(port, SC16IS7XX_MCR_REG, SC16IS7XX_MCR_TCRTLR_BIT, 0);
>