diff mbox series

[v5,09/14] serial: liteuart: fix rx loop variable types

Message ID	20221118145512.509950-10-gsomlo@gmail.com
State	New
Headers	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; From: Gabriel Somlo <gsomlo@gmail.com> To: linux-kernel@vger.kernel.org Cc: linux-serial@vger.kernel.org, gregkh@linuxfoundation.org, jirislaby@kernel.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: [PATCH v5 09/14] serial: liteuart: fix rx loop variable types Date: Fri, 18 Nov 2022 09:55:07 -0500 Message-Id: <20221118145512.509950-10-gsomlo@gmail.com> In-Reply-To: <20221118145512.509950-1-gsomlo@gmail.com> References: <20221118145512.509950-1-gsomlo@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	serial: liteuart: add IRQ support \| [v5,00/14] serial: liteuart: add IRQ support [v5,01/14] serial: liteuart: use KBUILD_MODNAME as driver name [v5,02/14] serial: liteuart: use bit number macros [v5,03/14] serial: liteuart: remove unused uart_ops stubs [v5,04/14] serial: liteuart: don't set unused port fields [v5,05/14] serial: liteuart: minor style fix in liteuart_init() [v5,06/14] serial: liteuart: move tty_flip_buffer_push() out of rx loop [v5,07/14] serial: liteuart: rx loop should only ack rx events [v5,08/14] serial: liteuart: simplify passing of uart_insert_char() flag [v5,09/14] serial: liteuart: fix rx loop variable types [v5,10/14] serial: liteuart: separate rx loop from poll timer [v5,11/14] serial: liteuart: move function definitions [v5,12/14] serial: liteuart: add IRQ support for the RX path [v5,13/14] serial: liteuart: add IRQ support for the TX path [v5,14/14] serial: liteuart: move polling putchar() function

Commit Message

Gabriel L. Somlo Nov. 18, 2022, 2:55 p.m. UTC

  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(-)

Comments

Jiri Slaby Nov. 21, 2022, 8:37 a.m. UTC | #1

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?

Wait, status is a bool in the end:

>   	while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {

But why is it passed to uart_insert_char() as such? That's ugly. Maybe 
drop all of this "status" and pass LSR_RXC directly. The while's 
condition would simply look like (!litex_read8(membase + OFF_RXEMPTY)) then.

>   		ch = litex_read8(membase + OFF_RXTX);

thanks,

Jiri Slaby Nov. 21, 2022, 8:42 a.m. UTC | #2

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?
> 
> Wait, status is a bool in the end:
> 
>>       while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
> 
> But why is it passed to uart_insert_char() as such? That's ugly. Maybe 
> drop all of this "status" and pass LSR_RXC directly.

Actually, even 0 directly, provided overrun parameter is always 0.

> The while's 
> condition would simply look like (!litex_read8(membase + OFF_RXEMPTY)) 
> then.
> 
>>           ch = litex_read8(membase + OFF_RXTX);
> 
> thanks,

Jiri Slaby Nov. 21, 2022, 8:45 a.m. UTC | #3

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?

And can you change membase to u8 * too 8-)?

Gabriel L. Somlo Nov. 21, 2022, 1:55 p.m. UTC | #4

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.

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

Jiri Slaby Nov. 22, 2022, 7:37 a.m. UTC | #5

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

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

Gabriel L. Somlo Nov. 22, 2022, 9:05 p.m. UTC | #6

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

Jiri Slaby Nov. 23, 2022, 5:39 a.m. UTC | #7

On 22. 11. 22, 22:05, Gabriel L. Somlo wrote:
> 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,

It's not :). Keep the uart_ helper.

diff mbox series

Patch

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;
 
 	while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
 		ch = litex_read8(membase + OFF_RXTX);