[PATCHv3 0/4] Get rid of pm_runtime_irq_safe() for 8250_omap

[PATCHv3 0/4] Get rid of pm_runtime_irq_safe() for 8250_omap

[Tony Lindgren]

Hi,

Here are v3 patches to get rid of pm_runtime_irq_safe() for the 8250_omap
driver. Based on comments from Andy, Johan and Greg, I improved a bunch of
things as listed below.

For removing the pm_runtime_irq_safe() usage, serial TX is the last
remaining issue. We deal with TX by waking up the port and returning 0
bytes written from write_room() and write() if the port is not available
because of PM runtime autoidle.

This series also removes the dependency to Andy's pending generic serial
layer PM runtime patches, and hopefully makes that work a bit easier :)

Regards,

Tony

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


Tony Lindgren (4):
  serial: core: Add wakeup() and start_pending_tx() for power management
  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_omap.c        | 44 ++++++++++----
 drivers/tty/serial/8250/8250_port.c        | 39 ++++++++++++-
 drivers/tty/serial/serial_core.c           | 68 +++++++++++++++++++++-
 include/linux/serial_core.h                |  3 +
 5 files changed, 149 insertions(+), 14 deletions(-)

-- 
2.33.0

[PATCH 1/4] serial: core: Add wakeup() and start_pending_tx() for power management

[Tony Lindgren]

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
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@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           | 68 +++++++++++++++++++++-
 include/linux/serial_core.h                |  3 +
 3 files changed, 78 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,35 @@ 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 (!port->runtime_suspended)
+		return 0;
+
+	if (port->ops->wakeup)
+		return port->ops->wakeup(port);
+
+	return 0;
+}
+
+static int uart_port_wakeup(struct uart_port *port)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&port->lock, flags);
+	ret = __uart_port_wakeup(port);
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	return ret;
+}
+
 /*
  * This routine is used by the interrupt handler to schedule processing in
  * the software interrupt portion of the driver.
@@ -123,8 +152,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 +172,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 +1116,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 +1456,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 +1783,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 */
+	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);
 
-- 
2.33.0

[PATCH 2/4] serial: 8250: Implement wakeup for TX and use it for 8250_omap

[Tony Lindgren]

We can use the wakeup() and uart_start_pending_tx() calls to wake up an
idle serial port and send out the pending TX buffer on runtime PM resume.
This allows us to remove the dependency to pm_runtime_irq_safe() for
8250_omap driver in the following patches.

We manage the port runtime_suspended flag in the serial port driver as
only the driver knows when the hardware is runtime PM suspended. Note that
The current flag for rpm_tx_active cannot be used as it is TX specific
for 8250_port.

We already have serial8250_start_tx() call serial8250_rpm_get_tx(), and
serial8250_stop_tx() call serial8250_rpm_put_tx() to take care of the
runtime PM usage count for TX. To have the serial port driver call
uart_start_pending_tx() on runtime resume, we must now use just
pm_runtime_get() for serial8250_start_tx() instead of the sync version.

With these changes we must now also flip the 8250_omap driver over to
call uart_start_pending_tx(). That's currently the only user of
UART_CAP_RPM.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_omap.c | 19 ++++++++++++++
 drivers/tty/serial/8250/8250_port.c | 39 ++++++++++++++++++++++++++++-
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1593,12 +1593,16 @@ static int omap8250_runtime_suspend(struct device *dev)
 {
 	struct omap8250_priv *priv = dev_get_drvdata(dev);
 	struct uart_8250_port *up;
+	struct uart_port *port;
+	unsigned long flags;
 
 	/* In case runtime-pm tries this before we are setup */
 	if (!priv)
 		return 0;
 
 	up = serial8250_get_port(priv->line);
+	port = &up->port;
+
 	/*
 	 * When using 'no_console_suspend', the console UART must not be
 	 * suspended. Since driver suspend is managed by runtime suspend,
@@ -1610,6 +1614,10 @@ static int omap8250_runtime_suspend(struct device *dev)
 			return -EBUSY;
 	}
 
+	spin_lock_irqsave(&port->lock, flags);
+	port->runtime_suspended = 1;
+	spin_unlock_irqrestore(&port->lock, flags);
+
 	if (priv->habit & UART_ERRATA_CLOCK_DISABLE) {
 		int ret;
 
@@ -1636,13 +1644,18 @@ static int omap8250_runtime_resume(struct device *dev)
 {
 	struct omap8250_priv *priv = dev_get_drvdata(dev);
 	struct uart_8250_port *up;
+	struct uart_port *port;
+	unsigned long flags;
 
 	/* In case runtime-pm tries this before we are setup */
 	if (!priv)
 		return 0;
 
 	up = serial8250_get_port(priv->line);
+	port = &up->port;
 
+	/* Restore state with interrupts disabled */
+	spin_lock_irqsave(&port->lock, flags);
 	if (omap8250_lost_context(up))
 		omap8250_restore_regs(up);
 
@@ -1651,6 +1664,12 @@ static int omap8250_runtime_resume(struct device *dev)
 
 	priv->latency = priv->calc_latency;
 	schedule_work(&priv->qos_work);
+
+	port->runtime_suspended = 0;
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	uart_start_pending_tx(port);
+
 	return 0;
 }
 #endif
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
@@ -724,7 +724,7 @@ void serial8250_rpm_get_tx(struct uart_8250_port *p)
 	rpm_active = xchg(&p->rpm_tx_active, 1);
 	if (rpm_active)
 		return;
