linux-kernel - Re: [PATCH v2 1/2] serial: dz: xmit buffer is UART_XMIT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 26 Aug 2022 17:16:50 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     "Maciej W. Rozycki" <macro@...am.me.uk>
cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        linux-serial <linux-serial@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] serial: dz: xmit buffer is UART_XMIT_SIZE'd

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.