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

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

[Tony Lindgren]

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;
+	}
 
 	if (em485)
 		start_tx_rs485(port);
-- 
2.35.1

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

[Tony Lindgren]

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

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

[Johan Hovold]

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

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1566 bytes --]

On Mon, Apr 11, 2022 at 12:48:05PM +0300, Tony Lindgren wrote:
> 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 <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
> @@ -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;
> +	}

Assuming you don't need serial8250_rpm_get_tx() to check the condition,
it would be easier to move the early return before the call to
serial8250_rpm_get_tx().

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [220411 09:54]:
> On Mon, Apr 11, 2022 at 12:48:04PM +0300, Tony Lindgren wrote:
> > 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.

Frankly "a bit of mess" applies "a bit more" than just the serial runtime
PM :)

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

AFAIK there's currently nothing paired with the serial8250_rpm_get_tx(up)
call at the beginning of serial8250_start_tx() for the early exit cases
if start_tx_rs485() or __start_tx() won't get called.

Care to clarify a bit more what you have in mind?

Regards,

Tony

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

[Tony Lindgren]

* Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [220411 09:59]:
> On Mon, Apr 11, 2022 at 12:48:05PM +0300, Tony Lindgren wrote:
> > 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 <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
> > @@ -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;
> > +	}
> 
> Assuming you don't need serial8250_rpm_get_tx() to check the condition,
> it would be easier to move the early return before the call to
> serial8250_rpm_get_tx().

Yeah good suggestion, that should work.

Regards,

Tony

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

[Johan Hovold]

On Mon, Apr 11, 2022 at 12:48:05PM +0300, Tony Lindgren wrote:
> 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 <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
> @@ -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;
> +	}

Move this before the runtime pm get instead?

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

Johan

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

[Johan Hovold]

On Mon, Apr 11, 2022 at 01:10:34PM +0300, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [220411 09:54]:
> > On Mon, Apr 11, 2022 at 12:48:04PM +0300, Tony Lindgren wrote:
> > > 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.
> 
> Frankly "a bit of mess" applies "a bit more" than just the serial runtime
> PM :)

Heh. I'm afraid that's all too true. :)

> > 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).
> 
> AFAIK there's currently nothing paired with the serial8250_rpm_get_tx(up)
> call at the beginning of serial8250_start_tx() for the early exit cases
> if start_tx_rs485() or __start_tx() won't get called.
> 
> Care to clarify a bit more what you have in mind?

The problem is that that serial8250_rpm_put_tx() you're adding may
suspend the device unconditionally (i.e. regardless of any previous
calls to serial8250_rpm_get_tx()).

If rs485 tx is just being deferred you mustn't suspend the device before
it has had a chance to start transmitting.

Johan

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

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [220411 10:23]:
> On Mon, Apr 11, 2022 at 01:10:34PM +0300, Tony Lindgren wrote:
> > * Johan Hovold <johan@kernel.org> [220411 09:54]:
> > > On Mon, Apr 11, 2022 at 12:48:04PM +0300, Tony Lindgren wrote:
> > > > 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.
> > 
> > Frankly "a bit of mess" applies "a bit more" than just the serial runtime
> > PM :)
> 
> Heh. I'm afraid that's all too true. :)
> 
> > > 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).
> > 
> > AFAIK there's currently nothing paired with the serial8250_rpm_get_tx(up)
> > call at the beginning of serial8250_start_tx() for the early exit cases
> > if start_tx_rs485() or __start_tx() won't get called.
> > 
> > Care to clarify a bit more what you have in mind?
> 
> The problem is that that serial8250_rpm_put_tx() you're adding may
> suspend the device unconditionally (i.e. regardless of any previous
> calls to serial8250_rpm_get_tx()).
> 
> If rs485 tx is just being deferred you mustn't suspend the device before
> it has had a chance to start transmitting.

Hmm I'm pretty sure rs485 has the runtime PM usage count is currently
unbalanced. To me it seems em485->start_tx_timer calls start_tx()
again from serial8250_em485_handle_start_tx().

Anyways, let's deal with the regression patch first, this can wait a bit.

Regards,

Tony

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

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [220411 10:10]:
> On Mon, Apr 11, 2022 at 12:48:05PM +0300, Tony Lindgren wrote:
> > 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;
> > +	}
> 
> Move this before the runtime pm get instead?

Yup good idea.

Tony

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

[Johan Hovold]

On Mon, Apr 11, 2022 at 01:32:58PM +0300, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [220411 10:23]:
> > On Mon, Apr 11, 2022 at 01:10:34PM +0300, Tony Lindgren wrote:
> > > * Johan Hovold <johan@kernel.org> [220411 09:54]:
> > > > On Mon, Apr 11, 2022 at 12:48:04PM +0300, Tony Lindgren wrote:
> > > > > 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;
> > > > > +	}

> > The problem is that that serial8250_rpm_put_tx() you're adding may
> > suspend the device unconditionally (i.e. regardless of any previous
> > calls to serial8250_rpm_get_tx()).
> > 
> > If rs485 tx is just being deferred you mustn't suspend the device before
> > it has had a chance to start transmitting.
> 
> Hmm I'm pretty sure rs485 has the runtime PM usage count is currently
> unbalanced. To me it seems em485->start_tx_timer calls start_tx()
> again from serial8250_em485_handle_start_tx().

It appears to call __start_tx(), but note that the only call to
serial8250_rpm_get_tx() is in serial8250_start_tx() which this patch
would have cancelled out.
 
Also note that the serial8250_rpm_get/set_tx() calls aren't supposed to
be balanced. get() can be called multiple times and will only increment
the PM usage counter once, while put() will decrement the counter
whenever get() has been called once (and hence potentially suspend the
device immediately).

Messy indeed.

Johan

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

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [220411 11:53]:
> On Mon, Apr 11, 2022 at 01:32:58PM +0300, Tony Lindgren wrote:
> > * Johan Hovold <johan@kernel.org> [220411 10:23]:
> > > On Mon, Apr 11, 2022 at 01:10:34PM +0300, Tony Lindgren wrote:
> > > > * Johan Hovold <johan@kernel.org> [220411 09:54]:
> > > > > On Mon, Apr 11, 2022 at 12:48:04PM +0300, Tony Lindgren wrote:
> > > > > > 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;
> > > > > > +	}
> 
> > > The problem is that that serial8250_rpm_put_tx() you're adding may
> > > suspend the device unconditionally (i.e. regardless of any previous
> > > calls to serial8250_rpm_get_tx()).
> > > 
> > > If rs485 tx is just being deferred you mustn't suspend the device before
> > > it has had a chance to start transmitting.
> > 
> > Hmm I'm pretty sure rs485 has the runtime PM usage count is currently
> > unbalanced. To me it seems em485->start_tx_timer calls start_tx()
> > again from serial8250_em485_handle_start_tx().
> 
> It appears to call __start_tx(), but note that the only call to
> serial8250_rpm_get_tx() is in serial8250_start_tx() which this patch
> would have cancelled out.

OK

> Also note that the serial8250_rpm_get/set_tx() calls aren't supposed to
> be balanced. get() can be called multiple times and will only increment
> the PM usage counter once, while put() will decrement the counter
> whenever get() has been called once (and hence potentially suspend the
> device immediately).
> 
> Messy indeed.

Yeah that is not nice.

I'll send a patch to prepare things for runtime PM that will hopefully
make things a bit easier as discussed earlier.

Regards,

Tony