[PATCHv3 0/6] serial: mxs-auart: devm conversion, clock rework, and IRQ ordering fixes

[PATCHv3 0/6] serial: mxs-auart: devm conversion, clock rework, and IRQ ordering fixes

[Rosen Penev]

This series cleans up the mxs-auart driver by converting to devm-managed
resources, fixing clock prepare/enable ordering, and addressing IRQ
registration races.

Patch 1 fixes compilation on 64-bit build with W=1

Patch 2 reworks the clock handling to use devm_clk_get_enabled and
reorders clk_prepare_enable after clk_set_rate to avoid
CLK_SET_RATE_GATE failures.

Patch 3 converts iomem mapping and GPIO IRQ requests to devm,
removing the manual cleanup paths.

Patch 4 moves the main UART IRQ registration after uart_add_one_port
so the port state is initialized before the handler can run, and
manages the module clock for console vs non-console ports correctly.

v3: two more sashiko fixes
v2: split off of_device_get_match_data change.

Rosen Penev (6):
  serial: mxs-auart: fix cast type for of_device_get_match_data
  serial: mxs-auart: rework clock handling in mxs_get_clks and probe
  serial: mxs-auart: use devm resources for iomem and GPIO IRQs
  serial: mxs-auart: fix IRQ registration ordering and manage console
    clock
  serial: mxs-auart: clamp RX DMA count to buffer size
  serial: mxs-auart: terminate DMA before releasing channels in exit

 drivers/tty/serial/mxs-auart.c | 145 +++++++++++++++------------------
 1 file changed, 66 insertions(+), 79 deletions(-)

--
2.54.0

[PATCHv3 1/6] serial: mxs-auart: fix cast type for of_device_get_match_data

[Rosen Penev]

of_device_get_match_data returns const void*. Cast to unsigned long to
avoid implicit integer truncation warnings. All the data parameters are
correct anyway.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/tty/serial/mxs-auart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 697318dbb146..de97c0f74e7d 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -1598,7 +1598,7 @@ static int mxs_auart_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	s->devtype = (enum mxs_auart_type)of_device_get_match_data(&pdev->dev);
+	s->devtype = (unsigned long)of_device_get_match_data(&pdev->dev);
 
 	ret = mxs_get_clks(s, pdev);
 	if (ret)
-- 
2.54.0

Re: [PATCHv3 1/6] serial: mxs-auart: fix cast type for of_device_get_match_data

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-bounds array access on probe error and driver remove paths.
- [High] NULL pointer dereference and uninitialized spinlock access due to early IRQ request.
--

commit f0caca84ea6688e77479d143060974cd96b52b46
Author: Rosen Penev <rosenp@gmail.com>

serial: mxs-auart: fix cast type for of_device_get_match_data

of_device_get_match_data returns const void*. Cast to unsigned long to
avoid implicit integer truncation warnings. All the data parameters are
correct anyway.

> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -1598,7 +1598,7 @@ static int mxs_auart_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	s->devtype = (enum mxs_auart_type)of_device_get_match_data(&pdev->dev);
> +	s->devtype = (unsigned long)of_device_get_match_data(&pdev->dev);
>  
>  	ret = mxs_get_clks(s, pdev);
>  	if (ret)

