Re: [PATCH] serial: mxs-auart: fix probe error paths and clock handling

[sashiko-bot@kernel.org]

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

New issues:
- [High] Hardware / Clock Initialization Sequence
- [Medium] Power Management / Clock Gating Regression

Pre-existing issues:
- [High] Concurrency / Early IRQ Registration
--

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

serial: mxs-auart: fix probe error paths and clock handling

This commit converts the mxs-auart driver to fully use devres helpers to
simplify error handling during probe. It switches to devm_clk_get_enabled()
and devm_platform_get_and_ioremap_resource(), and uses devm_request_irq()
for GPIO interrupts.

> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index 693b491f1e755..0f3df8222ca76 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)
>  {
>  	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);
>  	}

[Severity: Medium]
Will this prevent the hardware clock from being dynamically gated when the UART
port is closed?

By switching to devm_clk_get_enabled() here, the base reference count for the
clock is incremented during probe and never drops to zero while the device is
bound.

Previously, the driver enabled the clock dynamically via clk_prepare_enable()
in mxs_auart_startup() and disabled it in mxs_auart_shutdown(), which seems
to allow for power savings.

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

[Severity: High]
Does this sequence violate the hardware dependency and clock constraints?

The patch now enables the mod clock before the ahb bus clock, which violates
the typical hardware dependency where the bus clock must be active first to
prevent bus hangs.

Furthermore, it calls clk_set_rate() on the mod clock after it has already
been enabled. Modifying the rate of an active clock violates the
CLK_SET_RATE_GATE constraint and could result in hardware glitches or cause
clk_set_rate() to fail with -EBUSY, which would fail the probe entirely.

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but is it safe to register this IRQ before the
port is fully initialized?

If the bootloader leaves the UART active with pending interrupts, an interrupt
can fire immediately after devm_request_irq() is called.

This happens before uart_add_one_port() is called, so mxs_auart_irq_handle()
will execute, acquire an uninitialized spinlock (s->port.lock), and dereference
s->port.state which is NULL at this point. This can result in a NULL pointer
dereference (for example, in mxs_auart_rx_chars() calling
tty_flip_buffer_push(&s->port.state->port)) and a kernel panic.

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