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 serial port controllers. For runtime PM, we need a way to find
the serial ports for each serial port controller device.
The serial core manages ports. Each serial controller can have multiple
ports. As serial core has no struct device, and the serial port device
drivers have their own driver data, we cannot currently start making
use of serial core generic data easily without changing all the serial
port device drivers.
We could consider adding a serial core specific struct device. It would
be a child of the serial port device, and would allow us eventually to use
device_links to add generic runtime PM calls for example. But as the serial
core layer is not a device driver, driver specific features would need to
be added, and are probably not justified for a virtual device.
Considering the above, let's improve the serial core layer so we can
manage the serial port controllers better. Let's register the controllers
with the serial core layer in addition to the serial ports.
To find the serial ports for a controller based on struct device, let's
add a new data structure for a serial_controller. Let's add the registered
devices into a radix_tree so we can look up the controller easily even
with many controllers registered. This allows us to keep track of the
runtime PM state for each serial port controller device.
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 just want to 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. For most cases, we just enable runtime PM and keep the
runtime PM usage count until all the serial controller ports are
unregistered. For drivers implementing runtime PM, we just keep track of
the configuration.
The serial core layer has the following use cases to deal with:
- 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
- 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
- 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 <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
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 | 148 ++++++++++++++++++++++++++++
include/linux/serial_core.h | 3 +
4 files changed, 153 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
@@ -995,6 +995,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
@@ -1338,6 +1338,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,7 +16,9 @@
#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/radix-tree.h>
#include <linux/seq_file.h>
#include <linux/device.h>
#include <linux/serial.h> /* for serial_state and serial_icounter_struct */
@@ -30,6 +32,25 @@
#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; /* Serial port device */
+ struct uart_driver *drv; /* For port specific uart_state */
+ struct kref ref; /* Enable count for runtime PM */
+ unsigned long implements_pm_runtime:1;
+ unsigned long supports_autosuspend:1;
+};
+
+/*
+ * Serial core port device instances. Update protected by port_mutex.
+ */
+static RADIX_TREE(serial_core_devices, GFP_NOWAIT);
+
/*
* This is used to lock changes in serial line configuration.
*/
@@ -175,6 +196,125 @@ static void uart_port_dtr_rts(struct uart_port *uport, int raise)
uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
}
+/* Called from uart_add_one_port() with port_mutex held */
+static int serial_core_pm_runtime_start(struct uart_port *port)
+{
+ struct uart_state *state = port->state;
+ struct serial_controller *controller = state->controller;
+ 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);
+ }
+}
+
+/* Called from uart_remove_one_port() and on error path with port_mutex held */
+static void serial_core_unregister_port(struct uart_port *port)
+{
+ unsigned long idx = (unsigned long)port->dev;
+ struct serial_controller *controller;
+
+ /* Check for a registered controller, no struct device early on */
+ controller = radix_tree_lookup(&serial_core_devices, idx);
+ if (!controller)
+ return;
+
+ kref_put(&controller->ref, serial_core_pm_runtime_cleanup);
+
+ controller = radix_tree_delete(&serial_core_devices, idx);
+ kfree(controller);
+}
+
+/* Called from uart_add_one_port() with port_mutex held */
+static int serial_core_register_port(struct uart_port *port,
+ struct uart_driver *drv)
+{
+ unsigned long idx = (unsigned long)port->dev;
+ struct serial_controller *controller;
+ int ret;
+
+ if (!idx)
+ return 0;
+
+ controller = radix_tree_lookup(&serial_core_devices, idx);
+ if (controller) {
+ port->state->controller = controller;
+ WARN_ON(port->supports_autosuspend != controller->supports_autosuspend);
+ return serial_core_pm_runtime_start(port);
+ }
+
+ controller = kzalloc(sizeof(*controller), GFP_KERNEL);
+ if (!controller)
+ return -ENOMEM;
+
+ controller->drv = drv;
+ controller->dev = port->dev;
+ controller->supports_autosuspend = port->supports_autosuspend;
+ port->state->controller = controller;
+
+ ret = radix_tree_insert(&serial_core_devices, idx, controller);
+ if (ret) {
+ kfree(controller);
+ return ret;
+ }
+
+ ret = serial_core_pm_runtime_start(port);
+ if (ret < 0)
+ serial_core_unregister_port(port);
+
+ return ret;
+}
+
/*
* Startup the port. This will be called once per open. All calls
* will be serialised by the per-port mutex.
@@ -2956,6 +3096,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.
@@ -2979,6 +3123,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;
@@ -3048,6 +3193,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
@@ -250,6 +250,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) */
@@ -285,6 +286,8 @@ enum uart_pm_state {
* This is the state information which is persistent across opens.
*/
struct uart_state {
+ struct serial_controller *controller;
+
struct tty_port port;
enum uart_pm_state pm_state;
--
2.36.1
On Wed, Jun 15, 2022 at 09:24:55AM +0300, 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 serial port controllers. For runtime PM, we need a way to find
> the serial ports for each serial port controller device.
>
> The serial core manages ports. Each serial controller can have multiple
> ports. As serial core has no struct device, and the serial port device
> drivers have their own driver data, we cannot currently start making
> use of serial core generic data easily without changing all the serial
> port device drivers.
>
> We could consider adding a serial core specific struct device. It would
> be a child of the serial port device, and would allow us eventually to use
> device_links to add generic runtime PM calls for example. But as the serial
> core layer is not a device driver, driver specific features would need to
> be added, and are probably not justified for a virtual device.
>
> Considering the above, let's improve the serial core layer so we can
> manage the serial port controllers better. Let's register the controllers
> with the serial core layer in addition to the serial ports.
>
> To find the serial ports for a controller based on struct device, let's
> add a new data structure for a serial_controller. Let's add the registered
> devices into a radix_tree so we can look up the controller easily even
> with many controllers registered. This allows us to keep track of the
> runtime PM state for each serial port controller device.
>
> 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 just want to 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. For most cases, we just enable runtime PM and keep the
> runtime PM usage count until all the serial controller ports are
> unregistered. For drivers implementing runtime PM, we just keep track of
> the configuration.
>
> The serial core layer has the following use cases to deal with:
>
> - 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
>
> - 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
>
> - 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.
LGTM,
Reviewed-by: Andy Shevchenko <[email protected]>
A couple of nit-picks below.
> Suggested-by: Johan Hovold <[email protected]>
> Signed-off-by: Tony Lindgren <[email protected]>
> ---
>
> 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 | 148 ++++++++++++++++++++++++++++
> include/linux/serial_core.h | 3 +
> 4 files changed, 153 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
> @@ -995,6 +995,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
> @@ -1338,6 +1338,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,7 +16,9 @@
> #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/radix-tree.h>
> #include <linux/seq_file.h>
> #include <linux/device.h>
> #include <linux/serial.h> /* for serial_state and serial_icounter_struct */
> @@ -30,6 +32,25 @@
> #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; /* Serial port device */
Serial port device is a bit unclear for non-prepared reader. Perhaps add
the word "physical" or another to specify the nature of the device (because
to me "serial port device" sounds like a duplication of something in struct
uart_port, but I have doubts).
> + struct uart_driver *drv; /* For port specific uart_state */
> + struct kref ref; /* Enable count for runtime PM */
> + unsigned long implements_pm_runtime:1;
> + unsigned long supports_autosuspend:1;
> +};
> +
> +/*
> + * Serial core port device instances. Update protected by port_mutex.
> + */
> +static RADIX_TREE(serial_core_devices, GFP_NOWAIT);
> +
> /*
> * This is used to lock changes in serial line configuration.
> */
> @@ -175,6 +196,125 @@ static void uart_port_dtr_rts(struct uart_port *uport, int raise)
> uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
> }
>
> +/* Called from uart_add_one_port() with port_mutex held */
> +static int serial_core_pm_runtime_start(struct uart_port *port)
> +{
> + struct uart_state *state = port->state;
> + struct serial_controller *controller = state->controller;
> + 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);
> + }
> +}
> +
> +/* Called from uart_remove_one_port() and on error path with port_mutex held */
> +static void serial_core_unregister_port(struct uart_port *port)
> +{
> + unsigned long idx = (unsigned long)port->dev;
> + struct serial_controller *controller;
> +
> + /* Check for a registered controller, no struct device early on */
> + controller = radix_tree_lookup(&serial_core_devices, idx);
> + if (!controller)
> + return;
> +
> + kref_put(&controller->ref, serial_core_pm_runtime_cleanup);
> +
> + controller = radix_tree_delete(&serial_core_devices, idx);
> + kfree(controller);
> +}
> +
> +/* Called from uart_add_one_port() with port_mutex held */
> +static int serial_core_register_port(struct uart_port *port,
> + struct uart_driver *drv)
> +{
> + unsigned long idx = (unsigned long)port->dev;
> + struct serial_controller *controller;
> + int ret;
> +
> + if (!idx)
> + return 0;
> +
> + controller = radix_tree_lookup(&serial_core_devices, idx);
> + if (controller) {
> + port->state->controller = controller;
> + WARN_ON(port->supports_autosuspend != controller->supports_autosuspend);
> + return serial_core_pm_runtime_start(port);
> + }
> +
> + controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> + if (!controller)
> + return -ENOMEM;
> +
> + controller->drv = drv;
> + controller->dev = port->dev;
> + controller->supports_autosuspend = port->supports_autosuspend;
> + port->state->controller = controller;
> +
> + ret = radix_tree_insert(&serial_core_devices, idx, controller);
> + if (ret) {
> + kfree(controller);
> + return ret;
> + }
> +
> + ret = serial_core_pm_runtime_start(port);
> + if (ret < 0)
> + serial_core_unregister_port(port);
> +
> + return ret;
> +}
> +
> /*
> * Startup the port. This will be called once per open. All calls
> * will be serialised by the per-port mutex.
> @@ -2956,6 +3096,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.
> @@ -2979,6 +3123,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;
> @@ -3048,6 +3193,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
> @@ -250,6 +250,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) */
> @@ -285,6 +286,8 @@ enum uart_pm_state {
> * This is the state information which is persistent across opens.
> */
> struct uart_state {
> + struct serial_controller *controller;
While good looking here, I believe resource wise is better to leave @port to be
the first member. The rationale is to get rid of pointer arithmetics at compile
time (and I believe the port is used much more and in more critical places).
However, I dunno if it will get a lot of benefit, would be nice to see
bloat-o-meter output for your variant and my proposal.
> struct tty_port port;
>
> enum uart_pm_state pm_state;
> --
> 2.36.1
--
With Best Regards,
Andy Shevchenko
* Andy Shevchenko <[email protected]> [220615 09:12]:
> On Wed, Jun 15, 2022 at 09:24:55AM +0300, Tony Lindgren wrote:
> > 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
> > @@ -30,6 +32,25 @@
> > #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; /* Serial port device */
>
> Serial port device is a bit unclear for non-prepared reader. Perhaps add
> the word "physical" or another to specify the nature of the device (because
> to me "serial port device" sounds like a duplication of something in struct
> uart_port, but I have doubts).
Hmm so we could add a list of all the registered struct uart_port or
uart_state to struct serial_controller. Then looking up struct device would
be just looking at the first list entry. We need to take port_mutex, but
that should be mostly when the device does runtime PM. We'll be needing that
list anyways later on to flush pending TX on runtime PM resume for each
port associated with the device.
> > 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
> > @@ -250,6 +250,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) */
> > @@ -285,6 +286,8 @@ enum uart_pm_state {
> > * This is the state information which is persistent across opens.
> > */
> > struct uart_state {
> > + struct serial_controller *controller;
>
> While good looking here, I believe resource wise is better to leave @port to be
> the first member. The rationale is to get rid of pointer arithmetics at compile
> time (and I believe the port is used much more and in more critical places).
> However, I dunno if it will get a lot of benefit, would be nice to see
> bloat-o-meter output for your variant and my proposal.
OK makes sense. And thanks for reviewing again :)
Regards,
Tony
* Andy Shevchenko <[email protected]> [220615 09:12]:
> However, I dunno if it will get a lot of benefit, would be nice to see
> bloat-o-meter output for your variant and my proposal.
Yup this saves some instructions, I'm seeing a 0.19% reduction.
Regards,
Tony
On Wed, Jun 15, 2022 at 09:24:55AM +0300, 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 serial port controllers. For runtime PM, we need a way to find
> the serial ports for each serial port controller device.
>
> The serial core manages ports. Each serial controller can have multiple
> ports. As serial core has no struct device, and the serial port device
> drivers have their own driver data, we cannot currently start making
> use of serial core generic data easily without changing all the serial
> port device drivers.
Really? Why not make struct uart_port a real struct device?
> We could consider adding a serial core specific struct device. It would
> be a child of the serial port device, and would allow us eventually to use
> device_links to add generic runtime PM calls for example. But as the serial
> core layer is not a device driver, driver specific features would need to
> be added, and are probably not justified for a virtual device.
I think it's very justified, let's not paper over this whole thing by
adding a kref stuck in in the middle and trying to hook up the PM code
to it, instead of just using all of the PM logic that the driver model
already provides.
> Considering the above, let's improve the serial core layer so we can
> manage the serial port controllers better. Let's register the controllers
> with the serial core layer in addition to the serial ports.
Why can't controllers be a device as well?
Let's try to work with the driver model here, not work around it, if at
all possible. We never did a full conversion of the serial layer to the
driver core all those decades ago. Perhaps now is the time to really do
that.
thanks,
greg k-h
* Greg Kroah-Hartman <[email protected]> [220627 12:23]:
> On Wed, Jun 15, 2022 at 09:24:55AM +0300, 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 serial port controllers. For runtime PM, we need a way to find
> > the serial ports for each serial port controller device.
> >
> > The serial core manages ports. Each serial controller can have multiple
> > ports. As serial core has no struct device, and the serial port device
> > drivers have their own driver data, we cannot currently start making
> > use of serial core generic data easily without changing all the serial
> > port device drivers.
>
> Really? Why not make struct uart_port a real struct device?
Okie dokie
> > We could consider adding a serial core specific struct device. It would
> > be a child of the serial port device, and would allow us eventually to use
> > device_links to add generic runtime PM calls for example. But as the serial
> > core layer is not a device driver, driver specific features would need to
> > be added, and are probably not justified for a virtual device.
>
> I think it's very justified, let's not paper over this whole thing by
> adding a kref stuck in in the middle and trying to hook up the PM code
> to it, instead of just using all of the PM logic that the driver model
> already provides.
OK. Having the serial controller be the parent device for the port device
will make runtime PM work as designed :)
> > Considering the above, let's improve the serial core layer so we can
> > manage the serial port controllers better. Let's register the controllers
> > with the serial core layer in addition to the serial ports.
>
> Why can't controllers be a device as well?
The controllers are devices already probed by the serial port drivers.
What's missing is mapping the ports (as devices based on the comments
above) to the controller devices. I don't think we need another struct
device for the serial controller in addition to the serial port driver
device and it's child port devices.
> Let's try to work with the driver model here, not work around it, if at
> all possible. We never did a full conversion of the serial layer to the
> driver core all those decades ago. Perhaps now is the time to really do
> that.
Yes so it seems.
Regards,
Tony
Hi,
I am returning to v2, as I managed to read only v3 and only now. But
here was already the point below.
On 27. 06. 22, 15:48, Tony Lindgren wrote:
>>> Considering the above, let's improve the serial core layer so we can
>>> manage the serial port controllers better. Let's register the controllers
>>> with the serial core layer in addition to the serial ports.
>>
>> Why can't controllers be a device as well?
>
> The controllers are devices already probed by the serial port drivers.
> What's missing is mapping the ports (as devices based on the comments
> above) to the controller devices. I don't think we need another struct
> device for the serial controller in addition to the serial port driver
> device and it's child port devices.
To be honest, I don't like the patch (even v3). We have uart_state which
I already hate and now we have another structure holding *some* other
info about a serial device (apart from uart_port). It's mess already and
hard to follow, esp. to newcomers.
AFAIU, what Greg suggests would be:
PCI/platform/acpi/whatever struct dev
-> serial controller 1 struct dev
-> serial port 1 struct dev (tty_port instance exists for this)
-> serial port 2 struct dev (tty_port instance exists for this)
-> ...
-> serial controller 2 struct dev
-> serial port 1 struct dev (tty_port instance exists for this)
-> serial port 2 struct dev (tty_port instance exists for this)
-> ...
And you are objecting that mostly (or in all cases?), there will never
be "serial controller 2"?
But given your description, I believe you need it anyway -- side note:
does really the PM layer/or you need it or would you be fine with
"serial port N" dev children? But provided you don't have the
controller, you work around it by struct serial_controller. So what's
actually the point of the workaround instead of sticking to proper
driver model? With the workaround you seem you have to implement all the
binding, lookup and such yourself anyway. And that renders the serial
even worse :P. Let's do the reverse instead.
The only thing I am not sure about, whether tty_port should be struct
dev too -- and if it should have serial port 1 as a parent. But likely
so. And then with pure tty (i.e. tty_driver's, not uart_driver's), it
would have PCI/platform/acpi/whatever as a parent directly.
In sum, the above structure makes perfect sense to me. There has only
been noone to do the real work yet. And having tty_port was a hard
prerequisite for this to happen. And that happened long time ago. All
this would need a lot of work initially¹⁾, but it paid off a lot in long
term.
¹⁾I know what I am writing about -- I converted HID. After all, the core
was only 1000 lines patch (cf 85cdaf524b7d) + patches to convert all the
drivers incrementally (like 8c19a51591).
thanks,
--
js
suse labs
Hi,
* Jiri Slaby <[email protected]> [221124 06:53]:
> Hi,
>
> I am returning to v2, as I managed to read only v3 and only now. But here
> was already the point below.
>
> On 27. 06. 22, 15:48, Tony Lindgren wrote:
> > > > Considering the above, let's improve the serial core layer so we can
> > > > manage the serial port controllers better. Let's register the controllers
> > > > with the serial core layer in addition to the serial ports.
> > >
> > > Why can't controllers be a device as well?
> >
> > The controllers are devices already probed by the serial port drivers.
> > What's missing is mapping the ports (as devices based on the comments
> > above) to the controller devices. I don't think we need another struct
> > device for the serial controller in addition to the serial port driver
> > device and it's child port devices.
>
> To be honest, I don't like the patch (even v3). We have uart_state which I
> already hate and now we have another structure holding *some* other info
> about a serial device (apart from uart_port). It's mess already and hard to
> follow, esp. to newcomers.
Yup the serial code sure is hard to follow..
> AFAIU, what Greg suggests would be:
>
> PCI/platform/acpi/whatever struct dev
> -> serial controller 1 struct dev
> -> serial port 1 struct dev (tty_port instance exists for this)
> -> serial port 2 struct dev (tty_port instance exists for this)
> -> ...
> -> serial controller 2 struct dev
> -> serial port 1 struct dev (tty_port instance exists for this)
> -> serial port 2 struct dev (tty_port instance exists for this)
> -> ...
Oh you want the serial controller struct device as a child of the
hardware controller struct device. Yeah that makes sense to put it there.
I was kind of thinking we want the port devices be direct children of
the hardware struct device, but I guess there is no such need.
> And you are objecting that mostly (or in all cases?), there will never be
> "serial controller 2"?
I'm was not aware of the need for multiple serial port controllers
connected to a single hardware controller struct device. Is there an
example for that somewhere?
Not that multiple serial controller struct devices matters with your
suggestion, just wondering.
> But given your description, I believe you need it anyway -- side note: does
> really the PM layer/or you need it or would you be fine with "serial port N"
> dev children? But provided you don't have the controller, you work around it
> by struct serial_controller. So what's actually the point of the workaround
> instead of sticking to proper driver model? With the workaround you seem you
> have to implement all the binding, lookup and such yourself anyway. And that
> renders the serial even worse :P. Let's do the reverse instead.
To me it seems your suggestion actually makes things easier for runtime
PM :)
We can just enable runtime PM for the serial controller struct device
without tinkering with the parent hardware controller struct device.
> The only thing I am not sure about, whether tty_port should be struct dev
> too -- and if it should have serial port 1 as a parent. But likely so. And
> then with pure tty (i.e. tty_driver's, not uart_driver's), it would have
> PCI/platform/acpi/whatever as a parent directly.
That seems like a separate set of patches, no? Or is there some need right
now to have some child struct device as a direct child of the hardware
controller struct device?
> In sum, the above structure makes perfect sense to me. There has only been
> noone to do the real work yet. And having tty_port was a hard prerequisite
> for this to happen. And that happened long time ago. All this would need a
> lot of work initially¹⁾, but it paid off a lot in long term.
>
> ¹⁾I know what I am writing about -- I converted HID. After all, the core was
> only 1000 lines patch (cf 85cdaf524b7d) + patches to convert all the drivers
> incrementally (like 8c19a51591).
Cool, thanks for your suggestions.
Regards,
Tony
On Thu, Nov 24, 2022 at 05:37:07PM +0200, Tony Lindgren wrote:
> * Jiri Slaby <[email protected]> [221124 06:53]:
> > On 27. 06. 22, 15:48, Tony Lindgren wrote:
...
> > And you are objecting that mostly (or in all cases?), there will never be
> > "serial controller 2"?
>
> I'm was not aware of the need for multiple serial port controllers
> connected to a single hardware controller struct device. Is there an
> example for that somewhere?
I would be on the safest side here by assuming that's possible
(QEMU and FPGA are the first that comes to my mind of such possibilities).
--
With Best Regards,
Andy Shevchenko