[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds array
access on the probe error and driver remove paths in mxs_auart_probe().

For Device Tree-probed devices, pdev->id is typically set to -1. The logical
port number is correctly retrieved via of_alias_get_id() and stored in
s->port.line, and the driver instance is registered in the array using:

    auart_port[s->port.line] = s;

However, on the probe error path, the driver uses pdev->id to clear the array:

out_free_qpio_irq:
    mxs_auart_free_gpio_irq(s);
    auart_port[pdev->id] = NULL;

Would this write NULL to auart_port[-1], silently corrupting adjacent memory
located before the auart_port array?

[Severity: High]
This is also a pre-existing issue, but does this function register the IRQ
handler too early?

In mxs_auart_probe(), devm_request_irq() is called before the hardware is
fully reset and before uart_add_one_port() registers the port:

    ret = devm_request_irq(&pdev->dev, irq, mxs_auart_irq_handle, 0,
                           dev_name(&pdev->dev), s);

If the bootloader leaves the UART hardware with interrupts enabled and
pending, could the interrupt handler fire immediately upon registration?

If it fires early, the handler attempts to lock s->port.lock, which is
uninitialized at this stage, and then calls mxs_auart_rx_chars() which pushes
to the buffer:

    tty_flip_buffer_push(&s->port.state->port);

Since s->port.state is only allocated later during uart_add_one_port(),
would this result in a NULL pointer dereference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611033856.6476-2-rosenp@gmail.com?part=1

[PATCHv3 2/6] serial: mxs-auart: rework clock handling in mxs_get_clks and probe

[Rosen Penev]

Use devm_clk_get_enabled for the AHB clock so its enable/disable
lifetime is managed by the driver model. Move the mod clock
(clk) prepare_enable out of mxs_get_clks and into probe so that
clk_set_rate is called while the clock is still disabled, avoiding
CLK_SET_RATE_GATE failures. Clean up the error labels accordingly.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/tty/serial/mxs-auart.c | 47 ++++++++++++----------------------
 1 file changed, 17 insertions(+), 30 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index de97c0f74e7d..aa59a48bfad7 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -1470,34 +1470,22 @@ static int mxs_get_clks(struct mxs_auart_port *s,
 		return PTR_ERR(s->clk);
 	}
 
-	s->clk_ahb = devm_clk_get(s->dev, "ahb");
+	s->clk_ahb = devm_clk_get_enabled(s->dev, "ahb");
 	if (IS_ERR(s->clk_ahb)) {
 		dev_err(s->dev, "Failed to get \"ahb\" clk\n");
 		return PTR_ERR(s->clk_ahb);
 	}
 
-	err = clk_prepare_enable(s->clk_ahb);
-	if (err) {
-		dev_err(s->dev, "Failed to enable ahb_clk!\n");
-		return err;
-	}
-
+	/*
+	 * Set mod clock rate while it is still disabled so
+	 * CLK_SET_RATE_GATE does not cause clk_set_rate to fail.
+	 * The mod clock will be enabled in mxs_auart_startup()
+	 * and in probe after mxs_get_clks returns.
+	 */
 	err = clk_set_rate(s->clk, clk_get_rate(s->clk_ahb));
-	if (err) {
+	if (err)
 		dev_err(s->dev, "Failed to set rate!\n");
-		goto disable_clk_ahb;
-	}
 
-	err = clk_prepare_enable(s->clk);
-	if (err) {
-		dev_err(s->dev, "Failed to enable clk!\n");
-		goto disable_clk_ahb;
-	}
-
-	return 0;
-
-disable_clk_ahb:
-	clk_disable_unprepare(s->clk_ahb);
 	return err;
 }
 
@@ -1604,17 +1592,21 @@ static int mxs_auart_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = clk_prepare_enable(s->clk);
+	if (ret)
+		return ret;
+
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!r) {
 		ret = -ENXIO;
-		goto out_disable_clks;
+		goto out_disable_clk;
 	}
 
 	s->port.mapbase = r->start;
 	s->port.membase = ioremap(r->start, resource_size(r));
 	if (!s->port.membase) {
 		ret = -ENOMEM;
-		goto out_disable_clks;
+		goto out_disable_clk;
 	}
 	s->port.ops = &mxs_auart_ops;
 	s->port.iotype = UPIO_MEM;
@@ -1681,11 +1673,8 @@ static int mxs_auart_probe(struct platform_device *pdev)
 out_iounmap:
 	iounmap(s->port.membase);
 
-out_disable_clks:
-	if (is_asm9260_auart(s)) {
-		clk_disable_unprepare(s->clk);
-		clk_disable_unprepare(s->clk_ahb);
-	}
+out_disable_clk:
+	clk_disable_unprepare(s->clk);
 	return ret;
 }
 
@@ -1697,10 +1686,8 @@ static void mxs_auart_remove(struct platform_device *pdev)
 	auart_port[pdev->id] = NULL;
 	mxs_auart_free_gpio_irq(s);
 	iounmap(s->port.membase);
