[PATCH v1 0/3] serial: Do not count XON/XOFF in the statistics

[PATCH v1 0/3] serial: Do not count XON/XOFF in the statistics

[Andy Shevchenko]

Some drivers count XON/XOFF in the Tx statistics, some do not.
I actually a bit uncertain, but I _think_ the correct way is not
to count them, hence this series.

Andy Shevchenko (3):
  serial: core: Don't count XON/XOFF in the statistics
  serial: fsl_lpuart: Don't count XON/XOFF in the statistics
  serial: sprd: Don't count XON/XOFF in the statistics

 drivers/tty/serial/fsl_lpuart.c  | 1 -
 drivers/tty/serial/sprd_serial.c | 1 -
 include/linux/serial_core.h      | 4 ++--
 3 files changed, 2 insertions(+), 4 deletions(-)

-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 1/3] serial: core: Don't count XON/XOFF in the statistics

[Andy Shevchenko]

Don't count XON/XOFF in the statistics in __uart_port_tx().
It will make it in align with other serial drivers. With
this change we may use uart_xmit_advance() API instead of
open coded analogue.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/serial_core.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 0a0f6e21d40e..9579d70a367e 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -767,7 +767,7 @@ enum UART_TX_FLAGS {
 	struct circ_buf *xmit = &__port->state->xmit;			      \
 	unsigned int pending;						      \
 									      \
-	for (; (for_test) && (tx_ready); (for_post), __port->icount.tx++) {   \
+	for (; (for_test) && (tx_ready); (for_post)) {			      \
 		if (__port->x_char) {					      \
 			(ch) = __port->x_char;				      \
 			(put_char);					      \
@@ -780,7 +780,7 @@ enum UART_TX_FLAGS {
 									      \
 		(ch) = xmit->buf[xmit->tail];				      \
 		(put_char);						      \
-		xmit->tail = (xmit->tail + 1) % UART_XMIT_SIZE;		      \
+		uart_xmit_advance(__port, 1);				      \
 	}								      \
 									      \
 	(tx_done);							      \
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 2/3] serial: fsl_lpuart: Don't count XON/XOFF in the statistics

[Andy Shevchenko]

Don't count XON/XOFF in the statistics in lpuart32_transmit_buffer().
It will make it in align with other serial drivers.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/fsl_lpuart.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index bbcbc91482af..a1cdb0ccc40e 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -761,7 +761,6 @@ static inline void lpuart32_transmit_buffer(struct lpuart_port *sport)
 
 	if (sport->port.x_char) {
 		lpuart32_write(&sport->port, sport->port.x_char, UARTDATA);
-		sport->port.icount.tx++;
 		sport->port.x_char = 0;
 		return;
 	}
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 3/3] serial: sprd: Don't count XON/XOFF in the statistics

[Andy Shevchenko]

Don't count XON/XOFF in the statistics in sprd_start_tx_dma().
It will make it in align with other serial drivers.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/sprd_serial.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 15f14fa593da..3554b9e7678f 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -323,7 +323,6 @@ static void sprd_start_tx_dma(struct uart_port *port)
 
 	if (port->x_char) {
 		serial_out(port, SPRD_TXD, port->x_char);
-		port->icount.tx++;
 		port->x_char = 0;
 		return;
 	}
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v1 0/3] serial: Do not count XON/XOFF in the statistics

[Andy Shevchenko]

Wed, Apr 03, 2024 at 05:46:14PM +0300, Andy Shevchenko kirjoitti:
> Some drivers count XON/XOFF in the Tx statistics, some do not.
> I actually a bit uncertain, but I _think_ the correct way is not
> to count them, hence this series.

Okay, it seems there are much more drivers doing that. Perhaps we need
to add that to the rest in this case (i.o.w. invert the series from removal
to addition)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/3] serial: Do not count XON/XOFF in the statistics

[Jiri Slaby]

On 04. 04. 24, 0:15, Andy Shevchenko wrote:
> Wed, Apr 03, 2024 at 05:46:14PM +0300, Andy Shevchenko kirjoitti:
>> Some drivers count XON/XOFF in the Tx statistics, some do not.
>> I actually a bit uncertain, but I _think_ the correct way is not
>> to count them, hence this series.
> 
> Okay, it seems there are much more drivers doing that. Perhaps we need
> to add that to the rest in this case (i.o.w. invert the series from removal
> to addition)?

