Re: [PATCH v6 3/8] clk: qcom: Add generic clkref_en support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `qcom_clk_ref_probe` API maps memory resource 0 exclusively, preventing its reuse by parent drivers like TLMM that already map their register space.
- [Medium] Missing `args_count` validation in the clock provider getter leads to an uninitialized stack memory read.
--

commit 425b78640ada4c9c7a746a68fc9313af14a52bae
Author: Qiang Yu <qiang.yu@oss.qualcomm.com>

clk: qcom: Add generic clkref_en support

This patch introduces generic support for Qualcomm clkref_en clocks. It
provides a dedicated clk_ref clock type and a common registration API to be
reused across different blocks like TCSR and TLMM.

> diff --git a/drivers/clk/qcom/clk-ref.c b/drivers/clk/qcom/clk-ref.c
> new file mode 100644
> index 0000000000000..8132e510f43c0
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-ref.c

[ ... ]

> +static struct clk_hw *qcom_clk_ref_provider_get(struct of_phandle_args *clkspec, void *data)
> +{
> +	struct qcom_clk_ref_provider *provider = data;
> +	unsigned int idx = clkspec->args[0];

[Severity: Medium]
What happens if the provider node is misconfigured with #clock-cells = <0>?

If clkspec->args_count is 0, won't this read uninitialized stack memory from
clkspec->args[0] and potentially grant access to a random valid clock? 

Should this validate that clkspec->args_count is at least 1 before accessing
the arguments array?

> +
> +	if (idx >= provider->num_refs)
> +		return ERR_PTR(-EINVAL);

[ ... ]

> +int qcom_clk_ref_probe(struct platform_device *pdev,
> +		       const struct regmap_config *config,
> +		       const struct qcom_clk_ref_desc *descs,
> +		       size_t num_clk_refs)
> +{
> +	struct qcom_clk_ref_provider *provider;
> +	struct device *dev = &pdev->dev;
> +	struct regmap *regmap;
> +	void __iomem *base;
> +	int ret;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);

[Severity: Medium]
Will this fail with -EBUSY when used by TLMM?

The commit message mentions this API will be used for TLMM on SM8750. However,
devm_platform_ioremap_resource() requests exclusive ownership of the memory
region. The TLMM driver already maps its entire register space during probe.

If TLMM has already mapped the region, won't this subsequent attempt to map
the same region fail?

Would it be better to have the caller pass an already instantiated struct
regmap pointer instead of mapping the region inside the generic probe?

> +	if (IS_ERR(base))
> +		return PTR_ERR(base);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260621-tcsr_qref_0622-v6-0-c939c22ded0c@oss.qualcomm.com?part=3