-	if (is_asm9260_auart(s)) {
+	if (is_asm9260_auart(s))
 		clk_disable_unprepare(s->clk);
-		clk_disable_unprepare(s->clk_ahb);
-	}
 }
 
 static struct platform_driver mxs_auart_driver = {
-- 
2.54.0

Re: [PATCHv3 2/6] serial: mxs-auart: rework clock handling in mxs_get_clks and probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Unconditional clock enablement in probe introduces a reference leak and power management regression for non-ASM9260 devices on driver removal.

Pre-existing issues:
- [Critical] Out-of-bounds array write or incorrect array cleanup due to a mismatch between `s->port.line` and `pdev->id`.
- [High] Potential kernel panic due to a Use-After-Unmap condition in the interrupt handler during device removal or probe failure.
--

commit 7ff1436c03e590a8440db3420eec3a2408bf70ee
Author: Rosen Penev <rosenp@gmail.com>

serial: mxs-auart: rework clock handling in mxs_get_clks and probe

This commit updates the driver to use devm_clk_get_enabled for the AHB
clock so its enable/disable lifetime is managed by the driver model. It
also moves the mod clock prepare_enable out of mxs_get_clks and into
probe to avoid CLK_SET_RATE_GATE failures.

> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c

[ ... ]

> @@ -1697,10 +1686,8 @@ static void mxs_auart_remove(struct platform_device *pdev)
>  	struct mxs_auart_port *s = platform_get_drvdata(pdev);
>  
>  	uart_remove_one_port(&auart_driver, &s->port);
>  	auart_port[pdev->id] = NULL;

[Severity: Critical]
This isn't a bug introduced by this patch, but does this clear the wrong
array entry?

In mxs_auart_probe(), the entry is set using the alias ID:
    auart_port[s->port.line] = s;

If pdev->id is -1 for device tree instantiated platform devices, wouldn't
this cause an out of bounds memory write into the BSS segment?

>  	mxs_auart_free_gpio_irq(s);
>  	iounmap(s->port.membase);

[Severity: High]
This is a pre-existing issue, but can this cause a use-after-unmap during
device removal?

Since the interrupt was requested using devm_request_irq() in probe, the
devres core will keep it active until after mxs_auart_remove() fully returns.

If an interrupt fires during this window, wouldn't mxs_auart_irq_handle()
attempt to read from the hardware registers using the explicitly unmapped
membase?

> -	if (is_asm9260_auart(s)) {
> +	if (is_asm9260_auart(s))
>  		clk_disable_unprepare(s->clk);
> -		clk_disable_unprepare(s->clk_ahb);
> -	}
>  }

[Severity: Medium]
Since s->clk is now unconditionally prepared and enabled for all device
variants during mxs_auart_probe(), shouldn't it be unconditionally disabled
here?

Guarding it behind is_asm9260_auart() seems like it will permanently leak
the clock enable reference for non-ASM9260 devices on module unload.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611033856.6476-3-rosenp@gmail.com?part=1

[PATCHv3 3/6] serial: mxs-auart: use devm resources for iomem and GPIO IRQs

[Rosen Penev]

Replace platform_get_resource + ioremap with
devm_platform_get_and_ioremap_resource and convert GPIO IRQ
request_irq/free_irq to devm_request_irq. This eliminates the
mxs_auart_free_gpio_irq function and its call sites, and the
out_iounmap error label. Simplify the remove function accordingly.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/tty/serial/mxs-auart.c | 53 +++++++---------------------------
 1 file changed, 11 insertions(+), 42 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index aa59a48bfad7..4499e3206e85 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -1517,15 +1517,6 @@ static int mxs_auart_init_gpios(struct mxs_auart_port *s, struct device *dev)
 	return 0;
 }
 
-static void mxs_auart_free_gpio_irq(struct mxs_auart_port *s)
-{
-	enum mctrl_gpio_idx i;
-
-	for (i = 0; i < UART_GPIO_MAX; i++)
-		if (s->gpio_irq[i] >= 0)
-			free_irq(s->gpio_irq[i], s);
-}
-
 static int mxs_auart_request_gpio_irq(struct mxs_auart_port *s)
 {
 	int *irq = s->gpio_irq;
@@ -1537,21 +1528,13 @@ static int mxs_auart_request_gpio_irq(struct mxs_auart_port *s)
 			continue;
 
 		irq_set_status_flags(irq[i], IRQ_NOAUTOEN);
-		err = request_irq(irq[i], mxs_auart_irq_handle,
-				IRQ_TYPE_EDGE_BOTH, dev_name(s->dev), s);
+		err = devm_request_irq(s->dev, irq[i], mxs_auart_irq_handle,
+				       IRQ_TYPE_EDGE_BOTH, dev_name(s->dev), s);
 		if (err)
 			dev_err(s->dev, "%s - Can't get %d irq\n",
 				__func__, irq[i]);
 	}
 
