Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 22 Nov 2022 16:05:05 -0500
From:   "Gabriel L. Somlo" <gsomlo@gmail.com>
To:     Jiri Slaby <jirislaby@kernel.org>
Cc:     linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        gregkh@linuxfoundation.org, kgugala@antmicro.com,
        mholenko@antmicro.com, joel@jms.id.au,
        david.abdurachmanov@gmail.com, florent@enjoy-digital.fr,
        geert@linux-m68k.org, ilpo.jarvinen@linux.intel.com
Subject: Re: [PATCH v5 09/14] serial: liteuart: fix rx loop variable types
Message-ID: <Y305gZnFSaUBtCRa@errol.ini.cmu.edu>
References: <20221118145512.509950-1-gsomlo@gmail.com>
 <20221118145512.509950-10-gsomlo@gmail.com>
 <44bf21b6-cbe4-4d73-0883-a9bcbd7d5971@kernel.org>
 <1b5a963c-2a5b-54fe-784e-fcc4d06c347e@kernel.org>
 <Y3uDZV8b+3GfQyUY@errol.ini.cmu.edu>
 <2f242291-99b6-a50f-cd52-e7dfd8c88c8f@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2f242291-99b6-a50f-cd52-e7dfd8c88c8f@kernel.org>
Precedence: bulk

On Tue, Nov 22, 2022 at 08:37:58AM +0100, Jiri Slaby wrote:
> 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@gmail.com>
> > > > > Reviewed-by: Ilpo J?rvinen <ilpo.jarvinen@linux.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 :).

So, looking at what uart_insert_char() does, I could simply do this
instead:

 	while(!litex_read8(membase + OFF_RXEMPTY) {
 		...
		/* LiteUART does not provide overrun bits */
 		if (!(uart_handle_sysrq_char(port, ch) ||
 		     tty_insert_flip_char(&port->state->port, ch, TTY_NORMAL)))
 			++port->icount.buf_overrun;
 
That is, `tty_insert_flip_char() is the portion of `uart_insert_char()`
that actually gets executed if status is 1 and overrun is 0...

I'm not quite confident about whether this is an improvement in legibility
and/or code quality, but please let me know what *you* think... :)

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

OK, u8 it is, then.
 
> > > 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.

OK, it should/will get updated in bulk once that change is made across
the entire subsystem, leaving as-is for now.

Thanks again for all your help and advice!
--Gabriel