[PATCH v2 0/6] serial: vt8500: Clean up the probe using devm and new serial core helpers

[PATCH v2 0/6] serial: vt8500: Clean up the probe using devm and new serial core helpers

[Alexey Charkov]

The driver has been carrying legacy vestiges in the form of a bitmask and
a local array to track which ports should be numbered which, both of them
not really needed anymore with DT aliases and existing in-core tracking.

Remove them altogether to simplify the driver and get rid of leaks Sashiko
found. While here, also convert the clock handling to use the resource
managed API, and propagate errors from uart_add_one_port().

This hinges upon two new reusable helpers in the serial core which allow
getting the console port for a given line number per driver, and another
one to auto-assign line numbers for which drivers currently keep an
assortment of local accounting boilerplate (some with locking and leaking
issues, such as vt8500 to date).

Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
Changes in v2:
- Added a core helper to get the console port for a given line number per driver
- Added UART_LINE_AUTO support for automatic line number assignment when no OF
  alias is provided
- Link to v1: https://patch.msgid.link/20261111-vt8500-serial-cleanup-v1-0-b33594a25f87@gmail.com

---
Alexey Charkov (6):
      serial: core: Add uart_console_port()
      serial: core: Allocate a free line for UART_LINE_AUTO ports
      serial: vt8500: Look up console ports with uart_console_port()
      serial: vt8500: Drop the port index bitmap
      serial: vt8500: Use devm_clk_get_enabled()
      serial: vt8500: Handle uart_add_one_port() failure

 drivers/tty/serial/serial_core.c   | 89 +++++++++++++++++++++++++++++++++++++-
 drivers/tty/serial/vt8500_serial.c | 89 +++++++++-----------------------------
 include/linux/serial_core.h        |  5 +++
 3 files changed, 112 insertions(+), 71 deletions(-)
---
base-commit: aaa2f954c21b205a0bcd0961e5b2be4e82e58455
change-id: 20261111-vt8500-serial-cleanup-dfc9731edd70

Best regards,
--  
Alexey Charkov <alchark@gmail.com>

[PATCH v2 1/6] serial: core: Add uart_console_port()

[Alexey Charkov]

Console ->setup() and ->write() callbacks need the port behind
co->index. Most drivers keep a static port array just for that lookup,
and the few that skip it read the uart_driver state table, which the
header reserves for the core.

Provide the lookup next to uart_console_device(), which already relies
on co->data pointing at the uart_driver. The core links a port into the
state table before registering its console and unregisters the console
before unlinking the port, so the result stays valid in both callbacks.

Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
 drivers/tty/serial/serial_core.c | 21 +++++++++++++++++++++
 include/linux/serial_core.h      |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index b06d2649be34..683ffb4fda92 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2890,6 +2890,27 @@ struct tty_driver *uart_console_device(struct console *co, int *index)
 }
 EXPORT_SYMBOL_GPL(uart_console_device);
 
