linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/2] serial: core: Start managing serial controllers to enable runtime PM
@ 2022-11-23  8:28 Tony Lindgren
  2022-11-23  8:28 ` [PATCH v3 2/2] serial: core: Add port port device to flush TX on runtime resume Tony Lindgren
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Tony Lindgren @ 2022-11-23  8:28 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Andy Shevchenko, Ilpo Järvinen, Jiri Slaby, Johan Hovold,
	Sebastian Andrzej Siewior, Vignesh Raghavendra, linux-serial,
	linux-omap, linux-kernel

We want to enable runtime PM for serial port device drivers in a generic
way. To do this, we want to have the serial core layer manage the
registered physical serial controller devices.

The serial layer has a few challenges to deal with:

1. The serial port mapping to a physical serial port controller device
   is currently not easily available after the physical serial controller
   struct device gets registered uart_add_one_port() time

2. The serial port device drivers have their own driver data. So we cannot
   currently start making use of serial core generic data easily without
   changing all the serial port device drivers

To find the serial ports for a controller based on struct device, let's
add a new data structure for a serial_controller. On registering a port,
we can use the drv->state array to find the associated serial port
controller and initialize the serial core controller.

As some serial port device drivers enable runtime PM in their probe before
registering with the serial core layer, and some do not enable runtime PM
at all currently, we need check the state in the serial core layer on
uart_port_startup(). We need to also consider that a serial port device
may have multiple ports.

Initially we enable runtime PM for all the serial port controller devices.
This allows us to add runtime PM calls and properly handle any errors
without a need for serial layer specific runtime PM wrapper functions.

After this patch no functional changes for the serial port device drivers
are intended. We just enable runtime PM and keep the runtime PM usage
count until all the serial controller ports are unregistered. For drivers
implementing PM runtime, we just keep track of the runtime PM
configuration.

For the serial port drivers, the serial core layer has the following use
cases to deal with:

1. If a serial port device driver does not implement runtime PM, the
   device state is set to active state, and the runtime PM usage count
   is kept until the last port for a device is unregistered

2. If a serial port device driver implements runtime PM, the runtime PM
   usage count is kept until the last port for the device is unregistered

3. If a serial port device driver implements runtime PM autosuspend,
   autosuspend is not prevented. This currently gets set only for the
   8250_omap driver to keep runtime PM working for it

For system suspend, things should be mostly detached from the runtime PM.
The serial port device drivers may call pm_runtime_force_suspend() and
pm_runtime_force_resume() as needed.

Suggested-by: Johan Hovold <johan@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
Changes since v2:

- Make each serial port a proper device as suggested by Greg. This is
  a separate patch that flushes the TX on runtime PM resume

Changes since v1:

- Use kref as suggested by Andy

- Fix memory leak on error as noted by Andy

- Use use unsigned char for supports_autosuspend as suggested by Andy

- Coding style improvments as suggested by Andy

---
 drivers/tty/serial/8250/8250_core.c |   1 +
 drivers/tty/serial/8250/8250_omap.c |   1 +
 drivers/tty/serial/serial_core.c    | 179 ++++++++++++++++++++++++++++
 include/linux/serial_core.h         |   4 +
 4 files changed, 185 insertions(+)

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
@@ -1004,6 +1004,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
@@ -1330,6 +1330,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>
@@ -30,6 +31,20 @@
 #include <linux/irq.h>
 #include <linux/uaccess.h>
 
+/*
+ * Serial port device specific data for serial core.
+ *
+ * Each port device can have multiple ports with struct uart_state allocated
+ * for each port. The array of ports is kept in struct uart_driver.
+ */
+struct serial_controller {
+	struct device *dev;			/* Physical controller device */
+	struct uart_driver *drv;		/* For port specific uart_state */
+	struct kref ref;			/* Registered port count */
+	unsigned long implements_pm_runtime:1;
+	unsigned long supports_autosuspend:1;
+};
+
 /*
  * This is used to lock changes in serial line configuration.
  */
@@ -177,6 +192,162 @@ static void uart_port_dtr_rts(struct uart_port *uport, int raise)
 		uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
 }
 
+static struct serial_controller *to_controller(struct uart_port *port)
+{
+	if (!port->dev)
+		return NULL;
+
+	return port->state->controller;
+}
+
+/*
+ * Starts runtime PM for the serial controller device if not already started
+ * by the serial port driver. Called from uart_add_one_port() with port_mutex
+ * held.
+ */
+static int serial_core_pm_runtime_start(struct uart_port *port)
+{
+	struct serial_controller *controller = to_controller(port);
+	struct device *dev = port->dev;
+	int ret = 0;
+
+	if (kref_get_unless_zero(&controller->ref))
+		return 0;
+
+	/* Init controller device on first reference */
+	kref_init(&controller->ref);
+
+	/* 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 handle the returned errors for runtime PM calls.
+	 */
+	if (!pm_runtime_enabled(dev)) {
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+	} else {
+		controller->implements_pm_runtime = 1;
+	}
+
+	/*
+	 * Keep the port device enabled unless autosuspend is supported.
+	 * Released on port shutdown.
+	 */
+	if (!controller->supports_autosuspend) {
+		ret = pm_runtime_resume_and_get(dev);
+		if (ret < 0) {
+			pm_runtime_dont_use_autosuspend(dev);
+			pm_runtime_disable(dev);
+		}
+	}
+
+	return ret;
+}
+
+/* Clean up the runtime PM settings done on serial_core_register_port() */
+static void serial_core_pm_runtime_cleanup(struct kref *ref)
+{
+	struct serial_controller *controller =
+		 container_of(ref, struct serial_controller, ref);
+	struct device *dev = controller->dev;
+
+	pm_runtime_dont_use_autosuspend(dev);
+	pm_suspend_ignore_children(dev, true);
+	if (!controller->supports_autosuspend)
+		pm_runtime_put_sync(dev);
+	if (!controller->implements_pm_runtime) {
+		pm_runtime_set_suspended(dev);
+		pm_runtime_disable(dev);
+	}
+	kfree(controller);
+}
+
+/*
+ * Find the registered serial port controller if one exists. Caller must
+ * hold port_mutex.
+ */
+static struct serial_controller *
+serial_core_find_controller(struct uart_driver *drv, struct device *dev)
+{
+	struct uart_state *state;
+	int i;
+
+	for (i = 0; i < drv->nr; i++) {
+		state = drv->state + i;
+		if (!state->uart_port->dev)
+			continue;
+
+		if (state->uart_port->dev == dev)
+			return state->controller;
+	}
+
+	return NULL;
+}
+
+/*
+ * Initialize a serial port device and serial port controller as
+ * needed. Called from uart_add_one_port() with port_mutex held.
+ */
+static int serial_core_register_port(struct uart_port *port,
+				     struct uart_driver *drv)
+{
+	struct serial_controller *controller;
+	bool allocated = false;
+	int ret;
+
+	if (!port->dev)
+		return 0;
+
+	controller = serial_core_find_controller(drv, port->dev);
+	if (!controller) {
+		controller = kzalloc(sizeof(*controller), GFP_KERNEL);
+		if (!controller)
+			return -ENOMEM;
+
+		controller->drv = drv;
+		controller->dev = port->dev;
+		controller->supports_autosuspend = port->supports_autosuspend;
+		allocated = true;
+	}
+
+	port->state->controller = controller;
+	WARN_ON(port->supports_autosuspend != controller->supports_autosuspend);
+
+	ret = serial_core_pm_runtime_start(port);
+	if (ret < 0)
+		goto err_free;
+
+	return 0;
+
+err_free:
+	port->state->controller = NULL;
+	if (allocated)
+		kfree(controller);
+
+	return ret;
+}
+
+/*
+ * Removes a serial port device and the serial port controller if the last
+ * port instance. Called from uart_add_one_port() error path with port_mutex
+ * held.
+ */
+static void serial_core_unregister_port(struct uart_port *port)
+{
+	struct serial_controller *controller = to_controller(port);
+
+	/* Check for a registered controller, no struct device early on */
+	if (!controller)
+		return;
+
+	port->state->controller = NULL;
+	kref_put(&controller->ref, serial_core_pm_runtime_cleanup);
+}
+
 /*
  * Startup the port.  This will be called once per open.  All calls
  * will be serialised by the per-port mutex.
@@ -3082,6 +3253,10 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 		goto out;
 	}
 
+	ret = serial_core_register_port(uport, drv);
+	if (ret)
+		goto out;
+
 	/*
 	 * If this port is in use as a console then the spinlock is already
 	 * initialised.
@@ -3105,6 +3280,7 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 				    GFP_KERNEL);
 	if (!uport->tty_groups) {
 		ret = -ENOMEM;
+		serial_core_unregister_port(uport);
 		goto out;
 	}
 	uport->tty_groups[0] = &tty_dev_attr_group;
@@ -3173,6 +3349,9 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
 		goto out;
 	}
 	uport->flags |= UPF_DEAD;
+
+	serial_core_unregister_port(uport);
+
 	mutex_unlock(&port->mutex);
 
 	/*
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
@@ -28,6 +28,7 @@
 
 struct uart_port;
 struct serial_struct;
+struct serial_port_controller;
 struct device;
 struct gpio_desc;
 
@@ -573,6 +574,7 @@ struct uart_port {
 	unsigned char		hub6;			/* this should be in the 8250 driver */
 	unsigned char		suspended;
 	unsigned char		console_reinit;
+	unsigned char		supports_autosuspend;
 	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) */
@@ -611,6 +613,8 @@ enum uart_pm_state {
 struct uart_state {
 	struct tty_port		port;
 
+	struct serial_controller *controller;
+
 	enum uart_pm_state	pm_state;
 	struct circ_buf		xmit;
 
-- 
2.38.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3 2/2] serial: core: Add port port device to flush TX on runtime resume
  2022-11-23  8:28 [PATCH v3 1/2] serial: core: Start managing serial controllers to enable runtime PM Tony Lindgren
@ 2022-11-23  8:28 ` Tony Lindgren
  2022-11-23 18:37   ` Andy Shevchenko
  2022-11-23 18:41 ` [PATCH v3 1/2] serial: core: Start managing serial controllers to enable runtime PM Andy Shevchenko
  2022-11-25 14:02 ` Ilpo Järvinen
  2 siblings, 1 reply; 8+ messages in thread
From: Tony Lindgren @ 2022-11-23  8:28 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Andy Shevchenko, Ilpo Järvinen, Jiri Slaby, Johan Hovold,
	Sebastian Andrzej Siewior, Vignesh Raghavendra, linux-serial,
	linux-omap, linux-kernel

With PM runtime enabled for the serial port controllers, we can now flush
pending TX for the port on runtime PM resume as suggested by
Johan Hovold <johan@kernel.org>.

To flush the pending TX, let's set up each port as a proper device as
suggested by Greg Kroah-Hartman <gregkh@linuxfoundation.org>.

We set up each port as a child device for the serial port controller
device. We use platform device for this and pass the port information
in platform_data.

Let's just do mimimal changes needed for now, more port specific code
can be then moved from serial_core.c to serial_port.c as needed.

Suggested-by: Johan Hovold <johan@kernel.org>
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/Makefile      |   2 +-
 drivers/tty/serial/serial_core.c |  77 ++++++++++++++++++++-
 drivers/tty/serial/serial_port.c | 111 +++++++++++++++++++++++++++++++
 drivers/tty/serial/serial_port.h |  16 +++++
 include/linux/serial_core.h      |   1 +
 5 files changed, 204 insertions(+), 3 deletions(-)
 create mode 100644 drivers/tty/serial/serial_port.c
 create mode 100644 drivers/tty/serial/serial_port.h

diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -3,7 +3,7 @@
 # Makefile for the kernel serial device drivers.
 #
 
-obj-$(CONFIG_SERIAL_CORE) += serial_core.o
+obj-$(CONFIG_SERIAL_CORE) += serial_core.o serial_port.o
 
 obj-$(CONFIG_SERIAL_EARLYCON) += earlycon.o
 obj-$(CONFIG_SERIAL_EARLYCON_ARM_SEMIHOST) += earlycon-arm-semihost.o
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/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
@@ -31,6 +32,8 @@
 #include <linux/irq.h>
 #include <linux/uaccess.h>
 
+#include "serial_port.h"
+
 /*
  * Serial port device specific data for serial core.
  *
@@ -151,9 +154,31 @@ static void __uart_start(struct tty_struct *tty)
 {
 	struct uart_state *state = tty->driver_data;
 	struct uart_port *port = state->uart_port;
+	struct device *port_dev;
+	int err;
+
+	if (!port || uart_tx_stopped(port))
+		return;
 
-	if (port && !uart_tx_stopped(port))
+	port_dev = port->state->port_dev;
+
+	err = pm_runtime_get(port_dev);
+	if (err < 0) {
+		/* Something went wrong, attempt to start TX anyways */
+		port->ops->start_tx(port);
+		pm_runtime_put_noidle(port_dev);
+		return;
+	}
+
+	/*
+	 * Start TX if enabled, and kick runtime PM. Otherwise we must
+	 * wait for a retry. See also serial_port.c for runtime PM
+	 * autosuspend timeout.
+	 */
+	if (pm_runtime_active(port_dev))
 		port->ops->start_tx(port);
+	pm_runtime_mark_last_busy(port_dev);
+	pm_runtime_put_autosuspend(port_dev);
 }
 
 static void uart_start(struct tty_struct *tty)
@@ -288,6 +313,37 @@ serial_core_find_controller(struct uart_driver *drv, struct device *dev)
 	return NULL;
 }
 
+static int serial_core_add_port_device(struct uart_port *port)
+{
+	struct serial_port_platdata pd;
+	struct platform_device *pdev;
+	int ret;
+
+	pdev = platform_device_alloc("serial-port", PLATFORM_DEVID_AUTO);
+	if (!pdev)
+		return -ENOMEM;
+
+	pdev->dev.parent = port->dev;
+	pd.state = port->state;
+
+	ret = platform_device_add_data(pdev, &pd, sizeof(pd));
+	if (ret)
+		goto err_put;
+
+	ret = platform_device_add(pdev);
+	if (ret)
+		goto err_put;
+
+	port->state->port_dev = &pdev->dev;
+
+	return 0;
+
+err_put:
+	platform_device_put(pdev);
+
+	return ret;
+}
+
 /*
  * Initialize a serial port device and serial port controller as
  * needed. Called from uart_add_one_port() with port_mutex held.
@@ -317,13 +373,21 @@ static int serial_core_register_port(struct uart_port *port,
 	port->state->controller = controller;
 	WARN_ON(port->supports_autosuspend != controller->supports_autosuspend);
 
+	ret = serial_core_add_port_device(port);
+	if (ret)
+		goto err_free;
+
 	ret = serial_core_pm_runtime_start(port);
 	if (ret < 0)
-		goto err_free;
+		goto err_del;
 
 	return 0;
 
+err_del:
+	platform_device_del(to_platform_device(port->state->port_dev));
+
 err_free:
+	port->state->port_dev = NULL;
 	port->state->controller = NULL;
 	if (allocated)
 		kfree(controller);
@@ -339,11 +403,14 @@ static int serial_core_register_port(struct uart_port *port,
 static void serial_core_unregister_port(struct uart_port *port)
 {
 	struct serial_controller *controller = to_controller(port);
+	struct device *dev = port->state->port_dev;
 
 	/* Check for a registered controller, no struct device early on */
 	if (!controller)
 		return;
 
+	platform_device_del(to_platform_device(dev));
+	port->state->port_dev = NULL;
 	port->state->controller = NULL;
 	kref_put(&controller->ref, serial_core_pm_runtime_cleanup);
 }
@@ -363,6 +430,10 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 	if (uport->type == PORT_UNKNOWN)
 		return 1;
 
+	retval = serial_port_get(state);
+	if (retval)
+		return retval;
+
 	/*
 	 * Make sure the device is in D0 state.
 	 */
@@ -2030,6 +2101,8 @@ static void uart_port_shutdown(struct tty_port *port)
 		/* Ensure that the IRQ handler isn't running on another CPU. */
 		synchronize_irq(uport->irq);
 	}
+
+	serial_port_put(state);
 }
 
 static int uart_carrier_raised(struct tty_port *port)
diff --git a/drivers/tty/serial/serial_port.c b/drivers/tty/serial/serial_port.c
new file mode 100644
--- /dev/null
+++ b/drivers/tty/serial/serial_port.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/*
+ * Serial port driver to provide port specific services for serial_core
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/serial_core.h>
+
+#include "serial_port.h"
+
+#define SERIAL_PORT_AUTOSUSPEND_DELAY_MS	500
+
+struct serial_port_data {
+	struct uart_state *state;
+};
+
+int serial_port_get(struct uart_state *state)
+{
+	if (!state)
+		return -ENODEV;
+
+	/* Prevent uart_port from unloading */
+	if (!state->port_dev || !state->port_dev->driver ||
+	    !try_module_get(state->port_dev->driver->owner))
+		return -ENODEV;
+
+	return 0;
+}
+EXPORT_SYMBOL(serial_port_get);
+
+void serial_port_put(struct uart_state *state)
+{
+	if (!state || !state->port_dev || !state->port_dev->driver)
+		return;
+
+	module_put(state->port_dev->driver->owner);
+}
+EXPORT_SYMBOL(serial_port_put);
+
+/* Only considers TX for now. Caller must take care of locking */
+static int __serial_port_busy(struct uart_port *port)
+{
+	return (!uart_tx_stopped(port) &&
+		uart_circ_chars_pending(&port->state->xmit));
+}
+
+static int serial_port_runtime_resume(struct device *dev)
+{
+	struct serial_port_data *ddata = dev_get_drvdata(dev);
+	struct uart_port *port = ddata->state->uart_port;
+	unsigned long flags;
+
+	/* Flush any pending TX for the port */
+	spin_lock_irqsave(&port->lock, flags);
+	if (__serial_port_busy(port))
+		port->ops->start_tx(port);
+	spin_unlock_irqrestore(&port->lock, flags);
+	pm_runtime_mark_last_busy(dev);
+
+	return 0;
+}
+
+static const struct dev_pm_ops serial_port_pm = {
+	SET_RUNTIME_PM_OPS(NULL, serial_port_runtime_resume, NULL)
+};
+
+static int serial_port_probe(struct platform_device *pdev)
+{
+	struct serial_port_platdata *pd = dev_get_platdata(&pdev->dev);
+	struct serial_port_data *ddata;
+
+	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	ddata->state = pd->state;
+	platform_set_drvdata(pdev, ddata);
+
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev,
+					 SERIAL_PORT_AUTOSUSPEND_DELAY_MS);
+	pm_runtime_use_autosuspend(&pdev->dev);
+
+	return 0;
+}
+
+static int serial_port_remove(struct platform_device *pdev)
+{
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static struct platform_driver serial_port_driver = {
+	.driver = {
+		.name = "serial-port",
+		.pm = &serial_port_pm,
+	},
+	.probe = serial_port_probe,
+	.remove = serial_port_remove,
+};
+
+module_platform_driver(serial_port_driver);
+
+MODULE_AUTHOR("Tony Lindgren <tony@atomide.com>");
+MODULE_DESCRIPTION("Serial controller port driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/serial_port.h b/drivers/tty/serial/serial_port.h
new file mode 100644
--- /dev/null
+++ b/drivers/tty/serial/serial_port.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+/**
+ * struct serial_port_platdata - Serial port platform data
+ * @state: serial port state
+ *
+ * Used by serial_core and serial_port only. Allocated on uart_add_one_port(),
+ * and freed on uart_remove_one_port(). Note that the life cycle for uart_state
+ * is different from serial_port_device.
+ */
+struct serial_port_platdata {
+	struct uart_state *state;
+};
+
+extern int serial_port_get(struct uart_state *state);
+extern void serial_port_put(struct uart_state *state);
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
@@ -614,6 +614,7 @@ struct uart_state {
 	struct tty_port		port;
 
 	struct serial_controller *controller;
+	struct device		*port_dev;
 
 	enum uart_pm_state	pm_state;
 	struct circ_buf		xmit;
-- 
2.38.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/2] serial: core: Add port port device to flush TX on runtime resume
  2022-11-23  8:28 ` [PATCH v3 2/2] serial: core: Add port port device to flush TX on runtime resume Tony Lindgren
