serial: adi_uart4: avoid double increment of icount.tx in DMA TX path

[qasim-ijaz]

In adi_uart4_serial_dma_tx(), the driver updates uart->port.icount.tx
manually and then calls uart_xmit_advance(&uart->port, uart->tx_count).

However, uart_xmit_advance() already advances the xmit_fifo TX queue and
accounts transmitted bytes into icount.tx, so the function counts each
completed DMA transfer twice.

Fix this by dropping the explicit icount.tx update and rely on uart_xmit_advance()
for TX accounting.

Fixes: eab94da ("serial: Add UART driver for SC5xx SoCs")
Signed-off-by: Qasim Ijaz qasim.ijaz@analog.com

PR Type

• Bug fix (a change that fixes an issue)
• New feature (a change that adds new functionality)
• Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

• I have conducted a self-review of my own code changes
• I have compiled my changes, including the documentation
• I have tested the changes on the relevant hardware
• I have updated the documentation outside this repo accordingly
• I have provided links for the relevant upstream lore

[pamolloy]

Looks like you may have committed with your GMail:

Commit sha: [c7436ac](https://github.com/analogdevicesinc/linux/pull/3228/commits/c7436ac15f777ff5aea1afe070637af905f9e6b5), Author: Qasim, Committer: GitHub; Expected "Qasim [qasdev00@gmail.com](mailto:qasdev00@gmail.com)", but got "Qasim Ijaz [qasim.ijaz@analog.com](mailto:qasim.ijaz@analog.com)".

If you have a work directory and personal directory on our machine you can do something like:

~/.gitconfig

[includeIf "gitdir:~/repos/work/adi/"]
	path = ~/repos/work/adi/.gitconfig

~/repos/work/adi/.gitconfig

[user]
	email = foo.bar@analog.com

[pamolloy]

c7436ac15 serial: adi_uart4: avoid double increment of icount.tx in DMA TX path
drivers/tty/serial/adi_uart4.c
#14:
Warning: checkpatch: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
Fix this by dropping the explicit icount.tx update and rely on uart_xmit_advance()

https://github.com/analogdevicesinc/linux/actions/runs/23827385771/job/69453339504?pr=3228#step:9:23

[pamolloy]

It is important to also explain the impact of the bug. From Claude:

1. Inflated statistics: Any tool reading serial port statistics would see 2x the actual transmitted bytes
   - setserial -a /dev/ttySC0 would show wrong numbers
   - Custom monitoring tools using ioctl(fd, TIOCGICOUNT, ...) would get bad data
   - Bandwidth accounting and diagnostics would be incorrect
2. Debugging confusion: When investigating serial port issues, developers would see:
   TX bytes: 2048 (actual: 1024)
   RX bytes: 512 (correct)
3. This mismatch could lead to wild goose chases about "why is TX 4x higher than RX?"
4. Counter overflow: Since it's a __u32, the counter wraps at 4GB. With double counting, it wraps at 2GB of actual data, potentially breaking applications that rely on the counter not wrapping frequently.

1. Nobody actively monitors these counters
   - Most embedded systems just care that the serial console works
   - The bug doesn't affect actual data transmission at all
   - Serial ports are typically "set and forget"
2. The statistics are rarely checked
   - TIOCGICOUNT is an obscure ioctl that few applications use
   - Unlike network interfaces (ifconfig, ip stats), serial port stats aren't routinely displayed
   - Most serial debugging focuses on data content, not byte counts

How did you test this change?

[pamolloy]

This made me curious about the non-DMA path (from Claude):

So yes, the adi_uart4 driver has another opportunity for cleanup - the PIO path should use uart_fifo_get() instead of manually calling kfifo_get() + icount.tx++. This would make it consistent with the DMA path's use of uart_xmit_advance().

[qasim-ijaz]

It is important to also explain the impact of the bug. From Claude:

1. Inflated statistics: Any tool reading serial port statistics would see 2x the actual transmitted bytes
   • setserial -a /dev/ttySC0 would show wrong numbers
   • Custom monitoring tools using ioctl(fd, TIOCGICOUNT, ...) would get bad data
   • Bandwidth accounting and diagnostics would be incorrect
2. Debugging confusion: When investigating serial port issues, developers would see:
   TX bytes: 2048 (actual: 1024)
   RX bytes: 512 (correct)
3. This mismatch could lead to wild goose chases about "why is TX 4x higher than RX?"
4. Counter overflow: Since it's a __u32, the counter wraps at 4GB. With double counting, it wraps at 2GB of actual data, potentially breaking applications that rely on the counter not wrapping frequently.

1. Nobody actively monitors these counters
   • Most embedded systems just care that the serial console works
   • The bug doesn't affect actual data transmission at all
   • Serial ports are typically "set and forget"
2. The statistics are rarely checked
   • TIOCGICOUNT is an obscure ioctl that few applications use
   • Unlike network interfaces (ifconfig, ip stats), serial port stats aren't routinely displayed
   • Most serial debugging focuses on data content, not byte counts

How did you test this change?

Hi @pamolloy, thanks for the extensive review!

I’ve updated the commit message to better explain the impact.

This does not affect actual data transmission, but it does make
icount.tx incorrect, so TX statistics reported by diagnostics/tools can
be inflated and become misleading during UART debugging.

I tested this change on both the ADSP-SC594 and ADSP-SC598 and verified
normal UART operation after the fix. The root cause is also clear from
serial core since "uart_xmit_advance()" is documented and implemented to
account transmitted bytes in icount.tx, so the extra manual increment
in adi_uart4_serial_dma_tx() causes double counting.

Link: https://elixir.bootlin.com/linux/v6.12/source/include/linux/serial_core.h#L826

[qasim-ijaz]

c7436ac15 serial: adi_uart4: avoid double increment of icount.tx in DMA TX path
drivers/tty/serial/adi_uart4.c
#14:
Warning: checkpatch: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
Fix this by dropping the explicit icount.tx update and rely on uart_xmit_advance()

https://github.com/analogdevicesinc/linux/actions/runs/23827385771/job/69453339504?pr=3228#step:9:23

Should be fixed now but seems like the file isn't being compiled during checks for some reason?

[gastmaier]

Does this driver really technically depends on a specific architecture

depends on ARCH_SC59X = y || ARCH_SC58X = y || ARCH_SC57X = y || ARCH_SC59X_64 = y

Or all those conditionals are being used to say "HEY! This is only works with my SOC!"?
Nevertheless, at least add || COMPILE_TEST so we can compile test the driver.

Ideally simplify the conditionals to represent the technical dependency, not "what this driver support at the moment".

Also, at sound/soc/adi/Kconfig, this logic is ARCH_SC57X || ARCH_SC58X || ARCH_SC59X || ARCH_SC59X_64, why is here different?

[pamolloy]

Hi @pamolloy, thanks for the extensive review!

I’ve updated the commit message to better explain the impact.

Thanks!

This does not affect actual data transmission, but it does make icount.tx incorrect, so TX statistics reported by diagnostics/tools can be inflated and become misleading during UART debugging.

I tested this change on both the ADSP-SC594 and ADSP-SC598 and verified normal UART operation after the fix. The root cause is also clear from serial core since "uart_xmit_advance()" is documented and implemented to account transmitted bytes in icount.tx, so the extra manual increment in adi_uart4_serial_dma_tx() causes double counting.

Can you also explain what the "Kernel serial diagnostics" are and what userspace tools use them? Enough so someone else can reproduce it relatively easily. I assume this would be useful if we were using the UART to transmit generic data and not using it as a console. When used as a console these statistics certainly aren't necessary.

[qasim-ijaz]

Hi @pamolloy, thanks for the extensive review!
I’ve updated the commit message to better explain the impact.

Thanks!

This does not affect actual data transmission, but it does make icount.tx incorrect, so TX statistics reported by diagnostics/tools can be inflated and become misleading during UART debugging.
I tested this change on both the ADSP-SC594 and ADSP-SC598 and verified normal UART operation after the fix. The root cause is also clear from serial core since "uart_xmit_advance()" is documented and implemented to account transmitted bytes in icount.tx, so the extra manual increment in adi_uart4_serial_dma_tx() causes double counting.

Can you also explain what the "Kernel serial diagnostics" are and what userspace tools use them? Enough so someone else can reproduce it relatively easily. I assume this would be useful if we were using the UART to transmit generic data and not using it as a console. When used as a console these statistics certainly aren't necessary.

Sure, so an example would be something like /proc/tty/driver/adi-uart4 this directly shows you various pieces of info such as the rx and tx counts the driver has completed:

root@adsp-sc598-som-ezkit:~# cat /proc/tty/driver/adi-uart4 
serinfo:1.0 driver revision:
0: uart:ADI-UART4 mmio:0x00000000 irq:0 tx:19594 rx:25 RTS|CTS|DTR|DSR|CD

To easily reproduce and view changes to the tx and rx you can boot up Linux on a ADSP-SC589 or ADSP-SC594 (or whatever). Then in a separate terminal ssh into the board. Then in the ssh session view the above. Then in the serial console type a single character, move back to the ssh session and view the data again, it should have increased now.

I did this exact experiment, so this is the count beforehand:

root@adsp-sc598-som-ezkit:~# cat /proc/tty/driver/adi-uart4
serinfo:1.0 driver revision:
0: uart:ADI-UART4 mmio:0x00000000 irq:0 tx:19594 rx:25 RTS|CTS|DTR|DSR|CD
root@adsp-sc598-som-ezkit:~# 

...Then type a character in the serial console (i pressed 'a' once) and view the counters again in the ssh session:

root@adsp-sc598-som-ezkit:~# cat /proc/tty/driver/adi-uart4
serinfo:1.0 driver revision:
0: uart:ADI-UART4 mmio:0x00000000 irq:0 tx:19596 rx:26 RTS|CTS|DTR|DSR|CD

As you can see the TX counter has gone to 19596 (should be 19595) which illustrates the double increment
once from "uart->port.icount.tx += uart->tx_count;" and another time from the xmit_advance call. Does that make sense?

[qasim-ijaz]

Does this driver really technically depends on a specific architecture

depends on ARCH_SC59X = y || ARCH_SC58X = y || ARCH_SC57X = y || ARCH_SC59X_64 = y

Or all those conditionals are being used to say "HEY! This is only works with my SOC!"? Nevertheless, at least add || COMPILE_TEST so we can compile test the driver.

Ideally simplify the conditionals to represent the technical dependency, not "what this driver support at the moment".

Also, at sound/soc/adi/Kconfig, this logic is ARCH_SC57X || ARCH_SC58X || ARCH_SC59X || ARCH_SC59X_64, why is here different?

Thank for your review. I will look into this.

[pamolloy]

Should be at the end of the file

[qasim-ijaz]

Should be fixed now.

[pamolloy]

Would be good to address the coccicheck warning as well:

coccicheck: 3-8  No need to set .owner here. The core will do it. 

[qasim-ijaz]

Good idea, I have fixed this.

[qasim-ijaz]

Hi @pamolloy, thanks for the extensive review!
I’ve updated the commit message to better explain the impact.

Thanks!

This does not affect actual data transmission, but it does make icount.tx incorrect, so TX statistics reported by diagnostics/tools can be inflated and become misleading during UART debugging.
I tested this change on both the ADSP-SC594 and ADSP-SC598 and verified normal UART operation after the fix. The root cause is also clear from serial core since "uart_xmit_advance()" is documented and implemented to account transmitted bytes in icount.tx, so the extra manual increment in adi_uart4_serial_dma_tx() causes double counting.

Can you also explain what the "Kernel serial diagnostics" are and what userspace tools use them? Enough so someone else can reproduce it relatively easily. I assume this would be useful if we were using the UART to transmit generic data and not using it as a console. When used as a console these statistics certainly aren't necessary.

Sure, so an example would be something like /proc/tty/driver/adi-uart4 this directly shows you various pieces of info such as the rx and tx counts the driver has completed:

root@adsp-sc598-som-ezkit:~# cat /proc/tty/driver/adi-uart4 
serinfo:1.0 driver revision:
0: uart:ADI-UART4 mmio:0x00000000 irq:0 tx:19594 rx:25 RTS|CTS|DTR|DSR|CD

To easily reproduce and view changes to the tx and rx you can boot up Linux on a ADSP-SC589 or ADSP-SC594 (or whatever). Then in a separate terminal ssh into the board. Then in the ssh session view the above. Then in the serial console type a single character, move back to the ssh session and view the data again, it should have increased now.

I did this exact experiment, so this is the count beforehand:

root@adsp-sc598-som-ezkit:~# cat /proc/tty/driver/adi-uart4
serinfo:1.0 driver revision:
0: uart:ADI-UART4 mmio:0x00000000 irq:0 tx:19594 rx:25 RTS|CTS|DTR|DSR|CD
root@adsp-sc598-som-ezkit:~# 

...Then type a character in the serial console (i pressed 'a' once) and view the counters again in the ssh session:

root@adsp-sc598-som-ezkit:~# cat /proc/tty/driver/adi-uart4
serinfo:1.0 driver revision:
0: uart:ADI-UART4 mmio:0x00000000 irq:0 tx:19596 rx:26 RTS|CTS|DTR|DSR|CD

As you can see the TX counter has gone to 19596 (should be 19595) which illustrates the double increment once from "uart->port.icount.tx += uart->tx_count;" and another time from the xmit_advance call. Does that make sense?

@pamolloy maybe i should add this info to the commit message? could be a good addition

[pamolloy]

As you can see the TX counter has gone to 19596 (should be 19595) which illustrates the double increment once from "uart->port.icount.tx += uart->tx_count;" and another time from the xmit_advance call. Does that make sense?

@pamolloy maybe i should add this info to the commit message? could be a good addition

Definitely, in this case where we're committing into our own tree and this commit will get squashed before mainlining.

If this patch were submitted mainline some of the above information would be nice to include in a cover letter (to show you've tested it), although most of the context isn't required for a mainline review since it'll get reviewed by subsystem maintainers. Of course within ADI we can't specialize in all subsystems so it is really helpful to include in the PR description in that case. 😄

We also have the System Level doctools documentation, which is a good place to document things, especially if it is applicable to our customers.