Re: [PATCH v3 09/18] clk: mediatek: Add MT8189 apmixedsys clock support

[sashiko-bot@kernel.org]

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

New issues:
- [High] The `PLL_SETCLR` macro omits `.ops` and `.pwr_reg`, causing `mtk_pll_prepare()` to corrupt offset 0 of the APMIXEDSYS controller.

Pre-existing issues:
- [High] Stale state in the static `pllfhs` array persists after unbind, causing unmapped memory access (Use-After-Free) if the driver is rebound and `fhctl_parse_dt()` fails.
--

--- Patch [9]: [PATCH v3 09/18] clk: mediatek: Add MT8189 apmixedsys clock support ---
commit 025aa9a1940ce762ce47408b18d63d2592369750
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

clk: mediatek: Add MT8189 apmixedsys clock support

Add support for the MT8189 apmixedsys clock controller, which provides
PLLs generated from SoC 26m.

> diff --git a/drivers/clk/mediatek/clk-mt8189-apmixedsys.c b/drivers/clk/mediatek/clk-mt8189-apmixedsys.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8189-apmixedsys.c

[ ... ]

> +#define PLL_SETCLR(_id, _name, _reg, _en_setclr_bit,		\
> +			_rstb_setclr_bit, _flags, _pd_reg,	\
> +			_pd_shift, _tuner_reg, _tuner_en_reg,	\
> +			_tuner_en_bit, _pcw_reg, _pcw_shift,	\
> +			_pcwbits) {				\
> +		.id = _id,					\
> +		.name = _name,					\
> +		.en_reg = MT8189_PLLEN_OFS,			\
> +		.reg = _reg,					\
> +		.pll_en_bit = _en_setclr_bit,			\
> +		.rst_bar_mask = BIT(_rstb_setclr_bit),		\
> +		.flags = _flags,				\
> +		.fmax = MT8189_PLL_FMAX,			\
> +		.fmin = MT8189_PLL_FMIN,			\
> +		.pd_reg = _pd_reg,				\
> +		.pd_shift = _pd_shift,				\
> +		.tuner_reg = _tuner_reg,			\
> +		.tuner_en_reg = _tuner_en_reg,			\
> +		.tuner_en_bit = _tuner_en_bit,			\
> +		.pcw_reg = _pcw_reg,				\
> +		.pcw_shift = _pcw_shift,			\
> +		.pcwbits = _pcwbits,				\
> +		.pcwibits = MT8189_INTEGER_BITS,		\
> +	}

[Severity: High]
Does this macro need to initialize .ops and .pwr_reg?

Because .pwr_reg defaults to 0 and .ops is missing, the clock core will fall
back to the default mtk_pll_ops. When mtk_pll_prepare() is called, it seems
it will unconditionally read and write to base + 0 (since pwr_reg is 0).

Will this inadvertently toggle bits 0 and 1 of the register at offset 0
every time a PLL is prepared or unprepared?

Additionally, falling back to the default ops causes the driver to perform
non-atomic read-modify-write operations on the enable register instead of
using the intended SET/CLR operations.

[ ... ]

> +static struct mtk_pllfh_data pllfhs[] = {
> +	FH(CLK_APMIXED_ARMPLL_LL, 0, 0x003C),
> +	FH(CLK_APMIXED_ARMPLL_BL, 1, 0x0050),
> +	FH(CLK_APMIXED_CCIPLL, 2, 0x0064),
> +	FH(CLK_APMIXED_MAINPLL, 3, 0x0078),
> +	FH(CLK_APMIXED_MMPLL, 4, 0x008C),
> +	FH(CLK_APMIXED_MFGPLL, 5, 0x00A0),
> +	FH(CLK_APMIXED_EMIPLL, 6, 0x00B4),
> +	FH(CLK_APMIXED_TVDPLL1, 7, 0x00C8),
> +	FH(CLK_APMIXED_TVDPLL2, 8, 0x00DC),
> +	FH(CLK_APMIXED_MSDCPLL, 9, 0x00F0),
> +	FH(CLK_APMIXED_UFSPLL, 10, 0x0104),
> +	FH(CLK_APMIXED_APUPLL, 11, 0x0118),
> +	FH(CLK_APMIXED_APUPLL2, 12, 0x012c),
> +};

[Severity: High]
This is a pre-existing issue, but does this static array retain stale state
across driver unbind and rebind cycles?

During a successful probe, fhctl_parse_dt() sets pllfh->state.fh_enable = 1
and stores the mapped I/O pointer in pllfh->state.base. When the driver is
unbound, mtk_clk_unregister_pllfhs() unmaps this memory but doesn't appear
to clear the state in the static array.

If the driver is later rebound and fhctl_parse_dt() fails (for example, due
to an of_iomap failure), it returns without updating the array. Will
mtk_clk_register_pllfhs() then read the stale fh_enable == 1 and pass the
unmapped state.base pointer to the clock framework, leading to unmapped memory
accesses on subsequent clock operations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-mt8189-clocks-system-base-v3-0-8e6d99ab3ad2@collabora.com?part=9