@ 2022-11-23 18:37   ` Andy Shevchenko
  2022-11-24  6:50     ` Tony Lindgren
  0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2022-11-23 18:37 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Greg Kroah-Hartman, Ilpo Järvinen, Jiri Slaby, Johan Hovold,
	Sebastian Andrzej Siewior, Vignesh Raghavendra, linux-serial,
	linux-omap, linux-kernel

On Wed, Nov 23, 2022 at 10:28:25AM +0200, Tony Lindgren wrote:
> With PM runtime enabled for the serial port controllers, we can now flush
> pending TX for the port on runtime PM resume as suggested by
> Johan Hovold <johan@kernel.org>.

I believe it's quite a duplication of email addresses (they are in Cc and will
be on lore.kernel.org) and also below.

> To flush the pending TX, let's set up each port as a proper device as
> suggested by Greg Kroah-Hartman <gregkh@linuxfoundation.org>.
> 
> We set up each port as a child device for the serial port controller
> device. We use platform device for this and pass the port information
> in platform_data.
> 
> Let's just do mimimal changes needed for now, more port specific code
> can be then moved from serial_core.c to serial_port.c as needed.

...

> +static int serial_core_add_port_device(struct uart_port *port)
> +{
> +	struct serial_port_platdata pd;
> +	struct platform_device *pdev;
> +	int ret;
> +
> +	pdev = platform_device_alloc("serial-port", PLATFORM_DEVID_AUTO);
> +	if (!pdev)
> +		return -ENOMEM;
> +
> +	pdev->dev.parent = port->dev;
> +	pd.state = port->state;
> +
> +	ret = platform_device_add_data(pdev, &pd, sizeof(pd));
> +	if (ret)
> +		goto err_put;
> +
> +	ret = platform_device_add(pdev);
> +	if (ret)
> +		goto err_put;
> +
> +	port->state->port_dev = &pdev->dev;
> +
> +	return 0;
> +
> +err_put:
> +	platform_device_put(pdev);
> +
> +	return ret;

Can we simply utilize platform_device_register_full()?

> +}

