* [PATCH] serial: mxs-auart: fix probe error paths and clock handling
@ 2026-05-28 23:06 Rosen Penev
2026-05-30 5:21 ` Greg Kroah-Hartman
0 siblings, 1 reply; 3+ messages in thread
From: Rosen Penev @ 2026-05-28 23:06 UTC (permalink / raw)
To: linux-serial
Cc: Greg Kroah-Hartman, Jiri Slaby, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam,
open list:TTY LAYER AND SERIAL DRIVERS,
open list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
Sashiko reported three pre-existing bugs in the mxs-auart driver:
- For non-ASM9260 variants, mxs_get_clks() obtained the clock but never
prepared/enabled it, leaving register accesses in probe at risk of
faulting if the bootloader had gated the clock.
- The error path and remove function used pdev->id instead of
s->port.line to clear the auart_port[] slot. For DT-probed devices
pdev->id is -1, causing an out-of-bounds write and leaving a dangling
pointer in the array.
- The probe error path called iounmap() while the IRQ was still
registered. An interrupt during that window would dereference the
unmapped membase.
All of this is a consequence of using mixed devm and non devm. Instead
of working around these issues, go full devm so that everything can be
cleaned up properly.
devm_clk_get_enabled allows removing clk_prepare_enable.
devm_platform_get_and_ioremap_resource is a bit worrying as it calls
request_mem_region. Manual review of the dts files shows no overlapping
memory regions, so should be good to go.
Use devm in mxs_auart_request_gpio_irq as devm is used in probe.
Assisted-by: Opencode:Big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail•com>
---
drivers/tty/serial/mxs-auart.c | 110 ++++++---------------------------
1 file changed, 20 insertions(+), 90 deletions(-)
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 697318dbb146..8b6463df11b3 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -1457,48 +1457,24 @@ static void mxs_init_regs(struct mxs_auart_port *s)
static int mxs_get_clks(struct mxs_auart_port *s,
struct platform_device *pdev)
{
- int err;
-
if (!is_asm9260_auart(s)) {
- s->clk = devm_clk_get(&pdev->dev, NULL);
+ s->clk = devm_clk_get_enabled(&pdev->dev, NULL);
return PTR_ERR_OR_ZERO(s->clk);
}
- s->clk = devm_clk_get(s->dev, "mod");
+ s->clk = devm_clk_get_enabled(s->dev, "mod");
if (IS_ERR(s->clk)) {
dev_err(s->dev, "Failed to get \"mod\" clk\n");
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;
- }
-
- err = clk_set_rate(s->clk, clk_get_rate(s->clk_ahb));
- 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;
+ return clk_set_rate(s->clk, clk_get_rate(s->clk_ahb));
}
static int mxs_auart_init_gpios(struct mxs_auart_port *s, struct device *dev)
@@ -1529,15 +1505,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;
@@ -1549,21 +1516,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;
}
@@ -1604,18 +1563,10 @@ 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;
- goto out_disable_clks;
- }
-
+ s->port.membase = devm_platform_get_and_ioremap_resource(pdev, 0, &r);
+ if (IS_ERR(s->port.membase))
+ return PTR_ERR(s->port.membase);
s->port.mapbase = r->start;
- s->port.membase = ioremap(r->start, resource_size(r));
- if (!s->port.membase) {
- ret = -ENOMEM;
- goto out_disable_clks;
- }
s->port.ops = &mxs_auart_ops;
s->port.iotype = UPIO_MEM;
s->port.fifosize = MXS_AUART_FIFO_SIZE;
@@ -1628,23 +1579,20 @@ static int mxs_auart_probe(struct platform_device *pdev)
s->mctrl_prev = 0;
irq = platform_get_irq(pdev, 0);
- if (irq < 0) {
- ret = irq;
- goto out_iounmap;
- }
+ if (irq < 0)
+ return irq;
s->port.irq = irq;
- ret = devm_request_irq(&pdev->dev, irq, mxs_auart_irq_handle, 0,
- dev_name(&pdev->dev), s);
+ ret = devm_request_irq(&pdev->dev, irq, mxs_auart_irq_handle, 0, dev_name(&pdev->dev), s);
if (ret)
- goto out_iounmap;
+ return ret;
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;
+ return ret;
}
/*
@@ -1652,15 +1600,17 @@ static int mxs_auart_probe(struct platform_device *pdev)
*/
ret = mxs_auart_request_gpio_irq(s);
if (ret)
- goto out_iounmap;
+ return ret;
auart_port[s->port.line] = s;
mxs_auart_reset_deassert(s);
ret = uart_add_one_port(&auart_driver, &s->port);
- if (ret)
- goto out_free_qpio_irq;
+ if (ret) {
+ auart_port[s->port.line] = NULL;
+ return ret;
+ }
/* ASM9260 don't have version reg */
if (is_asm9260_auart(s)) {
@@ -1673,20 +1623,6 @@ 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);
-
-out_disable_clks:
- if (is_asm9260_auart(s)) {
- clk_disable_unprepare(s->clk);
- clk_disable_unprepare(s->clk_ahb);
- }
- return ret;
}
static void mxs_auart_remove(struct platform_device *pdev)
@@ -1694,13 +1630,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);
- clk_disable_unprepare(s->clk_ahb);
- }
+ auart_port[s->port.line] = NULL;
}
static struct platform_driver mxs_auart_driver = {
--
2.54.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] serial: mxs-auart: fix probe error paths and clock handling
2026-05-28 23:06 [PATCH] serial: mxs-auart: fix probe error paths and clock handling Rosen Penev
@ 2026-05-30 5:21 ` Greg Kroah-Hartman
2026-05-30 19:33 ` Rosen Penev
0 siblings, 1 reply; 3+ messages in thread
From: Greg Kroah-Hartman @ 2026-05-30 5:21 UTC (permalink / raw)
To: Rosen Penev
Cc: linux-serial, Jiri Slaby, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam,
open list:TTY LAYER AND SERIAL DRIVERS,
open list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
On Thu, May 28, 2026 at 04:06:16PM -0700, Rosen Penev wrote:
> Sashiko reported three pre-existing bugs in the mxs-auart driver:
>
> - For non-ASM9260 variants, mxs_get_clks() obtained the clock but never
> prepared/enabled it, leaving register accesses in probe at risk of
> faulting if the bootloader had gated the clock.
> - The error path and remove function used pdev->id instead of
> s->port.line to clear the auart_port[] slot. For DT-probed devices
> pdev->id is -1, causing an out-of-bounds write and leaving a dangling
> pointer in the array.
This feels like it should be a different patch, and not burried in this
change, right?
> - The probe error path called iounmap() while the IRQ was still
> registered. An interrupt during that window would dereference the
> unmapped membase.
>
> All of this is a consequence of using mixed devm and non devm. Instead
> of working around these issues, go full devm so that everything can be
> cleaned up properly.
Are you sure that devm and irq code is going to work properly? That's
always a very tricky code path, was this tested with the hardware to
ensure it still works on teardown correctly?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] serial: mxs-auart: fix probe error paths and clock handling
2026-05-30 5:21 ` Greg Kroah-Hartman
@ 2026-05-30 19:33 ` Rosen Penev
0 siblings, 0 replies; 3+ messages in thread
From: Rosen Penev @ 2026-05-30 19:33 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-serial, Jiri Slaby, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam,
open list:TTY LAYER AND SERIAL DRIVERS,
open list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
On Sat, May 30, 2026 at 3:25 AM Greg Kroah-Hartman
<gregkh@linuxfoundation•org> wrote:
>
> On Thu, May 28, 2026 at 04:06:16PM -0700, Rosen Penev wrote:
> > Sashiko reported three pre-existing bugs in the mxs-auart driver:
> >
> > - For non-ASM9260 variants, mxs_get_clks() obtained the clock but never
> > prepared/enabled it, leaving register accesses in probe at risk of
> > faulting if the bootloader had gated the clock.
> > - The error path and remove function used pdev->id instead of
> > s->port.line to clear the auart_port[] slot. For DT-probed devices
> > pdev->id is -1, causing an out-of-bounds write and leaving a dangling
> > pointer in the array.
>
> This feels like it should be a different patch, and not burried in this
> change, right?
>
> > - The probe error path called iounmap() while the IRQ was still
> > registered. An interrupt during that window would dereference the
> > unmapped membase.
> >
> > All of this is a consequence of using mixed devm and non devm. Instead
> > of working around these issues, go full devm so that everything can be
> > cleaned up properly.
>
> Are you sure that devm and irq code is going to work properly? That's
> always a very tricky code path, was this tested with the hardware to
> ensure it still works on teardown correctly?
Nope. v2 incoming.
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-05-30 19:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-28 23:06 [PATCH] serial: mxs-auart: fix probe error paths and clock handling Rosen Penev
2026-05-30 5:21 ` Greg Kroah-Hartman
2026-05-30 19:33 ` Rosen Penev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox