[PATCH v2 0/2] serial: dz: UART_XMIT_SIZE/WAKEUP_CHARS cleanups

[PATCH v2 0/2] serial: dz: UART_XMIT_SIZE/WAKEUP_CHARS cleanups

[Ilpo Järvinen]

v2: 

- Make separate patch out of xmit mask change (which, in theory, is
a fix but due to lack of issue reports, likely doesn't occur for real).
- Replace also DZ_WAKEUP_CHARS with the normal WAKEUP_CHARS (matching
to n_tty_poll)

Ilpo Järvinen (2):
  serial: dz: xmit buffer is UART_XMIT_SIZE'd
  serial: dz: Remove custom DZ_WAKEUP_CHARS

 drivers/tty/serial/dz.c | 4 ++--
 drivers/tty/serial/dz.h | 3 ---
 2 files changed, 2 insertions(+), 5 deletions(-)

-- 
2.30.2

[PATCH v2 1/2] serial: dz: xmit buffer is UART_XMIT_SIZE'd

[Ilpo Järvinen]

Instead of DZ_XMIT_SIZE, use the normal UART_XMIT_SIZE directly as it's
the correct size of the xmit circular buffer.

In theory, the Tx code would be buggy if UART_XMIT_SIZE differs from
4096 (occurs when PAGE_SIZE > 4k), however, given the lack of issue
reports such configuration likely doesn't occur with any real platform
with dz HW. The inconsisted sizes would cause missing characters and
never-ending bogus Tx when ->head reaches the region above 4k. The
issue, if it would be real, would predate git days.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/dz.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/dz.c b/drivers/tty/serial/dz.c
index 2e21acf39720..5d2588f3e6a9 100644
--- a/drivers/tty/serial/dz.c
+++ b/drivers/tty/serial/dz.c
@@ -279,7 +279,7 @@ static inline void dz_transmit_chars(struct dz_mux *mux)
 	 * so we go one char at a time) :-<
 	 */
 	tmp = xmit->buf[xmit->tail];
-	xmit->tail = (xmit->tail + 1) & (DZ_XMIT_SIZE - 1);
+	xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
 	dz_out(dport, DZ_TDR, tmp);
 	dport->port.icount.tx++;
 
-- 
2.30.2

Re: [PATCH v2 1/2] serial: dz: xmit buffer is UART_XMIT_SIZE'd

[Maciej W. Rozycki]

On Thu, 25 Aug 2022, Ilpo Järvinen wrote:

> In theory, the Tx code would be buggy if UART_XMIT_SIZE differs from
> 4096 (occurs when PAGE_SIZE > 4k), however, given the lack of issue
> reports such configuration likely doesn't occur with any real platform
> with dz HW. The inconsisted sizes would cause missing characters and
> never-ending bogus Tx when ->head reaches the region above 4k. The
> issue, if it would be real, would predate git days.

 This is misleading.  There are exactly 3 machine models (2 major ones and 
1 extra submodel) that we currently support which make use of this serial 
port hardware and driver, and they all have their R2000/R3000 MIPS CPU 
soldered onto their respective mainboards.  And the CPUs they use all have 
their page size hardwired to 4KiB, so it's not the lack of reports, but a 
firm assertion that this driver as it stands shall never be used with a 
different page size.

 There exists an option card using a DZ11-compatible chipset that can be 
used with systems we currently support with page sizes of up to 64KiB, but 
to the best of my knowledge only a number of prototype cards has been made 
and I have heard of exactly one person having such a card.  Therefore we 
do not support it and may never do, so it is not a concern for the driver 
as it stands and shall not be mentioned.

 Please just state then that the change is for design consistency with the 
serial core and redefine DZ_XMIT_SIZE in terms of UART_XMIT_SIZE as I 
suggested for v1.  I'll ack such a change.  Please drop 2/2 at this stage 
as it does not fix any bug and does not appear to add any value to this 
driver.

 Thanks,

  Maciej

Re: [PATCH v2 1/2] serial: dz: xmit buffer is UART_XMIT_SIZE'd

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3039 bytes --]

On Fri, 26 Aug 2022, Maciej W. Rozycki wrote:

