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]

Date:   Mon, 21 Nov 2022 08:55:49 -0500
From:   "Gabriel L. Somlo" <gsomlo@...il.com>
To:     Jiri Slaby <jirislaby@...nel.org>
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

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.

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

    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... :)
 
> 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.

Thanks again,
--Gabriel

> -- 
> js
> suse labs
>