| Message ID | 20210930062906.58937-2-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Get rid of pm_runtime_irq_safe() for 8250_omap | expand |
On Thu, Sep 30, 2021 at 9:30 AM Tony Lindgren <tony@atomide.com> wrote: > > If the serial driver implements PM runtime with autosuspend, the port may > be powered down on TX. To wake up the port, let's add new wakeup() call > for serial drivers to implement as needed. We can call wakeup() from > __uart_start() and flow control related functions before attempting to > write to the serial port registers. > > Let's keep track of the serial port with a new runtime_suspended flag > that the device driver runtime PM suspend and resume can manage with > atomic_set(). This is because only the device driver knows what the > device runtime PM state as in Documentation/power/runtime_pm.rst > under "9. Autosuspend, or automatically-delayed suspend" for locking. > > To allow the serial port drivers to send out pending tx on runtime PM > resume, let's add start_pending_tx() as suggested by Johan Hovold > <johan@kernel.org>. ... > + wakeup(port) > + Wake up port if it has been runtime PM suspended. > + > + Locking: port->lock taken. > + > + Interrupts: locally disabled. > + This call must not sleep If it's suspended via ACPI methods, it can't be resumed here, right? Only what we can do is to schedule a resume, but it means we may not access registers immediately after and we have to be sure that the device is resumed. Dead end?
* Andy Shevchenko <andy.shevchenko@gmail.com> [210930 07:11]: > On Thu, Sep 30, 2021 at 9:30 AM Tony Lindgren <tony@atomide.com> wrote: > > > > If the serial driver implements PM runtime with autosuspend, the port may > > be powered down on TX. To wake up the port, let's add new wakeup() call > > for serial drivers to implement as needed. We can call wakeup() from > > __uart_start() and flow control related functions before attempting to > > write to the serial port registers. > > > > Let's keep track of the serial port with a new runtime_suspended flag > > that the device driver runtime PM suspend and resume can manage with > > atomic_set(). This is because only the device driver knows what the > > device runtime PM state as in Documentation/power/runtime_pm.rst > > under "9. Autosuspend, or automatically-delayed suspend" for locking. > > > > To allow the serial port drivers to send out pending tx on runtime PM > > resume, let's add start_pending_tx() as suggested by Johan Hovold > > <johan@kernel.org>. > > ... > > > + wakeup(port) > > + Wake up port if it has been runtime PM suspended. > > + > > + Locking: port->lock taken. > > + > > + Interrupts: locally disabled. > > > + This call must not sleep > > If it's suspended via ACPI methods, it can't be resumed here, right? It should work for that too assuming the runtime PM resume function is implemented. > Only what we can do is to schedule a resume, but it means we may not > access registers immediately after and we have to be sure that the > device is resumed. Yeah the only thing we do in the 8250_port wakeup() is schedule a resume if needed. Then the 8250 port device driver can call start_pending_tx() at the end of it's runtime PM resume function. > Dead end? I don't think so :) In serial_core we bail out on uart_port_wakeup() errors before register access. But maybe I missed some more moles to whack there :) Regards, Tony
On Thu, Sep 30, 2021 at 09:29:03AM +0300, Tony Lindgren wrote: > If the serial driver implements PM runtime with autosuspend, the port may > be powered down on TX. To wake up the port, let's add new wakeup() call > for serial drivers to implement as needed. We can call wakeup() from > __uart_start() and flow control related functions before attempting to > write to the serial port registers. > > Let's keep track of the serial port with a new runtime_suspended flag > that the device driver runtime PM suspend and resume can manage with > atomic_set(). This is because only the device driver knows what the > device runtime PM state as in Documentation/power/runtime_pm.rst > under "9. Autosuspend, or automatically-delayed suspend" for locking. > > To allow the serial port drivers to send out pending tx on runtime PM > resume, let's add start_pending_tx() as suggested by Johan Hovold > <johan@kernel.org>. > > Suggested-by: Johan Hovold <johan@kernel.org> > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > Documentation/driver-api/serial/driver.rst | 9 ++++ > drivers/tty/serial/serial_core.c | 56 +++++++++++++++++++++- > include/linux/serial_core.h | 3 ++ > 3 files changed, 66 insertions(+), 2 deletions(-) > > diff --git a/Documentation/driver-api/serial/driver.rst b/Documentation/driver-api/serial/driver.rst > --- a/Documentation/driver-api/serial/driver.rst > +++ b/Documentation/driver-api/serial/driver.rst > @@ -234,6 +234,15 @@ hardware. > > Interrupts: caller dependent. > > + wakeup(port) > + Wake up port if it has been runtime PM suspended. > + > + Locking: port->lock taken. > + > + Interrupts: locally disabled. > + > + This call must not sleep > + > flush_buffer(port) > Flush any write buffers, reset any DMA state and stop any > ongoing DMA transfers. > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -91,6 +91,23 @@ static inline struct uart_port *uart_port_check(struct uart_state *state) > return state->uart_port; > } > > +/* > + * This routine can be used before register access to wake up a serial > + * port that has been runtime PM suspended by the serial port driver. > + * Note that the runtime_suspended flag is managed by the serial port > + * device driver runtime PM. > + */ > +static int uart_port_wakeup(struct uart_port *port) > +{ > + if (!atomic_read(&port->runtime_suspended)) > + return 0; And if the value changes right after you read this? Why not use a real lock here? Don't use an atomic if you don't need it. thanks, greg k-h
* Greg Kroah-Hartman <gregkh@linuxfoundation.org> [211013 12:34]: > On Thu, Sep 30, 2021 at 09:29:03AM +0300, Tony Lindgren wrote: > > --- a/drivers/tty/serial/serial_core.c > > +++ b/drivers/tty/serial/serial_core.c > > @@ -91,6 +91,23 @@ static inline struct uart_port *uart_port_check(struct uart_state *state) > > return state->uart_port; > > } > > > > +/* > > + * This routine can be used before register access to wake up a serial > > + * port that has been runtime PM suspended by the serial port driver. > > + * Note that the runtime_suspended flag is managed by the serial port > > + * device driver runtime PM. > > + */ > > +static int uart_port_wakeup(struct uart_port *port) > > +{ > > + if (!atomic_read(&port->runtime_suspended)) > > + return 0; > > And if the value changes right after you read this? > > Why not use a real lock here? Don't use an atomic if you don't need it. Yeah good point, we should just use port->lock. Regards, Tony
diff --git a/Documentation/driver-api/serial/driver.rst b/Documentation/driver-api/serial/driver.rst --- a/Documentation/driver-api/serial/driver.rst +++ b/Documentation/driver-api/serial/driver.rst @@ -234,6 +234,15 @@ hardware. Interrupts: caller dependent. + wakeup(port) + Wake up port if it has been runtime PM suspended. + + Locking: port->lock taken. + + Interrupts: locally disabled. + + This call must not sleep + flush_buffer(port) Flush any write buffers, reset any DMA state and stop any ongoing DMA transfers. diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -91,6 +91,23 @@ static inline struct uart_port *uart_port_check(struct uart_state *state) return state->uart_port; } +/* + * This routine can be used before register access to wake up a serial + * port that has been runtime PM suspended by the serial port driver. + * Note that the runtime_suspended flag is managed by the serial port + * device driver runtime PM. + */ +static int uart_port_wakeup(struct uart_port *port) +{ + if (!atomic_read(&port->runtime_suspended)) + return 0; + + if (port->ops->wakeup) + return port->ops->wakeup(port); + + return 0; +} + /* * This routine is used by the interrupt handler to schedule processing in * the software interrupt portion of the driver. @@ -123,8 +140,13 @@ static void __uart_start(struct tty_struct *tty) struct uart_state *state = tty->driver_data; struct uart_port *port = state->uart_port; - if (port && !uart_tx_stopped(port)) - port->ops->start_tx(port); + if (!port || uart_tx_stopped(port)) + return; + + if (uart_port_wakeup(port) < 0) + return; + + port->ops->start_tx(port); } static void uart_start(struct tty_struct *tty) @@ -138,6 +160,21 @@ static void uart_start(struct tty_struct *tty) uart_port_unlock(port, flags); } +/* + * This routine can be called from the serial driver runtime PM resume function + * to transmit buffered data if the serial port was not active on uart_write(). + */ +void uart_start_pending_tx(struct uart_port *port) +{ + unsigned long flags; + + spin_lock_irqsave(&port->lock, flags); + if (!uart_tx_stopped(port) && uart_circ_chars_pending(&port->state->xmit)) + port->ops->start_tx(port); + spin_unlock_irqrestore(&port->lock, flags); +} +EXPORT_SYMBOL(uart_start_pending_tx); + static void uart_update_mctrl(struct uart_port *port, unsigned int set, unsigned int clear) { @@ -1067,6 +1104,11 @@ uart_tiocmset(struct tty_struct *tty, unsigned int set, unsigned int clear) if (!uport) goto out; + if (uart_port_wakeup(uport) < 0) { + ret = -EAGAIN; + goto out; + } + if (!tty_io_error(tty)) { uart_update_mctrl(uport, set, clear); ret = 0; @@ -1402,6 +1444,11 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg) goto out_up; } + if (uart_port_wakeup(uport) < 0) { + ret = -EAGAIN; + goto out_up; + } + /* * All these rely on hardware being present and need to be * protected against the tty being hung up. @@ -1724,7 +1771,12 @@ static void uart_dtr_rts(struct tty_port *port, int raise) uport = uart_port_ref(state); if (!uport) return; + + if (uart_port_wakeup(uport) < 0) + goto out; + uart_port_dtr_rts(uport, raise); +out: uart_port_deref(uport); } diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h --- a/include/linux/serial_core.h +++ b/include/linux/serial_core.h @@ -40,6 +40,7 @@ struct uart_ops { void (*set_mctrl)(struct uart_port *, unsigned int mctrl); unsigned int (*get_mctrl)(struct uart_port *); void (*stop_tx)(struct uart_port *); + int (*wakeup)(struct uart_port *); void (*start_tx)(struct uart_port *); void (*throttle)(struct uart_port *); void (*unthrottle)(struct uart_port *); @@ -250,6 +251,7 @@ struct uart_port { unsigned char suspended; unsigned char console_reinit; const char *name; /* port name */ + atomic_t runtime_suspended; /* port runtime state set by port driver */ struct attribute_group *attr_group; /* port specific attributes */ const struct attribute_group **tty_groups; /* all attributes (serial core use only) */ struct serial_rs485 rs485; @@ -414,6 +416,7 @@ bool uart_match_port(const struct uart_port *port1, /* * Power Management */ +void uart_start_pending_tx(struct uart_port *port); int uart_suspend_port(struct uart_driver *reg, struct uart_port *port); int uart_resume_port(struct uart_driver *reg, struct uart_port *port);
If the serial driver implements PM runtime with autosuspend, the port may be powered down on TX. To wake up the port, let's add new wakeup() call for serial drivers to implement as needed. We can call wakeup() from __uart_start() and flow control related functions before attempting to write to the serial port registers. Let's keep track of the serial port with a new runtime_suspended flag that the device driver runtime PM suspend and resume can manage with atomic_set(). This is because only the device driver knows what the device runtime PM state as in Documentation/power/runtime_pm.rst under "9. Autosuspend, or automatically-delayed suspend" for locking. To allow the serial port drivers to send out pending tx on runtime PM resume, let's add start_pending_tx() as suggested by Johan Hovold <johan@kernel.org>. Suggested-by: Johan Hovold <johan@kernel.org> Signed-off-by: Tony Lindgren <tony@atomide.com> --- Documentation/driver-api/serial/driver.rst | 9 ++++ drivers/tty/serial/serial_core.c | 56 +++++++++++++++++++++- include/linux/serial_core.h | 3 ++ 3 files changed, 66 insertions(+), 2 deletions(-)