-	pm_runtime_get_sync(p->port.dev);
+	pm_runtime_get(p->port.dev);
 }
 EXPORT_SYMBOL_GPL(serial8250_rpm_get_tx);
 
@@ -2507,6 +2507,42 @@ static void serial8250_shutdown(struct uart_port *port)
 		serial8250_do_shutdown(port);
 }
 
+/*
+ * Wakes up the serial port if it has been runtime PM suspended.
+ *
+ * Note that we rely on the serial8250_rpm functions to manage the
+ * runtime PM usage count. We also currently depend on the runtime
+ * PM autosuspend timeout to keep the port awake until start_tx().
+ * Eventually we should just use runtime PM functions and not rely
+ * on the autosuspend timeout.
+ *
+ * Caller must hold port->lock for port->runtime_suspended status.
+ * Also the port drivers must hold port->lock when changing the
+ * state for port->runtime_suspended in runtime PM functions.
+ */
+static int serial8250_wakeup(struct uart_port *port)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	struct device *dev = up->port.dev;
+	int err;
+
+	if (!(up->capabilities & UART_CAP_RPM))
+		return 0;
+
+	if (!port->runtime_suspended) {
+		pm_runtime_mark_last_busy(dev);
+		return 0;
+	}
+
+	err = pm_request_resume(dev);
+	if (err < 0) {
+		dev_warn(dev, "wakeup failed: %d\n", err);
+		return err;
+	}
+
+	return -EINPROGRESS;
+}
+
 /* Nuvoton NPCM UARTs have a custom divisor calculation */
 static unsigned int npcm_get_divisor(struct uart_8250_port *up,
 		unsigned int baud)
@@ -3235,6 +3271,7 @@ static const struct uart_ops serial8250_pops = {
 	.break_ctl	= serial8250_break_ctl,
 	.startup	= serial8250_startup,
 	.shutdown	= serial8250_shutdown,
+	.wakeup		= serial8250_wakeup,
 	.set_termios	= serial8250_set_termios,
 	.set_ldisc	= serial8250_set_ldisc,
 	.pm		= serial8250_pm,
-- 
2.33.0

[PATCH 3/4] serial: 8250_omap: Require a valid wakeirq for deeper idle states

[Tony Lindgren]

For deeper idle states the 8250 device gets powered off. The wakeup is
handled with a separate wakeirq controller monitoring the RX pin.

Let's check for a valid wakeirq before enabling deeper idle states.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_omap.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -133,6 +133,7 @@ struct omap8250_priv {
 	spinlock_t rx_dma_lock;
 	bool rx_dma_broken;
 	bool throttled;
+	unsigned int allow_rpm:1;
 };
 
 struct omap8250_dma_params {
@@ -676,6 +677,7 @@ static int omap_8250_startup(struct uart_port *port)
 		ret = dev_pm_set_dedicated_wake_irq(port->dev, priv->wakeirq);
 		if (ret)
 			return ret;
+		priv->allow_rpm = 1;
 	}
 
 	pm_runtime_get_sync(port->dev);
@@ -722,6 +724,10 @@ static int omap_8250_startup(struct uart_port *port)
 	if (up->dma && !(priv->habit & UART_HAS_EFR2))
 		up->dma->rx_dma(up);
 
