Re: [PATCH 2/2] interconnect: qcom: add MSM8x60 NoC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Double/Triple counting of bandwidth leads to artificially inflated clock rates.
- [Low] Kconfig dependency allows built-in driver to reference module symbols, causing linker errors.
- [Low] The name field in struct msm8660_icc_node is incorrectly typed as unsigned char * instead of const char *.
--

commit 55dcd848add92308b8d692adbadfc253e5ad283f
Author: Herman van Hazendonk <github.com@herrie.org>

interconnect: qcom: add MSM8x60 NoC driver

Add a Qualcomm interconnect driver for the MSM8x60 family modelling the
four NoC fabrics (APPSS, System, MMSS, DFAB) that connect masters and
slaves on these Scorpion-class SoCs.

> diff --git a/drivers/interconnect/qcom/Kconfig b/drivers/interconnect/qcom/Kconfig
> --- a/drivers/interconnect/qcom/Kconfig
> +++ b/drivers/interconnect/qcom/Kconfig
[ ... ]
> +config INTERCONNECT_QCOM_MSM8660
> +	# bool, not tristate: this provider must be registered at
> +	# core_initcall to be ready before icc_init (subsys_initcall) walks
> +	# devicetree consumers; a loadable module cannot satisfy that
> +	# ordering. Build-in only.
> +	bool "Qualcomm MSM8x60 interconnect driver"
> +	depends on INTERCONNECT_QCOM=y
> +	depends on MFD_QCOM_RPM

[Severity: Low]
Does this allow the driver to be built-in while MFD_QCOM_RPM is a module?

Since INTERCONNECT_QCOM_MSM8660 is a bool symbol, depending on MFD_QCOM_RPM
without =y means this can be selected as =y even if MFD_QCOM_RPM=m. This
would result in undefined reference linker errors for qcom_rpm_write.

> diff --git a/drivers/interconnect/qcom/msm8660.c b/drivers/interconnect/qcom/msm8660.c
> --- /dev/null
> +++ b/drivers/interconnect/qcom/msm8660.c
[ ... ]
> +struct msm8660_icc_node {
> +	unsigned char *name;

[Severity: Low]
This isn't a bug, but should name be a const char * rather than unsigned
char *?

Initializing this with string literals (via the DEFINE_QNODE macro) and
assigning it to node->name (which expects const char *) results in compiler
warnings about pointer type mismatches.

[ ... ]
> +static int msm8660_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
> +	struct msm8660_icc_node *src_qn;
> +	struct msm8660_icc_provider *qp;
> +	u64 sum_bw, max_peak_bw, rate;
> +	u32 agg_avg = 0, agg_peak = 0;
> +	struct icc_provider *provider;
> +	struct icc_node *n;
> +	int ret, i;
> +
> +	src_qn = src->data;
> +	provider = src->provider;
> +	qp = to_msm8660_icc_provider(provider);
> +
> +	list_for_each_entry(n, &provider->nodes, node_list)
> +		provider->aggregate(n, 0, n->avg_bw, n->peak_bw,
> +				    &agg_avg, &agg_peak);

[Severity: High]
Does this loop result in double counting the requested bandwidth?

Because an interconnect path traverses multiple nodes (e.g., a master and a
slave) and the framework applies the requested bandwidth to all nodes along
the path, summing all nodes in the provider means the same traffic is counted
multiple times.

This artificially inflates the computed clock rate, which could cause
clk_set_rate() to fail or unnecessarily waste power.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603163410.2312712-1-github.com@herrie.org?part=2