Re: [PATCH v2 4/5] clk: samsung: Introduce Exynos9610 clock controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Clock ID collision for `dout_cmu_usb_usb30drd` due to reused `CLK_DOUT_CMU_USB_BUS` ID.
- [High] The `dout_cmu_core_cci` divider incorrectly targets the `CORE_BUS` hardware register.
- [High] All PCLK clock gates for CMGP I2C and USI mistakenly map to IPCLK register offsets.
- [High] OCC clocks in the core domain suffer from register, naming, and parent copy-paste errors.
- [High] Several configured clock registers are omitted from the `clk_regs` save/restore arrays.
- [High] The `dispaud_cmu_info` structure is bound to the wrong sysreg save/restore array.
- [High] Six clock gates will be orphaned due to nonexistent parent clock references (typographical errors).
- [High] The `CLK_DOUT_CLK_CPUCL0_CPU` divider is instantiated with a bit width of 0.
- [High] Mux width is too narrow for `mout_cmu_cpucl0_switch`.
- [High] The probe function is marked `__init` but registered as a persistent platform driver, creating an execute-after-free risk.
--

--- Patch [4]: [PATCH v2 4/5] clk: samsung: Introduce Exynos9610 clock controller driver ---
commit 5ae09894e4056fd99b1810f99f65fba5b78df46a
Author: Alexandru Chimac <alex@chimac.ro>

clk: samsung: Introduce Exynos9610 clock controller driver

This commit introduces the Common Clock Framework driver for the Exynos 9610
SoC. It provides support for numerous clock domains, including TOP, APM,
CAM, CMGP, CORE, CPUCL0, DISPAUD, FSYS, G2D, G3D, PERI, and USB.

