[v3,2/2] serial: core: Add port port device to flush TX on runtime resume
Commit Message
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
Comments
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);
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
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!
@@ -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
@@ -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)
new file mode 100644
@@ -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");
new file mode 100644
@@ -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);
@@ -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;