Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Mon, 11 Apr 2022 11:57:12 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Tony Lindgren <tony@atomide.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Andy Shevchenko <andriy.shevchenko@intel.com>,
        Jiri Slaby <jirislaby@kernel.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        linux-serial@vger.kernel.org, linux-omap@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        "Matwey V . Kornilov" <matwey@sai.msu.ru>,
        Steffen Trumtrar <s.trumtrar@pengutronix.de>,
        Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= 
        <u.kleine-koenig@pengutronix.de>
Subject: Re: [PATCH 1/2] serial: 8250: Fix runtime PM for start_tx() for RS485
Message-ID: <YlP7eArvvNWnbMF2@hovoldconsulting.com>
References: <20220411094805.45696-1-tony@atomide.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220411094805.45696-1-tony@atomide.com>
Precedence: bulk

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@sai.msu.ru>
> Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Tony Lindgren <tony@atomide.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