> diff --git a/drivers/clk/samsung/clk-exynos9610.c b/drivers/clk/samsung/clk-exynos9610.c
> index 0000000000000..39c1650247e8d
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-exynos9610.c
[ ... ]
> static const unsigned long top_clk_regs[] __initconst = {
> 	PLL_LOCKTIME_PLL_MMC,
> 	PLL_LOCKTIME_PLL_SHARED0,
[ ... ]
> 	CLK_CON_DIV_CLKCMU_CAM_BUS,
> 	CLK_CON_DIV_CLKCMU_CORE_BUS,
[ ... ]

[Severity: High]
Are some registers missing from the save and restore array?

The CLK_CON_DIV_PLL_MMC_DIV2 and the four CIS dividers 
(CLK_CON_DIV_CLKCMU_CIS_CLK0 through CLK3) appear to be omitted from 
top_clk_regs. Could this cause these dividers to revert to power-on defaults 
after a suspend and resume cycle?

[ ... ]
> static const struct samsung_mux_clock top_mux_clks[] __initconst = {
[ ... ]
> 	MUX(CLK_MOUT_CMU_CPUCL0_DBG, "mout_cmu_cpucl0_dbg", mout_cmu_cpucl0_dbg_p,
> 	    CLK_CON_MUX_MUX_CLKCMU_CPUCL0_DBG, 0, 1),
> 	MUX(CLK_MOUT_CMU_CPUCL0_SWITCH, "mout_cmu_cpucl0_switch", mout_cmu_cpucl0_switch_p,
> 	    CLK_CON_MUX_MUX_CLKCMU_CPUCL0_SWITCH, 0, 1),

[Severity: High]
Does this mux have a bit width that is too narrow?

The mout_cmu_cpucl0_switch_p parent array has 4 elements, but the width 
parameter is set to 1 bit. A 1-bit width can only represent indices 0 and 1, 
preventing the selection of parents at indices 2 and 3 when the CPU frequency 
scaling attempts to reparent the clock.

[ ... ]
> static const struct samsung_div_clock top_div_clks[] __initconst = {
[ ... ]
> 	DIV(CLK_DOUT_CMU_CORE_BUS, "dout_cmu_core_bus", "gout_cmu_core_bus",
> 	    CLK_CON_DIV_CLKCMU_CORE_BUS, 0, 4),
> 	DIV(CLK_DOUT_CMU_CORE_CCI, "dout_cmu_core_cci", "gout_cmu_core_cci",
> 	    CLK_CON_DIV_CLKCMU_CORE_BUS, 0, 4),

[Severity: High]
Is the wrong hardware register targeted by this divider?

The dout_cmu_core_cci divider targets CLK_CON_DIV_CLKCMU_CORE_BUS instead of 
CLK_CON_DIV_CLKCMU_CORE_CCI. Could this cause writes to the CCI divider to 
inadvertently modify the CORE_BUS divider, potentially hanging the core bus?

[ ... ]
> 	DIV(CLK_DOUT_CMU_USB_BUS, "dout_cmu_usb_bus", "gout_cmu_usb_bus",
> 	    CLK_CON_DIV_CLKCMU_USB_BUS, 0, 4),
> 	DIV(CLK_DOUT_CMU_USB_DPGTC, "dout_cmu_usb_dpgtc", "gout_cmu_usb_dpgtc",
> 	    CLK_CON_DIV_CLKCMU_USB_DPGTC, 0, 4),
> 	DIV(CLK_DOUT_CMU_USB_BUS, "dout_cmu_usb_usb30drd", "gout_cmu_usb_usb30drd",
> 	    CLK_CON_DIV_CLKCMU_USB_USB30DRD, 0, 4),

[Severity: High]
Is there an ID collision in this divider array?

The CLK_DOUT_CMU_USB_BUS identifier is reused for dout_cmu_usb_usb30drd. 
This duplicate definition overrides the earlier USB bus divider registration.

[ ... ]
> static const struct samsung_gate_clock top_gate_clks[] __initconst = {
> 	GATE(CLK_GOUT_CMU_MIF_SWITCH, "gout_cmu_mif_switch",
> 	     "mux_cmu_mif_switch", CLK_CON_GAT_CLKCMU_MIF_SWITCH,
> 	     21, 0, 0),

[Severity: High]
Is there a typographical error in the parent clock name?

This gate requests mux_cmu_mif_switch as its parent instead of the 
registered mout_cmu_mif_switch. Could this orphan the clock gate and 
prevent proper clock management for the MIF switch?

[ ... ]
> static const struct samsung_div_clock cpucl0_div_clks[] __initconst = {
[ ... ]
> 	DIV(CLK_DOUT_CLK_CPUCL0_CPU, "dout_clk_cpucl0_cpu", "mout_clk_cpucl0_pll",
> 	    CLK_CON_DIV_DIV_CLK_CPUCL0_CPU, 0, 0),

[Severity: High]
Could the bit width of 0 for this divider break CPU frequency scaling?

A 0-bit width results in a zero bitmask for the clock divider, causing 
configuration operations to become no-ops. Will this prevent dynamic 
frequency scaling for the CPU cluster?

[ ... ]
> static const struct samsung_gate_clock cmgp_gate_clks[] __initconst = {
[ ... ]
> 	GATE(CLK_GOUT_CMGP_I2C_CMGP00_IPCLK, "gout_clk_cmgp_i2c_cmgp00_ipclk",
> 	     "mout_clk_cmgp_i2c", CLK_CON_GAT_GOUT_CMGP_I2C_CMGP00_IPCLK,
> 	     21, 0, 0),
> 	GATE(CLK_GOUT_CMGP_I2C_CMGP00_PCLK, "gout_clk_cmgp_i2c_cmgp00_pclk",
> 	     "gout_cmu_cmgp_bus", CLK_CON_GAT_GOUT_CMGP_I2C_CMGP00_IPCLK,
> 	     21, 0, 0),

[Severity: High]
Do these PCLK gates accidentally map to the IPCLK register offsets?

This pattern repeats across the CMGP I2C and USI PCLK definitions, where the 
IPCLK register offsets are used for the PCLK gate clocks. Could toggling 
the PCLK gates overwrite the IPCLK state while leaving the actual PCLK 
unconfigured, leading to unclocked peripheral access hangs?

[ ... ]
> static const struct samsung_gate_clock core_gate_clks[] __initconst = {
[ ... ]
> 	GATE(CLK_GOUT_CORE_CCI_CLK, "gout_core_cci_clk", "mout_pll_core_cci_user",
> 	     CLK_CON_GAT_GOUT_CORE_CCI_CLK, 21, 0, 0),
> 	GATE(CLK_GOUT_CORE_CCI_OCC_CLK, "gout_core_cci_occ_clk",
> 	     "mout_pll_core_cci_user", CLK_CON_GAT_GOUT_CORE_CCI_CLK,
> 	     21, 0, 0),
> 	GATE(CLK_GOUT_CORE_G3D_CLK, "gout_core_g3d_clk", "mout_pll_core_g3d_user",
> 	     CLK_CON_GAT_GOUT_CORE_G3D_CLK, 21, 0, 0),
> 	GATE(CLK_GOUT_CORE_G3D_OCC_CLK, "gout_core_cci_g3d_occ_clk",
> 	     "mout_pll_core_cci_user", CLK_CON_GAT_GOUT_CORE_G3D_OCC_CLK,

[Severity: High]
Are there copy-paste errors in the CCI and G3D OCC clock definitions?

CLK_GOUT_CORE_CCI_OCC_CLK targets the CLK_CON_GAT_GOUT_CORE_CCI_CLK register, 
meaning both clocks will manipulate the exact same hardware bit. Also, 
CLK_GOUT_CORE_G3D_OCC_CLK uses the mout_pll_core_cci_user parent instead of 
the intended G3D parent. Could this break on-chip controller operations and 
clock gating for the Core and GPU domains?

[ ... ]
> static const struct samsung_gate_clock dispaud_gate_clks[] __initconst = {
[ ... ]
> 	GATE(CLK_GOUT_DISPAUD_BTM_DPU_ACLK, "gout_dispaud_btm_dpu_aclk",
> 	     "mout_cmu_dispaud_disp_user", CLK_CON_GAT_GOUT_DISPAUD_BTM_DPU_ACLK,
> 	     21, 0, 0),

[Severity: High]
Does this clock gate request an incorrect parent?

It requests mout_cmu_dispaud_disp_user instead of the registered 
mout_pll_dispaud_disp_user. This error appears on multiple gates in this 
block. Will this prevent proper clock tree registration for the DPU?

[ ... ]
> static const struct samsung_cmu_info dispaud_cmu_info __initconst = {
[ ... ]
> 	.sysreg_clk_regs	= drcg_cam_memclk_sysreg,
> 	.nr_sysreg_clk_regs	= ARRAY_SIZE(drcg_cam_memclk_sysreg),

[Severity: High]
Is the wrong sysreg save and restore array bound here?

The sysreg_clk_regs field points to drcg_cam_memclk_sysreg (which uses 
offset 0x1000) instead of drcg_memclk_sysreg. During suspend and resume, could 
the DISPAUD domain perform reads and writes to address offset 0x1000 instead of 
its proper DRCG offset (0x104) and fail to restore the correct setting?

[ ... ]
> static const unsigned long fsys_clk_regs[] __initconst = {
> 	PLL_CON0_MUX_CLKCMU_FSYS_BUS_USER,
[ ... ]
> 	CLK_CON_GAT_GOUT_FSYS_UFS_EMBD_ACLK,
> 	CLK_CON_GAT_GOUT_FSYS_UFS_EMBD_FMP_CLK,
> 	CLK_CON_GAT_GOUT_FSYS_XIU_D_ACLK,
> };

[Severity: High]
Is a UFS register missing from this array?

The CLK_CON_GAT_GOUT_FSYS_UFS_EMBD_CLK_UNIPRO register appears to be omitted 
from fsys_clk_regs. Could this cause the register to revert to power-on 
defaults after a suspend and resume cycle?

[ ... ]
> static int __init exynos9610_cmu_probe(struct platform_device *pdev)
> {
[ ... ]
> static struct platform_driver exynos9610_cmu_driver __refdata = {
> 	.driver = {
> 		.name = "exynos9610-cmu",
> 		.of_match_table = exynos9610_cmu_of_match,
> 		.suppress_bind_attrs = true,
> 	},
> 	.probe = exynos9610_cmu_probe,
> };

[Severity: High]
Could marking the probe function as __init create an execute-after-free risk?

The exynos9610_cmu_probe function is mapped into a persistent platform driver 
but marked __init, meaning it resides in memory that is freed after boot. 
If a compatible device is bound dynamically post-boot, will the driver core 
jump into freed memory? Additionally, the use of __refdata suppresses the 
section mismatch warning that would normally catch this.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-exynos9610-clocks-v2-0-c13ce8ef1861@chimac.ro?part=4