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

[PATCH 0/3] 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 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 2 converts iomem mapping and GPIO IRQ requests to devm,
removing the manual cleanup paths.

Patch 3 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.

Rosen Penev (3):
  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

 drivers/tty/serial/mxs-auart.c | 133 ++++++++++++++-------------------
 1 file changed, 55 insertions(+), 78 deletions(-)

-- 
2.54.0

[PATCH 1/3] 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 697318dbb146..1390fa000a5b 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: [PATCH 1/3] serial: mxs-auart: rework clock handling in mxs_get_clks and probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unconditional execution of `clk_prepare_enable(s->clk)` in `probe()` causes a clock enable leak and power regression for non-ASM9260 devices. Note: I found the fix later in the series in patch "serial: mxs-auart: fix IRQ registration ordering and manage console clock".
--

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

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

This commit reworks clock handling by moving the mod clock enable into probe
to avoid CLK_SET_RATE_GATE failures and uses devm_clk_get_enabled for the
AHB clock.

> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index 693b491f1e755..8d050ceb72d2f 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
[ ... ]
> @@ -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;
> +
[ ... ]
> @@ -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);
> -	}
>  }

[Severity: Medium]
Since clk_prepare_enable(s->clk) is now called unconditionally in probe for
all device variants, does keeping the is_asm9260_auart(s) check here cause
a clock enable reference leak for non-ASM9260 hardware when the device is
removed?

I noted that this is fixed later in the series by the patch "serial:
mxs-auart: fix IRQ registration ordering and manage console clock", but
would this intermediate step break bisectability by introducing a temporary
leak?

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

Re: [PATCH 1/3] serial: mxs-auart: rework clock handling in mxs_get_clks and probe

[Frank Li]

On Tue, Jun 02, 2026 at 07:58:55PM -0700, Rosen Penev wrote:
> 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 697318dbb146..1390fa000a5b 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;
> +

why not direct enable clock when get?
	s->clk = devm_clk_get(s->dev, "mod");

Frank
>  	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
>

[PATCH 2/3] 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 | 55 ++++++++--------------------------
 1 file changed, 12 insertions(+), 43 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 1390fa000a5b..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;
 }
 
@@ -1586,7 +1569,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)
@@ -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

Re: [PATCH 2/3] serial: mxs-auart: use devm resources for iomem and GPIO IRQs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing `clk_disable_unprepare()` for `s->clk` in `mxs_auart_remove()` causes a clock enable count leak.
--

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

serial: mxs-auart: use devm resources for iomem and GPIO IRQs

This commit replaces platform_get_resource and ioremap with
devm_platform_get_and_ioremap_resource. It also converts GPIO IRQ requests to
use devm_request_irq, eliminating the need for manual cleanup and simplifying
the remove function.

> 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

[ ... ]

> @@ -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);

[Severity: Medium]
Does removing clk_disable_unprepare() here cause a clock enable count leak
when the driver is unbound?

Looking at mxs_auart_probe(), the driver acquires s->clk using devm_clk_get(),
but it is explicitly prepared and enabled via clk_prepare_enable(s->clk).
Because it cannot use devm_clk_get_enabled() (its rate must be set while the
clock is disabled), its lifecycle is unmanaged.

While subsequent patches might disable the clock for non-console ports at the
end of probe(), console ports leave it enabled. Won't deleting this manual
cleanup leave the clock's prepare and enable counts unbalanced on unbind?

> +	auart_port[s->port.line] = NULL;
>  }

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

Re: [PATCH 2/3] serial: mxs-auart: use devm resources for iomem and GPIO IRQs

[Frank Li]

On Tue, Jun 02, 2026 at 07:58:56PM -0700, Rosen Penev wrote:
> 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 | 55 ++++++++--------------------------
>  1 file changed, 12 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index 1390fa000a5b..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;
>  }
>
> @@ -1586,7 +1569,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);

This change need seperate patch

Frank
>
>  	ret = mxs_get_clks(s, pdev);
>  	if (ret)
> @@ -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
>

[PATCH 3/3] 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.

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

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 4499e3206e85..d40d82f112c0 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -1603,10 +1603,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 +1623,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 +1656,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);
--
2.54.0

Re: [PATCH 3/3] serial: mxs-auart: fix IRQ registration ordering and manage console clock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Disabling the module clock at the end of `probe()` for non-console ports causes a kernel panic when the serial core queries the port status (e.g., via `/proc/tty/driver/serial`). The driver does not implement a `.pm` callback or use `pm_runtime`. Consequently, `uart_change_pm()` acts as a no-op, and `uport->ops->get_mctrl()` performs an unclocked MMIO read of `REG_STAT`.
- [High] A spurious interrupt arriving while the port is closed will cause a kernel panic due to the clock being disabled. The main IRQ is requested without `IRQF_NO_AUTOEN` (or similar mechanisms to mask it at the irqchip). If triggered, the ISR unconditionally accesses unclocked MMIO registers.
--

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

serial: mxs-auart: fix IRQ registration ordering and manage console clock

This commit moves the main UART IRQ registration after uart_add_one_port() to
ensure port states are initialized before the handler runs, and masks
interrupts to prevent spurious IRQs. It also disables the module clock for
non-console ports after probe, assuming startup will re-enable it on port
open.

> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index 60a683a497405..35ba2ae740976 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
[ ... ]
> @@ -1627,9 +1623,28 @@ static int mxs_auart_probe(struct platform_device *pdev)
[ ... ]
> +	/*
> +	 * 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);

[Severity: High]
Does this leave us vulnerable to a crash if a spurious interrupt arrives
while the port is closed?

The main IRQ is requested without IRQF_NO_AUTOEN (or similar mechanisms), so
it remains unmasked at the interrupt controller. If an interrupt fires after
the clock is disabled below, mxs_auart_irq_handle() will perform an
unconditional unclocked MMIO read:

drivers/tty/serial/mxs-auart.c:mxs_auart_irq_handle() {
	...
	stat = mxs_read(s, REG_STAT);
	...
}

[ ... ]
> @@ -1641,10 +1656,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);

[Severity: High]
Will this cause a crash when the serial core queries the port status (e.g.,
when reading /proc/tty/driver/serial)?

The serial core invokes uart_line_info(), which calls uport->ops->get_mctrl()
on the port. Because this driver lacks a .pm callback and doesn't use
pm_runtime, the serial core's uart_change_pm() acts as a no-op and the clock
will remain disabled.

This results in mxs_auart_get_mctrl() executing an unclocked MMIO read:

drivers/tty/serial/mxs-auart.c:mxs_auart_get_mctrl() {
	...
	u32 stat = mxs_read(s, REG_STAT);
	...
}

>  
> -out_free_qpio_irq:
> -	auart_port[s->port.line] = NULL;
> +	return 0;

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