[PATCH v2 RESEND] serial: sc16is7xx: fix TX gap caused by kfifo circular buffer wrap-around

[PATCH v2 RESEND] serial: sc16is7xx: fix TX gap caused by kfifo circular buffer wrap-around

[Paul Mbewe]

kfifo_out_linear_ptr() returns only one contiguous linear segment of the
xmit kfifo. When transmit data wraps around the end of the kfifo, only
the first segment up to the buffer end is sent. The remaining data at
the start of the kfifo is not sent until the next TX interrupt fires,
resulting in a visible mid-frame TX gap on the wire.

The resulting gap is unintended: data remains queued in the xmit kfifo,
but the hardware TX FIFO drains empty before the remaining segment is
sent. Such gaps can break timing-sensitive serial protocols such as
Modbus RTU.

Modbus RTU requires a message to be transmitted as a continuous stream.
For baud rates above 19200, the Modbus Serial Line guide recommends a
fixed 750 us inter-character timeout. On the tested 115200-baud system,
oscilloscope measurements showed mid-frame gaps exceeding that value.
Receivers using the recommended timeout may therefore discard the
incomplete message.

The incomplete transfer also causes unnecessary TX interrupts: instead
of using all available hardware TX FIFO space in one go, the driver
requires an extra interrupt to send the remaining segment after the
wrap.

After the tty xmit buffer was converted to a kfifo, the driver used
uart_fifo_out() to copy data into a linear staging buffer, allowing a
transfer to span the kfifo wrap-around boundary. Commit 133f4c00b8b2
("serial: sc16is7xx: fix TX fifo corruption") replaced uart_fifo_out()
with a single kfifo_out_linear_ptr() call to remove the shared TX/RX
buffer. Since kfifo_out_linear_ptr() exposes only one contiguous
segment, that change lost the wrap-around handling.

Fix this by calling kfifo_out_linear_ptr() in a loop, advancing through
all contiguous segments until the available hardware TX FIFO space is
exhausted or the xmit kfifo is empty.

This fixes the kfifo wrap-around gap independently of the stale-TXLVL
refill issue.

Tested on SC16IS752 over SPI driving RS-485 at 115200 baud 8N1 on an
i.MX6ULL-based board. Oscilloscope measurements confirmed mid-frame
breaks at the kfifo wrap-around boundary before the fix; no such breaks
were observed afterward.

Fixes: 133f4c00b8b2 ("serial: sc16is7xx: fix TX fifo corruption")
Cc: stable@vger.kernel.org
Reported-by: Tobias Gannert <tobias.gannert@ziehl-abegg.de>
Reviewed-by: Joachim Knorr <joachim.knorr@ziehl-abegg.de>
Link: https://lore.kernel.org/linux-serial/20260623112225.82386-2-paultyson.mbewe@ziehl-abegg.de/
Signed-off-by: Paul Mbewe <paultyson.mbewe@ziehl-abegg.de>
---
Changes in v2:
  - Corrected the Modbus RTU timing description for baud rates above 19200
  - Distinguished the xmit kfifo from the hardware TX FIFO
  - Clarified that this fixes one independent TX gap source
  - Corrected the introducing-commit history for direct kfifo access
  - Removed the max310x comparison
  - Addressed Hugo Villeneuve's review: clarified that multiple segments
    are sent, scoped tail to the loop, and added the requested blank line

 drivers/tty/serial/sc16is7xx.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 7107a0fb1e7b..fa7805d2cde2 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -652,7 +652,6 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
 	struct tty_port *tport = &port->state->port;
 	unsigned long flags;
 	unsigned int txlen;
-	unsigned char *tail;
 
 	if (unlikely(port->x_char)) {
 		sc16is7xx_port_write(port, SC16IS7XX_THR_REG, port->x_char);
@@ -677,9 +676,19 @@ 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 multiple segments */
+	while (txlen > 0 && !kfifo_is_empty(&tport->xmit_fifo)) {
+		unsigned char *tail;
+		unsigned int to_send;
+
+		to_send = kfifo_out_linear_ptr(&tport->xmit_fifo, &tail, txlen);
+		if (!to_send)
+			break;
+
+		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

Re: [PATCH v2 RESEND] serial: sc16is7xx: fix TX gap caused by kfifo circular buffer wrap-around

[Hugo Villeneuve]

Hi Paul,

On Mon, 28 Sep 2026 10:55:59 +0200
Paul Mbewe <paultyson.mbewe@ziehl-abegg.de> wrote:

> kfifo_out_linear_ptr() returns only one contiguous linear segment of the
> xmit kfifo. When transmit data wraps around the end of the kfifo, only
> the first segment up to the buffer end is sent. The remaining data at
> the start of the kfifo is not sent until the next TX interrupt fires,
> resulting in a visible mid-frame TX gap on the wire.
> 
> The resulting gap is unintended: data remains queued in the xmit kfifo,
> but the hardware TX FIFO drains empty before the remaining segment is
> sent. Such gaps can break timing-sensitive serial protocols such as
> Modbus RTU.
> 
> Modbus RTU requires a message to be transmitted as a continuous stream.
> For baud rates above 19200, the Modbus Serial Line guide recommends a
> fixed 750 us inter-character timeout. On the tested 115200-baud system,
> oscilloscope measurements showed mid-frame gaps exceeding that value.
> Receivers using the recommended timeout may therefore discard the
> incomplete message.
> 
> The incomplete transfer also causes unnecessary TX interrupts: instead
> of using all available hardware TX FIFO space in one go, the driver
> requires an extra interrupt to send the remaining segment after the
> wrap.
> 
> After the tty xmit buffer was converted to a kfifo, the driver used
> uart_fifo_out() to copy data into a linear staging buffer, allowing a
> transfer to span the kfifo wrap-around boundary. Commit 133f4c00b8b2
> ("serial: sc16is7xx: fix TX fifo corruption") replaced uart_fifo_out()
> with a single kfifo_out_linear_ptr() call to remove the shared TX/RX
> buffer. Since kfifo_out_linear_ptr() exposes only one contiguous
> segment, that change lost the wrap-around handling.
> 
> Fix this by calling kfifo_out_linear_ptr() in a loop, advancing through
> all contiguous segments until the available hardware TX FIFO space is
> exhausted or the xmit kfifo is empty.
> 
> This fixes the kfifo wrap-around gap independently of the stale-TXLVL
> refill issue.
> 
> Tested on SC16IS752 over SPI driving RS-485 at 115200 baud 8N1 on an
> i.MX6ULL-based board. Oscilloscope measurements confirmed mid-frame
> breaks at the kfifo wrap-around boundary before the fix; no such breaks
> were observed afterward.
> 
> Fixes: 133f4c00b8b2 ("serial: sc16is7xx: fix TX fifo corruption")
> Cc: stable@vger.kernel.org
> Reported-by: Tobias Gannert <tobias.gannert@ziehl-abegg.de>
> Reviewed-by: Joachim Knorr <joachim.knorr@ziehl-abegg.de>
> Link: https://lore.kernel.org/linux-serial/20260623112225.82386-2-paultyson.mbewe@ziehl-abegg.de/
> Signed-off-by: Paul Mbewe <paultyson.mbewe@ziehl-abegg.de>
> ---
> Changes in v2:
>   - Corrected the Modbus RTU timing description for baud rates above 19200
>   - Distinguished the xmit kfifo from the hardware TX FIFO
>   - Clarified that this fixes one independent TX gap source
>   - Corrected the introducing-commit history for direct kfifo access
>   - Removed the max310x comparison
>   - Addressed Hugo Villeneuve's review: clarified that multiple segments
>     are sent, scoped tail to the loop, and added the requested blank line
> 
>  drivers/tty/serial/sc16is7xx.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 7107a0fb1e7b..fa7805d2cde2 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -652,7 +652,6 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
>          struct tty_port *tport = &port->state->port;
>          unsigned long flags;
>          unsigned int txlen;
> -       unsigned char *tail;
>  
>          if (unlikely(port->x_char)) {
>                  sc16is7xx_port_write(port, SC16IS7XX_THR_REG, port->x_char);
> @@ -677,9 +676,19 @@ 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 multiple segments */
> +       while (txlen > 0 && !kfifo_is_empty(&tport->xmit_fifo)) {
> +               unsigned char *tail;
> +               unsigned int to_send;
> +
> +               to_send = kfifo_out_linear_ptr(&tport->xmit_fifo, &tail, txlen);
> +               if (!to_send)
> +                       break;
> +
> +               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
> 
> _______________________________________ 
> 
> ZIEHL-ABEGG 
> 
> Executive Board: Joachim Ley (Chairman), Marco Altherr, Wolfgang Mayer
> Supervisory Board: Dennis Ziehl (Chairman) 
> 
> Court of Registry: District Court Stuttgart HRB 746188
> Company Seat: K&#xFC;nzelsau, Germany 
> 
> Der Inhalt dieser E-Mail und/oder jegliche Anh&#xE4;nge k&#xF6;nnen vertrauliche Mitteilungen enthalten und sind ausschlie&#xDF;lich f&#xFC;r den bezeichneten Adressaten bestimmt.
> Wenn Sie nicht der vorgesehene Adressat dieser E-Mail oder dessen Vertreter sein sollten, so beachten Sie bitte, dass jede Form der Kenntnisnahme, Ver&#xF6;ffentlichung,
> Vervielf&#xE4;ltigung oder Weitergabe des Inhalts dieser E-Mail einschlie&#xDF;lich Anh&#xE4;ngen unzul&#xE4;ssig ist.
> Wir bitten Sie, sich in diesem Fall mit dem Absender der E-Mail in Verbindung zu setzen. 
> 
> The content of this e-mail and/or attachments may contain confidential information and is intended solely for the named recipient.
> If you are not the intended recipient of this e-mail or on its distribution list, please note that any type of disclosure, publication,
> copying or distribution of the content of this e-mail including attachments is strictly forbidden.
> In this case, we would kindly ask you to notify the sender of the e-mail.

It seems you keep sending your patches with that signature, which you
can't do on public kernel mailing lists, and that is why Greg deleted
your previous patch submission.

-- 
Hugo Villeneuve