-	/*
-	 * If something went wrong, rollback.
-	 * Be careful: i may be unsigned.
-	 */
-	while (err && (i-- > 0))
-		if (irq[i] >= 0)
-			free_irq(irq[i], s);
-
 	return err;
 }
 
@@ -1596,18 +1579,12 @@ static int mxs_auart_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!r) {
-		ret = -ENXIO;
+	s->port.membase = devm_platform_get_and_ioremap_resource(pdev, 0, &r);
+	if (IS_ERR(s->port.membase)) {
+		ret = PTR_ERR(s->port.membase);
 		goto out_disable_clk;
 	}
-
 	s->port.mapbase = r->start;
-	s->port.membase = ioremap(r->start, resource_size(r));
-	if (!s->port.membase) {
-		ret = -ENOMEM;
-		goto out_disable_clk;
-	}
 	s->port.ops = &mxs_auart_ops;
 	s->port.iotype = UPIO_MEM;
 	s->port.fifosize = MXS_AUART_FIFO_SIZE;
@@ -1622,21 +1599,21 @@ static int mxs_auart_probe(struct platform_device *pdev)
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		ret = irq;
-		goto out_iounmap;
+		goto out_disable_clk;
 	}
 
 	s->port.irq = irq;
 	ret = devm_request_irq(&pdev->dev, irq, mxs_auart_irq_handle, 0,
 			       dev_name(&pdev->dev), s);
 	if (ret)
-		goto out_iounmap;
+		goto out_disable_clk;
 
 	platform_set_drvdata(pdev, s);
 
 	ret = mxs_auart_init_gpios(s, &pdev->dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to initialize GPIOs.\n");
-		goto out_iounmap;
+		goto out_disable_clk;
 	}
 
 	/*
@@ -1644,7 +1621,7 @@ static int mxs_auart_probe(struct platform_device *pdev)
 	 */
 	ret = mxs_auart_request_gpio_irq(s);
 	if (ret)
-		goto out_iounmap;
+		goto out_disable_clk;
 
 	auart_port[s->port.line] = s;
 
@@ -1667,11 +1644,7 @@ static int mxs_auart_probe(struct platform_device *pdev)
 	return 0;
 
 out_free_qpio_irq:
-	mxs_auart_free_gpio_irq(s);
-	auart_port[pdev->id] = NULL;
-
-out_iounmap:
-	iounmap(s->port.membase);
+	auart_port[s->port.line] = NULL;
 
 out_disable_clk:
 	clk_disable_unprepare(s->clk);
@@ -1683,11 +1656,7 @@ static void mxs_auart_remove(struct platform_device *pdev)
 	struct mxs_auart_port *s = platform_get_drvdata(pdev);
 
 	uart_remove_one_port(&auart_driver, &s->port);
-	auart_port[pdev->id] = NULL;
-	mxs_auart_free_gpio_irq(s);
-	iounmap(s->port.membase);
-	if (is_asm9260_auart(s))
-		clk_disable_unprepare(s->clk);
+	auart_port[s->port.line] = NULL;
 }
 
 static struct platform_driver mxs_auart_driver = {
-- 
2.54.0

[PATCHv3 4/6] serial: mxs-auart: fix IRQ registration ordering and manage console clock

[Rosen Penev]

Move the main UART IRQ registration after uart_add_one_port so that
s->port.state and s->port.lock are initialized before the interrupt
handler can run. Mask all UART interrupts before adding the port to
prevent spurious IRQs left by the bootloader.

After probe succeeds, disable the module clock for non-console ports
since startup will re-enable it on port open. For console ports, keep
the clock prepared so auart_console_write() can safely call
clk_enable() from atomic context.

Guard the IRQ handler and get_mctrl with clk_enable/clk_disable since
GPIO IRQs and serial-core status queries can fire while the clock is
disabled for non-console ports.

In remove, disable the clock for console ports to balance the enable
done in probe, preventing a clock leak on unbind.

Assisted-by: opencode:big-pickle
---
 drivers/tty/serial/mxs-auart.c | 49 +++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 4499e3206e85..e2b656638ab3 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -738,9 +738,13 @@ static u32 mxs_auart_modem_status(struct mxs_auart_port *s, u32 mctrl)
 static u32 mxs_auart_get_mctrl(struct uart_port *u)
 {
 	struct mxs_auart_port *s = to_auart_port(u);
-	u32 stat = mxs_read(s, REG_STAT);
+	u32 stat;
 	u32 mctrl = 0;
 
+	clk_enable(s->clk);
+	stat = mxs_read(s, REG_STAT);
+	clk_disable(s->clk);
+
 	if (stat & AUART_STAT_CTS)
 		mctrl |= TIOCM_CTS;
 
@@ -1079,6 +1083,7 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
 	struct mxs_auart_port *s = context;
 	u32 mctrl_temp = s->mctrl_prev;
 
+	clk_enable(s->clk);
 	uart_port_lock(&s->port);
 
 	stat = mxs_read(s, REG_STAT);
@@ -1118,6 +1123,7 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
 	}
 
 	uart_port_unlock(&s->port);
+	clk_disable(s->clk);
 
 	return IRQ_HANDLED;
 }
