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

[sashiko-bot@kernel.org]

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