Re: [PATCH v4 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] Use-After-Free of cross-fabric interconnect nodes on probe failure.
- [High] Unintentional active-clock voting during sleep state prevents power collapse.
- [High] Swallowing `clk_bulk_prepare_enable` failures leads to unrecoverable bus hangs.
--

commit 96cdb073dc4d0f48f3ea0aaf0d60b6577a01c2b5
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, Daytona) that connect masters
and slaves on these Scorpion-class SoCs. The driver implements the
interconnect-provider API to manage bandwidth between specific masters
and slaves via the RPM arbitration tables.

> diff --git a/drivers/interconnect/qcom/msm8660.c b/drivers/interconnect/qcom/msm8660.c
> new file mode 100644
> index 0000000000000..13105e7bcb50c
> --- /dev/null
> +++ b/drivers/interconnect/qcom/msm8660.c
> @@ -0,0 +1,1716 @@
[ ... ]
> +static int msm8660_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
[ ... ]
> +	if (qp->rate != rate) {
> +		for (i = 0; i < qp->num_clks; i++) {
> +			ret = clk_set_rate(qp->bus_clks[i].clk, rate);

[Severity: High]
Does this unintentionally override the zero-bandwidth sleep vote?

The driver iterates over all clocks in the bus_clks array and applies the
active bandwidth rate (with a minimum floor of 384 MHz) to every clock
using clk_set_rate(). The clock arrays (e.g., msm8660_afab_clocks) contain
both context-agnostic clocks (like bus and ebi1) and active-only clocks
(bus_a, ebi1_a).

In the Qualcomm clock framework, calling clk_set_rate() on a context-agnostic
clock applies the rate to both the active AND sleep states in the RPM.
By forcing 384 MHz on the non-_a clocks, the fabric and memory buses
are kept highly clocked even during CPU deep sleep (power collapse),
resulting in significant power regression.

> +			if (ret) {
> +				dev_err(provider->dev,
> +					"%s clk_set_rate(%llu) error: %d\n",
> +					qp->bus_clks[i].id, rate, ret);
[ ... ]
> +static int msm8660_icc_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	if (qp->num_clks) {
> +		ret = clk_bulk_prepare_enable(qp->num_clks, qp->bus_clks);
> +		if (ret) {
> +			dev_warn(dev, "Failed to enable bus clocks: %d\n", ret);
> +			qp->num_clks = 0;
> +		} else {

[Severity: High]
Is it safe to swallow this error and proceed with probing?

Because devm_clk_bulk_get_optional() already safely handles clocks
missing from the Device Tree, a failure from clk_bulk_prepare_enable()
represents a genuine hardware or driver fault (e.g., a PLL failing to lock).

If the error is not propagated and the interconnect provider is registered
despite the clocks being disabled, consumers will assume the fabric is active.
Any subsequent MMIO access over the unclocked fabric will result in a
synchronous external abort, causing an unrecoverable system crash.

> +			/*
> +			 * Register the cleanup right after a successful
[ ... ]
> +	for (i = 0; i < num_nodes; i++) {
> +		if (!qnodes[i])
> +			continue;
> +
> +		if (!qnodes[i]->node)
> +			qnodes[i]->node = icc_node_create_dyn();
> +		node = qnodes[i]->node;
> +		if (IS_ERR(node)) {
> +			ret = PTR_ERR(node);
> +			goto err_remove_nodes;
> +		}
> +
> +		ret = icc_node_set_name(node, provider, qnodes[i]->name);
> +		if (ret) {
> +			icc_node_destroy(node->id);
> +			qnodes[i]->node = NULL;
> +			goto err_remove_nodes;
> +		}
> +
> +		node->data = qnodes[i];
> +		icc_node_add(node, provider);
> +		data->nodes[i] = node;
> +	}
[ ... ]
> +err_remove_nodes:
> +	icc_nodes_remove(provider);

[Severity: High]
Can this cause a use-after-free on probe failure?

The interconnect framework's icc_link_nodes() automatically forward-allocates
a target node if it doesn't exist yet. When an earlier fabric (e.g., APPSS)
probes, it creates links to nodes belonging to later fabrics (e.g., MMSS),
allocating them in memory.

When the target fabric later probes, its Pass 1 loop adopts these
pre-allocated nodes and adds them to its own provider. If the target fabric
subsequently fails to probe (e.g., due to an ENOMEM in Pass 2 or
icc_provider_register() failure), the error path calls
icc_nodes_remove(provider).

This destroys and frees all nodes belonging to the provider, including the
ones the earlier fabric already successfully linked to. The earlier, active
fabric is left with a dangling pointer in its links array. If path-finding
traverses this link, it will result in a use-after-free.

> +	msm8660_clear_node_cache(qnodes, num_nodes);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606-submit-interconnect-msm8660-v4-0-6e1e5c5efa26@herrie.org?part=2