Re: [PATCH 1/2] interconnect: qcom: x1e80100: Revert ABI break

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Returning directly on `-EPROBE_DEFER` during QoS clock initialization bypasses node cleanup, leaking nodes and causing a Use-After-Free on probe retry.
--

commit dc792c2aa6455fa0377849e9d95a326611b7bb15
Author: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
interconnect: qcom: x1e80100: Revert ABI break

This patch reverts the enablement of QoS configuration for the x1e80100 
interconnect driver. It removes the QoS box definitions and the requirement
for QoS clocks, as QoS is not mandatory for the device to operate.

Closes: https://krzk.eu/#/builders/102/builds/70/steps/23/logs/warnings__3_

> diff --git a/drivers/interconnect/qcom/x1e80100.c b/drivers/interconnect/qcom/x1e80100.c
> index 8075e0ff20597..2ba2823c7860e 100644
> --- a/drivers/interconnect/qcom/x1e80100.c
> +++ b/drivers/interconnect/qcom/x1e80100.c
[ ... ]
> @@ -1795,21 +1466,11 @@ static struct qcom_icc_node * const aggre1_noc_nodes[] = {
>  	[SLAVE_A1NOC_SNOC] = &qns_a1noc_snoc,
>  };
>  
> -static const struct regmap_config x1e80100_aggre1_noc_regmap_config = {
> -	.reg_bits = 32,
> -	.reg_stride = 4,
> -	.val_bits = 32,
> -	.max_register = 0x14400,
> -	.fast_io = true,
> -};
> -
>  static const struct qcom_icc_desc x1e80100_aggre1_noc = {
> -	.config = &x1e80100_aggre1_noc_regmap_config,
>  	.nodes = aggre1_noc_nodes,
>  	.num_nodes = ARRAY_SIZE(aggre1_noc_nodes),
>  	.bcms = aggre1_noc_bcms,
>  	.num_bcms = ARRAY_SIZE(aggre1_noc_bcms),
> -	.qos_requires_clocks = true,
>  };

[Severity: High]
This is a pre-existing issue, but it looks like reverting the
qos_requires_clocks flag here might be masking a severe structural flaw
in qcom_icc_rpmh_probe() that triggers a use-after-free on probe retry.

In qcom_icc_rpmh_probe(), interconnect nodes are created via
icc_node_create_dyn() and added to the provider, with their pointers stored
in the driver's statically allocated qnodes definitions (qn->node).

If the subsequent call to devm_clk_bulk_get_all() returns -EPROBE_DEFER
(which only happens when qos_requires_clocks is true), the function
executes return dev_err_probe(...) and returns immediately. This completely
bypasses the err_remove_nodes cleanup path which calls icc_nodes_remove().

Because of this bypass:
1. The provider struct is freed by devres, but the nodes remain registered
globally in icc_idr with a dangling node->provider pointer.
2. On probe retry, the driver sees qn->node is already non-NULL and reuses
the old node.
3. The subsequent icc_node_add() hits WARN_ON(node->provider) (since it
points to the freed provider) and returns early without updating the pointer.
4. Any later interconnect operation (like qcom_icc_set()) dereferences the
dangling node->provider, leading to a guaranteed use-after-free.

Does this framework bug explain why enabling the clocks broke the ABI and
caused warnings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520184944.182148-3-krzysztof.kozlowski@oss.qualcomm.com?part=1