@@ -1603,10 +1609,6 @@ static int mxs_auart_probe(struct platform_device *pdev)
 	}
 
 	s->port.irq = irq;
-	ret = devm_request_irq(&pdev->dev, irq, mxs_auart_irq_handle, 0,
-			       dev_name(&pdev->dev), s);
-	if (ret)
-		goto out_disable_clk;
 
 	platform_set_drvdata(pdev, s);
 
@@ -1627,9 +1629,28 @@ static int mxs_auart_probe(struct platform_device *pdev)
 
 	mxs_auart_reset_deassert(s);
 
+	/* Mask all UART interrupts to prevent spurious IRQs from bootloader */
+	mxs_write(0, s, REG_INTR);
+
 	ret = uart_add_one_port(&auart_driver, &s->port);
-	if (ret)
-		goto out_free_qpio_irq;
+	if (ret) {
+		auart_port[s->port.line] = NULL;
+		goto out_disable_clk;
+	}
+
+	/*
+	 * Request the main IRQ after uart_add_one_port so that
+	 * s->port.state and s->port.lock are initialized before
+	 * the handler can run in response to a bootloader-left
+	 * interrupt.
+	 */
+	ret = devm_request_irq(&pdev->dev, irq, mxs_auart_irq_handle, 0,
+			       dev_name(&pdev->dev), s);
+	if (ret) {
+		uart_remove_one_port(&auart_driver, &s->port);
+		auart_port[s->port.line] = NULL;
+		goto out_disable_clk;
+	}
 
 	/* ASM9260 don't have version reg */
 	if (is_asm9260_auart(s)) {
@@ -1641,10 +1662,16 @@ static int mxs_auart_probe(struct platform_device *pdev)
 			 (version >> 16) & 0xff, version & 0xffff);
 	}
 
-	return 0;
+	/*
+	 * Disable clock -- startup will re-enable when the port is opened.
+	 * For the console port the clock must stay prepared so that
+	 * auart_console_write() can safely call clk_enable() from
+	 * atomic context.
+	 */
+	if (!uart_console(&s->port))
+		clk_disable_unprepare(s->clk);
 
-out_free_qpio_irq:
-	auart_port[s->port.line] = NULL;
+	return 0;
 
 out_disable_clk:
 	clk_disable_unprepare(s->clk);
@@ -1657,6 +1684,8 @@ static void mxs_auart_remove(struct platform_device *pdev)
 
 	uart_remove_one_port(&auart_driver, &s->port);
 	auart_port[s->port.line] = NULL;