+	/* Block runtime PM if no wakeirq, paired with shutdown */
+	if (!priv->allow_rpm)
+		pm_runtime_get(port->dev);
+
 	pm_runtime_mark_last_busy(port->dev);
 	pm_runtime_put_autosuspend(port->dev);
 	return 0;
@@ -760,6 +766,10 @@ static void omap_8250_shutdown(struct uart_port *port)
 		serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
 	serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
 
+	/* Clear possible PM runtime block to pair with startup */
+	if (!priv->allow_rpm)
+		pm_runtime_put(port->dev);
+
 	pm_runtime_mark_last_busy(port->dev);
 	pm_runtime_put_autosuspend(port->dev);
 	free_irq(port->irq, port);
-- 
2.33.0

[PATCH 4/4] serial: 8250_omap: Drop the use of pm_runtime_irq_safe()

[Tony Lindgren]

We can finally drop the pm_runtime_irq_safe() usage for 8250_omap driver.

We already have the serial layer RX wake path fixed for power management.
We no longer allow deeper idle states unless the kernel console has been
detached, and we require that the RX wakeirq is configured.

For TX path, we now use the prep_tx() and uart_flush_tx() calls.

To drop pm_runtime_irq_safe(), we remove all PM runtime calls from the
interrupt context. If we ever see an interrupt for an idled port, we just
bail out. We now also need to restore the port context with interrupts
disabled to prevent interrupts from happening while restoring the port.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_omap.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -621,6 +621,9 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 	unsigned int iir, lsr;
 	int ret;
 
+	if (port->runtime_suspended)
+		return IRQ_NONE;
+
 #ifdef CONFIG_SERIAL_8250_DMA
 	if (up->dma) {
 		ret = omap_8250_dma_handle_irq(port);
@@ -628,7 +631,6 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 	}
 #endif
 
-	serial8250_rpm_get(up);
 	lsr = serial_port_in(port, UART_LSR);
 	iir = serial_port_in(port, UART_IIR);
 	ret = serial8250_handle_irq(port, iir);
@@ -662,8 +664,6 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 		schedule_delayed_work(&up->overrun_backoff, delay);
 	}
 
-	serial8250_rpm_put(up);
-
 	return IRQ_RETVAL(ret);
 }
 
@@ -1191,13 +1191,9 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 	unsigned char status;
 	u8 iir;
 
-	serial8250_rpm_get(up);
-
 	iir = serial_port_in(port, UART_IIR);
-	if (iir & UART_IIR_NO_INT) {
-		serial8250_rpm_put(up);
+	if (iir & UART_IIR_NO_INT)
 		return IRQ_HANDLED;
-	}
 
 	spin_lock(&port->lock);
 
@@ -1226,7 +1222,6 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 
 	uart_unlock_and_check_sysrq(port);
 
-	serial8250_rpm_put(up);
 	return 1;
 }
 
@@ -1420,8 +1415,6 @@ static int omap8250_probe(struct platform_device *pdev)
 	if (!of_get_available_child_count(pdev->dev.of_node))
 		pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
 
-	pm_runtime_irq_safe(&pdev->dev);
-
 	pm_runtime_get_sync(&pdev->dev);
 
 	omap_serial_fill_features_erratas(&up, priv);
-- 
2.33.0

Re: [PATCH 1/4] serial: core: Add wakeup() and start_pending_tx() for power management

[Johan Hovold]

On Fri, Oct 15, 2021 at 02:26:23PM +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
> 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@kernel.org>.
> 
> Suggested-by: Johan Hovold <johan@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

So this looks somewhat like a step in the right direction, but...