...

> +EXPORT_SYMBOL(serial_port_get);

Can we move these to namespace from day 1?

...

> +	return (!uart_tx_stopped(port) &&
> +		uart_circ_chars_pending(&port->state->xmit));

Outer parentheses are redundant.

...

> +static const struct dev_pm_ops serial_port_pm = {
> +	SET_RUNTIME_PM_OPS(NULL, serial_port_runtime_resume, NULL)
> +};

Can't we use new macros / helpers which starts from DEFINE_...

...

> +	pm_runtime_set_autosuspend_delay(&pdev->dev,
> +					 SERIAL_PORT_AUTOSUSPEND_DELAY_MS);

With temporary

	struct device *dev = &pdev->dev;

this in particular become one line.

...

> +		.pm = &serial_port_pm,

As per above, use pm_ptr() ?

...

> +

Unneeded blank line.

> +module_platform_driver(serial_port_driver);

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 1/2] serial: core: Start managing serial controllers to enable runtime PM
  2022-11-23  8:28 [PATCH v3 1/2] serial: core: Start managing serial controllers to enable runtime PM Tony Lindgren
  2022-11-23  8:28 ` [PATCH v3 2/2] serial: core: Add port port device to flush TX on runtime resume Tony Lindgren
@ 2022-11-23 18:41 ` Andy Shevchenko
  2022-11-25 14:02 ` Ilpo Järvinen
  2 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2022-11-23 18:41 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Greg Kroah-Hartman, Ilpo Järvinen, Jiri Slaby, Johan Hovold,
	Sebastian Andrzej Siewior, Vignesh Raghavendra, linux-serial,
	linux-omap, linux-kernel

On Wed, Nov 23, 2022 at 10:28:24AM +0200, Tony Lindgren wrote:
> We want to enable runtime PM for serial port device drivers in a generic
> way. To do this, we want to have the serial core layer manage the
> registered physical serial controller devices.
> 
> The serial layer has a few challenges to deal with:
> 
> 1. The serial port mapping to a physical serial port controller device
>    is currently not easily available after the physical serial controller
>    struct device gets registered uart_add_one_port() time
> 
> 2. The serial port device drivers have their own driver data. So we cannot
>    currently start making use of serial core generic data easily without
>    changing all the serial port device drivers
> 
> To find the serial ports for a controller based on struct device, let's
> add a new data structure for a serial_controller. On registering a port,
> we can use the drv->state array to find the associated serial port
> controller and initialize the serial core controller.
> 
> As some serial port device drivers enable runtime PM in their probe before
> registering with the serial core layer, and some do not enable runtime PM
> at all currently, we need check the state in the serial core layer on
> uart_port_startup(). We need to also consider that a serial port device
> may have multiple ports.
> 
> Initially we enable runtime PM for all the serial port controller devices.
> This allows us to add runtime PM calls and properly handle any errors
> without a need for serial layer specific runtime PM wrapper functions.
> 
> After this patch no functional changes for the serial port device drivers
> are intended. We just enable runtime PM and keep the runtime PM usage
> count until all the serial controller ports are unregistered. For drivers
> implementing PM runtime, we just keep track of the runtime PM
> configuration.
> 
> For the serial port drivers, the serial core layer has the following use
> cases to deal with:
> 
> 1. If a serial port device driver does not implement runtime PM, the
>    device state is set to active state, and the runtime PM usage count
>    is kept until the last port for a device is unregistered
> 
> 2. If a serial port device driver implements runtime PM, the runtime PM
>    usage count is kept until the last port for the device is unregistered
> 
> 3. If a serial port device driver implements runtime PM autosuspend,
>    autosuspend is not prevented. This currently gets set only for the
>    8250_omap driver to keep runtime PM working for it
> 
> For system suspend, things should be mostly detached from the runtime PM.
> The serial port device drivers may call pm_runtime_force_suspend() and
> pm_runtime_force_resume() as needed.

...

> +static struct serial_controller *to_controller(struct uart_port *port)

Perhaps to_serial_controller()? Or to_serial_ctrl() ?

> +{
> +	if (!port->dev)
> +		return NULL;
> +
> +	return port->state->controller;
> +}

...

> +/*
> + * Starts runtime PM for the serial controller device if not already started
> + * by the serial port driver. Called from uart_add_one_port() with port_mutex
> + * held.

Perhaps we may utilize lockdep_assert() here?

> + */

Also maybe might_sleep() can be added where it's appropriate?

...

> +static int serial_core_register_port(struct uart_port *port,
> +				     struct uart_driver *drv)
> +{
> +	struct serial_controller *controller;
> +	bool allocated = false;
> +	int ret;
> +
> +	if (!port->dev)
> +		return 0;
> +
> +	controller = serial_core_find_controller(drv, port->dev);
> +	if (!controller) {

> +		controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> +		if (!controller)
> +			return -ENOMEM;
> +
> +		controller->drv = drv;
> +		controller->dev = port->dev;
> +		controller->supports_autosuspend = port->supports_autosuspend;
> +		allocated = true;

Maybe split this to a serial_core_controller_alloc()?

> +	}
> +
> +	port->state->controller = controller;
> +	WARN_ON(port->supports_autosuspend != controller->supports_autosuspend);
> +
> +	ret = serial_core_pm_runtime_start(port);
> +	if (ret < 0)
> +		goto err_free;
> +
> +	return 0;
> +
> +err_free:
> +	port->state->controller = NULL;
> +	if (allocated)
> +		kfree(controller);
> +
> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/2] serial: core: Add port port device to flush TX on runtime resume
  2022-11-23 18:37   ` Andy Shevchenko
