linux-kernel - Re: [PATCH 1/2] serial: 8250: Fix runtime PM for start

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, 11 Apr 2022 11:57:12 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Tony Lindgren <tony@...mide.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Jiri Slaby <jirislaby@...nel.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Vignesh Raghavendra <vigneshr@...com>,
        linux-serial@...r.kernel.org, linux-omap@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        "Matwey V . Kornilov" <matwey@....msu.ru>,
        Steffen Trumtrar <s.trumtrar@...gutronix.de>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
Subject: Re: [PATCH 1/2] serial: 8250: Fix runtime PM for start_tx() for RS485

On Mon, Apr 11, 2022 at 12:48:04PM +0300, Tony Lindgren wrote:
> The early return from serial8250_start_tx() added by commit e490c9144cfa
> ("tty: Add software emulated RS485 support for 8250") failed to call
> serial8250_rpm_put_tx() that normally gets called on __stop_tx().
> 
> Likely this is a harmless issue as the RS485 using folks probably are not
> using runtime PM for the serial ports.
> 
> Fixes: e490c9144cfa ("tty: Add software emulated RS485 support for 8250")
> Cc: Matwey V. Kornilov <matwey@....msu.ru>
> Cc: Steffen Trumtrar <s.trumtrar@...gutronix.de>
> Cc: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
> Signed-off-by: Tony Lindgren <tony@...mide.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1681,8 +1681,10 @@ static void serial8250_start_tx(struct uart_port *port)
>  		return;
>  
>  	if (em485 &&
> -	    em485->active_timer == &em485->start_tx_timer)
> +	    em485->active_timer == &em485->start_tx_timer) {
> +		serial8250_rpm_put_tx(up);
>  		return;
> +	}

I was just taking a quick look at your report about this and also
noticed this return statement.

The runtime PM implementation is a bit of mess as we've discussed
elsewhere, but the change you propose here doesn't look right.

start_tx() can be deferred in the rs485 case, but that doesn't mean you
should suspend the device here. In fact, that look like it would just
break runtime PM (the parts that may work to some extent).

>  
>  	if (em485)
>  		start_tx_rs485(port);

Johan