linux-kernel - Re: [PATCH v3 4/6] serial: 8250: Adjust misleading LSR related comment

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:   Tue, 7 Jun 2022 12:22:49 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc:     "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 4/6] serial: 8250: Adjust misleading LSR related comment

On Tue, Jun 7, 2022 at 11:16 AM Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com> wrote:
>
> serial8250_rx_chars() has max_count based character limit. If it
> triggers, the function returns old LSR value (and it has never returned

the old

> only flags which were not handled). Adjust the comment to match
> behavior and warn about which flags can be depended on.
>
> While I'd have moved LSR read before LSR read and used serial_lsr_in()
> also here but I came across this old discussion about the topic:

>   https://www.spinics.net/lists/linux-serial/msg20555.html

Can it be transformed to lore.kernel.org link? and maybe even moved as
BugLink tag?

> ...so I left it as it is (it works as long as the callers only use
> a subset of the LSR flags which holds true today).

With comments addressed,
Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>

> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index a0ea048eb2ad..686891f1b2ca 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1782,9 +1782,12 @@ void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
>  EXPORT_SYMBOL_GPL(serial8250_read_char);
>
>  /*
> - * serial8250_rx_chars: processes according to the passed in LSR
> - * value, and returns the remaining LSR bits not handled
> - * by this Rx routine.
> + * serial8250_rx_chars: Read characters. The first LSR value must be passed
> + * in.

While at it, I would do the following:
1) keep on one line;
2) replace : with -.

The idea behind is to easily convert to kernel doc in the future if
needed, or at least be consistent with kernel doc format.

> + *
> + * Returns LSR bits. The caller should rely only non-rx related LSR bits

rely only on

non-Rx

> + * (such as THRE) because the LSR value might come from an already consumed
> + * character.
>   */


-- 
With Best Regards,
Andy Shevchenko