Re: [PATCH v2 09/14] clk: samsung: exynos8855: add block hsi clock support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: samsung: exynos8855: out-of-bounds write in samsung_clk_add_lookup() due to undersized CLKS_NR_HSI
- [High] clk: samsung: exynos8855: missing CLK_IS_CRITICAL on CLK_GOUT_CMU_HSI leading to system hang
--

commit 24ea6e22468660550351b038f08cf4b975d943f7
Author: Raghav Sharma <raghav.s@samsung.com>

clk: samsung: exynos8855: add block hsi clock support

This commit adds block HSI clock support for the Exynos 8855 SoC. It registers
compatible strings and cmu_info data for CMU_HSI, which provides clocks for
the HSI block.

> diff --git a/drivers/clk/samsung/clk-exynos8855.c b/drivers/clk/samsung/clk-exynos8855.c
> index 2bf889a478bc9..16d71db11ad06 100644
> --- a/drivers/clk/samsung/clk-exynos8855.c
> +++ b/drivers/clk/samsung/clk-exynos8855.c
> @@ -19,7 +19,9 @@
>  #define CLKS_NR_TOP                     (CLKCMU_DOUT_G3D_HTU + 1)
>  #define CLKS_NR_PERIC                   (CLK_GOUT_USI_PERIC + 1)
>  #define CLKS_NR_PERIS                   (CLK_GOUT_WDT1_PERIS + 1)
> +#define CLKS_NR_HSI                     (CLK_GOUT_SYSREG_HSI  + 1)

[Severity: High]
Does this correctly calculate the maximum clock index?

If CLK_GOUT_SYSREG_HSI is 4, this sets CLKS_NR_HSI to 5. This macro
determines the .nr_clk_ids for the HSI block, sizing the
ctx->clk_data.hws array in samsung_clk_init() for 5 elements.

Later in this patch, the hsi_gate_clks array registers clocks with
higher IDs:

    GATE(CLK_GOUT_UFS_EMBD_UNIPRO_HSI, "gout_ufs_embd_unipro_hsi",
         "mout_clkcmu_hsi_ufs_embd_user",
         CMU_HSI_CLK_CON_GAT_BLK_HSI_UID_UFS_EMBD_IPCLKPORT_I_CLK_UNIPRO,
         21, 0, 0),

If CLK_GOUT_UFS_EMBD_UNIPRO_HSI is 6, will registering this clock cause
samsung_clk_add_lookup() to write past the end of the allocated flexible
array?

[ ... ]

> +static const struct samsung_gate_clock hsi_gate_clks[] __initconst = {
> +	/* System will hang if this critical clock is gated */
> +	GATE(CLK_GOUT_CMU_HSI, "gout_cmu_hsi",
> +	     "mout_clkcmu_hsi_noc_user",
> +	     CMU_HSI_CLK_CON_GAT_BLK_HSI_UID_CMU_HSI_IPCLKPORT_PCLK,
> +	     21, 0, 0),

[Severity: High]
Since this clock is critical to prevent a system hang, should the flags
argument be CLK_IS_CRITICAL instead of 0?

Without the critical flag, will the Common Clock Framework dynamically
disable this clock during late init if no active consumer has enabled
it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009120137.2801911-1-raghav.s@samsung.com?part=9