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

[Hugo Villeneuve]

On Tue, 25 Aug 2026 09:57:41 -0400
Hugo Villeneuve <hugo@xxxxxxxxxxx> wrote:

> 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

Hi Paul,
do you still have plans to resubmit that patch (also with my suggestions
above)? If you do, you can probably send it as a standalone patch
independent of this series...

-- 
Hugo Villeneuve