Interesting, perhaps cut & paste?

XON and XOFF are overhead IMO. So should not be counted. When they are, 
they mangle statistics as in transmitted (real) bytes per second.

How are they handled on the RX side?

thanks,
-- 
js
suse labs

Re: [PATCH v1 0/3] serial: Do not count XON/XOFF in the statistics

[Andy Shevchenko]

On Thu, Apr 04, 2024 at 07:16:55AM +0200, Jiri Slaby wrote:
> On 04. 04. 24, 0:15, Andy Shevchenko wrote:
> > Wed, Apr 03, 2024 at 05:46:14PM +0300, Andy Shevchenko kirjoitti:
> > > Some drivers count XON/XOFF in the Tx statistics, some do not.
> > > I actually a bit uncertain, but I _think_ the correct way is not
> > > to count them, hence this series.
> > 
> > Okay, it seems there are much more drivers doing that. Perhaps we need
> > to add that to the rest in this case (i.o.w. invert the series from removal
> > to addition)?
> 
> Interesting, perhaps cut & paste?
> 
> XON and XOFF are overhead IMO. So should not be counted. When they are, they
> mangle statistics as in transmitted (real) bytes per second.
> 
> How are they handled on the RX side?

It took me a while.

All serial drivers accept everything and those that care, update statics for
anything they receive. This is because of layering. The Rx XON/XOFF seems
(note I am completely unfamiliar with mysterious ways of TTY layers) to be
handled on TTY level by n_tty_receive_char_flow_ctrl(), i.o.w. we may not
skip counting it easily.

Now the question is, shall we count the control characters on output or not?
Whatever decision we made, we should document (if not yet) and align drivers
accordingly.

Another Q is what do books / other OS / projects usually do with them
WRT statistics?

If we count everything on a wire, then we must count them, otherwise
it depends on how we treat them.

P.S.
This series as is should be abandoned. But we may continue discussing topic
under this cover letter.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/3] serial: Do not count XON/XOFF in the statistics

[Greg Kroah-Hartman]

On Thu, Apr 04, 2024 at 06:54:38PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 04, 2024 at 07:16:55AM +0200, Jiri Slaby wrote:
> > On 04. 04. 24, 0:15, Andy Shevchenko wrote:
> > > Wed, Apr 03, 2024 at 05:46:14PM +0300, Andy Shevchenko kirjoitti:
> > > > Some drivers count XON/XOFF in the Tx statistics, some do not.
> > > > I actually a bit uncertain, but I _think_ the correct way is not
> > > > to count them, hence this series.
> > > 
> > > Okay, it seems there are much more drivers doing that. Perhaps we need
> > > to add that to the rest in this case (i.o.w. invert the series from removal
> > > to addition)?
> > 
> > Interesting, perhaps cut & paste?
> > 
> > XON and XOFF are overhead IMO. So should not be counted. When they are, they
> > mangle statistics as in transmitted (real) bytes per second.
> > 
> > How are they handled on the RX side?
> 
> It took me a while.
> 
> All serial drivers accept everything and those that care, update statics for
> anything they receive. This is because of layering. The Rx XON/XOFF seems
> (note I am completely unfamiliar with mysterious ways of TTY layers) to be
> handled on TTY level by n_tty_receive_char_flow_ctrl(), i.o.w. we may not
> skip counting it easily.
> 
> Now the question is, shall we count the control characters on output or not?
> Whatever decision we made, we should document (if not yet) and align drivers
> accordingly.
> 
> Another Q is what do books / other OS / projects usually do with them
> WRT statistics?

It depends on where you are measuring stuff.

If you want "raw" bytes on the wire, you count them.  If you want to see
what the port sends based on what userspace sent/recieved, you don't.

> If we count everything on a wire, then we must count them, otherwise
> it depends on how we treat them.

Agreed.  We need to pick one and stick with it.

I think, at the driver level, we should count it, as it's a ldisc thing,
not a driver thing, right?

thanks,

greg k-h