| Message ID | 20220411094805.45696-1-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] serial: 8250: Fix runtime PM for start_tx() for RS485 | expand |
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
* 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
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
* 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
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
* 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
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);
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(-)