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

[Johan Hovold <johan@kernel.org>]

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