+/**
+ * uart_console_port - look up the port behind a console
+ * @co: console whose @data points at its &struct uart_driver
+ *
+ * Serial core registers a port's console only after linking the port into
+ * the driver's state table, and unregisters it before unlinking the port, so
+ * the result stays valid for the console's ->setup() and ->write() callbacks.
+ *
+ * Return: the port registered on line @co->index, or %NULL if there is none.
+ */
+struct uart_port *uart_console_port(struct console *co)
+{
+	struct uart_driver *drv = co->data;
+
+	if (co->index < 0 || co->index >= drv->nr)
+		return NULL;
+
+	return drv->state[co->index].uart_port;
+}
+EXPORT_SYMBOL_GPL(uart_console_port);
+
 static ssize_t uartclk_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 4eeb8253752a..9ac1ce1c10b5 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -1127,6 +1127,7 @@ void uart_parse_options(const char *options, int *baud, int *parity, int *bits,
 int uart_set_options(struct uart_port *port, struct console *co, int baud,
 		     int parity, int bits, int flow);
 struct tty_driver *uart_console_device(struct console *co, int *index);
+struct uart_port *uart_console_port(struct console *co);
 void uart_console_write(struct uart_port *port, const char *s,
 			unsigned int count,
 			void (*putchar)(struct uart_port *, unsigned char));

-- 
2.56.0

[PATCH v2 2/6] serial: core: Allocate a free line for UART_LINE_AUTO ports

[Alexey Charkov]

Drivers that accept ports without a "serial" alias each carry their own
line allocator, be it a bitmap, an IDA or a scan of a static port array.
Each one has to get locking against concurrent probes and release on
failed registration right by itself, and the state table already holds
the same information.

Let such drivers set the line to UART_LINE_AUTO and pick it while
registering the port under port_mutex, which serialises every change to
the state table, so the choice cannot race another registration and
needs no separate bookkeeping. Skip lines that an enabled device claims
through a "serial" alias, so that the device still finds its line free
if it probes later, and hand those out only once no other line is free.

Restore UART_LINE_AUTO when registration fails, so that a retry with the
same port allocates afresh.

Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
 drivers/tty/serial/serial_core.c | 68 ++++++++++++++++++++++++++++++++++++++--
 include/linux/serial_core.h      |  4 +++
 2 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 683ffb4fda92..e0f9551ff7f8 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3351,16 +3351,73 @@ static int serial_core_port_device_add(struct serial_ctrl_device *ctrl_dev,
 	return 0;
 }
 
+/* Whether an enabled device claims @line through a "serial" alias */
+static bool serial_core_line_is_aliased(unsigned int line)
+{
+	char alias[sizeof("serial") + 10];
+	struct device_node *np;
+	bool available;
+
+	snprintf(alias, sizeof(alias), "serial%u", line);
+	np = of_find_node_by_path(alias);
+	available = of_device_is_available(np);
+	of_node_put(np);
+
+	return available;
+}
+
+/*
+ * Pick a line for a port registered with UART_LINE_AUTO. Skip lines that an
+ * enabled device claims through a "serial" alias, so that the device still
+ * finds its line free if it probes later, and hand one of them out only when
+ * no other line is free. Caller must hold port_mutex, which serialises every
+ * change to the driver's state table.
+ */
+static int serial_core_alloc_line(struct uart_driver *drv,
+				  struct uart_port *port)
+{
+	unsigned int line, fallback = drv->nr;
+
+	lockdep_assert_held(&port_mutex);
+
+	for (line = 0; line < drv->nr; line++) {
+		if (drv->state[line].uart_port)
+			continue;
+
+		if (!serial_core_line_is_aliased(line))
+			break;
+
+		if (fallback == drv->nr)
+			fallback = line;
+	}
+
+	if (line == drv->nr)
+		line = fallback;
+	if (line == drv->nr)
+		return -ENOSPC;
+
+	port->line = line;
+
+	return 0;
+}
+
 /*
  * Initialize a serial core port device, and a controller device if needed.
  */
 int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
 {
 	struct serial_ctrl_device *ctrl_dev, *new_ctrl_dev = NULL;
+	bool auto_line = port->line == UART_LINE_AUTO;
 	int ret;
 
 	guard(mutex)(&port_mutex);
 
+	if (auto_line) {
+		ret = serial_core_alloc_line(drv, port);
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 * Prevent serial_port_runtime_resume() from trying to use the port
 	 * until serial_core_add_one_port() has completed
@@ -3371,8 +3428,10 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
 	ctrl_dev = serial_core_ctrl_find(drv, port->dev, port->ctrl_id);
 	if (!ctrl_dev) {
 		new_ctrl_dev = serial_core_ctrl_device_add(port);
-		if (IS_ERR(new_ctrl_dev))
-			return PTR_ERR(new_ctrl_dev);
+		if (IS_ERR(new_ctrl_dev)) {
+			ret = PTR_ERR(new_ctrl_dev);
+			goto err_free_line;
+		}
 		ctrl_dev = new_ctrl_dev;
 	}
 
@@ -3402,6 +3461,11 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
 err_unregister_ctrl_dev:
 	serial_base_ctrl_device_remove(new_ctrl_dev);
 
+err_free_line:
+	/* Let a retry with the same port pick a line afresh */
+	if (auto_line)
+		port->line = UART_LINE_AUTO;
+
 	return ret;
 }
 
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 9ac1ce1c10b5..a6955c446eaf 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -1135,6 +1135,10 @@ void uart_console_write(struct uart_port *port, const char *s,
 /*
  * Port/driver registration/removal
  */
+
+/* Set uart_port.line to this to have uart_add_one_port() pick a free line */
+#define UART_LINE_AUTO		UINT_MAX
+
 int uart_register_driver(struct uart_driver *uart);
 void uart_unregister_driver(struct uart_driver *uart);
 int uart_add_one_port(struct uart_driver *reg, struct uart_port *port);

-- 
2.56.0

[PATCH v2 3/6] serial: vt8500: Look up console ports with uart_console_port()

[Alexey Charkov]

Ask the serial core for the console's port, which it registers before
the console, and drop the private vt8500_uart_ports[] array that only
mirrors the core state and gets in the way.

The helper also range-checks the console index, which makes the checks
in the console callbacks redundant.

Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
 drivers/tty/serial/vt8500_serial.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/serial/vt8500_serial.c b/drivers/tty/serial/vt8500_serial.c
index a7f167a73416..4abd4a8d5ef3 100644
--- a/drivers/tty/serial/vt8500_serial.c
+++ b/drivers/tty/serial/vt8500_serial.c
@@ -444,7 +444,6 @@ static int vt8500_verify_port(struct uart_port *port,
 	return 0;
 }
 
-static struct vt8500_port *vt8500_uart_ports[VT8500_MAX_PORTS];
 static struct uart_driver vt8500_uart_driver;
 
 #ifdef CONFIG_SERIAL_VT8500_CONSOLE
@@ -472,46 +471,38 @@ static void vt8500_console_putchar(struct uart_port *port, unsigned char c)
 static void vt8500_console_write(struct console *co, const char *s,
 			      unsigned int count)
 {
-	struct vt8500_port *vt8500_port = vt8500_uart_ports[co->index];
+	struct uart_port *port = uart_console_port(co);
 	unsigned long ier;
 
-	BUG_ON(co->index < 0 || co->index >= vt8500_uart_driver.nr);
+	ier = vt8500_read(port, VT8500_URIER);
+	vt8500_write(port, VT8500_URIER, 0);
 
-	ier = vt8500_read(&vt8500_port->uart, VT8500_URIER);
-	vt8500_write(&vt8500_port->uart, VT8500_URIER, 0);
-
-	uart_console_write(&vt8500_port->uart, s, count,
-			   vt8500_console_putchar);
+	uart_console_write(port, s, count, vt8500_console_putchar);
 
 	/*
 	 *	Finally, wait for transmitter to become empty
 	 *	and switch back to FIFO
 	 */
-	wait_for_xmitr(&vt8500_port->uart);
-	vt8500_write(&vt8500_port->uart, VT8500_URIER, ier);
+	wait_for_xmitr(port);
+	vt8500_write(port, VT8500_URIER, ier);
 }
 
 static int __init vt8500_console_setup(struct console *co, char *options)
 {
-	struct vt8500_port *vt8500_port;
+	struct uart_port *port;
 	int baud = 9600;
 	int bits = 8;
 	int parity = 'n';
 	int flow = 'n';
 
-	if (unlikely(co->index >= vt8500_uart_driver.nr || co->index < 0))
-		return -ENXIO;
-
-	vt8500_port = vt8500_uart_ports[co->index];
-
-	if (!vt8500_port)
+	port = uart_console_port(co);
+	if (!port)
 		return -ENODEV;
 
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
 
-	return uart_set_options(&vt8500_port->uart,
-				 co, baud, parity, bits, flow);
+	return uart_set_options(port, co, baud, parity, bits, flow);
 }
 
 static struct console vt8500_console = {
@@ -683,8 +674,6 @@ static int vt8500_serial_probe(struct platform_device *pdev)
 	snprintf(vt8500_port->name, sizeof(vt8500_port->name),
 		 "VT8500 UART%d", pdev->id);
 
-	vt8500_uart_ports[port] = vt8500_port;
-
 	uart_add_one_port(&vt8500_uart_driver, &vt8500_port->uart);
 
 	platform_set_drvdata(pdev, vt8500_port);

-- 
2.56.0

[PATCH v2 4/6] serial: vt8500: Drop the port index bitmap

[Alexey Charkov]

Stop reserving a line in the vt8500_ports_in_use bitmap before probe
acquires its resources: nothing ever clears the bit, so an early probe
failure strands the line, and a retry then fails with -EBUSY on an
aliased port or drifts to the next free index on an unaliased one.

Keep using an in-range "serial" alias, and otherwise let the serial core
pick a free line while it registers the port, so that a line is only
ever held by a registered port. That also stops an unaliased port from
taking the line of an aliased one that probes later.

Fixes: 4001130df1a7 ("serial: vt8500: Add devicetree support for vt8500-serial")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-ca000721-4a5b-4d88-bdb8-7f1e438dc81e
Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
 drivers/tty/serial/vt8500_serial.c | 37 +++++--------------------------------
 1 file changed, 5 insertions(+), 32 deletions(-)

diff --git a/drivers/tty/serial/vt8500_serial.c b/drivers/tty/serial/vt8500_serial.c
index 4abd4a8d5ef3..0c8ddd55f94d 100644
--- a/drivers/tty/serial/vt8500_serial.c
+++ b/drivers/tty/serial/vt8500_serial.c
@@ -90,7 +90,6 @@
 
 #define VT8500_RECOMMENDED_CLK		12000000
 #define VT8500_OVERSAMPLING_DIVISOR	13
-#define VT8500_MAX_PORTS	6
 
 struct vt8500_port {
 	struct uart_port	uart;
@@ -101,13 +100,6 @@ struct vt8500_port {
 	unsigned int		vt8500_uart_flags;
 };
 
-/*
- * we use this variable to keep track of which ports
- * have been allocated as we can't use pdev->id in
- * devicetree
- */
-static DECLARE_BITMAP(vt8500_ports_in_use, VT8500_MAX_PORTS);
-
 static inline void vt8500_write(struct uart_port *port, unsigned int val,
 			     unsigned int off)
 {
@@ -606,29 +598,6 @@ static int vt8500_serial_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
-	if (np) {
-		port = of_alias_get_id(np, "serial");
-		if (port >= VT8500_MAX_PORTS)
-			port = -1;
-	} else {
-		port = -1;
-	}
-
-	if (port < 0) {
-		/* calculate the port id */
-		port = find_first_zero_bit(vt8500_ports_in_use,
-					   VT8500_MAX_PORTS);
-	}
-
-	if (port >= VT8500_MAX_PORTS)
-		return -ENODEV;
-
-	/* reserve the port id */
-	if (test_and_set_bit(port, vt8500_ports_in_use)) {
-		/* port already in use - shouldn't really happen */
-		return -EBUSY;
-	}
-
 	vt8500_port = devm_kzalloc_obj(&pdev->dev, struct vt8500_port);
 	if (!vt8500_port)
 		return -ENOMEM;
@@ -661,7 +630,11 @@ static int vt8500_serial_probe(struct platform_device *pdev)
 	vt8500_port->uart.irq = irq;
 	vt8500_port->uart.fifosize = 16;
 	vt8500_port->uart.ops = &vt8500_uart_pops;
-	vt8500_port->uart.line = port;
+	port = of_alias_get_id(np, "serial");
+	if (port >= 0 && port < vt8500_uart_driver.nr)
+		vt8500_port->uart.line = port;
+	else
+		vt8500_port->uart.line = UART_LINE_AUTO;
 	vt8500_port->uart.dev = &pdev->dev;
 	vt8500_port->uart.flags = UPF_IOREMAP | UPF_BOOT_AUTOCONF;
 	vt8500_port->uart.has_sysrq = IS_ENABLED(CONFIG_SERIAL_VT8500_CONSOLE);

-- 
2.56.0

[PATCH v2 5/6] serial: vt8500: Use devm_clk_get_enabled()

[Alexey Charkov]

Let devres own the UART clock, so that no probe error path, present or
future, has to remember to disable and put it.

Pass the clock lookup error up through dev_err_probe() instead of
replacing it with -EINVAL, so that a clock provider that is not ready
yet defers the probe rather than failing it for good.

Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
 drivers/tty/serial/vt8500_serial.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/vt8500_serial.c b/drivers/tty/serial/vt8500_serial.c
index 0c8ddd55f94d..cd5188282f64 100644
--- a/drivers/tty/serial/vt8500_serial.c
+++ b/drivers/tty/serial/vt8500_serial.c
@@ -586,7 +586,6 @@ static int vt8500_serial_probe(struct platform_device *pdev)
 	struct resource *mmres;
 	struct device_node *np = pdev->dev.of_node;
 	const unsigned int *flags;
-	int ret;
 	int port;
 	int irq;
 
@@ -606,18 +605,10 @@ static int vt8500_serial_probe(struct platform_device *pdev)
 	if (IS_ERR(vt8500_port->uart.membase))
 		return PTR_ERR(vt8500_port->uart.membase);
 
-	vt8500_port->clk = of_clk_get(pdev->dev.of_node, 0);
-	if (IS_ERR(vt8500_port->clk)) {
-		dev_err(&pdev->dev, "failed to get clock\n");
-		return  -EINVAL;
-	}
-
-	ret = clk_prepare_enable(vt8500_port->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to enable clock\n");
-		clk_put(vt8500_port->clk);
-		return ret;
-	}
+	vt8500_port->clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(vt8500_port->clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(vt8500_port->clk),
+				     "failed to get and enable clock\n");
 
 	vt8500_port->vt8500_uart_flags = *flags;
 	vt8500_port->clk_predivisor = DIV_ROUND_CLOSEST(

-- 
2.56.0

[PATCH v2 6/6] serial: vt8500: Handle uart_add_one_port() failure

[Alexey Charkov]

Return the error from uart_add_one_port() instead of reporting a bound
device for a port that never registered, which would keep its clock
running for nothing. Let devres release the clock and the remaining
resources on that path.

Set the driver data before adding the port, so that the probe can end
on the registration call.

Fixes: 304e12665a4a ("serial: Add support for UART on VIA VT8500 and compatibles")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-ce64cdae-9faa-4109-b7da-29b8a8c99e76
Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
 drivers/tty/serial/vt8500_serial.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/tty/serial/vt8500_serial.c b/drivers/tty/serial/vt8500_serial.c
index cd5188282f64..906b8dd2f4ff 100644
--- a/drivers/tty/serial/vt8500_serial.c
+++ b/drivers/tty/serial/vt8500_serial.c
@@ -638,11 +638,9 @@ static int vt8500_serial_probe(struct platform_device *pdev)
 	snprintf(vt8500_port->name, sizeof(vt8500_port->name),
 		 "VT8500 UART%d", pdev->id);
 
-	uart_add_one_port(&vt8500_uart_driver, &vt8500_port->uart);
-
 	platform_set_drvdata(pdev, vt8500_port);
 
-	return 0;
+	return uart_add_one_port(&vt8500_uart_driver, &vt8500_port->uart);
 }
 
 static struct platform_driver vt8500_platform_driver = {

-- 
2.56.0