linux-kernel - Re: [PATCH v5 09/14] serial: liteuart: fix rx loop variable types

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <2f242291-99b6-a50f-cd52-e7dfd8c88c8f@kernel.org>
Date:   Tue, 22 Nov 2022 08:37:58 +0100
From:   Jiri Slaby <jirislaby@...nel.org>
To:     "Gabriel L. Somlo" <gsomlo@...il.com>
Cc:     linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
        gregkh@...uxfoundation.org, kgugala@...micro.com,
        mholenko@...micro.com, joel@....id.au,
        david.abdurachmanov@...il.com, florent@...oy-digital.fr,
        geert@...ux-m68k.org, ilpo.jarvinen@...ux.intel.com
Subject: Re: [PATCH v5 09/14] serial: liteuart: fix rx loop variable types

On 21. 11. 22, 14:55, Gabriel L. Somlo wrote:
> Hi Jiri,
> 
> Thanks for the feedback!
> 
> On Mon, Nov 21, 2022 at 09:45:05AM +0100, Jiri Slaby wrote:
>> On 21. 11. 22, 9:37, Jiri Slaby wrote:
>>> On 18. 11. 22, 15:55, Gabriel Somlo wrote:
>>>> Update variable types to match the signature of uart_insert_char()
>>>> which consumes them.
>>>>
>>>> Signed-off-by: Gabriel Somlo <gsomlo@...il.com>
>>>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
>>>> ---
>>>>    drivers/tty/serial/liteuart.c | 3 +--
>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/liteuart.c
>>>> b/drivers/tty/serial/liteuart.c
>>>> index 81aa7c1da73c..42ac9aee050a 100644
>>>> --- a/drivers/tty/serial/liteuart.c
>>>> +++ b/drivers/tty/serial/liteuart.c
>>>> @@ -73,8 +73,7 @@ static void liteuart_timer(struct timer_list *t)
>>>>        struct liteuart_port *uart = from_timer(uart, t, timer);
>>>>        struct uart_port *port = &uart->port;
>>>>        unsigned char __iomem *membase = port->membase;
>>>> -    int ch;
>>>> -    unsigned long status;
>>>> +    unsigned int status, ch;
>>>
>>> These should be u8 after all, right?
> 
> OK, so:
> 
>    - I can hard-code `status` as `1`, like so:
> 
> 	while(!litex_read8(membase + OFF_RXEMPTY) {
> 		...
> 		if (!(uart_handle_sysrq_char(port, ch)))
> 			uart_insert_char(port, 1, 0, ch, TTY_NORMAL);
> 
>      ... since `status` would always be `1` inside the loop. So I'm
>      basically going to get rid of it altogether.

Yes, I had that in my mind. Except passing 1 to uart_insert_char() when 
overflow is hardwired to 0 makes no sense IMO :).

>    - `ch` is indeed *produced* by `litex_read8()`, which would make it
>      `u8`. It is subsequently *consumed* by `uart_handle_sysrq_char()`
>      and `uart_insert_char()`, which both expect `unsigned int`.

Ignore uart_handle_sysrq_char and uart_insert_char. They should be fixed 
one day. It should really be u8. All down the call chain (it magically 
turns into int in the sysrq handlers, then char is expected).

>      If you think it's better to go with the type when the value is
>      produced (as opposed to when it's consumed), I'm OK with that for
>      the upcoming v6 of the series... :)

Yes, please. We should slowly convert _all_ of them.

>> And can you change membase to u8 * too 8-)?
> 
> Hmmm, in `struct uart_port` (in include/linux/serial_core.h), the
> `membase` field is declared as:
> 
> 	unsigned char __iomem   *membase;
> 
> which is why I'm thinking we should leave it as-is? Unless there are
> plans (or a pending patch I'm unaware of) to switch the field in
> include/linux/serial_core.h to `u8` as well? -- Please advise.

Ah, then keep it. I somehow thought it's void *. And yes, even this 
should be u8 __iomem *, eventually.

thanks,
-- 
js
suse labs