Re: [PATCH] serial: msm: select PM_OPP

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Scatterlist DMA length is accessed after the scatterlist is unmapped, leading to possible underflow and FIFO corruption.
- [High] The driver unconditionally ignores the return value of clk_prepare_enable(), which can cause a synchronous external abort if the clock fails to enable.
--

commit 2cb7eaaedfaf012a0738d9fb2147009502a57d43
Author: Dmitry Sinyavin <sinyavin@gmail.com>

serial: msm: select PM_OPP

Commit 173ebdedcd84 made the MSM serial driver unconditionally configure
the core clock through the OPP framework during probe. This commit selects
PM_OPP to keep minimal configurations functional when CONFIG_PM_OPP is
disabled.

> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index cf7dba473b203..bf680d22199b1 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -904,6 +904,7 @@ config SERIAL_JSM
>  config SERIAL_MSM
>  	tristate "MSM on-chip serial port support"
>  	depends on ARCH_QCOM || COMPILE_TEST
> +	select PM_OPP
>  	select SERIAL_CORE
>  
>  config SERIAL_MSM_CONSOLE

While reviewing this patch, I noticed a few regressions in the
msm_serial driver itself:

drivers/tty/serial/msm_serial.c:msm_complete_tx_dma() {
...
	dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir);
...
	count = sg_dma_len(&dma->tx_sg) - state.residue;
	uart_xmit_advance(port, count);
...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this code underflow the
count and corrupt the TTY ring buffer?

The msm_complete_tx_dma() function unconditionally reads
sg_dma_len(&dma->tx_sg) immediately after calling dma_unmap_sg(&dma->tx_sg).

On architectures where CONFIG_DMA_API_DEBUG or SWIOTLB invalidates SG
entries by setting the length to 0, sg_dma_len() evaluates to 0.

Since state.residue can be non-zero and count is unsigned, wouldn't
count = 0 - state.residue underflow to a massive value and advance the
pointer out of bounds?


drivers/tty/serial/msm_serial.c:msm_init_clock() {
...
	clk_prepare_enable(msm_port->clk);
	clk_prepare_enable(msm_port->pclk);
	msm_serial_set_mnd_regs(port);
...
}

[Severity: High]
This is a pre-existing issue, but can this cause a synchronous external
abort if the clock fails to enable?

The msm_init_clock() function calls clk_prepare_enable() but does not
check the return value.

If the clock subsystem fails to enable the clock, execution proceeds
directly to msm_serial_set_mnd_regs(), which performs MMIO register
writes to the unclocked peripheral.

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