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 [off-list ref]
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.
[ ... ]
quoted hunk ↗ jump to hunk
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;
}
quoted hunk ↗ jump to hunk
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