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(-)
--
2.33.1
From: Andy Shevchenko <[email protected]>
8250 driver has wrong implementation of runtime power management, i.e.
it uses an irq_safe flag. The irq_safe flag takes a permanent usage count
on the parent device preventing the parent from idling. This patch
prepares for making runtime power management generic by adding runtime PM
calls to serial core once for all UART drivers.
As we have serial drivers that do not enable runtime PM, and drivers that
enable runtime PM, we add new functions for serial_pm_resume_and_get() and
serial_pm_autosuspend() functions to handle errors and allow the use also
for cases when runtime PM is not enabled. The other option considered was
to not check for runtime PM enable errors. But some CPUs can hang when the
clocks are not enabled for the device, so ignoring the errors is not a good
option. Eventually with the serial port drivers updated, we should be able
to just switch to using the standard runtime PM calls with no need for the
wrapper functions.
Note that this patch only adds runtime PM calls to the functions where we
can call them for synchronous wake-up without the need for irq_safe flag.
Additionally we also need asynchronous wake-up support for __uart_start(),
that is added in a separate patch.
Signed-off-by: Andy Shevchenko <[email protected]>
Co-developed-by: Tony Lindgren <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
drivers/tty/serial/serial_core.c | 150 ++++++++++++++++++++++++++++---
1 file changed, 140 insertions(+), 10 deletions(-)
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
@@ -16,6 +16,7 @@
#include <linux/console.h>
#include <linux/gpio/consumer.h>
#include <linux/of.h>
+#include <linux/pm_runtime.h>
#include <linux/proc_fs.h>
#include <linux/seq_file.h>
#include <linux/device.h>
@@ -91,6 +92,27 @@ static inline struct uart_port *uart_port_check(struct uart_state *state)
return state->uart_port;
}
+/*
+ * Enables runtime PM suspended serial port. If runtime PM is not
+ * enabled by the serial port driver, only increments the runtime PM
+ * usage count. May sleep.
+ */
+static int serial_pm_resume_and_get(struct device *dev)
+{
+ if (!pm_runtime_enabled(dev)) {
+ pm_runtime_get_noresume(dev);
+ return 0;
+ }
+
+ return pm_runtime_resume_and_get(dev);
+}
+
+static void serial_pm_autosuspend(struct device *dev)
+{
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+}
+
/*
* This routine is used by the interrupt handler to schedule processing in
* the software interrupt portion of the driver.
@@ -143,13 +165,18 @@ uart_update_mctrl(struct uart_port *port, unsigned int set, unsigned int clear)
{
unsigned long flags;
unsigned int old;
+ int err;
+ err = serial_pm_resume_and_get(port->dev);
+ if (err < 0)
+ return;
spin_lock_irqsave(&port->lock, flags);
old = port->mctrl;
port->mctrl = (old & ~clear) | set;
if (old != port->mctrl)
port->ops->set_mctrl(port, port->mctrl);
spin_unlock_irqrestore(&port->lock, flags);
+ serial_pm_autosuspend(port->dev);
}
#define uart_set_mctrl(port, set) uart_update_mctrl(port, set, 0)
@@ -218,7 +245,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
free_page(page);
}
+ retval = serial_pm_resume_and_get(uport->dev);
+ if (retval < 0)
+ goto out;
retval = uport->ops->startup(uport);
+ serial_pm_autosuspend(uport->dev);
+
if (retval == 0) {
if (uart_console(uport) && uport->cons->cflag) {
tty->termios.c_cflag = uport->cons->cflag;
@@ -244,7 +276,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
*/
if (retval && capable(CAP_SYS_ADMIN))
return 1;
-
+out:
return retval;
}
@@ -481,6 +513,7 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
struct uart_port *uport = uart_port_check(state);
struct ktermios *termios;
int hw_stopped;
+ int err;
/*
* If we have no tty, termios, or the port does not exist,
@@ -490,6 +523,11 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
return;
termios = &tty->termios;
+
+ err = serial_pm_resume_and_get(uport->dev);
+ if (err < 0)
+ return;
+
uport->ops->set_termios(uport, termios, old_termios);
/*
@@ -518,6 +556,7 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
__uart_start(tty);
}
spin_unlock_irq(&uport->lock);
+ serial_pm_autosuspend(uport->dev);
}
static int uart_put_char(struct tty_struct *tty, unsigned char c)
@@ -1015,7 +1054,14 @@ static int uart_get_lsr_info(struct tty_struct *tty,
struct uart_port *uport = uart_port_check(state);
unsigned int result;
+ result = serial_pm_resume_and_get(uport->dev);
+ if (result < 0) {
+ result = TIOCSER_TEMT;
+ goto out;
+ }
+
result = uport->ops->tx_empty(uport);
+ serial_pm_autosuspend(uport->dev);
/*
* If we're about to load something into the transmit
@@ -1027,7 +1073,7 @@ static int uart_get_lsr_info(struct tty_struct *tty,
((uart_circ_chars_pending(&state->xmit) > 0) &&
!uart_tx_stopped(uport)))
result &= ~TIOCSER_TEMT;
-
+out:
return put_user(result, value);
}
@@ -1045,9 +1091,14 @@ static int uart_tiocmget(struct tty_struct *tty)
if (!tty_io_error(tty)) {
result = uport->mctrl;
+
+ result = serial_pm_resume_and_get(uport->dev);
+ if (result < 0)
+ goto out;
spin_lock_irq(&uport->lock);
result |= uport->ops->get_mctrl(uport);
spin_unlock_irq(&uport->lock);
+ serial_pm_autosuspend(uport->dev);
}
out:
mutex_unlock(&port->mutex);
@@ -1088,8 +1139,12 @@ static int uart_break_ctl(struct tty_struct *tty, int break_state)
if (!uport)
goto out;
+ ret = serial_pm_resume_and_get(uport->dev);
+ if (ret < 0)
+ goto out;
if (uport->type != PORT_UNKNOWN && uport->ops->break_ctl)
uport->ops->break_ctl(uport, break_state);
+ serial_pm_autosuspend(uport->dev);
ret = 0;
out:
mutex_unlock(&port->mutex);
@@ -1138,7 +1193,11 @@ static int uart_do_autoconfig(struct tty_struct *tty, struct uart_state *state)
* This will claim the ports resources if
* a port is found.
*/
+ ret = serial_pm_resume_and_get(uport->dev);
+ if (ret < 0)
+ goto out;
uport->ops->config_port(uport, flags);
+ serial_pm_autosuspend(uport->dev);
ret = uart_startup(tty, state, 1);
if (ret == 0)
@@ -1443,14 +1502,21 @@ static void uart_set_ldisc(struct tty_struct *tty)
struct uart_state *state = tty->driver_data;
struct uart_port *uport;
struct tty_port *port = &state->port;
+ int err;
if (!tty_port_initialized(port))
return;
mutex_lock(&state->port.mutex);
uport = uart_port_check(state);
- if (uport && uport->ops->set_ldisc)
+ if (uport && uport->ops->set_ldisc) {
+ err = serial_pm_resume_and_get(uport->dev);
+ if (err < 0)
+ goto out;
uport->ops->set_ldisc(uport, &tty->termios);
+ serial_pm_autosuspend(uport->dev);
+ }
+out:
mutex_unlock(&state->port.mutex);
}
@@ -1542,6 +1608,7 @@ static void uart_tty_port_shutdown(struct tty_port *port)
{
struct uart_state *state = container_of(port, struct uart_state, port);
struct uart_port *uport = uart_port_check(state);
+ int err;
/*
* At this point, we stop accepting input. To do this, we
@@ -1550,9 +1617,13 @@ static void uart_tty_port_shutdown(struct tty_port *port)
if (WARN(!uport, "detached port still initialized!\n"))
return;
+ err = serial_pm_resume_and_get(uport->dev);
+ if (err < 0)
+ return;
spin_lock_irq(&uport->lock);
uport->ops->stop_rx(uport);
spin_unlock_irq(&uport->lock);
+ serial_pm_autosuspend(uport->dev);
uart_port_shutdown(port);
@@ -1668,6 +1739,7 @@ static void uart_port_shutdown(struct tty_port *port)
{
struct uart_state *state = container_of(port, struct uart_state, port);
struct uart_port *uport = uart_port_check(state);
+ int err;
/*
* clear delta_msr_wait queue to avoid mem leaks: we may free
@@ -1681,8 +1753,13 @@ static void uart_port_shutdown(struct tty_port *port)
/*
* Free the IRQ and disable the port.
*/
- if (uport)
+ if (uport) {
+ err = serial_pm_resume_and_get(uport->dev);
+ if (err < 0)
+ return;
uport->ops->shutdown(uport);
+ serial_pm_autosuspend(uport->dev);
+ }
/*
* Ensure that the IRQ handler isn't running on another CPU.
@@ -1803,7 +1880,7 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i)
struct uart_port *uport;
char stat_buf[32];
unsigned int status;
- int mmio;
+ int mmio, err;
mutex_lock(&port->mutex);
uport = uart_port_check(state);
@@ -1824,12 +1901,16 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i)
}
if (capable(CAP_SYS_ADMIN)) {
+ err = serial_pm_resume_and_get(uport->dev);
+ if (err < 0)
+ goto out;
pm_state = state->pm_state;
if (pm_state != UART_PM_STATE_ON)
uart_change_pm(state, UART_PM_STATE_ON);
spin_lock_irq(&uport->lock);
status = uport->ops->get_mctrl(uport);
spin_unlock_irq(&uport->lock);
+ serial_pm_autosuspend(uport->dev);
if (pm_state != UART_PM_STATE_ON)
uart_change_pm(state, pm_state);
@@ -2050,6 +2131,7 @@ uart_set_options(struct uart_port *port, struct console *co,
{
struct ktermios termios;
static struct ktermios dummy;
+ int ret;
/*
* Ensure that the serial-console lock is initialised early.
@@ -2089,7 +2171,17 @@ uart_set_options(struct uart_port *port, struct console *co,
*/
port->mctrl |= TIOCM_DTR;
- port->ops->set_termios(port, &termios, &dummy);
+ /* At early stage device is not created yet, we can't do PM */
+ if (port->dev) {
+ ret = serial_pm_resume_and_get(port->dev);
+ if (ret < 0)
+ return ret;
+ port->ops->set_termios(port, &termios, &dummy);
+ serial_pm_autosuspend(port->dev);
+ } else {
+ port->ops->set_termios(port, &termios, &dummy);
+ }
+
/*
* Allow the setting of the UART parameters with a NULL console
* too:
@@ -2143,6 +2235,7 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
struct tty_port *port = &state->port;
struct device *tty_dev;
struct uart_match match = {uport, drv};
+ int err;
mutex_lock(&port->mutex);
@@ -2168,11 +2261,15 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
tty_port_set_suspended(port, 1);
tty_port_set_initialized(port, 0);
+ err = serial_pm_resume_and_get(uport->dev);
+ if (err < 0)
+ goto unlock;
spin_lock_irq(&uport->lock);
ops->stop_tx(uport);
ops->set_mctrl(uport, 0);
ops->stop_rx(uport);
spin_unlock_irq(&uport->lock);
+ serial_pm_autosuspend(uport->dev);
/*
* Wait for the transmitter to empty.
@@ -2183,7 +2280,11 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
dev_err(uport->dev, "%s: Unable to drain transmitter\n",
uport->name);
+ err = serial_pm_resume_and_get(uport->dev);
+ if (err < 0)
+ goto unlock;
ops->shutdown(uport);
+ serial_pm_autosuspend(uport->dev);
}
/*
@@ -2206,6 +2307,7 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
struct device *tty_dev;
struct uart_match match = {uport, drv};
struct ktermios termios;
+ int ret;
mutex_lock(&port->mutex);
@@ -2236,33 +2338,50 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
if (port->tty && termios.c_cflag == 0)
termios = port->tty->termios;
+ ret = serial_pm_resume_and_get(uport->dev);
+ if (ret < 0)
+ goto unlock;
if (console_suspend_enabled)
uart_change_pm(state, UART_PM_STATE_ON);
uport->ops->set_termios(uport, &termios, NULL);
+ serial_pm_autosuspend(uport->dev);
+
if (console_suspend_enabled)
console_start(uport->cons);
}
if (tty_port_suspended(port)) {
const struct uart_ops *ops = uport->ops;
- int ret;
+ ret = serial_pm_resume_and_get(uport->dev);
+ if (ret < 0)
+ goto unlock;
uart_change_pm(state, UART_PM_STATE_ON);
spin_lock_irq(&uport->lock);
ops->set_mctrl(uport, 0);
spin_unlock_irq(&uport->lock);
+ serial_pm_autosuspend(uport->dev);
+
if (console_suspend_enabled || !uart_console(uport)) {
/* Protected by port mutex for now */
struct tty_struct *tty = port->tty;
+ ret = serial_pm_resume_and_get(uport->dev);
+ if (ret < 0)
+ goto unlock;
ret = ops->startup(uport);
+ serial_pm_autosuspend(uport->dev);
if (ret == 0) {
if (tty)
uart_change_speed(tty, state, NULL);
+ ret = serial_pm_resume_and_get(uport->dev);
+ if (ret < 0)
+ goto unlock;
spin_lock_irq(&uport->lock);
ops->set_mctrl(uport, uport->mctrl);
ops->start_tx(uport);
spin_unlock_irq(&uport->lock);
+ serial_pm_autosuspend(uport->dev);
tty_port_set_initialized(port, 1);
} else {
/*
@@ -2276,10 +2395,10 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
tty_port_set_suspended(port, 0);
}
-
+unlock:
mutex_unlock(&port->mutex);
- return 0;
+ return ret;
}
static inline void
@@ -2329,6 +2448,7 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
struct uart_port *port)
{
unsigned int flags;
+ int err;
/*
* If there isn't a port here, don't do anything further.
@@ -2356,6 +2476,10 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
uart_report_port(drv, port);
+ err = serial_pm_resume_and_get(port->dev);
+ if (err < 0)
+ return;
+
/* Power up port for set_mctrl() */
uart_change_pm(state, UART_PM_STATE_ON);
@@ -2367,6 +2491,7 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
spin_lock_irqsave(&port->lock, flags);
port->ops->set_mctrl(port, port->mctrl & TIOCM_DTR);
spin_unlock_irqrestore(&port->lock, flags);
+ serial_pm_autosuspend(port->dev);
/*
* If this driver supports console, and it hasn't been
@@ -3084,11 +3209,16 @@ EXPORT_SYMBOL_GPL(uart_handle_dcd_change);
*/
void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
{
+ int err;
+
lockdep_assert_held_once(&uport->lock);
uport->icount.cts++;
if (uart_softcts_mode(uport)) {
+ err = serial_pm_resume_and_get(uport->dev);
+ if (err < 0)
+ return;
if (uport->hw_stopped) {
if (status) {
uport->hw_stopped = 0;
@@ -3101,7 +3231,7 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
uport->ops->stop_tx(uport);
}
}
-
+ serial_pm_autosuspend(uport->dev);
}
}
EXPORT_SYMBOL_GPL(uart_handle_cts_change);
--
2.33.1
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.
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
<[email protected]>.
Suggested-by: Johan Hovold <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
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);
+
+ 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;
+
+ 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);
+
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);
--
2.33.1
From: Andy Shevchenko <[email protected]>
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 <[email protected]>
[[email protected]: use port->runtime_suspended]
Signed-off-by: Tony Lindgren <[email protected]>
---
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;
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);
}
/*
--
2.33.1
From: Andy Shevchenko <[email protected]>
Since we now have runtime PM calls in serial_core.c the individual
drivers do not need them anymore for the struct uart_ops related
functions.
Remove runtime PM calls in 8250 driver. This still leaves the flag for
UART_CAP_RPM for serial8250_rpm_get_tx(), serial8250_rpm_put_tx() and
serial8250_wakeup() to manage the reference count for serial TX.
Signed-off-by: Andy Shevchenko <[email protected]>
[[email protected]: updated to remove the exported functions too]
Signed-off-by: Tony Lindgren <[email protected]>
---
drivers/tty/serial/8250/8250.h | 3 -
drivers/tty/serial/8250/8250_port.c | 98 ++++++++---------------------
2 files changed, 27 insertions(+), 74 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -152,9 +152,6 @@ static inline bool serial8250_clear_THRI(struct uart_8250_port *up)
struct uart_8250_port *serial8250_get_port(int line);
-void serial8250_rpm_get(struct uart_8250_port *p);
-void serial8250_rpm_put(struct uart_8250_port *p);
-
void serial8250_rpm_get_tx(struct uart_8250_port *p);
void serial8250_rpm_put_tx(struct uart_8250_port *p);
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
@@ -573,23 +573,6 @@ void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p)
}
EXPORT_SYMBOL_GPL(serial8250_clear_and_reinit_fifos);
-void serial8250_rpm_get(struct uart_8250_port *p)
-{
- if (!(p->capabilities & UART_CAP_RPM))
- return;
- pm_runtime_get_sync(p->port.dev);
-}
-EXPORT_SYMBOL_GPL(serial8250_rpm_get);
-
-void serial8250_rpm_put(struct uart_8250_port *p)
-{
- if (!(p->capabilities & UART_CAP_RPM))
- return;
- pm_runtime_mark_last_busy(p->port.dev);
- pm_runtime_put_autosuspend(p->port.dev);
-}
-EXPORT_SYMBOL_GPL(serial8250_rpm_put);
-
/**
* serial8250_em485_init() - put uart_8250_port into rs485 emulating
* @p: uart_8250_port port instance
@@ -724,7 +707,14 @@ 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(p->port.dev);
+
+ /*
+ * Device has to be powered on at this point. Here we just increase
+ * reference count to prevent autosuspend until the TX FIFO becomes
+ * empty. Paired with serial8250_rpm_put_tx(). See also a comment
+ * in serial8250_tx_chars().
+ */
+ pm_runtime_get_noresume(p->port.dev);
}
EXPORT_SYMBOL_GPL(serial8250_rpm_get_tx);
@@ -752,8 +742,6 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
{
unsigned char lcr = 0, efr = 0;
- serial8250_rpm_get(p);
-
if (p->capabilities & UART_CAP_SLEEP) {
if (p->capabilities & UART_CAP_EFR) {
lcr = serial_in(p, UART_LCR);
@@ -769,8 +757,6 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
serial_out(p, UART_LCR, lcr);
}
}
-
- serial8250_rpm_put(p);
}
#ifdef CONFIG_SERIAL_8250_RSA
@@ -1432,13 +1418,9 @@ static void serial8250_stop_rx(struct uart_port *port)
{
struct uart_8250_port *up = up_to_u8250p(port);
- serial8250_rpm_get(up);
-
up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
up->port.read_status_mask &= ~UART_LSR_DR;
serial_port_out(port, UART_IER, up->ier);
-
- serial8250_rpm_put(up);
}
/**
@@ -1477,8 +1459,11 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
stop_tx_timer);
struct uart_8250_port *p = em485->port;
unsigned long flags;
+ int err;
- serial8250_rpm_get(p);
+ err = pm_runtime_resume_and_get(p->port.dev);
+ if (err < 0)
+ goto out_rpm_err;
spin_lock_irqsave(&p->port.lock, flags);
if (em485->active_timer == &em485->stop_tx_timer) {
p->rs485_stop_tx(p);
@@ -1486,8 +1471,9 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
em485->tx_stopped = true;
}
spin_unlock_irqrestore(&p->port.lock, flags);
- serial8250_rpm_put(p);
-
+ pm_runtime_mark_last_busy(p->port.dev);
+ pm_runtime_put_autosuspend(p->port.dev);
+out_rpm_err:
return HRTIMER_NORESTART;
}
@@ -1545,7 +1531,6 @@ static void serial8250_stop_tx(struct uart_port *port)
{
struct uart_8250_port *up = up_to_u8250p(port);
- serial8250_rpm_get(up);
__stop_tx(up);
/*
@@ -1555,7 +1540,6 @@ static void serial8250_stop_tx(struct uart_port *port)
up->acr |= UART_ACR_TXDIS;
serial_icr_write(up, UART_ACR, up->acr);
}
- serial8250_rpm_put(up);
}
static inline void __start_tx(struct uart_port *port)
@@ -1703,9 +1687,7 @@ static void serial8250_enable_ms(struct uart_port *port)
up->ier |= UART_IER_MSI;
- serial8250_rpm_get(up);
serial_port_out(port, UART_IER, up->ier);
- serial8250_rpm_put(up);
}
void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
@@ -1984,15 +1966,11 @@ static unsigned int serial8250_tx_empty(struct uart_port *port)
unsigned long flags;
unsigned int lsr;
- serial8250_rpm_get(up);
-
spin_lock_irqsave(&port->lock, flags);
lsr = serial_port_in(port, UART_LSR);
up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
spin_unlock_irqrestore(&port->lock, flags);
- serial8250_rpm_put(up);
-
return (lsr & BOTH_EMPTY) == BOTH_EMPTY ? TIOCSER_TEMT : 0;
}
@@ -2002,9 +1980,7 @@ unsigned int serial8250_do_get_mctrl(struct uart_port *port)
unsigned int status;
unsigned int val;
- serial8250_rpm_get(up);
status = serial8250_modem_status(up);
- serial8250_rpm_put(up);
val = serial8250_MSR_to_TIOCM(status);
if (up->gpios)
@@ -2054,7 +2030,6 @@ static void serial8250_break_ctl(struct uart_port *port, int break_state)
struct uart_8250_port *up = up_to_u8250p(port);
unsigned long flags;
- serial8250_rpm_get(up);
spin_lock_irqsave(&port->lock, flags);
if (break_state == -1)
up->lcr |= UART_LCR_SBC;
@@ -2062,7 +2037,6 @@ static void serial8250_break_ctl(struct uart_port *port, int break_state)
up->lcr &= ~UART_LCR_SBC;
serial_port_out(port, UART_LCR, up->lcr);
spin_unlock_irqrestore(&port->lock, flags);
- serial8250_rpm_put(up);
}
/*
@@ -2107,33 +2081,21 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
static int serial8250_get_poll_char(struct uart_port *port)
{
- struct uart_8250_port *up = up_to_u8250p(port);
unsigned char lsr;
- int status;
-
- serial8250_rpm_get(up);
lsr = serial_port_in(port, UART_LSR);
+ if (!(lsr & UART_LSR_DR))
+ return NO_POLL_CHAR;
- if (!(lsr & UART_LSR_DR)) {
- status = NO_POLL_CHAR;
- goto out;
- }
-
- status = serial_port_in(port, UART_RX);
-out:
- serial8250_rpm_put(up);
- return status;
+ return serial_port_in(port, UART_RX);
}
-
static void serial8250_put_poll_char(struct uart_port *port,
unsigned char c)
{
unsigned int ier;
struct uart_8250_port *up = up_to_u8250p(port);
- serial8250_rpm_get(up);
/*
* First save the IER then disable the interrupts
*/
@@ -2155,7 +2117,6 @@ static void serial8250_put_poll_char(struct uart_port *port,
*/
wait_for_xmitr(up, BOTH_EMPTY);
serial_port_out(port, UART_IER, ier);
- serial8250_rpm_put(up);
}
#endif /* CONFIG_CONSOLE_POLL */
@@ -2178,7 +2139,6 @@ int serial8250_do_startup(struct uart_port *port)
if (port->iotype != up->cur_iotype)
set_io_from_upio(port);
- serial8250_rpm_get(up);
if (port->type == PORT_16C950) {
/* Wake up and initialize UART */
up->acr = 0;
@@ -2244,8 +2204,7 @@ int serial8250_do_startup(struct uart_port *port)
if (!(port->flags & UPF_BUGGY_UART) &&
(serial_port_in(port, UART_LSR) == 0xff)) {
dev_info_ratelimited(port->dev, "LSR safety check engaged!\n");
- retval = -ENODEV;
- goto out;
+ return -ENODEV;
}
/*
@@ -2333,7 +2292,7 @@ int serial8250_do_startup(struct uart_port *port)
retval = up->ops->setup_irq(up);
if (retval)
- goto out;
+ return retval;
/*
* Now, initialize the UART
@@ -2432,10 +2391,7 @@ int serial8250_do_startup(struct uart_port *port)
outb_p(0x80, icp);
inb_p(icp);
}
- retval = 0;
-out:
- serial8250_rpm_put(up);
- return retval;
+ return 0;
}
EXPORT_SYMBOL_GPL(serial8250_do_startup);
@@ -2451,7 +2407,6 @@ void serial8250_do_shutdown(struct uart_port *port)
struct uart_8250_port *up = up_to_u8250p(port);
unsigned long flags;
- serial8250_rpm_get(up);
/*
* Disable interrupts from this port
*/
@@ -2495,7 +2450,6 @@ void serial8250_do_shutdown(struct uart_port *port)
* the IRQ chain.
*/
serial_port_in(port, UART_RX);
- serial8250_rpm_put(up);
up->ops->release_irq(up);
}
@@ -2737,6 +2691,7 @@ void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk)
unsigned int baud, quot, frac = 0;
struct ktermios *termios;
unsigned long flags;
+ int err;
mutex_lock(&port->state->port.mutex);
@@ -2753,7 +2708,9 @@ void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk)
baud = serial8250_get_baud_rate(port, termios, NULL);
quot = serial8250_get_divisor(port, baud, &frac);
- serial8250_rpm_get(up);
+ err = pm_runtime_resume_and_get(port->dev);
+ if (err < 0)
+ goto out_lock;
spin_lock_irqsave(&port->lock, flags);
uart_update_timeout(port, termios->c_cflag, baud);
@@ -2762,7 +2719,8 @@ void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk)
serial_port_out(port, UART_LCR, up->lcr);
spin_unlock_irqrestore(&port->lock, flags);
- serial8250_rpm_put(up);
+ pm_runtime_mark_last_busy(port->dev);
+ pm_runtime_put_autosuspend(port->dev);
out_lock:
mutex_unlock(&port->state->port.mutex);
@@ -2793,7 +2751,6 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
* Ok, we're now changing the port state. Do it with
* interrupts disabled.
*/
- serial8250_rpm_get(up);
spin_lock_irqsave(&port->lock, flags);
up->lcr = cval; /* Save computed LCR */
@@ -2899,7 +2856,6 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
}
serial8250_set_mctrl(port, port->mctrl);
spin_unlock_irqrestore(&port->lock, flags);
- serial8250_rpm_put(up);
/* Don't rewrite B0 */
if (tty_termios_baud_rate(termios))
--
2.33.1
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 <[email protected]>
---
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);
@@ -2509,6 +2509,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 (!pm_runtime_enabled(dev))
+ 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)
@@ -3237,6 +3273,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.1
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.
Signed-off-by: Tony Lindgren <[email protected]>
---
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.1
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 <[email protected]>
---
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_noresume(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_noidle(port->dev);
+
pm_runtime_mark_last_busy(port->dev);
pm_runtime_put_autosuspend(port->dev);
free_irq(port->irq, port);
--
2.33.1
Hi Tony,
I love your patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on v5.16-rc1 next-20211116]
[cannot apply to tty/tty-testing]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Tony-Lindgren/Serial-port-generic-PM-to-fix-8250-PM/20211115-164354
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: hexagon-randconfig-r045-20211115 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project fbe72e41b99dc7994daac300d208a955be3e4a0a)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/d9568fa846ba1319eacfd03d39b48c3af05fe9f9
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Tony-Lindgren/Serial-port-generic-PM-to-fix-8250-PM/20211115-164354
git checkout d9568fa846ba1319eacfd03d39b48c3af05fe9f9
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=hexagon
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/tty/serial/serial_core.c:2353:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (tty_port_suspended(port)) {
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/tty/serial/serial_core.c:2401:9: note: uninitialized use occurs here
return ret;
^~~
drivers/tty/serial/serial_core.c:2353:2: note: remove the 'if' if its condition is always true
if (tty_port_suspended(port)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/tty/serial/serial_core.c:2310:9: note: initialize the variable 'ret' to silence this warning
int ret;
^
= 0
1 warning generated.
vim +2353 drivers/tty/serial/serial_core.c
^1da177e4c3f41 drivers/serial/serial_core.c Linus Torvalds 2005-04-16 2302
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2303 int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
^1da177e4c3f41 drivers/serial/serial_core.c Linus Torvalds 2005-04-16 2304 {
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2305 struct uart_state *state = drv->state + uport->line;
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2306 struct tty_port *port = &state->port;
03a74dcc7eebe6 drivers/serial/serial_core.c Arjan van de Ven 2008-05-23 2307 struct device *tty_dev;
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2308 struct uart_match match = {uport, drv};
ba15ab0e8de0d4 drivers/serial/serial_core.c Deepak Saxena 2009-09-19 2309 struct ktermios termios;
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2310 int ret;
^1da177e4c3f41 drivers/serial/serial_core.c Linus Torvalds 2005-04-16 2311
a2bceae065ed8c drivers/serial/serial_core.c Alan Cox 2009-09-19 2312 mutex_lock(&port->mutex);
^1da177e4c3f41 drivers/serial/serial_core.c Linus Torvalds 2005-04-16 2313
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2314 tty_dev = device_find_child(uport->dev, &match, serial_match_port);
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2315 if (!uport->suspended && device_may_wakeup(tty_dev)) {
aef3ad103a686f drivers/tty/serial/serial_core.c Andy Shevchenko 2017-08-13 2316 if (irqd_is_wakeup_set(irq_get_irq_data((uport->irq))))
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2317 disable_irq_wake(uport->irq);
5a65dcc04cda41 drivers/tty/serial/serial_core.c Federico Vaga 2013-04-15 2318 put_device(tty_dev);
a2bceae065ed8c drivers/serial/serial_core.c Alan Cox 2009-09-19 2319 mutex_unlock(&port->mutex);
b3b708fa2780cd drivers/serial/serial_core.c Guennadi Liakhovetski 2007-10-16 2320 return 0;
b3b708fa2780cd drivers/serial/serial_core.c Guennadi Liakhovetski 2007-10-16 2321 }
5a65dcc04cda41 drivers/tty/serial/serial_core.c Federico Vaga 2013-04-15 2322 put_device(tty_dev);
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2323 uport->suspended = 0;
b3b708fa2780cd drivers/serial/serial_core.c Guennadi Liakhovetski 2007-10-16 2324
^1da177e4c3f41 drivers/serial/serial_core.c Linus Torvalds 2005-04-16 2325 /*
^1da177e4c3f41 drivers/serial/serial_core.c Linus Torvalds 2005-04-16 2326 * Re-enable the console device after suspending.
^1da177e4c3f41 drivers/serial/serial_core.c Linus Torvalds 2005-04-16 2327 */
5933a161abcb8d drivers/tty/serial/serial_core.c Yin Kangkai 2011-01-30 2328 if (uart_console(uport)) {
891b9dd1076435 drivers/serial/serial_core.c Jason Wang 2010-08-21 2329 /*
891b9dd1076435 drivers/serial/serial_core.c Jason Wang 2010-08-21 2330 * First try to use the console cflag setting.
891b9dd1076435 drivers/serial/serial_core.c Jason Wang 2010-08-21 2331 */
891b9dd1076435 drivers/serial/serial_core.c Jason Wang 2010-08-21 2332 memset(&termios, 0, sizeof(struct ktermios));
891b9dd1076435 drivers/serial/serial_core.c Jason Wang 2010-08-21 2333 termios.c_cflag = uport->cons->cflag;
891b9dd1076435 drivers/serial/serial_core.c Jason Wang 2010-08-21 2334
891b9dd1076435 drivers/serial/serial_core.c Jason Wang 2010-08-21 2335 /*
891b9dd1076435 drivers/serial/serial_core.c Jason Wang 2010-08-21 2336 * If that's unset, use the tty termios setting.
891b9dd1076435 drivers/serial/serial_core.c Jason Wang 2010-08-21 2337 */
adc8d746caa67f drivers/tty/serial/serial_core.c Alan Cox 2012-07-14 2338 if (port->tty && termios.c_cflag == 0)
adc8d746caa67f drivers/tty/serial/serial_core.c Alan Cox 2012-07-14 2339 termios = port->tty->termios;
891b9dd1076435 drivers/serial/serial_core.c Jason Wang 2010-08-21 2340
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2341 ret = serial_pm_resume_and_get(uport->dev);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2342 if (ret < 0)
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2343 goto unlock;
94abc56f4d90f2 drivers/tty/serial/serial_core.c Ning Jiang 2011-09-05 2344 if (console_suspend_enabled)
6f538fe31c1d45 drivers/tty/serial/serial_core.c Linus Walleij 2012-12-07 2345 uart_change_pm(state, UART_PM_STATE_ON);
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2346 uport->ops->set_termios(uport, &termios, NULL);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2347 serial_pm_autosuspend(uport->dev);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2348
5933a161abcb8d drivers/tty/serial/serial_core.c Yin Kangkai 2011-01-30 2349 if (console_suspend_enabled)
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2350 console_start(uport->cons);
^1da177e4c3f41 drivers/serial/serial_core.c Linus Torvalds 2005-04-16 2351 }
^1da177e4c3f41 drivers/serial/serial_core.c Linus Torvalds 2005-04-16 2352
80f02d5424301b drivers/tty/serial/serial_core.c Peter Hurley 2016-04-09 @2353 if (tty_port_suspended(port)) {
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2354 const struct uart_ops *ops = uport->ops;
^1da177e4c3f41 drivers/serial/serial_core.c Linus Torvalds 2005-04-16 2355
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2356 ret = serial_pm_resume_and_get(uport->dev);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2357 if (ret < 0)
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2358 goto unlock;
6f538fe31c1d45 drivers/tty/serial/serial_core.c Linus Walleij 2012-12-07 2359 uart_change_pm(state, UART_PM_STATE_ON);
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2360 spin_lock_irq(&uport->lock);
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2361 ops->set_mctrl(uport, 0);
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2362 spin_unlock_irq(&uport->lock);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2363 serial_pm_autosuspend(uport->dev);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2364
4547be7809a3b7 drivers/serial/serial_core.c Stanislav Brabec 2009-12-02 2365 if (console_suspend_enabled || !uart_console(uport)) {
192251352f912b drivers/serial/serial_core.c Alan Cox 2010-06-01 2366 /* Protected by port mutex for now */
192251352f912b drivers/serial/serial_core.c Alan Cox 2010-06-01 2367 struct tty_struct *tty = port->tty;
4ed71addf51a62 drivers/tty/serial/serial_core.c Tamseel Shams 2020-07-16 2368
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2369 ret = serial_pm_resume_and_get(uport->dev);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2370 if (ret < 0)
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2371 goto unlock;
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2372 ret = ops->startup(uport);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2373 serial_pm_autosuspend(uport->dev);
ee31b337852ca8 drivers/serial/serial_core.c Russell King 2005-11-13 2374 if (ret == 0) {
192251352f912b drivers/serial/serial_core.c Alan Cox 2010-06-01 2375 if (tty)
192251352f912b drivers/serial/serial_core.c Alan Cox 2010-06-01 2376 uart_change_speed(tty, state, NULL);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2377 ret = serial_pm_resume_and_get(uport->dev);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2378 if (ret < 0)
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2379 goto unlock;
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2380 spin_lock_irq(&uport->lock);
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2381 ops->set_mctrl(uport, uport->mctrl);
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2382 ops->start_tx(uport);
ccce6debb62d94 drivers/serial/serial_core.c Alan Cox 2009-09-19 2383 spin_unlock_irq(&uport->lock);
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2384 serial_pm_autosuspend(uport->dev);
d41861ca19c9e9 drivers/tty/serial/serial_core.c Peter Hurley 2016-04-09 2385 tty_port_set_initialized(port, 1);
ee31b337852ca8 drivers/serial/serial_core.c Russell King 2005-11-13 2386 } else {
ee31b337852ca8 drivers/serial/serial_core.c Russell King 2005-11-13 2387 /*
ee31b337852ca8 drivers/serial/serial_core.c Russell King 2005-11-13 2388 * Failed to resume - maybe hardware went away?
ee31b337852ca8 drivers/serial/serial_core.c Russell King 2005-11-13 2389 * Clear the "initialized" flag so we won't try
ee31b337852ca8 drivers/serial/serial_core.c Russell King 2005-11-13 2390 * to call the low level drivers shutdown method.
ee31b337852ca8 drivers/serial/serial_core.c Russell King 2005-11-13 2391 */
192251352f912b drivers/serial/serial_core.c Alan Cox 2010-06-01 2392 uart_shutdown(tty, state);
ee31b337852ca8 drivers/serial/serial_core.c Russell King 2005-11-13 2393 }
4547be7809a3b7 drivers/serial/serial_core.c Stanislav Brabec 2009-12-02 2394 }
a6b93a90850881 drivers/serial/serial_core.c Russell King 2006-10-01 2395
80f02d5424301b drivers/tty/serial/serial_core.c Peter Hurley 2016-04-09 2396 tty_port_set_suspended(port, 0);
^1da177e4c3f41 drivers/serial/serial_core.c Linus Torvalds 2005-04-16 2397 }
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2398 unlock:
a2bceae065ed8c drivers/serial/serial_core.c Alan Cox 2009-09-19 2399 mutex_unlock(&port->mutex);
^1da177e4c3f41 drivers/serial/serial_core.c Linus Torvalds 2005-04-16 2400
d9568fa846ba13 drivers/tty/serial/serial_core.c Andy Shevchenko 2021-11-15 2401 return ret;
^1da177e4c3f41 drivers/serial/serial_core.c Linus Torvalds 2005-04-16 2402 }
^1da177e4c3f41 drivers/serial/serial_core.c Linus Torvalds 2005-04-16 2403
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Mon, Nov 15, 2021 at 10:43 AM Tony Lindgren <[email protected]> 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).
--
With Best Regards,
Andy Shevchenko
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:57AM +0200, Tony Lindgren wrote:
> From: Andy Shevchenko <[email protected]>
>
> 8250 driver has wrong implementation of runtime power management, i.e.
> it uses an irq_safe flag. The irq_safe flag takes a permanent usage count
> on the parent device preventing the parent from idling. This patch
> prepares for making runtime power management generic by adding runtime PM
> calls to serial core once for all UART drivers.
>
> As we have serial drivers that do not enable runtime PM, and drivers that
> enable runtime PM, we add new functions for serial_pm_resume_and_get() and
> serial_pm_autosuspend() functions to handle errors and allow the use also
> for cases when runtime PM is not enabled. The other option considered was
> to not check for runtime PM enable errors. But some CPUs can hang when the
> clocks are not enabled for the device, so ignoring the errors is not a good
> option. Eventually with the serial port drivers updated, we should be able
> to just switch to using the standard runtime PM calls with no need for the
> wrapper functions.
A third option which needs to be considered is to always enable runtime
pm in core but to keep the ports active while they are opened unless a
driver opts in for more aggressive power management. This is how USB
devices are handled for example.
A next step could then be to move over uart_change_pm() to be handled
from the pm callbacks.
> Note that this patch only adds runtime PM calls to the functions where we
> can call them for synchronous wake-up without the need for irq_safe flag.
> Additionally we also need asynchronous wake-up support for __uart_start(),
> that is added in a separate patch.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> Co-developed-by: Tony Lindgren <[email protected]>
> Signed-off-by: Tony Lindgren <[email protected]>
> ---
> drivers/tty/serial/serial_core.c | 150 ++++++++++++++++++++++++++++---
> 1 file changed, 140 insertions(+), 10 deletions(-)
>
> 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
> @@ -16,6 +16,7 @@
> #include <linux/console.h>
> #include <linux/gpio/consumer.h>
> #include <linux/of.h>
> +#include <linux/pm_runtime.h>
> #include <linux/proc_fs.h>
> #include <linux/seq_file.h>
> #include <linux/device.h>
> @@ -91,6 +92,27 @@ static inline struct uart_port *uart_port_check(struct uart_state *state)
> return state->uart_port;
> }
>
> +/*
> + * Enables runtime PM suspended serial port. If runtime PM is not
> + * enabled by the serial port driver, only increments the runtime PM
> + * usage count. May sleep.
> + */
> +static int serial_pm_resume_and_get(struct device *dev)
> +{
> + if (!pm_runtime_enabled(dev)) {
> + pm_runtime_get_noresume(dev);
> + return 0;
> + }
> +
> + return pm_runtime_resume_and_get(dev);
> +}
> +
> +static void serial_pm_autosuspend(struct device *dev)
> +{
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> +}
The put helper needs a "put" in its name. If these are at all needed,
they could both reflect the underlying helpers names, that is:
serial_pm_resume_and_get()
serial_pm_put_autosuspend()
When wrapping the generic helpers with subsystem specific helpers like
this you should also pass in a pointer to the uart_port instead of a
struct device.
> +
> /*
> * This routine is used by the interrupt handler to schedule processing in
> * the software interrupt portion of the driver.
> @@ -143,13 +165,18 @@ uart_update_mctrl(struct uart_port *port, unsigned int set, unsigned int clear)
> {
> unsigned long flags;
> unsigned int old;
> + int err;
Nit: Pleas use "ret" consistently rather than introducing "err" in some
of the helpers.
>
> + err = serial_pm_resume_and_get(port->dev);
> + if (err < 0)
> + return;
Adding a newline after get and before put would make several functions
more readable (e.g. when doing more than just a single subdriver call).
> spin_lock_irqsave(&port->lock, flags);
> old = port->mctrl;
> port->mctrl = (old & ~clear) | set;
> if (old != port->mctrl)
> port->ops->set_mctrl(port, port->mctrl);
> spin_unlock_irqrestore(&port->lock, flags);
> + serial_pm_autosuspend(port->dev);
> }
>
> #define uart_set_mctrl(port, set) uart_update_mctrl(port, set, 0)
> @@ -218,7 +245,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
> free_page(page);
> }
>
> + retval = serial_pm_resume_and_get(uport->dev);
> + if (retval < 0)
> + goto out;
Just return retval here.
> retval = uport->ops->startup(uport);
> + serial_pm_autosuspend(uport->dev);
> +
> if (retval == 0) {
> if (uart_console(uport) && uport->cons->cflag) {
> tty->termios.c_cflag = uport->cons->cflag;
> @@ -244,7 +276,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
> */
> if (retval && capable(CAP_SYS_ADMIN))
> return 1;
> -
> +out:
> return retval;
> }
>
> @@ -481,6 +513,7 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
> struct uart_port *uport = uart_port_check(state);
> struct ktermios *termios;
> int hw_stopped;
> + int err;
>
> /*
> * If we have no tty, termios, or the port does not exist,
> @@ -490,6 +523,11 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
> return;
>
> termios = &tty->termios;
> +
> + err = serial_pm_resume_and_get(uport->dev);
> + if (err < 0)
> + return;
> +
> uport->ops->set_termios(uport, termios, old_termios);
>
> /*
> @@ -518,6 +556,7 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
> __uart_start(tty);
> }
> spin_unlock_irq(&uport->lock);
> + serial_pm_autosuspend(uport->dev);
> }
>
> static int uart_put_char(struct tty_struct *tty, unsigned char c)
> @@ -1015,7 +1054,14 @@ static int uart_get_lsr_info(struct tty_struct *tty,
> struct uart_port *uport = uart_port_check(state);
> unsigned int result;
>
> + result = serial_pm_resume_and_get(uport->dev);
> + if (result < 0) {
> + result = TIOCSER_TEMT;
Why not return an error?
> + goto out;
> + }
> +
> result = uport->ops->tx_empty(uport);
> + serial_pm_autosuspend(uport->dev);
>
> /*
> * If we're about to load something into the transmit
> @@ -1027,7 +1073,7 @@ static int uart_get_lsr_info(struct tty_struct *tty,
> ((uart_circ_chars_pending(&state->xmit) > 0) &&
> !uart_tx_stopped(uport)))
> result &= ~TIOCSER_TEMT;
> -
> +out:
> return put_user(result, value);
> }
>
> @@ -1045,9 +1091,14 @@ static int uart_tiocmget(struct tty_struct *tty)
>
> if (!tty_io_error(tty)) {
> result = uport->mctrl;
> +
> + result = serial_pm_resume_and_get(uport->dev);
> + if (result < 0)
> + goto out;
> spin_lock_irq(&uport->lock);
> result |= uport->ops->get_mctrl(uport);
> spin_unlock_irq(&uport->lock);
> + serial_pm_autosuspend(uport->dev);
> }
> out:
> mutex_unlock(&port->mutex);
> @@ -1088,8 +1139,12 @@ static int uart_break_ctl(struct tty_struct *tty, int break_state)
> if (!uport)
> goto out;
>
> + ret = serial_pm_resume_and_get(uport->dev);
> + if (ret < 0)
> + goto out;
> if (uport->type != PORT_UNKNOWN && uport->ops->break_ctl)
> uport->ops->break_ctl(uport, break_state);
> + serial_pm_autosuspend(uport->dev);
> ret = 0;
> out:
> mutex_unlock(&port->mutex);
> @@ -1138,7 +1193,11 @@ static int uart_do_autoconfig(struct tty_struct *tty, struct uart_state *state)
> * This will claim the ports resources if
> * a port is found.
> */
> + ret = serial_pm_resume_and_get(uport->dev);
> + if (ret < 0)
> + goto out;
> uport->ops->config_port(uport, flags);
> + serial_pm_autosuspend(uport->dev);
>
> ret = uart_startup(tty, state, 1);
> if (ret == 0)
> @@ -1443,14 +1502,21 @@ static void uart_set_ldisc(struct tty_struct *tty)
> struct uart_state *state = tty->driver_data;
> struct uart_port *uport;
> struct tty_port *port = &state->port;
> + int err;
>
> if (!tty_port_initialized(port))
> return;
>
> mutex_lock(&state->port.mutex);
> uport = uart_port_check(state);
> - if (uport && uport->ops->set_ldisc)
> + if (uport && uport->ops->set_ldisc) {
> + err = serial_pm_resume_and_get(uport->dev);
> + if (err < 0)
> + goto out;
> uport->ops->set_ldisc(uport, &tty->termios);
> + serial_pm_autosuspend(uport->dev);
> + }
> +out:
> mutex_unlock(&state->port.mutex);
> }
>
> @@ -1542,6 +1608,7 @@ static void uart_tty_port_shutdown(struct tty_port *port)
> {
> struct uart_state *state = container_of(port, struct uart_state, port);
> struct uart_port *uport = uart_port_check(state);
> + int err;
>
> /*
> * At this point, we stop accepting input. To do this, we
> @@ -1550,9 +1617,13 @@ static void uart_tty_port_shutdown(struct tty_port *port)
> if (WARN(!uport, "detached port still initialized!\n"))
> return;
>
> + err = serial_pm_resume_and_get(uport->dev);
> + if (err < 0)
> + return;
You probably need to do whatever you can to continue on errors here
rather than just bail out.
> spin_lock_irq(&uport->lock);
> uport->ops->stop_rx(uport);
> spin_unlock_irq(&uport->lock);
> + serial_pm_autosuspend(uport->dev);
>
> uart_port_shutdown(port);
>
> @@ -1668,6 +1739,7 @@ static void uart_port_shutdown(struct tty_port *port)
> {
> struct uart_state *state = container_of(port, struct uart_state, port);
> struct uart_port *uport = uart_port_check(state);
> + int err;
>
> /*
> * clear delta_msr_wait queue to avoid mem leaks: we may free
> @@ -1681,8 +1753,13 @@ static void uart_port_shutdown(struct tty_port *port)
> /*
> * Free the IRQ and disable the port.
> */
> - if (uport)
> + if (uport) {
> + err = serial_pm_resume_and_get(uport->dev);
> + if (err < 0)
> + return;
Same here.
> uport->ops->shutdown(uport);
> + serial_pm_autosuspend(uport->dev);
> + }
>
> /*
> * Ensure that the IRQ handler isn't running on another CPU.
> @@ -1803,7 +1880,7 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i)
> struct uart_port *uport;
> char stat_buf[32];
> unsigned int status;
> - int mmio;
> + int mmio, err;
>
> mutex_lock(&port->mutex);
> uport = uart_port_check(state);
> @@ -1824,12 +1901,16 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i)
> }
>
> if (capable(CAP_SYS_ADMIN)) {
> + err = serial_pm_resume_and_get(uport->dev);
> + if (err < 0)
> + goto out;
> pm_state = state->pm_state;
> if (pm_state != UART_PM_STATE_ON)
> uart_change_pm(state, UART_PM_STATE_ON);
> spin_lock_irq(&uport->lock);
> status = uport->ops->get_mctrl(uport);
> spin_unlock_irq(&uport->lock);
> + serial_pm_autosuspend(uport->dev);
> if (pm_state != UART_PM_STATE_ON)
> uart_change_pm(state, pm_state);
The interaction with uart_change_pm() looks inconsistent. Why resume
before the state change and also suspend *before* updating the pm state?
That is, shouldn't the suspend go after uart_change_pm()? And similar in
other places.
> @@ -2050,6 +2131,7 @@ uart_set_options(struct uart_port *port, struct console *co,
> {
> struct ktermios termios;
> static struct ktermios dummy;
> + int ret;
>
> /*
> * Ensure that the serial-console lock is initialised early.
> @@ -2089,7 +2171,17 @@ uart_set_options(struct uart_port *port, struct console *co,
> */
> port->mctrl |= TIOCM_DTR;
>
> - port->ops->set_termios(port, &termios, &dummy);
> + /* At early stage device is not created yet, we can't do PM */
> + if (port->dev) {
Checking port->dev here looks a bit hacky.
Can you expand on this and also on how this relates to console ports
presumably never being runtime suspended?
> + ret = serial_pm_resume_and_get(port->dev);
> + if (ret < 0)
> + return ret;
> + port->ops->set_termios(port, &termios, &dummy);
> + serial_pm_autosuspend(port->dev);
> + } else {
> + port->ops->set_termios(port, &termios, &dummy);
> + }
> +
> /*
> * Allow the setting of the UART parameters with a NULL console
> * too:
> @@ -2143,6 +2235,7 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
> struct tty_port *port = &state->port;
> struct device *tty_dev;
> struct uart_match match = {uport, drv};
> + int err;
>
> mutex_lock(&port->mutex);
>
> @@ -2168,11 +2261,15 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
> tty_port_set_suspended(port, 1);
> tty_port_set_initialized(port, 0);
>
> + err = serial_pm_resume_and_get(uport->dev);
> + if (err < 0)
> + goto unlock;
> spin_lock_irq(&uport->lock);
> ops->stop_tx(uport);
> ops->set_mctrl(uport, 0);
> ops->stop_rx(uport);
> spin_unlock_irq(&uport->lock);
> + serial_pm_autosuspend(uport->dev);
Just keep the device active until after ->shutdown() below.
> /*
> * Wait for the transmitter to empty.
> @@ -2183,7 +2280,11 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
> dev_err(uport->dev, "%s: Unable to drain transmitter\n",
> uport->name);
Note that there's a call to tx_empty() just above which also requires
the device to be resumed.
This is also missing in wait_until_sent().
>
> + err = serial_pm_resume_and_get(uport->dev);
> + if (err < 0)
> + goto unlock;
> ops->shutdown(uport);
> + serial_pm_autosuspend(uport->dev);
> }
>
> /*
> @@ -2206,6 +2307,7 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
> struct device *tty_dev;
> struct uart_match match = {uport, drv};
> struct ktermios termios;
> + int ret;
>
> mutex_lock(&port->mutex);
>
> @@ -2236,33 +2338,50 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
> if (port->tty && termios.c_cflag == 0)
> termios = port->tty->termios;
>
> + ret = serial_pm_resume_and_get(uport->dev);
> + if (ret < 0)
> + goto unlock;
> if (console_suspend_enabled)
> uart_change_pm(state, UART_PM_STATE_ON);
> uport->ops->set_termios(uport, &termios, NULL);
> + serial_pm_autosuspend(uport->dev);
> +
> if (console_suspend_enabled)
> console_start(uport->cons);
> }
>
> if (tty_port_suspended(port)) {
> const struct uart_ops *ops = uport->ops;
> - int ret;
>
> + ret = serial_pm_resume_and_get(uport->dev);
> + if (ret < 0)
> + goto unlock;
> uart_change_pm(state, UART_PM_STATE_ON);
> spin_lock_irq(&uport->lock);
> ops->set_mctrl(uport, 0);
> spin_unlock_irq(&uport->lock);
> + serial_pm_autosuspend(uport->dev);
Similar here, just keep the device active until you're done.
> +
> if (console_suspend_enabled || !uart_console(uport)) {
> /* Protected by port mutex for now */
> struct tty_struct *tty = port->tty;
>
> + ret = serial_pm_resume_and_get(uport->dev);
> + if (ret < 0)
> + goto unlock;
> ret = ops->startup(uport);
> + serial_pm_autosuspend(uport->dev);
Ditto.
> if (ret == 0) {
> if (tty)
> uart_change_speed(tty, state, NULL);
> + ret = serial_pm_resume_and_get(uport->dev);
> + if (ret < 0)
> + goto unlock;
> spin_lock_irq(&uport->lock);
> ops->set_mctrl(uport, uport->mctrl);
> ops->start_tx(uport);
> spin_unlock_irq(&uport->lock);
> + serial_pm_autosuspend(uport->dev);
> tty_port_set_initialized(port, 1);
> } else {
> /*
> @@ -2276,10 +2395,10 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
>
> tty_port_set_suspended(port, 0);
> }
> -
> +unlock:
> mutex_unlock(&port->mutex);
>
> - return 0;
> + return ret;
> }
>
> static inline void
> @@ -2329,6 +2448,7 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
> struct uart_port *port)
> {
> unsigned int flags;
> + int err;
>
> /*
> * If there isn't a port here, don't do anything further.
> @@ -2356,6 +2476,10 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
>
> uart_report_port(drv, port);
>
> + err = serial_pm_resume_and_get(port->dev);
> + if (err < 0)
> + return;
> +
> /* Power up port for set_mctrl() */
> uart_change_pm(state, UART_PM_STATE_ON);
>
> @@ -2367,6 +2491,7 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
> spin_lock_irqsave(&port->lock, flags);
> port->ops->set_mctrl(port, port->mctrl & TIOCM_DTR);
> spin_unlock_irqrestore(&port->lock, flags);
> + serial_pm_autosuspend(port->dev);
>
> /*
> * If this driver supports console, and it hasn't been
> @@ -3084,11 +3209,16 @@ EXPORT_SYMBOL_GPL(uart_handle_dcd_change);
> */
> void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
> {
> + int err;
> +
> lockdep_assert_held_once(&uport->lock);
>
> uport->icount.cts++;
>
> if (uart_softcts_mode(uport)) {
> + err = serial_pm_resume_and_get(uport->dev);
This won't fly as this callback is called in atomic context so you
cannot do a synchronous resume here.
> + if (err < 0)
> + return;
> if (uport->hw_stopped) {
> if (status) {
> uport->hw_stopped = 0;
> @@ -3101,7 +3231,7 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
> uport->ops->stop_tx(uport);
> }
> }
> -
> + serial_pm_autosuspend(uport->dev);
> }
> }
> EXPORT_SYMBOL_GPL(uart_handle_cts_change);
Johan
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
> <[email protected]>.
>
> Suggested-by: Johan Hovold <[email protected]>
> Signed-off-by: Tony Lindgren <[email protected]>
> ---
> 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
On Mon, Nov 15, 2021 at 10:41:59AM +0200, Tony Lindgren wrote:
> From: Andy Shevchenko <[email protected]>
>
> 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 <[email protected]>
> [[email protected]: use port->runtime_suspended]
> Signed-off-by: Tony Lindgren <[email protected]>
> ---
> 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
On Mon, Nov 15, 2021 at 10:42:00AM +0200, Tony Lindgren wrote:
> 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 <[email protected]>
> ---
> 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);
>
> @@ -2509,6 +2509,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 (!pm_runtime_enabled(dev))
> + 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;
> +}
Why can't this be done in core?
> +
> /* Nuvoton NPCM UARTs have a custom divisor calculation */
> static unsigned int npcm_get_divisor(struct uart_8250_port *up,
> unsigned int baud)
> @@ -3237,6 +3273,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,
Johan
On Mon, Nov 15, 2021 at 10:42:01AM +0200, Tony Lindgren wrote:
> 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 <[email protected]>
> ---
> 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);
Note that you still have synchronous resumes here despite this function
being called in atomic context. It didn't look like these were removed
by the next patch that drops irq_safe.
> @@ -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_noresume(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_noidle(port->dev);
> +
> pm_runtime_mark_last_busy(port->dev);
> pm_runtime_put_autosuspend(port->dev);
> free_irq(port->irq, port);
Johan
On Mon, Nov 15, 2021 at 10:42:02AM +0200, Tony Lindgren wrote:
> 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.
Looks like this commit message is outdated. These functions no longer
exist.
> 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.
>
> Signed-off-by: Tony Lindgren <[email protected]>
Johan
On Mon, Nov 15, 2021 at 10:42:03AM +0200, Tony Lindgren wrote:
> From: Andy Shevchenko <[email protected]>
>
> Since we now have runtime PM calls in serial_core.c the individual
> drivers do not need them anymore for the struct uart_ops related
> functions.
>
> Remove runtime PM calls in 8250 driver. This still leaves the flag for
> UART_CAP_RPM for serial8250_rpm_get_tx(), serial8250_rpm_put_tx() and
> serial8250_wakeup() to manage the reference count for serial TX.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> [[email protected]: updated to remove the exported functions too]
> Signed-off-by: Tony Lindgren <[email protected]>
> @@ -1477,8 +1459,11 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
> stop_tx_timer);
> struct uart_8250_port *p = em485->port;
> unsigned long flags;
> + int err;
>
> - serial8250_rpm_get(p);
> + err = pm_runtime_resume_and_get(p->port.dev);
This also won't work as this function is called on timer expiry; you
cannot do a synchronous resume here.
> + if (err < 0)
> + goto out_rpm_err;
> spin_lock_irqsave(&p->port.lock, flags);
> if (em485->active_timer == &em485->stop_tx_timer) {
> p->rs485_stop_tx(p);
> @@ -1486,8 +1471,9 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
> em485->tx_stopped = true;
> }
> spin_unlock_irqrestore(&p->port.lock, flags);
> - serial8250_rpm_put(p);
> -
> + pm_runtime_mark_last_busy(p->port.dev);
> + pm_runtime_put_autosuspend(p->port.dev);
> +out_rpm_err:
> return HRTIMER_NORESTART;
> }
> void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
> @@ -1984,15 +1966,11 @@ static unsigned int serial8250_tx_empty(struct uart_port *port)
> unsigned long flags;
> unsigned int lsr;
>
> - serial8250_rpm_get(up);
> -
> spin_lock_irqsave(&port->lock, flags);
> lsr = serial_port_in(port, UART_LSR);
> up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
> spin_unlock_irqrestore(&port->lock, flags);
>
> - serial8250_rpm_put(up);
> -
> return (lsr & BOTH_EMPTY) == BOTH_EMPTY ? TIOCSER_TEMT : 0;
> }
As I mentioned elsewhere, the corresponding get+put is missing in serial
core now.
> static void serial8250_put_poll_char(struct uart_port *port,
> unsigned char c)
> {
> unsigned int ier;
> struct uart_8250_port *up = up_to_u8250p(port);
>
> - serial8250_rpm_get(up);
> /*
> * First save the IER then disable the interrupts
> */
> @@ -2155,7 +2117,6 @@ static void serial8250_put_poll_char(struct uart_port *port,
> */
> wait_for_xmitr(up, BOTH_EMPTY);
> serial_port_out(port, UART_IER, ier);
> - serial8250_rpm_put(up);
> }
And this is a console callback; where is it guaranteed that the console
is never suspended?
Johan
Hi,
* Johan Hovold <[email protected]> [211130 10:29]:
> On Mon, Nov 15, 2021 at 10:41:57AM +0200, Tony Lindgren wrote:
> > From: Andy Shevchenko <[email protected]>
> >
> > 8250 driver has wrong implementation of runtime power management, i.e.
> > it uses an irq_safe flag. The irq_safe flag takes a permanent usage count
> > on the parent device preventing the parent from idling. This patch
> > prepares for making runtime power management generic by adding runtime PM
> > calls to serial core once for all UART drivers.
> >
> > As we have serial drivers that do not enable runtime PM, and drivers that
> > enable runtime PM, we add new functions for serial_pm_resume_and_get() and
> > serial_pm_autosuspend() functions to handle errors and allow the use also
> > for cases when runtime PM is not enabled. The other option considered was
> > to not check for runtime PM enable errors. But some CPUs can hang when the
> > clocks are not enabled for the device, so ignoring the errors is not a good
> > option. Eventually with the serial port drivers updated, we should be able
> > to just switch to using the standard runtime PM calls with no need for the
> > wrapper functions.
>
> A third option which needs to be considered is to always enable runtime
> pm in core but to keep the ports active while they are opened unless a
> driver opts in for more aggressive power management. This is how USB
> devices are handled for example.
>
> A next step could then be to move over uart_change_pm() to be handled
> from the pm callbacks.
Yes that would be nice to do eventually :)
> > @@ -1824,12 +1901,16 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i)
> > }
> >
> > if (capable(CAP_SYS_ADMIN)) {
> > + err = serial_pm_resume_and_get(uport->dev);
> > + if (err < 0)
> > + goto out;
> > pm_state = state->pm_state;
> > if (pm_state != UART_PM_STATE_ON)
> > uart_change_pm(state, UART_PM_STATE_ON);
> > spin_lock_irq(&uport->lock);
> > status = uport->ops->get_mctrl(uport);
> > spin_unlock_irq(&uport->lock);
> > + serial_pm_autosuspend(uport->dev);
> > if (pm_state != UART_PM_STATE_ON)
> > uart_change_pm(state, pm_state);
>
> The interaction with uart_change_pm() looks inconsistent. Why resume
> before the state change and also suspend *before* updating the pm state?
Good point.
> That is, shouldn't the suspend go after uart_change_pm()? And similar in
> other places.
Yes agreed, runtime PM may disable the clock and shut down the UART so
should be done after uart_change_pm().
BTW, Andy has follow-up patches to also drop the old uart_pm in favor of
runtime PM :)
> > @@ -2050,6 +2131,7 @@ uart_set_options(struct uart_port *port, struct console *co,
> > {
> > struct ktermios termios;
> > static struct ktermios dummy;
> > + int ret;
> >
> > /*
> > * Ensure that the serial-console lock is initialised early.
> > @@ -2089,7 +2171,17 @@ uart_set_options(struct uart_port *port, struct console *co,
> > */
> > port->mctrl |= TIOCM_DTR;
> >
> > - port->ops->set_termios(port, &termios, &dummy);
> > + /* At early stage device is not created yet, we can't do PM */
> > + if (port->dev) {
>
> Checking port->dev here looks a bit hacky.
As this is kernel console related we may be able to just leave out the
runtime PM calls here, see the two commits below. Andy, do you have some
comments?
> Can you expand on this and also on how this relates to console ports
> presumably never being runtime suspended?
See the following two commits for kernel console handling:
bedb404e91bb ("serial: 8250_port: Don't use power management for kernel console")
a3cb39d258ef ("serial: core: Allow detach and attach serial device for console")
Thanks for looking through the patches again, I'll take a look at all
your comments and will repost after the merge window.
Regards,
Tony
* Johan Hovold <[email protected]> [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
On Thu, Dec 09, 2021 at 09:29:44AM +0200, Tony Lindgren wrote:
> * Johan Hovold <[email protected]> [211130 10:29]:
> > On Mon, Nov 15, 2021 at 10:41:57AM +0200, Tony Lindgren wrote:
> > > From: Andy Shevchenko <[email protected]>
> > >
> > > 8250 driver has wrong implementation of runtime power management, i.e.
> > > it uses an irq_safe flag. The irq_safe flag takes a permanent usage count
> > > on the parent device preventing the parent from idling. This patch
> > > prepares for making runtime power management generic by adding runtime PM
> > > calls to serial core once for all UART drivers.
> > >
> > > As we have serial drivers that do not enable runtime PM, and drivers that
> > > enable runtime PM, we add new functions for serial_pm_resume_and_get() and
> > > serial_pm_autosuspend() functions to handle errors and allow the use also
> > > for cases when runtime PM is not enabled. The other option considered was
> > > to not check for runtime PM enable errors. But some CPUs can hang when the
> > > clocks are not enabled for the device, so ignoring the errors is not a good
> > > option. Eventually with the serial port drivers updated, we should be able
> > > to just switch to using the standard runtime PM calls with no need for the
> > > wrapper functions.
> >
> > A third option which needs to be considered is to always enable runtime
> > pm in core but to keep the ports active while they are opened unless a
> > driver opts in for more aggressive power management. This is how USB
> > devices are handled for example.
> >
> > A next step could then be to move over uart_change_pm() to be handled
> > from the pm callbacks.
>
> Yes that would be nice to do eventually :)
>
> > > @@ -1824,12 +1901,16 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i)
> > > }
> > >
> > > if (capable(CAP_SYS_ADMIN)) {
> > > + err = serial_pm_resume_and_get(uport->dev);
> > > + if (err < 0)
> > > + goto out;
> > > pm_state = state->pm_state;
> > > if (pm_state != UART_PM_STATE_ON)
> > > uart_change_pm(state, UART_PM_STATE_ON);
> > > spin_lock_irq(&uport->lock);
> > > status = uport->ops->get_mctrl(uport);
> > > spin_unlock_irq(&uport->lock);
> > > + serial_pm_autosuspend(uport->dev);
> > > if (pm_state != UART_PM_STATE_ON)
> > > uart_change_pm(state, pm_state);
> >
> > The interaction with uart_change_pm() looks inconsistent. Why resume
> > before the state change and also suspend *before* updating the pm state?
>
> Good point.
>
> > That is, shouldn't the suspend go after uart_change_pm()? And similar in
> > other places.
>
> Yes agreed, runtime PM may disable the clock and shut down the UART so
> should be done after uart_change_pm().
>
> BTW, Andy has follow-up patches to also drop the old uart_pm in favor of
> runtime PM :)
Yeah, but they are just removals without any proper replacements where the
current drivers already are using some kind of runtime PM. I was focused and
able to test only 8250 part.
> > > @@ -2050,6 +2131,7 @@ uart_set_options(struct uart_port *port, struct console *co,
> > > {
> > > struct ktermios termios;
> > > static struct ktermios dummy;
> > > + int ret;
> > >
> > > /*
> > > * Ensure that the serial-console lock is initialised early.
> > > @@ -2089,7 +2171,17 @@ uart_set_options(struct uart_port *port, struct console *co,
> > > */
> > > port->mctrl |= TIOCM_DTR;
> > >
> > > - port->ops->set_termios(port, &termios, &dummy);
> > > + /* At early stage device is not created yet, we can't do PM */
> > > + if (port->dev) {
> >
> > Checking port->dev here looks a bit hacky.
>
> As this is kernel console related we may be able to just leave out the
> runtime PM calls here, see the two commits below. Andy, do you have some
> comments?
IIRC the uart_set_options() can be called during early boot stages where no
device is present and we need to handle those cases, otherwise it will be
KABOOMs. I.o.w. we3 may not get rid of those checks (at least in a simple way).
> > Can you expand on this and also on how this relates to console ports
> > presumably never being runtime suspended?
>
> See the following two commits for kernel console handling:
>
> bedb404e91bb ("serial: 8250_port: Don't use power management for kernel console")
> a3cb39d258ef ("serial: core: Allow detach and attach serial device for console")
>
> Thanks for looking through the patches again, I'll take a look at all
> your comments and will repost after the merge window.
Thanks, Tony, for moving this forward and Johan for good review!
--
With Best Regards,
Andy Shevchenko
Hi,
* Andy Shevchenko <[email protected]> [211209 10:37]:
> On Thu, Dec 09, 2021 at 09:29:44AM +0200, Tony Lindgren wrote:
> > * Johan Hovold <[email protected]> [211130 10:29]:
> > > On Mon, Nov 15, 2021 at 10:41:57AM +0200, Tony Lindgren wrote:
> > > > From: Andy Shevchenko <[email protected]>
> > > >
> > > > 8250 driver has wrong implementation of runtime power management, i.e.
> > > > it uses an irq_safe flag. The irq_safe flag takes a permanent usage count
> > > > on the parent device preventing the parent from idling. This patch
> > > > prepares for making runtime power management generic by adding runtime PM
> > > > calls to serial core once for all UART drivers.
> > > >
> > > > As we have serial drivers that do not enable runtime PM, and drivers that
> > > > enable runtime PM, we add new functions for serial_pm_resume_and_get() and
> > > > serial_pm_autosuspend() functions to handle errors and allow the use also
> > > > for cases when runtime PM is not enabled. The other option considered was
> > > > to not check for runtime PM enable errors. But some CPUs can hang when the
> > > > clocks are not enabled for the device, so ignoring the errors is not a good
> > > > option. Eventually with the serial port drivers updated, we should be able
> > > > to just switch to using the standard runtime PM calls with no need for the
> > > > wrapper functions.
> > >
> > > A third option which needs to be considered is to always enable runtime
> > > pm in core but to keep the ports active while they are opened unless a
> > > driver opts in for more aggressive power management. This is how USB
> > > devices are handled for example.
> > >
> > > A next step could then be to move over uart_change_pm() to be handled
> > > from the pm callbacks.
> >
> > Yes that would be nice to do eventually :)
I think we should do the "third option" above as the first preparatory patch.
It can be done separately from the rest of the series, and we avoid adding
serial layer specific wrappers around runtime PM calls in the later patches.
Below is what I came up with for the preparatory patch, can you guys please
take a look and see if you have better ideas?
For system suspend and resume, it seems we don't need to do anything as
runtime PM is anyways disabled already in prepare.
Andy, care to give the following also a try for your serial port test
cases?
Regards,
Tony
8< --------------------
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -997,6 +997,7 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
uart->port.regshift = up->port.regshift;
uart->port.iotype = up->port.iotype;
uart->port.flags = up->port.flags | UPF_BOOT_AUTOCONF;
+ uart->port.supports_autosuspend = up->port.supports_autosuspend;
uart->bugs = up->bugs;
uart->port.mapbase = up->port.mapbase;
uart->port.mapsize = up->port.mapsize;
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
@@ -1352,6 +1352,7 @@ static int omap8250_probe(struct platform_device *pdev)
up.rs485_start_tx = serial8250_em485_start_tx;
up.rs485_stop_tx = serial8250_em485_stop_tx;
up.port.has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
+ up.port.supports_autosuspend = 1;
ret = of_alias_get_id(np, "serial");
if (ret < 0) {
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
@@ -16,6 +16,7 @@
#include <linux/console.h>
#include <linux/gpio/consumer.h>
#include <linux/of.h>
+#include <linux/pm_runtime.h>
#include <linux/proc_fs.h>
#include <linux/seq_file.h>
#include <linux/device.h>
@@ -184,6 +185,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
int init_hw)
{
struct uart_port *uport = uart_port_check(state);
+ struct device *dev = uport->dev;
unsigned long flags;
unsigned long page;
int retval = 0;
@@ -191,6 +193,32 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
if (uport->type == PORT_UNKNOWN)
return 1;
+ /* Always enable autosuspend and consider child devices for serdev */
+ pm_runtime_use_autosuspend(dev);
+ pm_suspend_ignore_children(dev, false);
+
+ /*
+ * If the port driver did not enable runtime PM in probe, do it now.
+ * Devices that did not enable runtime PM get set active so we can
+ * properly handler the returned errors for runtime PM calls.
+ */
+ if (!pm_runtime_enabled(dev)) {
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+ } else {
+ uport->implements_pm_runtime = 1;
+ }
+
+ /*
+ * Keep the port enabled unless autosuspend is supported.
+ * Released in uart_shutdown().
+ */
+ if (!uport->supports_autosuspend) {
+ retval = pm_runtime_resume_and_get(dev);
+ if (retval < 0)
+ return retval;
+ }
+
/*
* Make sure the device is in D0 state.
*/
@@ -279,6 +307,7 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
{
struct uart_port *uport = uart_port_check(state);
struct tty_port *port = &state->port;
+ struct device *dev = uport->dev;
unsigned long flags;
char *xmit_buf = NULL;
@@ -313,6 +342,19 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
*/
tty_port_set_suspended(port, 0);
+ /* Runtime PM paired with configuration done in uart_port_startup() */
+ if (uport) {
+ dev = uport->dev;
+ pm_runtime_dont_use_autosuspend(dev);
+ pm_suspend_ignore_children(dev, true);
+ if (!uport->supports_autosuspend)
+ pm_runtime_put_sync(dev);
+ if (!uport->implements_pm_runtime) {
+ pm_runtime_set_suspended(dev);
+ pm_runtime_disable(dev);
+ }
+ }
+
/*
* Do not free() the transmit buffer page under the port lock since
* this can create various circular locking scenarios. For instance,
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
@@ -249,6 +249,8 @@ struct uart_port {
unsigned char hub6; /* this should be in the 8250 driver */
unsigned char suspended;
unsigned char console_reinit;
+ unsigned long implements_pm_runtime:1;
+ unsigned long supports_autosuspend:1;
const char *name; /* port name */
struct attribute_group *attr_group; /* port specific attributes */
const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
--
2.34.1
First of all, my apology for the long delay in answering / commenting on this.
Second, Cc'ed to Ilpo.
On Tue, Jan 18, 2022 at 11:41:03AM +0200, Tony Lindgren wrote:
> * Andy Shevchenko <[email protected]> [211209 10:37]:
> > On Thu, Dec 09, 2021 at 09:29:44AM +0200, Tony Lindgren wrote:
> > > * Johan Hovold <[email protected]> [211130 10:29]:
> > > > On Mon, Nov 15, 2021 at 10:41:57AM +0200, Tony Lindgren wrote:
> > > > > From: Andy Shevchenko <[email protected]>
> > > > >
> > > > > 8250 driver has wrong implementation of runtime power management, i.e.
> > > > > it uses an irq_safe flag. The irq_safe flag takes a permanent usage count
> > > > > on the parent device preventing the parent from idling. This patch
> > > > > prepares for making runtime power management generic by adding runtime PM
> > > > > calls to serial core once for all UART drivers.
> > > > >
> > > > > As we have serial drivers that do not enable runtime PM, and drivers that
> > > > > enable runtime PM, we add new functions for serial_pm_resume_and_get() and
> > > > > serial_pm_autosuspend() functions to handle errors and allow the use also
> > > > > for cases when runtime PM is not enabled. The other option considered was
> > > > > to not check for runtime PM enable errors. But some CPUs can hang when the
> > > > > clocks are not enabled for the device, so ignoring the errors is not a good
> > > > > option. Eventually with the serial port drivers updated, we should be able
> > > > > to just switch to using the standard runtime PM calls with no need for the
> > > > > wrapper functions.
> > > >
> > > > A third option which needs to be considered is to always enable runtime
> > > > pm in core but to keep the ports active while they are opened unless a
> > > > driver opts in for more aggressive power management. This is how USB
> > > > devices are handled for example.
> > > >
> > > > A next step could then be to move over uart_change_pm() to be handled
> > > > from the pm callbacks.
> > >
> > > Yes that would be nice to do eventually :)
>
> I think we should do the "third option" above as the first preparatory patch.
> It can be done separately from the rest of the series, and we avoid adding
> serial layer specific wrappers around runtime PM calls in the later patches.
>
> Below is what I came up with for the preparatory patch, can you guys please
> take a look and see if you have better ideas?
>
> For system suspend and resume, it seems we don't need to do anything as
> runtime PM is anyways disabled already in prepare.
>
> Andy, care to give the following also a try for your serial port test
> cases?
I'm a bit off of the UART work nowadays, but luckily we have Ilpo who is
pretty much ramped up on the topic. Please, include him in all your work
WRT 8250 UART improvements. I hope Ilpo might have time to test the patch
mentioned in this message or give an idea how to do better, if possibly.
--
With Best Regards,
Andy Shevchenko
* Andy Shevchenko <[email protected]> [220411 10:37]:
> I'm a bit off of the UART work nowadays, but luckily we have Ilpo who is
> pretty much ramped up on the topic. Please, include him in all your work
> WRT 8250 UART improvements. I hope Ilpo might have time to test the patch
> mentioned in this message or give an idea how to do better, if possibly.
OK, I'll be sending out a preparatory patch that starts managing serial
port devices in addition to serial port with serial_core.c.
Regards,
Tony