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
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 <[email protected]>
---
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
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
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/[email protected]/T/#t
--
i.