Re: [PATCH RFC 2/2] clk: scmi: optionally register only clocks referenced in the device tree

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] clk: scmi: ERR_PTR(-EINVAL) from scmi_clk_get_hw() turned
  into -ENOENT by clk_get()
--

commit 37cea3339cbbcf997cf01fad1c4be8bccecc21cd
Author: Ghennadi Procopciuc <ghennadi.procopciuc@oss.nxp.com>

clk: scmi: optionally register only clocks referenced in the device tree

This commit adds the dt_referenced_only module parameter and Kconfig
option to register only the SCMI clocks referenced by enabled device tree
nodes and their parents, setting omitted entries in clk_data->hws to
ERR_PTR(-EINVAL).

> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index 2dd50c5b4ea8f..a8ec782ac279b 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c

[ ... ]

> @@ -391,14 +480,31 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
>  	transport_is_atomic = handle->is_transport_atomic(handle,
>  							  &atomic_threshold_us);
>  
> +	unsigned long *ids __free(bitmap) = bitmap_zalloc(count, GFP_KERNEL);
> +
> +	if (!ids)
> +		return -ENOMEM;
> +
> +	if (dt_referenced_only && np) {
> +		scmi_clk_mark_dt_refs(np, ids, count);
> +		err = scmi_clk_mark_parents(ph, ids, count);
> +		if (err)
> +			return err;
> +
> +		if (bitmap_empty(ids, count))
> +			dev_warn(dev, "no clock referenced in the device tree, none registered\n");
> +	} else {
> +		bitmap_fill(ids, count);
> +	}
> +
>  	sclks = devm_kcalloc(dev, count, sizeof(*sclks), GFP_KERNEL);
>  	if (!sclks)
>  		return -ENOMEM;
>  
>  	for (idx = 0; idx < count; idx++)
> -		hws[idx] = &sclks[idx].hw;
> +		hws[idx] = test_bit(idx, ids) ? &sclks[idx].hw : ERR_PTR(-EINVAL);

[Severity: Medium]
Does setting hws[idx] to ERR_PTR(-EINVAL) actually prevent
clk_get_optional() from returning a NULL clock?

When a consumer looks up a clock via clk_get() (including
devm_clk_get_optional()), clk_get() ignores any error from
of_clk_get_hw() other than -EPROBE_DEFER and falls back to
__clk_get_sys():

drivers/clk/clkdev.c:clk_get() {
	if (dev && dev->of_node) {
		hw = of_clk_get_hw(dev->of_node, 0, con_id);
		if (!IS_ERR(hw) || PTR_ERR(hw) == -EPROBE_DEFER)
			return clk_hw_create_clk(dev, hw, dev_id, con_id, NULL);
	}

	return __clk_get_sys(dev, dev_id, con_id);
}

__clk_get_sys() then fails to find a matching clkdev entry and returns
ERR_PTR(-ENOENT), which clk_get_optional() turns into NULL instead of
failing the lookup with -EINVAL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-b4-scmi-clk-on-demand-v1-0-7fd162097b57@oss.nxp.com?part=2