[PATCH] serial: msm: select PM_OPP

[PATCH] serial: msm: select PM_OPP

[Dmitry Sinyavin]

Commit 173ebdedcd84 ("serial: msm: Use OPP table for DVFS support")
made the MSM serial driver unconditionally configure the core clock
through the OPP framework during probe.

When CONFIG_PM_OPP is disabled, devm_pm_opp_set_clkname() is a stub that
returns -EOPNOTSUPP, so every MSM serial port fails to probe. Select
PM_OPP to keep minimal configurations functional.

Fixes: 173ebdedcd84 ("serial: msm: Use OPP table for DVFS support")
Signed-off-by: Dmitry Sinyavin <sinyavin@gmail.com>
---
Tested with an ARM multi_v7_defconfig after forcing SERIAL_MSM=y and
PM_OPP=n. olddefconfig selected PM_OPP=y, and msm_serial.o built with
Clang and W=1.

 drivers/tty/serial/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index cf7dba473b20..bf680d22199b 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

base-commit: 45c13f3f9e3bb15fd89ff2864c6f627a3b4b4229
-- 
2.53.0

Re: [PATCH] serial: msm: select PM_OPP

[sashiko-bot]

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

Re: [PATCH] serial: msm: select PM_OPP

[sinyavin]

Thanks for the review.

Both findings concern pre-existing code and are unrelated to this
Kconfig-only change, so they are outside the scope of this patch.