> On Thu, 25 Aug 2022, Ilpo Järvinen wrote:
> 
> > In theory, the Tx code would be buggy if UART_XMIT_SIZE differs from
> > 4096 (occurs when PAGE_SIZE > 4k), however, given the lack of issue
> > reports such configuration likely doesn't occur with any real platform
> > with dz HW. The inconsisted sizes would cause missing characters and
> > never-ending bogus Tx when ->head reaches the region above 4k. The
> > issue, if it would be real, would predate git days.
> 
>  This is misleading.  There are exactly 3 machine models (2 major ones and 
> 1 extra submodel) that we currently support which make use of this serial 
> port hardware and driver, and they all have their R2000/R3000 MIPS CPU 
> soldered onto their respective mainboards.  And the CPUs they use all have 
> their page size hardwired to 4KiB, so it's not the lack of reports, but a 
> firm assertion that this driver as it stands shall never be used with a 
> different page size.

Ah, sorry. I misread your original statement to contain a question to me 
rather than just you stating a fact.

>  There exists an option card using a DZ11-compatible chipset that can be 
> used with systems we currently support with page sizes of up to 64KiB, but 
> to the best of my knowledge only a number of prototype cards has been made 
> and I have heard of exactly one person having such a card.  Therefore we 
> do not support it and may never do, so it is not a concern for the driver 
> as it stands and shall not be mentioned.
> 
>  Please just state then that the change is for design consistency with the 
> serial core and redefine DZ_XMIT_SIZE in terms of UART_XMIT_SIZE as I 
> suggested for v1.

You had a small error in your suggestion for v1 though (which confused me
somewhat as there obviously was an error in it and I guessed wrong what 
you meant):

>> Also I'd rather:
>>
>>#define DZ_WAKEUP_CHARS      UART_XMIT_SIZE

...I guess with that you actually meant doing simply (and nothing else):

#define DW_XMIT_SIZE UART_XMIT_SIZE

?

But whatever. That line 1/2 is touching is anyway going to die pretty soon
if the 2nd part (yet to be submitted) of the uart_xmit_advance() series 
(1st part here [1]) gets applied so I don't care too much what the 
xmit->tail line will be in between.

I just thought it would have been nice to also get rid of what clearly 
appears to be just a duplicated define of something core already has.

> I'll ack such a change.  Please drop 2/2 at this stage 
> as it does not fix any bug and does not appear to add any value to this 
> driver.

Ok.

I personally don't see the connection between *WAKEUP_CHARS and circular 
buffer size would be strong enough to warrant defining former using the 
latter. ...If it would be there, the other drivers would have a similar 
construct. But I can leave it as is, no significant harm done.

Thanks a lot for your feedback and insight!

[1] https://lore.kernel.org/linux-serial/20220825091707.8112-1-ilpo.jarvinen@linux.intel.com/T/#t

-- 
 i.

[PATCH v2 2/2] serial: dz: Remove custom DZ_WAKEUP_CHARS

[Ilpo Järvinen]

Almost all serial drivers use WAKEUP_CHARS (256) from serial_core.h which
also matches what n_tty_poll() uses for asserting EPOLLOUT.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/dz.c | 2 +-
 drivers/tty/serial/dz.h | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/tty/serial/dz.c b/drivers/tty/serial/dz.c
index 5d2588f3e6a9..5df46f22abaa 100644
--- a/drivers/tty/serial/dz.c
+++ b/drivers/tty/serial/dz.c
@@ -283,7 +283,7 @@ static inline void dz_transmit_chars(struct dz_mux *mux)
 	dz_out(dport, DZ_TDR, tmp);
 	dport->port.icount.tx++;
 
-	if (uart_circ_chars_pending(xmit) < DZ_WAKEUP_CHARS)
+	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(&dport->port);
 
 	/* Are we are done. */
diff --git a/drivers/tty/serial/dz.h b/drivers/tty/serial/dz.h
index 3b3e31954f24..4b502bfad560 100644
--- a/drivers/tty/serial/dz.h
+++ b/drivers/tty/serial/dz.h
@@ -124,7 +124,4 @@
 
 #define DZ_NB_PORT 4
 
-#define DZ_XMIT_SIZE   4096                 /* buffer size */
-#define DZ_WAKEUP_CHARS   DZ_XMIT_SIZE/4
-
 #endif /* DZ_SERIAL_H */
-- 
2.30.2