Re: [PATCH v11 5/5] clk: en7523: add support for Airoha AN7583 clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] clk: en7523: NULL deref in clk_hw_get_dev() before PCIe clk
  registration
- [Medium] clk: en7523: AN7583 MDIO clocks registered as fixed-rate
  clocks
--

commit aa0856354194d69ec115be8f509c3f8848e95a7f
Author: Christian Marangi <ansuelsmth@gmail.com>

clk: en7523: add support for Airoha AN7583 clock

This commit adds clock and reset controller support for the Airoha AN7583
SoC to the en7523 clock driver.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 8e24eef651a17..821a5f5ad4d8e 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
[ ... ]
> @@ -368,6 +377,138 @@ static const struct en_clk_desc en7581_base_clks[] = {
>  	}
>  };
>  
> +static const struct en_clk_desc an7583_base_clks[] = {
[ ... ]
> +	}, {
> +		.id = AN7583_CLK_MDIO0,
> +		.name = "mdio0",
> +
> +		.base_reg = REG_CRYPTO_CLKSRC2,
> +
> +		.base_value = 25000000,
> +
> +		.div_bits = 4,
> +		.div_shift = 15,
> +		.div_step = 1,
> +		.div_offset = 1,
> +	}, {
> +		.id = AN7583_CLK_MDIO1,
> +		.name = "mdio1",
> +
> +		.base_reg = REG_CRYPTO_CLKSRC2,
> +
> +		.base_value = 25000000,
> +
> +		.div_bits = 4,
> +		.div_shift = 19,
> +		.div_step = 1,
> +		.div_offset = 1,
> +	}
> +};

[Severity: Medium]
Can clk_set_rate() configure the divider for AN7583_CLK_MDIO0 and
AN7583_CLK_MDIO1 when they are registered through en7581_register_clocks()?

en7581_register_clocks() registers every entry in soc_data->base_clks with
devm_clk_hw_register_fixed_rate(), whose clk_fixed_rate_ops has no
.determine_rate or .set_rate callback.  When airoha_mdio_probe() calls
clk_set_rate() to lower the MDIO clock from its 25 MHz default to 2.5 MHz,
the call succeeds without writing the 4-bit divider in REG_CRYPTO_CLKSRC2,
leaving the MDIO bus running at its boot frequency.

[ ... ]
> @@ -1323,6 +1559,18 @@ static const struct en_clk_soc_data en7581_data = {
>  	.hw_init = en7581_clk_hw_init,
>  };
>  
> +static const struct en_clk_soc_data an7583_data = {
> +	.base_clks = an7583_base_clks,
> +	/* We increment num_clocks by 1 to account for additional PCIe clock */
> +	.num_clocks = ARRAY_SIZE(an7583_base_clks) + 1,
> +	.pcie_ops = {
> +		.is_enabled = en7581_pci_is_enabled,
> +		.prepare = en7581_pci_enable,
> +		.unprepare = en7581_pci_disable,
> +	},
> +	.hw_init = an7583_clk_hw_init,
> +};

[Severity: High]
Does assigning en7581_pci_disable to .unprepare cause a NULL pointer
dereference in clk_hw_get_dev() during probe?

an7583_clk_hw_init() -> en7581_register_clocks() ->
en7523_register_pcie_clk() calls init.ops->unprepare(&cg->hw) before
devm_clk_hw_register(dev, &cg->hw) initializes cg->hw.core:

drivers/clk/clk-en7523.c:en7523_register_pcie_clk() {
    ...
	if (init.ops->unprepare)
		init.ops->unprepare(&cg->hw);

	if (devm_clk_hw_register(dev, &cg->hw))
		return NULL;
    ...
}

en7581_pci_disable() calls clk_hw_get_dev(hw), which dereferences
hw->core->dev while hw->core is still NULL.

Also, pcie_ops pairs .is_enabled with .prepare and .unprepare instead of
matching .enable/.disable or .is_prepared, whereas en7581_data wires
en7581_pci_enable and en7581_pci_disable to .enable and .disable.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106115852.189098-1-ansuelsmth@gmail.com?part=5