[PATCH 0/2] serial: 8250_dw: clock-notifier cleanup

[PATCH 0/2] serial: 8250_dw: clock-notifier cleanup

[Stepan Ionichev]

Two-patch series addressing Andy's review of the leak-fix on v1.

Patch 1 keeps the same single-line leak fix as v1, but with:
- the correct "serial: 8250_dw:" prefix (underscore),
- a Fixes: tag pointing at the original clk_notifier introduction,
- Cc: stable@ so the fix gets picked up by stable branches that
  still carry the notifier code.

Patch 2 drops the clock-notifier infrastructure entirely from
mainline, as suggested by Andy. The notifier was introduced for the
Baikal-T1 SoC (shared baudclk between UART ports) and has no other
in-tree user; Baikal-T1 support has been removed from the kernel.

If a future platform needs the cross-device baudclk-rate notification
pattern again, it can be reintroduced in a more general form.

Stepan Ionichev (2):
  serial: 8250_dw: unregister 8250 port if clk_notifier_register() fails
  serial: 8250_dw: remove clock-notifier infrastructure

 drivers/tty/serial/8250/8250_dw.c | 79 -------------------------------
 1 file changed, 79 deletions(-)

-- 
2.43.0

[PATCH 1/2] serial: 8250_dw: unregister 8250 port if clk_notifier_register() fails

[Stepan Ionichev]

dw8250_probe() registers the 8250 port via serial8250_register_8250_port()
and then, if the device has a clock, registers a clock notifier. If
clk_notifier_register() fails, probe returns the error but leaves the
8250 port registered. The matching serial8250_unregister_port() lives
in dw8250_remove(), which is not called when probe fails, so the port
slot stays occupied until the device is rebound or the system is
rebooted. The devm-allocated driver data is freed while the port still
references it (via the saved private_data and serial_in/serial_out
callbacks), so any access to that port slot before a rebind is a
use-after-free hazard.

Unregister the port on the clk_notifier_register() error path.

Fixes: cc816969d7b5 ("serial: 8250_dw: Fix common clocks usage race condition")
Cc: stable@vger.kernel.org
Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
---
 drivers/tty/serial/8250/8250_dw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 94beadb40..7dbd79a91 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -850,8 +850,10 @@ static int dw8250_probe(struct platform_device *pdev)
 	 */
 	if (data->clk) {
 		err = clk_notifier_register(data->clk, &data->clk_notifier);
-		if (err)
+		if (err) {
+			serial8250_unregister_port(data->data.line);
 			return dev_err_probe(dev, err, "Failed to set the clock notifier\n");
+		}
 		queue_work(system_dfl_wq, &data->clk_work);
 	}
 
-- 
2.43.0

[PATCH 2/2] serial: 8250_dw: remove clock-notifier infrastructure

[Stepan Ionichev]

The clock notifier and matching work_struct in dw8250_data were added
in 2020 for the Baikal-T1 SoC, whose multiple UART ports share a
single reference clock and need to be informed when another consumer
re-rates that clock.