> ---
>  Documentation/driver-api/serial/driver.rst |  9 +++
>  drivers/tty/serial/serial_core.c           | 68 +++++++++++++++++++++-
>  include/linux/serial_core.h                |  3 +
>  3 files changed, 78 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,35 @@ 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 (!port->runtime_suspended)
> +		return 0;
> +
> +	if (port->ops->wakeup)
> +		return port->ops->wakeup(port);
> +
> +	return 0;
> +}
> +
> +static int uart_port_wakeup(struct uart_port *port)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&port->lock, flags);
> +	ret = __uart_port_wakeup(port);
> +	spin_unlock_irqrestore(&port->lock, flags);
> +
> +	return ret;
> +}
> +
>  /*
>   * This routine is used by the interrupt handler to schedule processing in
>   * the software interrupt portion of the driver.
> @@ -123,8 +152,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 +172,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 +1116,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;
> +	}

...this isn't right. You should just resume the device synchronously
here and not return some random error to user space, which it is
unlikely to even handle.

Now this may require moving more of the runtime PM into serial core,
where it should have been added in the first place, due to a lot of the
serial callbacks being called with the port spin lock held.

The current implementation is just broken. Take uart_dtr_rts(), for
example, nothing makes sure that the device is active before accessing
the modem control registers there. You're currently just relying on
luck and pm_runtime_irq_safe() (which you are now trying to remove).

> +
>  	if (!tty_io_error(tty)) {
>  		uart_update_mctrl(uport, set, clear);
>  		ret = 0;
> @@ -1402,6 +1456,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 +1783,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);
>  }

Heh, here you do try to do something about dtr_rts(), but you can't just
ignore the request and wish for the best in case the device is
suspended. :) There needs to be a synchronous resume here too.

Johan

Re: [PATCHv3 0/4] Get rid of pm_runtime_irq_safe() for 8250_omap

[Johan Hovold]

On Fri, Oct 15, 2021 at 02:26:22PM +0300, Tony Lindgren wrote:
> Hi,
> 
> Here are v3 patches to get rid of pm_runtime_irq_safe() for the 8250_omap
> driver. Based on comments from Andy, Johan and Greg, I improved a bunch of
> things as listed below.
> 
> For removing the pm_runtime_irq_safe() usage, serial TX is the last
> remaining issue. We deal with TX by waking up the port and returning 0
> bytes written from write_room() and write() if the port is not available
> because of PM runtime autoidle.

Oh, there's a lot more than TX that needs fixing... And I believe the
second sentence no longer applies since v1.

> 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

Johan

Re: [PATCH 1/4] serial: core: Add wakeup() and start_pending_tx() for power management

[Tony Lindgren]

Hi,

* Johan Hovold <johan@kernel.org> [211018 07:09]:
> On Fri, Oct 15, 2021 at 02:26:23PM +0300, Tony Lindgren wrote:
> > @@ -1067,6 +1116,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;
> > +	}
> 
> ...this isn't right. You should just resume the device synchronously
> here and not return some random error to user space, which it is
> unlikely to even handle.

OK I'll check what we can already wake synchronously :)

> Now this may require moving more of the runtime PM into serial core,
> where it should have been added in the first place, due to a lot of the
> serial callbacks being called with the port spin lock held.

Yup.. So the good news is that Andy already has the generic serial layer
runtime PM changes in his WIP tree. I'll take a look if we can already
add some of that without bringing in all the other dependencies.

> The current implementation is just broken. Take uart_dtr_rts(), for
> example, nothing makes sure that the device is active before accessing
> the modem control registers there. You're currently just relying on
> luck and pm_runtime_irq_safe() (which you are now trying to remove).

Yeah agreed, it's broken. It is usable for at least two limited cases
though, which are a serial port console with PM, and bluetooth with PM.

The serial port console typically only has RX and TX lines connected, and
the bluetooth typically uses out-of-band GPIO pins for wakeups.

To enable the serial port PM in general, we need to make sure it is
enabled only for applications where it can be used. So it needs to be
enabled from the user space as we do for the serial console, or enabled
from the consumer device driver for things like bluetooth.

Sure the TX should work in all other cases too..

> > +
> > +	if (uart_port_wakeup(uport) < 0)
> > +		goto out;
> > +
> >  	uart_port_dtr_rts(uport, raise);
> > +out:
> >  	uart_port_deref(uport);
> >  }
> 
> Heh, here you do try to do something about dtr_rts(), but you can't just
> ignore the request and wish for the best in case the device is
> suspended. :) There needs to be a synchronous resume here too.

Well for the current use cases the port should be already awake at
this point :) But yeah, for the TX path we should be able to handle
all the cases.

Regards,

Tony

Re: [PATCH 1/4] serial: core: Add wakeup() and start_pending_tx() for power management

[Tony Lindgren]

* 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

Re: [PATCH 1/4] serial: core: Add wakeup() and start_pending_tx() for power management

[Greg Kroah-Hartman]

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

Re: [PATCH 1/4] serial: core: Add wakeup() and start_pending_tx() for power management

[Tony Lindgren]

* 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

Re: [PATCH 1/4] serial: core: Add wakeup() and start_pending_tx() for power management

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

[PATCH 1/4] serial: core: Add wakeup() and start_pending_tx() for power management

[Tony Lindgren]

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;
+
+	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);
 
-- 
2.33.0