@ 2022-11-24  6:50     ` Tony Lindgren
  2022-11-24  8:41       ` Andy Shevchenko
  0 siblings, 1 reply; 8+ messages in thread
From: Tony Lindgren @ 2022-11-24  6:50 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Greg Kroah-Hartman, Ilpo Järvinen, Jiri Slaby, Johan Hovold,
	Sebastian Andrzej Siewior, Vignesh Raghavendra, linux-serial,
	linux-omap, linux-kernel

Hi,

Thanks for your review again, will fix what you noted. One idea
for an improvment below though.

* Andy Shevchenko <andriy.shevchenko@intel.com> [221123 18:37]:
> On Wed, Nov 23, 2022 at 10:28:25AM +0200, Tony Lindgren wrote:
> > +EXPORT_SYMBOL(serial_port_get);
> 
> Can we move these to namespace from day 1?

Assuming you mean EXPORT_SYMBOL_NS(), sure.

But we might be better off doing the following:

- Move already exported uart_add_one_port() and uart_remove_one_port()
  from serial_core to serial_port as wrapper functions for serial_core

- Export new functions in serial_core for serial_core_register_port()
  and serial_core_unregister_port() for serial_port to call

This would ensure both serial_core and serial_port modules are
always loaded when a hardware specific serial port driver is
loaded.