Baikal SoC support has since been removed from the kernel (see e.g.
commit 5d6c477687ae ("clk: baikal-t1: Remove not-going-to-be-supported
code for Baikal SoC") and the matching removals across bus/, mtd/,
PCI/, hwmon/, memory/). No remaining in-tree user needs the
cross-device baudclk rate-change notification path: the only
configuration that wired up the notifier was Baikal-T1's shared
reference clock topology.

Drop the now-unused clock-notifier and its deferred-update worker:

  - struct dw8250_data fields clk_notifier and clk_work,
  - the clk_to_dw8250_data() and work_to_dw8250_data() helpers,
  - the dw8250_clk_work_cb() and dw8250_clk_notifier_cb() callbacks,
  - the INIT_WORK / notifier_call setup in dw8250_probe(),
  - the clk_notifier_register() / queue_work() in dw8250_probe(),
  - the matching clk_notifier_unregister() / flush_work() in
    dw8250_remove(),
  - the stale comment in dw8250_set_termios() about the worker
    blocking,
  - the linux/notifier.h and linux/workqueue.h includes that are
    no longer used.

dw8250_set_termios() keeps calling clk_set_rate() directly, which is
all the remaining single-UART configurations require.

Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
---
 drivers/tty/serial/8250/8250_dw.c | 81 -------------------------------
 1 file changed, 81 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 7dbd79a91..41c5abccd 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -19,13 +19,11 @@
 #include <linux/lockdep.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
-#include <linux/notifier.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
 #include <linux/reset.h>
 #include <linux/slab.h>
-#include <linux/workqueue.h>
 
 #include <asm/byteorder.h>
 
@@ -86,8 +84,6 @@ struct dw8250_data {
 	u32			msr_mask_off;
 	struct clk		*clk;
 	struct clk		*pclk;
-	struct notifier_block	clk_notifier;
-	struct work_struct	clk_work;
 	struct reset_control	*rst;
 
 	unsigned int		skip_autocfg:1;
@@ -102,16 +98,6 @@ static inline struct dw8250_data *to_dw8250_data(struct dw8250_port_data *data)
 	return container_of(data, struct dw8250_data, data);
 }
 
-static inline struct dw8250_data *clk_to_dw8250_data(struct notifier_block *nb)
-{
-	return container_of(nb, struct dw8250_data, clk_notifier);
-}
-
-static inline struct dw8250_data *work_to_dw8250_data(struct work_struct *work)
-{
-	return container_of(work, struct dw8250_data, clk_work);
-}
-
 static inline u32 dw8250_modify_msr(struct uart_port *p, unsigned int offset, u32 value)
 {
 	struct dw8250_data *d = to_dw8250_data(p->private_data);
@@ -484,46 +470,6 @@ static int dw8250_handle_irq(struct uart_port *p)
 	return 1;
 }
 
-static void dw8250_clk_work_cb(struct work_struct *work)
-{
-	struct dw8250_data *d = work_to_dw8250_data(work);
-	struct uart_8250_port *up;
-	unsigned long rate;
-
-	rate = clk_get_rate(d->clk);
-	if (rate <= 0)
-		return;
-
-	up = serial8250_get_port(d->data.line);
-
-	serial8250_update_uartclk(&up->port, rate);
-}
-
-static int dw8250_clk_notifier_cb(struct notifier_block *nb,
-				  unsigned long event, void *data)
-{
-	struct dw8250_data *d = clk_to_dw8250_data(nb);
-
-	/*
-	 * We have no choice but to defer the uartclk update due to two
-	 * deadlocks. First one is caused by a recursive mutex lock which
-	 * happens when clk_set_rate() is called from dw8250_set_termios().
-	 * Second deadlock is more tricky and is caused by an inverted order of
-	 * the clk and tty-port mutexes lock. It happens if clock rate change
-	 * is requested asynchronously while set_termios() is executed between
-	 * tty-port mutex lock and clk_set_rate() function invocation and
-	 * vise-versa. Anyway if we didn't have the reference clock alteration
-	 * in the dw8250_set_termios() method we wouldn't have needed this
-	 * deferred event handling complication.
-	 */
-	if (event == POST_RATE_CHANGE) {
-		queue_work(system_dfl_wq, &d->clk_work);
-		return NOTIFY_OK;
-	}
-
-	return NOTIFY_DONE;
-}
-
 static void
 dw8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old)
 {
@@ -547,10 +493,6 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
 	clk_disable_unprepare(d->clk);
 	rate = clk_round_rate(d->clk, newrate);
 	if (rate > 0) {
-		/*
-		 * Note that any clock-notifier worker will block in
-		 * serial8250_update_uartclk() until we are done.
-		 */
 		ret = clk_set_rate(d->clk, newrate);
 		if (!ret)
 			p->uartclk = rate;
@@ -783,9 +725,6 @@ static int dw8250_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(data->clk),
 				     "failed to get baudclk\n");
 
-	INIT_WORK(&data->clk_work, dw8250_clk_work_cb);
-	data->clk_notifier.notifier_call = dw8250_clk_notifier_cb;
-
 	if (data->clk)
 		p->uartclk = clk_get_rate(data->clk);
 
@@ -843,20 +782,6 @@ static int dw8250_probe(struct platform_device *pdev)
 	if (data->data.line < 0)
 		return data->data.line;
 
-	/*
-	 * Some platforms may provide a reference clock shared between several
-	 * devices. In this case any clock state change must be known to the
-	 * UART port at least post factum.
-	 */
-	if (data->clk) {
-		err = clk_notifier_register(data->clk, &data->clk_notifier);
-		if (err) {
-			serial8250_unregister_port(data->data.line);
-			return dev_err_probe(dev, err, "Failed to set the clock notifier\n");
-		}
-		queue_work(system_dfl_wq, &data->clk_work);
-	}
-
 	platform_set_drvdata(pdev, data);
 
 	pm_runtime_enable(dev);
@@ -871,12 +796,6 @@ static void dw8250_remove(struct platform_device *pdev)
 
 	pm_runtime_get_sync(dev);
 
-	if (data->clk) {
-		clk_notifier_unregister(data->clk, &data->clk_notifier);
-
-		flush_work(&data->clk_work);
-	}
-
 	serial8250_unregister_port(data->data.line);
 
 	pm_runtime_disable(dev);
-- 
2.43.0