+	if (uart_console(&s->port))
+		clk_disable_unprepare(s->clk);
 }
 
 static struct platform_driver mxs_auart_driver = {
-- 
2.54.0

[PATCHv3 5/6] serial: mxs-auart: clamp RX DMA count to buffer size

[Rosen Penev]

In dma_rx_callback(), the RX count from the hardware status register
(AUART_STAT_RXCOUNT_MASK = 0xffff) is passed directly to
tty_insert_flip_string() without any bounds check. Since rx_dma_buf
is allocated with UART_XMIT_SIZE (4096 bytes), a hardware fault or
compromised peripheral reporting a count larger than 4096 would cause
an out-of-bounds read, potentially leaking kernel memory.

Clamp the count to UART_XMIT_SIZE before use.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/tty/serial/mxs-auart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index e2b656638ab3..fe48a372d022 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -823,7 +823,7 @@ static void dma_rx_callback(void *arg)
 	stat &= ~(AUART_STAT_OERR | AUART_STAT_BERR |
 			AUART_STAT_PERR | AUART_STAT_FERR);
 
-	count = stat & AUART_STAT_RXCOUNT_MASK;
+	count = min_t(u32, stat & AUART_STAT_RXCOUNT_MASK, UART_XMIT_SIZE);
 	tty_insert_flip_string(port, s->rx_dma_buf, count);
 
 	mxs_write(stat, s, REG_STAT);
-- 
2.54.0

Re: [PATCHv3 5/6] serial: mxs-auart: clamp RX DMA count to buffer size

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-bounds array write to the global `auart_port` array during error handling and removal due to `pdev->id` being `-1`.
- [High] The driver frees DMA buffers and releases channels without first terminating active DMA transfers, leading to potential memory corruption or use-after-free in asynchronous callbacks.
- [High] If `mxs_auart_dma_tx()` fails, the error is ignored, leaving `MXS_AUART_DMA_TX_SYNC` set forever and locking up TX. Additionally, popped FIFO data is permanently lost.
- [Medium] Attempting to free an unallocated GPIO IRQ on error rollback triggers a kernel warning.
- [Medium] DMA mappings are leaked in `mxs_auart_dma_prep_rx()` and `mxs_auart_dma_tx()` if the DMA engine descriptor preparation fails, and `dma_map_sg()` return values are unchecked.
--

commit 5c41f469d68450e9f6b63b5ed62412e5d03637ef
Author: Rosen Penev <rosenp@gmail.com>

serial: mxs-auart: clamp RX DMA count to buffer size

In dma_rx_callback(), the rx count from the hardware status register
is passed directly to tty_insert_flip_string() without any bounds check.
Since rx_dma_buf is allocated with the transmit size, a hardware fault or
compromised peripheral reporting a large count would cause an out-of-bounds
read. This clamps the count before use to prevent leaking kernel memory.

> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index 693b491f1e755..7e5348e5d63f4 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -819,7 +819,7 @@ static void dma_rx_callback(void *arg)
>  	stat &= ~(AUART_STAT_OERR | AUART_STAT_BERR |
>  			AUART_STAT_PERR | AUART_STAT_FERR);
>  
> -	count = stat & AUART_STAT_RXCOUNT_MASK;
> +	count = min_t(u32, stat & AUART_STAT_RXCOUNT_MASK, UART_XMIT_SIZE);
>  	tty_insert_flip_string(port, s->rx_dma_buf, count);

[Severity: High]
This is a pre-existing issue, but does this callback risk a use-after-free
if a dma transfer is in progress during shutdown?

When the uart is shut down, mxs_auart_dma_exit_channel() frees the buffers:

mxs_auart_dma_exit_channel() {
    ...
    kfree(s->tx_dma_buf);
    kfree(s->rx_dma_buf);
    ...
}

It appears this happens without calling dmaengine_terminate_sync() or
dmaengine_terminate_all(). Could a hardware dma controller continue writing
to the freed memory, or a pending tasklet invoke dma_rx_callback() and
dereference s->rx_dma_buf after it is freed?

>  
>  	mxs_write(stat, s, REG_STAT);

[Severity: High]
This is a pre-existing issue, but can the global auart_port array be
corrupted during error handling or removal?

When instantiated via device tree, pdev->id is implicitly -1. In
mxs_auart_probe(), the port is safely registered using its device tree
alias id:

mxs_auart_probe() {
    ...
    auart_port[s->port.line] = s;
    ...
}

However, the cleanup path and mxs_auart_remove() attempt to clear the
array entry using pdev->id:

mxs_auart_probe() {
    ...
out_free_qpio_irq:
    auart_port[pdev->id] = NULL;
}

Could this result in an out-of-bounds write to the array at index -1,
corrupting adjacent memory?

[Severity: Medium]
This is a pre-existing issue, but does the rollback loop in
mxs_auart_request_gpio_irq() attempt to free an unallocated irq?

When request_irq() fails, the loop increments before exiting:

mxs_auart_request_gpio_irq() {
    ...
    for (i = 0; !err && i < UART_GPIO_MAX; i++) {
        ...
        err = request_irq(irq[i], ...);
    }
    ...
}

The subsequent cleanup loop then decrements:

    while (err && (i-- > 0))
        if (irq[i] >= 0)
            free_irq(irq[i], s);

If request_irq() fails for index N, the for loop leaves i at N + 1. The
cleanup loop's first iteration operates on i = N, calling free_irq() on the
interrupt that just failed to request. Does this trigger a warning for
trying to free an already-free irq?

[Severity: Medium]
This is a pre-existing issue, but do mxs_auart_dma_prep_rx() and
mxs_auart_dma_tx() leak dma mappings on failure?

In both functions, dma_map_sg() is called without checking its return value.
If dmaengine_prep_slave_sg() subsequently fails, the functions return
early:

mxs_auart_dma_prep_rx() {
    ...
    dma_map_sg(...);
    desc = dmaengine_prep_slave_sg(...);
    if (!desc) {
        dev_err(s->dev, "step 2 error\n");
        return -1;
    }
    ...
}

Should these failure paths call dma_unmap_sg() to prevent leaking mapping
entries and address space?

[Severity: High]
This is a pre-existing issue, but does ignoring the return value of
mxs_auart_dma_tx() risk permanently locking up transmit?

In mxs_auart_tx_chars(), data is popped from the fifo before calling
mxs_auart_dma_tx():

mxs_auart_tx_chars() {
    ...
    i = kfifo_out(&port->state->xmit.fifo, ...);
    if (i) {
        mxs_auart_dma_tx(s, i);
    } else {
    ...
}

If mxs_auart_dma_tx() fails and returns an error, mxs_auart_tx_chars()
ignores the return value and leaves the transmit sync flag set.
Will subsequent calls to mxs_auart_tx_chars() return immediately,
locking up the transmit path? Is the data popped from the fifo also
permanently lost?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611033856.6476-6-rosenp@gmail.com?part=1

[PATCHv3 6/6] serial: mxs-auart: terminate DMA before releasing channels in exit

[Rosen Penev]

mxs_auart_dma_exit_channel() calls dma_release_channel() and then
kfree() on the DMA buffers without first terminating any in-flight
transfers. If an asynchronous DMA transfer completes after the buffers
have been freed, the callback will access freed memory.

Call dmaengine_terminate_sync() on each channel before releasing it
to safely abort pending transfers.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/tty/serial/mxs-auart.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index fe48a372d022..ec2c60dd0f52 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -872,10 +872,12 @@ static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s)
 static void mxs_auart_dma_exit_channel(struct mxs_auart_port *s)
 {
 	if (s->tx_dma_chan) {
+		dmaengine_terminate_sync(s->tx_dma_chan);
 		dma_release_channel(s->tx_dma_chan);
 		s->tx_dma_chan = NULL;
 	}
 	if (s->rx_dma_chan) {
+		dmaengine_terminate_sync(s->rx_dma_chan);
 		dma_release_channel(s->rx_dma_chan);
 		s->rx_dma_chan = NULL;
 	}
-- 
2.54.0

Re: [PATCHv3 6/6] serial: mxs-auart: terminate DMA before releasing channels in exit

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Terminating DMA transfers prevents their completion callbacks from executing, permanently leaking DMA scatterlist mappings.
- [High] The MXS_AUART_DMA_ENABLED flag is cleared too late, allowing concurrent callbacks and transmit operations to trigger a use-after-free and NULL pointer dereference on DMA channels/buffers.
- [High] Missing port lock in `mxs_auart_settermios` causes a read-modify-write race on shared hardware state.
- [High] DMA mappings are leaked on error paths in `mxs_auart_dma_tx` and `mxs_auart_dma_prep_rx`.
--

commit 0b036d5417b4eadfea5ff0f7490fcbdee3bda8bf
Author: Rosen Penev <rosenp@gmail.com>

serial: mxs-auart: terminate DMA before releasing channels in exit

The commit adds dmaengine_terminate_sync() to safely abort pending transfers
before releasing channels and freeing buffers, fixing a potential
use-after-free.

> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index 693b491f1e755..a853fb908591f 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -868,10 +868,12 @@ static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s)
>  static void mxs_auart_dma_exit_channel(struct mxs_auart_port *s)
>  {
>  	if (s->tx_dma_chan) {
> +		dmaengine_terminate_sync(s->tx_dma_chan);

[Severity: High]
This isn't a bug introduced by this patch, but does terminating the transfers
here lead to a leak of DMA scatterlist mappings?

The driver maps the DMA buffers using dma_map_sg() when preparing RX and TX
transfers, and only unmaps them inside the completion callbacks
(dma_rx_callback and dma_tx_callback).

Because dmaengine_terminate_sync() explicitly aborts pending transfers, the
dmaengine API guarantees that their completion callbacks will not be invoked.
As a result, the dma_unmap_sg() calls are skipped, leading to a persistent
leak of DMA mapping resources (e.g., IOVA space or SWIOTLB bounce buffers)
every time active transfers are terminated.

Furthermore, is it safe to call kfree() on memory while it is still actively
mapped via the DMA API?

>  		dma_release_channel(s->tx_dma_chan);
>  		s->tx_dma_chan = NULL;
>  	}
>  	if (s->rx_dma_chan) {
> +		dmaengine_terminate_sync(s->rx_dma_chan);
>  		dma_release_channel(s->rx_dma_chan);
>  		s->rx_dma_chan = NULL;
>  	}

[ ... ]

[Severity: High]
This is a pre-existing issue, but does clearing the MXS_AUART_DMA_ENABLED flag
after freeing the buffers open a window for a use-after-free and NULL pointer
dereference?

In mxs_auart_dma_exit(), mxs_auart_dma_exit_channel() is executed before the
MXS_AUART_DMA_ENABLED flag is cleared:

mxs_auart_dma_exit() {
    ...
    mxs_auart_dma_exit_channel(s);
    s->flags &= ~MXS_AUART_DMA_ENABLED;
}

Because this teardown is not protected by the port spinlock and can be invoked
dynamically via mxs_auart_settermios(), it can race with concurrent transmit
operations.

Specifically, mxs_auart_tx_chars() (called under the port lock) checks
auart_dma_enabled(s), which will incorrectly return true. It then attempts to
access the freed s->tx_dma_buf and initiates a transfer on the NULL
s->tx_dma_chan via mxs_auart_dma_tx().

Should the MXS_AUART_DMA_ENABLED flag be cleared under the port spinlock
before dismantling the DMA channels and freeing the buffers?


[Severity: High]
This is a pre-existing issue, but is there a read-modify-write race on shared
hardware state in mxs_auart_settermios()?

The serial core calls the driver's .set_termios callback without holding the
port spinlock. The mxs_auart_settermios() function reads the shared hardware
register REG_CTRL2, modifies it locally, and later writes the full 32-bit
value back without holding uart_port_lock(&s->port):

mxs_auart_settermios() {
    ...
    ctrl2 = mxs_read(s, REG_CTRL2);
    ...
    mxs_write(ctrl2, s, REG_CTRL2);
}

If a concurrent context like mxs_auart_start_tx() or mxs_auart_stop_tx()
(which correctly hold the port lock) uses hardware-atomic bit operations
to enable or disable the transmitter during this window,
mxs_auart_settermios() will overwrite REG_CTRL2 with a stale cached value.

Can this unintentionally undo concurrent hardware changes and lead to a stalled
transmitter or receiver?


[Severity: High]
This is a pre-existing issue, but do the error paths in mxs_auart_dma_tx()
and mxs_auart_dma_prep_rx() leak DMA mappings?

The driver maps its DMA buffers via dma_map_sg() in both functions. If the
subsequent calls to dmaengine_prep_slave_sg() fail (e.g., due to descriptor
exhaustion), the driver immediately returns an error without calling
dma_unmap_sg():

mxs_auart_dma_tx() {
    ...
    dma_map_sg(s->dev, sgl, 1, DMA_TO_DEVICE);
    desc = dmaengine_prep_slave_sg(channel, sgl,
        1, DMA_MEM_TO_DEV, DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
    if (!desc) {
        dev_err(s->dev, "step 2 error\n");
        return -EINVAL;
    }
    ...
}

Does this permanently leak the DMA mappings (such as IOMMU entries or SWIOTLB
bounce buffers) every time the preparation fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611033856.6476-7-rosenp@gmail.com?part=1