2022-04-12 17:29:42

by Tony Lindgren

Subject: [PATCH 1/2] serial: 8250: Fix runtime PM for start_tx() for RS485

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 <[email protected]>
Cc: Steffen Trumtrar <[email protected]>
Cc: Uwe Kleine-König <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
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;
+ }

if (em485)
start_tx_rs485(port);
--
2.35.1

2022-04-12 21:50:12

by Tony Lindgren

Subject: [PATCH 2/2] serial: 8250: Fix runtime PM for start_tx() for empty buffer

Commit 932d596378b0 ("serial: 8250: Return early in .start_tx() if there
are no chars to send") caused a regression where the drivers implementing
runtime PM stopped idling.

We need to call serial8250_rpm_put_tx() on early exit, it normally gets
called later on at __stop_tx().

Fixes: 932d596378b0 ("serial: 8250: Return early in .start_tx() if there are no chars to send")
Cc: Steffen Trumtrar <[email protected]>
Cc: Uwe Kleine-König <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
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
@@ -1677,8 +1677,10 @@ static void serial8250_start_tx(struct uart_port *port)

serial8250_rpm_get_tx(up);

- if (!port->x_char && uart_circ_empty(&port->state->xmit))
+ if (!port->x_char && uart_circ_empty(&port->state->xmit)) {
+ serial8250_rpm_put_tx(up);
return;
+ }

if (em485 &&
em485->active_timer == &em485->start_tx_timer) {
--
2.35.1

2022-04-12 22:41:45

by Johan Hovold

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 <[email protected]>
> Cc: Steffen Trumtrar <[email protected]>
> Cc: Uwe Kleine-König <[email protected]>
> Signed-off-by: Tony Lindgren <[email protected]>
> ---
> 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