Re: [PATCH 1/2] serial: sc16is7xx: fix TX gap caused by kfifo circular buffer wrap-around

[Hugo Villeneuve]

Hi Paul,

On Tue, 23 Jun 2026 13:22:24 +0200
Paul Mbewe <paultyson.mbewe@xxxxxxxxxxxxxx> wrote:

> kfifo_out_linear_ptr() returns only one contiguous linear segment of the
> circular kfifo buffer. When transmit data wraps around the end of the
> buffer, only the first segment (up to the buffer end) is sent. The
> remaining data at the start of the buffer is not sent until the next TX
> interrupt fires, resulting in a visible inter-frame gap on the wire.
> 
> This gap violates the Modbus RTU 1.5 character-time inter-character
> silence limit. Receivers interpret any silence exceeding 1.5 character
> times as an end-of-frame marker, splitting a single valid frame into
> two malformed fragments and corrupting communication on the bus.
> 
> The incomplete transfer also causes unnecessary TX interrupts: instead
> of draining the full available FIFO space in one pass, the driver fires
> an extra interrupt per wrap-around just to send the remaining bytes.
> 
> The pre-kfifo code handled wrap-around by copying bytes one at a time
> from the circ_buf into a linear staging buffer. The conversion to kfifo
> replaced this with a single kfifo_out_linear_ptr() call, losing the
> wrap-around handling. The max310x driver (a similar SPI UART) correctly
> handles this with a while loop.
> 
> Fix this by calling kfifo_out_linear_ptr() in a loop, advancing through
> all contiguous segments until the available TX FIFO space is exhausted
> or the kfifo is empty.
> 
> Tested on SC16IS752 (SPI) driving RS-485 at 115200 baud 8N1 on an
> i.MX6ULL based board. Oscilloscope confirmed mid-frame breaks at the
> kfifo wrap-around boundary before the fix; no breaks observed after.
> 
> Fixes: 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Tobias Gannert <tobias.gannert@xxxxxxxxxxxxxx>
> Tested-by: Tobias Gannert <tobias.gannert@xxxxxxxxxxxxxx>
> Reviewed-by: Joachim Knorr <joachim.knorr@xxxxxxxxxxxxxx>
> Signed-off-by: Paul Mbewe <paultyson.mbewe@xxxxxxxxxxxxxx>
> ---
>  drivers/tty/serial/sc16is7xx.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 1a2c4c14f6aa..395a219280be 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -730,9 +730,17 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
>  		txlen = 0;
>  	}
>  
> -	txlen = kfifo_out_linear_ptr(&tport->xmit_fifo, &tail, txlen);
> -	sc16is7xx_fifo_write(port, tail, txlen);
> -	uart_xmit_advance(port, txlen);
> +	/* Handle circular buffer wrap-around by sending in contiguous segments */

Maybe I would drop "by sending in contiguous segments", because it is
more confusing than helping for me, because before your patch it was
already sending one contiguous segment...
Or maybe change to "... by sending multiple segments"?


> +	while (txlen > 0 && !kfifo_is_empty(&tport->xmit_fifo)) {
> +		unsigned int to_send;

You can now move this variable here since it is used only in the
while() scope:
                unsigned char *tail;

> +
> +		to_send = kfifo_out_linear_ptr(&tport->xmit_fifo, &tail, txlen);
> +		if (!to_send)
> +			break;

Insert empty line

> +		sc16is7xx_fifo_write(port, tail, to_send);
> +		uart_xmit_advance(port, to_send);
> +		txlen -= to_send;
> +	}
>  
>  	uart_port_lock_irqsave(port, &flags);
>  	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
> -- 
> 2.43.0
> 
> 
> 


-- 
Hugo Villeneuve