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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- 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.