This should also leave out the need for serial_port_get() and
serial_port_put().

Regards,

Tony


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/2] serial: core: Add port port device to flush TX on runtime resume
  2022-11-24  6:50     ` Tony Lindgren
@ 2022-11-24  8:41       ` Andy Shevchenko
  0 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2022-11-24  8:41 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Greg Kroah-Hartman, Ilpo Järvinen, Jiri Slaby, Johan Hovold,
	Sebastian Andrzej Siewior, Vignesh Raghavendra, linux-serial,
	linux-omap, linux-kernel

On Thu, Nov 24, 2022 at 08:50:41AM +0200, Tony Lindgren wrote:
> * Andy Shevchenko <andriy.shevchenko@intel.com> [221123 18:37]:
> > On Wed, Nov 23, 2022 at 10:28:25AM +0200, Tony Lindgren wrote:
> > > +EXPORT_SYMBOL(serial_port_get);
> > 
> > Can we move these to namespace from day 1?
> 
> Assuming you mean EXPORT_SYMBOL_NS(), sure.
> 
> But we might be better off doing the following:
> 
> - Move already exported uart_add_one_port() and uart_remove_one_port()
>   from serial_core to serial_port as wrapper functions for serial_core
> 
> - Export new functions in serial_core for serial_core_register_port()
>   and serial_core_unregister_port() for serial_port to call
> 
> This would ensure both serial_core and serial_port modules are
> always loaded when a hardware specific serial port driver is
> loaded.
> 
> This should also leave out the need for serial_port_get() and
> serial_port_put().

