Re: [PATCH v11 04/11] clk: realtek: Introduce a common probe()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The clock registration loop does not check for ERR_PTR values in the hw array, which will cause a kernel panic if a clock is intentionally omitted.
- [Medium] The clock provider is exposed to the system before the probe is fully complete, risking exposure of an orphaned clock on error paths.
--

--- Patch [4]: [PATCH v11 04/11] clk: realtek: Introduce a common probe() ---
commit 1821fe944a61d09ec77d386cf175a9aadc142c21
Author: Cheng-Yu Lee <cylee12@realtek.com>

clk: realtek: Introduce a common probe()

Add rtk_clk_probe() to set up the shared regmap, register clock hardware,
and add the clock provider. It also creates an auxiliary device for the
reset driver if the "#reset-cells" property is present.

> diff --git a/drivers/clk/realtek/clk-rtk-common.c b/drivers/clk/realtek/clk-rtk-common.c
> new file mode 100644
> index 0000000000000..86adeedbdeada
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-rtk-common.c

[ ... ]

> +int rtk_clk_probe(struct platform_device *pdev, const struct rtk_clk_desc *desc)
> +{

[ ... ]

> +	for (i = 0; i < desc->clk_data->num; i++) {
> +		hw = desc->clk_data->hws[i];
> +		if (!hw)
> +			continue;
> +
> +		ret = devm_clk_hw_register(dev, hw);

[Severity: High]
Will this loop crash if the hw array contains ERR_PTR(-ENOENT)?

In clock drivers, the clk_hw_onecell_data::hws array often contains
ERR_PTR(-ENOENT) for clock indices that are intentionally omitted or
unsupported on specific SoC variants.

Since the check only looks for NULL, an ERR_PTR would bypass this check and
be passed to devm_clk_hw_register(). This typically causes a panic when
clk_hw_register() attempts to dereference the pointer.

Would it be better to check for IS_ERR_OR_NULL(hw) here?

> +		if (ret)
> +			return dev_err_probe(dev, ret, "failed to register hw of clk%d\n", i);
> +	}
> +
> +	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +					  desc->clk_data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to add clock provider\n");
> +
> +	return rtk_reset_controller_register(dev, desc->aux_name, regmap);

[Severity: Medium]
Is there a risk of exposing an orphaned clock on the error path here?

The clock provider is registered via devm_of_clk_add_hw_provider() before
rtk_reset_controller_register() is called. If the reset controller
registration fails, the probe aborts and devres unregisters the clock
provider.

During the window between registration and cleanup, concurrent consumers
could look up and acquire the clocks, only for them to be unregistered
immediately. This leaves consumers with orphaned clocks that cannot
properly control the hardware.

Would it be safer to make the clock provider registration the very last step
of the probe function?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728142806.1954638-1-eleanor.lin@realtek.com?part=4