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@xxxxxxxxxxxxxxx/T/#t -- i.