Yes, this is good idea!

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 1/2] serial: core: Start managing serial controllers to enable runtime PM
  2022-11-23  8:28 [PATCH v3 1/2] serial: core: Start managing serial controllers to enable runtime PM Tony Lindgren
  2022-11-23  8:28 ` [PATCH v3 2/2] serial: core: Add port port device to flush TX on runtime resume Tony Lindgren
  2022-11-23 18:41 ` [PATCH v3 1/2] serial: core: Start managing serial controllers to enable runtime PM Andy Shevchenko
@ 2022-11-25 14:02 ` Ilpo Järvinen
  2022-11-28  6:45   ` Tony Lindgren
  2 siblings, 1 reply; 8+ messages in thread
From: Ilpo Järvinen @ 2022-11-25 14:02 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Greg Kroah-Hartman, Andy Shevchenko, Jiri Slaby, Johan Hovold,
	Sebastian Andrzej Siewior, Vignesh Raghavendra, linux-serial,
	linux-omap, LKML

On Wed, 23 Nov 2022, Tony Lindgren wrote:

> We want to enable runtime PM for serial port device drivers in a generic
> way. To do this, we want to have the serial core layer manage the
> registered physical serial controller devices.
> 
> The serial layer has a few challenges to deal with:
> 
> 1. The serial port mapping to a physical serial port controller device
>    is currently not easily available after the physical serial controller
>    struct device gets registered uart_add_one_port() time
> 
> 2. The serial port device drivers have their own driver data. So we cannot
>    currently start making use of serial core generic data easily without
>    changing all the serial port device drivers
> 
> To find the serial ports for a controller based on struct device, let's
> add a new data structure for a serial_controller. On registering a port,
> we can use the drv->state array to find the associated serial port
> controller and initialize the serial core controller.
> 
> As some serial port device drivers enable runtime PM in their probe before
> registering with the serial core layer, and some do not enable runtime PM
> at all currently, we need check the state in the serial core layer on
> uart_port_startup(). We need to also consider that a serial port device
> may have multiple ports.
> 
> Initially we enable runtime PM for all the serial port controller devices.
> This allows us to add runtime PM calls and properly handle any errors
> without a need for serial layer specific runtime PM wrapper functions.
> 
> After this patch no functional changes for the serial port device drivers
> are intended. We just enable runtime PM and keep the runtime PM usage
> count until all the serial controller ports are unregistered. For drivers
> implementing PM runtime, we just keep track of the runtime PM
> configuration.
> 
> For the serial port drivers, the serial core layer has the following use
> cases to deal with:
> 
> 1. If a serial port device driver does not implement runtime PM, the
>    device state is set to active state, and the runtime PM usage count
>    is kept until the last port for a device is unregistered
> 
> 2. If a serial port device driver implements runtime PM, the runtime PM
>    usage count is kept until the last port for the device is unregistered
> 
> 3. If a serial port device driver implements runtime PM autosuspend,
>    autosuspend is not prevented. This currently gets set only for the
>    8250_omap driver to keep runtime PM working for it
> 
> For system suspend, things should be mostly detached from the runtime PM.
> The serial port device drivers may call pm_runtime_force_suspend() and
> pm_runtime_force_resume() as needed.
> 
> Suggested-by: Johan Hovold <johan@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> Changes since v2:
> 
> - Make each serial port a proper device as suggested by Greg. This is
>   a separate patch that flushes the TX on runtime PM resume
> 
> Changes since v1:
> 
> - Use kref as suggested by Andy
> 
> - Fix memory leak on error as noted by Andy
> 
> - Use use unsigned char for supports_autosuspend as suggested by Andy
> 
> - Coding style improvments as suggested by Andy
> 
> ---
>  drivers/tty/serial/8250/8250_core.c |   1 +
>  drivers/tty/serial/8250/8250_omap.c |   1 +
>  drivers/tty/serial/serial_core.c    | 179 ++++++++++++++++++++++++++++
>  include/linux/serial_core.h         |   4 +
>  4 files changed, 185 insertions(+)
> 
> 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
> @@ -1004,6 +1004,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
> @@ -1330,6 +1330,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>
> @@ -30,6 +31,20 @@
>  #include <linux/irq.h>
>  #include <linux/uaccess.h>
>  
> +/*
> + * Serial port device specific data for serial core.
> + *
> + * Each port device can have multiple ports with struct uart_state allocated
> + * for each port. The array of ports is kept in struct uart_driver.
> + */
> +struct serial_controller {
> +	struct device *dev;			/* Physical controller device */
> +	struct uart_driver *drv;		/* For port specific uart_state */
> +	struct kref ref;			/* Registered port count */
> +	unsigned long implements_pm_runtime:1;
> +	unsigned long supports_autosuspend:1;
> +};
> +
>  /*
>   * This is used to lock changes in serial line configuration.
>   */
> @@ -177,6 +192,162 @@ static void uart_port_dtr_rts(struct uart_port *uport, int raise)
>  		uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
>  }
>  
> +static struct serial_controller *to_controller(struct uart_port *port)
> +{
> +	if (!port->dev)
> +		return NULL;
> +
> +	return port->state->controller;
> +}
> +
> +/*
> + * Starts runtime PM for the serial controller device if not already started
> + * by the serial port driver. Called from uart_add_one_port() with port_mutex
> + * held.
> + */
> +static int serial_core_pm_runtime_start(struct uart_port *port)
> +{
> +	struct serial_controller *controller = to_controller(port);
> +	struct device *dev = port->dev;
> +	int ret = 0;
> +
> +	if (kref_get_unless_zero(&controller->ref))
> +		return 0;
> +
> +	/* Init controller device on first reference */
> +	kref_init(&controller->ref);
> +
> +	/* 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 handle the returned errors for runtime PM calls.
> +	 */
> +	if (!pm_runtime_enabled(dev)) {
> +		pm_runtime_set_active(dev);
> +		pm_runtime_enable(dev);
> +	} else {
> +		controller->implements_pm_runtime = 1;
> +	}
> +
> +	/*
> +	 * Keep the port device enabled unless autosuspend is supported.
> +	 * Released on port shutdown.
> +	 */
> +	if (!controller->supports_autosuspend) {
> +		ret = pm_runtime_resume_and_get(dev);

Should this be done regardless of autosuspend if the port is console?

There's a problem in the current place where this being called from 
though, uart_console_enabled() doesn't return the correct value with at 
least some devices this early:

https://lore.kernel.org/linux-serial/AS8PR04MB84047F39CD10C00CEE29213F92219@AS8PR04MB8404.eurprd04.prod.outlook.com/

-- 
 i.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 1/2] serial: core: Start managing serial controllers to enable runtime PM
  2022-11-25 14:02 ` Ilpo Järvinen
@ 2022-11-28  6:45   ` Tony Lindgren
  0 siblings, 0 replies; 8+ messages in thread
From: Tony Lindgren @ 2022-11-28  6:45 UTC (permalink / raw)
  To: Ilpo Järvinen
  Cc: Greg Kroah-Hartman, Andy Shevchenko, Jiri Slaby, Johan Hovold,
	Sebastian Andrzej Siewior, Vignesh Raghavendra, linux-serial,
	linux-omap, LKML

Hi,

* Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> [221125 14:02]:
> On Wed, 23 Nov 2022, Tony Lindgren wrote:
> > +	/*
> > +	 * Keep the port device enabled unless autosuspend is supported.
> > +	 * Released on port shutdown.
> > +	 */
> > +	if (!controller->supports_autosuspend) {
> > +		ret = pm_runtime_resume_and_get(dev);
> 
> Should this be done regardless of autosuspend if the port is console?

Well hopefully no need to check anything here for the console unless
enabled state needs to be inherited from the early console.

Note that with what Jiri is proposing, we can just unconditionally do the
pm_runtime_resume_and_get() here on the generic serial port controller
struct device. We can also leave out the tinkering of the serial port
hardware struct device.

The hardware serial port controller drivers that do support autosuspend
can just decrement the runtime PM usage count for the generic serial port
controller child device as needed to enable aggressive PM.

> There's a problem in the current place where this being called from 
> though, uart_console_enabled() doesn't return the correct value with at 
> least some devices this early:
> 
> https://lore.kernel.org/linux-serial/AS8PR04MB84047F39CD10C00CEE29213F92219@AS8PR04MB8404.eurprd04.prod.outlook.com/

OK. Seems the issues with the boot console and early serial port struct
device or whatever might be missing are there even without runtime PM.

Regards,

Tony

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-11-28  6:45 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-23  8:28 [PATCH v3 1/2] serial: core: Start managing serial controllers to enable runtime PM Tony Lindgren
2022-11-23  8:28 ` [PATCH v3 2/2] serial: core: Add port port device to flush TX on runtime resume Tony Lindgren
2022-11-23 18:37   ` Andy Shevchenko
2022-11-24  6:50     ` Tony Lindgren
2022-11-24  8:41       ` Andy Shevchenko
2022-11-23 18:41 ` [PATCH v3 1/2] serial: core: Start managing serial controllers to enable runtime PM Andy Shevchenko
2022-11-25 14:02 ` Ilpo Järvinen
2022-11-28  6:45   ` Tony Lindgren

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).