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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] State persistence in static structures causing use-after-free and permanent probe failures on retry/rebind.
- [High] Manual removal of a managed device link causing reference count double-drop.
- [High] Ignored return value from icc_link_nodes() memory allocation.
- [Low] Dead code due to incorrect fallback checking of devm_clk_bulk_get_optional() return value.
--

commit 409fc3daaab0214cd873901a825998159cc133c8
Author: Herman van Hazendonk <github.com@herrie.org>
Subject: interconnect: qcom: add MSM8x60 NoC driver

This commit adds a Qualcomm interconnect driver for the MSM8x60 family
modeling the four NoC fabrics (APPSS, System, MMSS, Daytona). The driver
manages bandwidth via RPM arbitration and fabric clocks, and registers
the provider at core_initcall.

> diff --git a/drivers/interconnect/qcom/msm8660.c b/drivers/interconnect/qcom/msm8660.c
> index 0000000000000..6e55ff8e5b0e4
> --- /dev/null
> +++ b/drivers/interconnect/qcom/msm8660.c
[ ... ]
> @@ -1321,79 +1321,80 @@ static struct qcom_rpm *msm8660_get_rpm(struct device *dev)
[ ... ]
> +	link = device_link_add(dev, &rpm_pdev->dev,
> +			       DL_FLAG_AUTOREMOVE_CONSUMER);
> +	put_device(&rpm_pdev->dev);
> +	if (!link)
> +		return dev_err_ptr_probe(dev, -EPROBE_DEFER,
> +					 "failed to add device link to RPM\n");
> +
> +	/*
> +	 * Safe to read drvdata now: the device link pins the supplier so
> +	 * it cannot be unbound until our consumer (this interconnect
> +	 * provider) is unbound first.
> +	 */
> +	rpm = dev_get_drvdata(&rpm_pdev->dev);
> +	if (!rpm) {
> +		device_link_remove(dev, &rpm_pdev->dev);
> +		return dev_err_ptr_probe(dev, -EPROBE_DEFER,

[Severity: High]
Since this device link is created with DL_FLAG_AUTOREMOVE_CONSUMER, the driver
core will automatically manage its lifecycle and clean it up when probe fails.

Does manually calling device_link_remove() here before deferring the probe
cause a double-put and potentially a use-after-free regression when the driver
core subsequently attempts its own cleanup?

> +					 "RPM not ready\n");
> +	}
> +
> +	return rpm;
> +}
> +
> +static int msm8660_icc_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	/*
> +	 * MSM8660 fabric clocks are managed by RPM firmware and may not be
> +	 * available in mainline Linux yet. Once the clock provider exists,
> +	 * we want to honour it; until then we run without per-fabric clock
> +	 * scaling. Only swallow -ENOENT (the clock provider exists but has
> +	 * no matching entry); propagate every other error including
> +	 * -EPROBE_DEFER (the provider exists but has not finished probing).
> +	 */
> +	ret = devm_clk_bulk_get_optional(dev, qp->num_clks, qp->bus_clks);
> +	if (ret == -ENOENT) {
> +		dev_warn(dev, "bus clocks not registered, continuing without clock scaling\n");

[Severity: Low]
Since devm_clk_bulk_get_optional() evaluates missing optional clocks as
non-fatal and intentionally returns 0 (success) instead of -ENOENT, is
this ret == -ENOENT check dead code?

> +		qp->num_clks = 0;
> +	} else if (ret) {
> +		return ret;
> +	}
[ ... ]
> +	for (i = 0; i < num_nodes; i++) {
> +		size_t j;
> +
> +		if (!qnodes[i])
> +			continue;
> +
> +		if (!qnodes[i]->node)
> +			qnodes[i]->node = icc_node_create_dyn();
> +		node = qnodes[i]->node;

[Severity: Critical]
The interconnect nodes are dynamically allocated via icc_node_create_dyn()
and assigned to the ->node field of the statically allocated, global
msm8660_icc_node structs.

If the driver fails to probe later and the error path calls
icc_nodes_remove(), the dynamically allocated memory is freed but the static
pointers are not reset to NULL.

On a subsequent probe retry or rebind, if (!qnodes[i]->node) will evaluate to
false. Will this cause the driver to pass dangling pointers (or an ERR_PTR if
the allocation originally failed) to the interconnect framework, leading to a
use-after-free regression?

> +		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);
> +			goto err_remove_nodes;
> +		}
> +
> +		node->data = qnodes[i];
> +		icc_node_add(node, provider);
> +
> +		for (j = 0; j < qnodes[i]->num_links; j++)
> +			icc_link_nodes(node, &qnodes[i]->link_nodes[j]->node);

[Severity: High]
Can icc_link_nodes() fail with -ENOMEM under memory pressure?

If the return value is ignored, could a failed allocation result in a silently
disconnected topology graph that causes routing failures at runtime?

> +
> +		data->nodes[i] = node;
> +	}
> +
> +	ret = icc_provider_register(provider);
> +	if (ret)
> +		goto err_remove_nodes;
> +
> +	platform_set_drvdata(pdev, qp);
> +
> +	dev_info(dev, "MSM8660 interconnect provider registered\n");
> +
> +	return 0;
> +
> +err_remove_nodes:
> +	icc_nodes_remove(provider);

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