Re: [PATCH] clk: tenstorrent: Assign .num before accessing .hws

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing validation of the match data pointer causes a direct NULL pointer dereference on probe.
- [High] Shared clock gate refcount is not rolled back when hardware enable fails, leading to silent hardware enablement failures.
- [High] PLL disable routine fails to close the downstream clock gate, continuing to leak a bypass clock signal.
--

commit f278096bfd5e9454d658c8bb4c16b0c59b91f633
Author: Aamir Ahmed <elb12345@hotmail.co.uk>

clk: tenstorrent: Assign .num before accessing .hws

Commit f316cdff8d67 annotated the hws member of struct clk_hw_onecell_data
with __counted_by, which informs bounds sanitizers about the number of
elements in .hws[]. This commit fixes a warning during probe by initializing
the .num member with the number of elements before the first array access
happens.

> diff --git a/drivers/clk/tenstorrent/atlantis-prcm.c b/drivers/clk/tenstorrent/atlantis-prcm.c
> index 6d4386eeb7dae..a68534a295ca1 100644
> --- a/drivers/clk/tenstorrent/atlantis-prcm.c
> +++ b/drivers/clk/tenstorrent/atlantis-prcm.c
> @@ -796,6 +796,8 @@ static int atlantis_prcm_clocks_register(struct device *dev,
>  	if (!clk_data)
>  		return -ENOMEM;
>  
> +	clk_data->num = num_clks;
> +
>  	for (i = 0; i < data->num; i++) {

[Severity: High]
This isn't a bug introduced by this patch, but can data be NULL here?

If the device is bound manually via sysfs without a matching device tree
node, of_device_get_match_data() can return NULL in atlantis_prcm_probe():

drivers/clk/tenstorrent/atlantis-prcm.c:atlantis_prcm_probe() {
    ...
	data = of_device_get_match_data(dev);

	ret = atlantis_prcm_clocks_register(dev, regmap, data);
    ...
}

Which is then passed into atlantis_prcm_clocks_register() and dereferenced:

drivers/clk/tenstorrent/atlantis-prcm.c:atlantis_prcm_clocks_register() {
    ...
	size_t num_clks = data->num;
    ...
}

Should data be validated before dereferencing it to avoid a NULL pointer
dereference?

>  		struct clk_hw *hw = data->hws[i];
>  		struct atlantis_clk_common *common =
>  			hw_to_atlantis_clk_common(hw);
>  		common->regmap = regmap;
>  
>  		ret = devm_clk_hw_register(dev, hw);
>  		if (ret)
>  			return ret;
>  
>  		clk_data->hws[common->clkid] = hw;
>  	}
>  
> -	clk_data->num = num_clks;
> -
>  	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
>  }

[Severity: High]
This is a pre-existing issue, but does this code leave the shared gate
refcount incremented if the hardware enable fails?

drivers/clk/tenstorrent/atlantis-prcm.c:atlantis_clk_gate_shared_enable() {
    ...
	if (need_enable) {
		if (!regmap_test_bits(gate->common.regmap,
				      gate->config.reg_offset,
				      gate->config.enable)) {
			pr_warn("%s: gate enable %d failed to enable\n",
				clk_hw_get_name(hw), gate->config.enable);
			return -EIO;
		}
	}
    ...
}

If regmap_test_bits() detects that the hardware bit failed to set, it
returns -EIO but leaves gate->config.share_count incremented. If the
consumer retries enabling the clock, the driver sees the refcount > 0, skips
the hardware write, and falsely returns success (0).

Should the error path re-acquire the lock and roll back the share_count?


[Severity: High]
This is also a pre-existing issue, but does the PLL disable routine fail to
close the downstream clock gate?

drivers/clk/tenstorrent/atlantis-prcm.c:atlantis_clk_pll_enable() {
    ...
	/* Enable Gate clk at PLL Output */
	return regmap_update_bits(pll->common.regmap, pll->config.cg_reg_offset,
				  pll->config.cg_reg_enable,
				  pll->config.cg_reg_enable);
}

drivers/clk/tenstorrent/atlantis-prcm.c:atlantis_clk_pll_disable() {
    ...
	/* Step 1: Switch to bypass mode before disabling */
	regmap_update_bits(pll->common.regmap, pll->config.reg_offset,
			   PLL_CFG_BYPASS_BIT, PLL_CFG_BYPASS_BIT);
	/* Step 2: Power down PLL */
	regmap_update_bits(pll->common.regmap, pll->config.en_reg_offset,
			   PLL_CFG_EN_BIT, 0);
}

When a consumer calls clk_disable() on the PLL, the driver switches to
bypass and clears the enable bit, but fails to clear
pll->config.cg_reg_enable symmetrically with atlantis_clk_pll_enable().

Because disabling the PLL also switches it to bypass mode, does the raw
bypass clock continuously propagate to downstream consumers that expect the
clock to be fully stopped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/AS8P251MB00013BD9A093C06D2D60E10EC8B42@AS8P251MB0001.EURP251.PROD.OUTLOOK.COM?part=1