Re: [PATCH v4 05/16] clk: at91: Allow enabling main_rc_osc through DT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: at91: uninitialized hw stored in sama7d65 and sama7g5 pmc_setup()
- [High] clk: at91: out-of-bounds chws write in sama7g5_pmc_setup() for MAIN_RC
--

commit e5d4dc667c53860fc5c8c99266b722d5024ab4e6
Author: Alexander Dahl <ada@thorsis.com>

clk: at91: Allow enabling main_rc_osc through DT

This commit exports the main_rc_osc clock in the PMC core clock array on
SAM9X60, SAM9X7, SAMA7D65, and SAMA7G5 so that consumers such as the OTPC
controller can reference and enable it via Device Tree.

[ ... ]

> diff --git a/drivers/clk/at91/sama7d65.c b/drivers/clk/at91/sama7d65.c
> index 0b5356467fa52..ef54d2fa6cf40 100644
> --- a/drivers/clk/at91/sama7d65.c
> +++ b/drivers/clk/at91/sama7d65.c
> @@ -1138,6 +1138,7 @@ static void __init sama7d65_pmc_setup(struct device_node *np)
>  						   50000000);
>  	if (IS_ERR(main_rc_hw))
>  		goto err_free;
> +	sama7d65_pmc->chws[SAMA7D65_PMC_MAIN_RC] = hw;

[Severity: High]
Should this assign main_rc_hw instead of hw?

In sama7d65_pmc_setup(), at91_clk_register_main_rc_osc() stores the
registered clock pointer in main_rc_hw, while the local variable hw is still
uninitialized at this point:

drivers/clk/at91/sama7d65.c:sama7d65_pmc_setup() {
	...
	struct clk_hw *hw, *main_rc_hw, *main_osc_hw, *main_xtal_hw;
	...
	main_rc_hw = at91_clk_register_main_rc_osc(regmap, "main_rc_osc", 12000000,
						   50000000);
	if (IS_ERR(main_rc_hw))
		goto err_free;
	sama7d65_pmc->chws[SAMA7D65_PMC_MAIN_RC] = hw;
}

>  
>  	bypass = of_property_read_bool(np, "atmel,osc-bypass");
>  
> diff --git a/drivers/clk/at91/sama7g5.c b/drivers/clk/at91/sama7g5.c
> index 99b4c1d9bdcca..9a6f0f30b2b7e 100644
> --- a/drivers/clk/at91/sama7g5.c
> +++ b/drivers/clk/at91/sama7g5.c
> @@ -1014,6 +1014,7 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>  						   50000000);
>  	if (IS_ERR(main_rc_hw))
>  		goto err_free;
> +	sama7g5_pmc->chws[SAMA7G5_PMC_MAIN_RC] = hw;

[Severity: High]
Doesn't this also assign the uninitialized hw variable instead of main_rc_hw,
and write past the end of sama7g5_pmc->chws[]?

In include/dt-bindings/clock/microchip,sama7g5-pmc.h, SAMA7G5_PMC_MCK1 is 13
and SAMA7G5_PMC_MAIN_RC is 14:

#define SAMA7G5_PMC_MCK1	13
#define SAMA7G5_PMC_MAIN_RC	14

However, sama7g5_pmc_setup() allocates sama7g5_pmc with ncore set to
SAMA7G5_PMC_MCK1 + 1 (14):

drivers/clk/at91/sama7g5.c:sama7g5_pmc_setup() {
	...
	sama7g5_pmc = pmc_data_allocate(SAMA7G5_PMC_MCK1 + 1,
					nck(sama7g5_systemck),
					nck(sama7g5_periphck),
					nck(sama7g5_gck), 8);
}

Because chws only has 14 entries (indices 0..13), indexing
sama7g5_pmc->chws[SAMA7G5_PMC_MAIN_RC] writes to index 14 (aliasing
sama7g5_pmc->shws[0]), and of_clk_hw_pmc_get() will reject lookups for
SAMA7G5_PMC_MAIN_RC:

drivers/clk/at91/pmc.c:of_clk_hw_pmc_get() {
	...
	case PMC_TYPE_CORE:
		if (idx < pmc_data->ncore)
			return pmc_data->chws[idx];
		break;
}

Should pmc_data_allocate() in sama7g5_pmc_setup() be updated to pass
SAMA7G5_PMC_MAIN_RC + 1?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105110715.2486432-1-ada@thorsis.com?part=5