On Mon, Nov 15, 2021 at 10:41:58AM +0200, 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() before attempting to write to the serial port registers.
This needs more detail on the approach taken to handle tx and the issues
involved (e.g. stalled ports etc).
> 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
> port->lock held. 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@xxxxxxxxxx>.
>
> Suggested-by: Johan Hovold <johan@xxxxxxxxxx>
> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> ---
> Documentation/driver-api/serial/driver.rst | 9 ++++
> drivers/tty/serial/serial_core.c | 49 +++++++++++++++++++++-
> include/linux/serial_core.h | 3 ++
> 3 files changed, 59 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
> @@ -107,12 +107,35 @@ static int serial_pm_resume_and_get(struct device *dev)
> return pm_runtime_resume_and_get(dev);
> }
>
> +/* Only increments the runtime PM usage count */
> +static void serial_pm_get_noresume(struct device *dev)
> +{
> + pm_runtime_get_noresume(dev);
> +}
> +
> static void serial_pm_autosuspend(struct device *dev)
> {
> pm_runtime_mark_last_busy(dev);
> pm_runtime_put_autosuspend(dev);
> }
>
> +/*
> + * 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 (!port->runtime_suspended)
> + return 0;
> +
> + if (port->ops->wakeup)
> + return port->ops->wakeup(port);
Why do you need a subdriver callback for this? Why no schedule a resume
in core rather than spreading the logic out over core and subdrivers?
> +
> + return 0;
> +}
> +
> /*
> * This routine is used by the interrupt handler to schedule processing in
> * the software interrupt portion of the driver.
> @@ -145,8 +168,15 @@ 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;
> +
This is racy since nothing prevents the device from being suspended
right here.
> + serial_pm_get_noresume(port->dev);
> + port->ops->start_tx(port);
> + serial_pm_autosuspend(port->dev);
> }
>
> static void uart_start(struct tty_struct *tty)
> @@ -160,6 +190,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);
Perhaps as an intermediate step, but this looks like another argument
for handling everything runtime PM related in core (i.e. enabling
runtime pm and generic suspend/resume ops) and adding callbacks to do
subdriver specific bits instead.
> +
> static void
> uart_update_mctrl(struct uart_port *port, unsigned int set, unsigned int clear)
> {
> 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 */
> + unsigned int runtime_suspended:1; /* 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);
Johan
