| Message ID | 20211115084203.56478-1-tony@atomide.com |
|---|---|
| Headers | show |
| Series | Serial port generic PM to fix 8250 PM | expand |
On Mon, Nov 15, 2021 at 10:43 AM Tony Lindgren <tony@atomide.com> wrote: > > Hi, > > Here are v4 patches for serial port generic PM. The scope has now expanded > a bit from the earlier attempts to get rid of pm_runtime_irq_safe() for > the 8250_omap driver. I've now picked up three patches from Andy's earlier > generic serial port PM series. Johan, do you have any objections / comments on the series? Otherwise I think it's good to go next week next revision (as kbuild bot complained about minor warning).
On Mon, Nov 15, 2021 at 10:41:56AM +0200, Tony Lindgren wrote: > Hi, > > Here are v4 patches for serial port generic PM. The scope has now expanded > a bit from the earlier attempts to get rid of pm_runtime_irq_safe() for > the 8250_omap driver. I've now picked up three patches from Andy's earlier > generic serial port PM series. So this looks like another step in the right direction but there are still some missing pieces. First, you need to provide an overview of the design decisions made here in cover letter. It's currently spread out over several patches and those commit messages still do not hold all the details. Specifically, it looks like tx can still stall indefinitely if the autosuspend timer fires. This can happen at low baud rates and also when using flow control. It also looks like the expected calls to update the last busy timestamp might be missing from the interrupt handlers or related helpers. Please also describe how this interacts with the console. Is a console port now never suspended? Where is that enforced? The final patch appears to rely on this when it drops PM calls from for example some console poll callbacks. > Changes since v3: > - Pick three patches from Andy's earlier serial port PM series to handle > issues pointed out by Johan Please also be more specific here when sending an updated series. I can't really tell what has changed from just this one sentence. Johan
On Mon, Nov 15, 2021 at 10:41:59AM +0200, Tony Lindgren wrote: > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > We can't and basically don't need to call runtime PM in IRQ handler. If > IRQ is ours, device must be powered on. Otherwise check if the device is > powered off and return immediately. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > [tony@atomide.com: use port->runtime_suspended] > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/tty/serial/8250/8250_port.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > 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 > @@ -1939,17 +1939,19 @@ EXPORT_SYMBOL_GPL(serial8250_handle_irq); > > static int serial8250_default_handle_irq(struct uart_port *port) > { > - struct uart_8250_port *up = up_to_u8250p(port); > unsigned int iir; > - int ret; > > - serial8250_rpm_get(up); > + /* > + * The IRQ might be shared with other peripherals so we must first > + * check that are we RPM suspended or not. If we are we assume that > + * the IRQ was not for us (we shouldn't be RPM suspended when the > + * interrupt is enabled). > + */ > + if (port->runtime_suspended) > + return 0; This function is called without holding the port lock that protects this flag. Also what prevents the device from being suspended after checking it? Note that this handler is called from a timer callback when polling and that case needs to be considered too. > > iir = serial_port_in(port, UART_IIR); > - ret = serial8250_handle_irq(port, iir); > - > - serial8250_rpm_put(up); > - return ret; > + return serial8250_handle_irq(port, iir); > } > > /* Johan
* Johan Hovold <johan@kernel.org> [211130 10:03]: > Specifically, it looks like tx can still stall indefinitely if the > autosuspend timer fires. This can happen at low baud rates and also when > using flow control. Yeah the TX part is still problematic. Note that this is purely because of current Linux serial layers implementation, and not because of any hardware reasons. Even after this series we still rely on serial8250_rpm_get_tx() and serial8250_rpm_put_tx() to decipher if we can idle the port.. If anybody has good ideas where we can add the serial core TX related paired runtime PM calls please let me know :) For TX DMA, we should not do runtime PM put until at the DMA callback function when completed. Regards, Tony
Hi, Here are v4 patches for serial port generic PM. The scope has now expanded a bit from the earlier attempts to get rid of pm_runtime_irq_safe() for the 8250_omap driver. I've now picked up three patches from Andy's earlier generic serial port PM series. Regards, Tony Changes since v3: - Pick three patches from Andy's earlier serial port PM series to handle issues pointed out by Johan Chganges since v2: - Use locking instead of atomic_t as suggested by Greg Changes since v1: - Separated out line discipline patches, n_tty -EAGAIN change I still need to retest - Changed prep_tx() to more generic wakeup() as also flow control needs it - Changed over to using wakeup() with device driver runtime PM instead of write_room() - Added runtime_suspended flag for drivers and generic serial layer PM to use Andy Shevchenko (3): serial: core: Add support of runtime PM serial: 8250_port: properly handle runtime PM in IRQ serial: 8250_port: Remove calls to runtime PM Tony Lindgren (4): serial: core: Add wakeup() and start_pending_tx() for asynchronous wake serial: 8250: Implement wakeup for TX and use it for 8250_omap serial: 8250_omap: Require a valid wakeirq for deeper idle states serial: 8250_omap: Drop the use of pm_runtime_irq_safe() Documentation/driver-api/serial/driver.rst | 9 + drivers/tty/serial/8250/8250.h | 3 - drivers/tty/serial/8250/8250_omap.c | 44 +++-- drivers/tty/serial/8250/8250_port.c | 151 ++++++++-------- drivers/tty/serial/serial_core.c | 199 +++++++++++++++++++-- include/linux/serial_core.h | 3 + 6 files changed, 305 insertions(+), 104 deletions(-)