Re: [PATCH 1/4] interconnect: icc-clk: support N:1 master-to-slave topology

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] icc: list corruption and data race in icc_clk_set() during concurrent unbind
- [Medium] icc: bypassed rate aggregation leaves shared clock at high rate
- [Medium] icc: missing error check for icc_link_create() in icc_clk_register()
--

commit fb3619debbc961d128e9125a7da752adeb391118
Author: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>

    interconnect: icc-clk: support N:1 master-to-slave topology

    icc_clk_register() assumes each icc_clk_data entry describes a unique
    master/slave pair and creates both nodes for every entry. When several
    masters share the same slave_id, the shared slave node is created and
    added again on each later entry. icc_node_create() returns the existing
    node, so icc_node_add() hits WARN_ON(node->provider):

[ ... ]

    This topology is needed on IPQ9650, where the silver and gold CPU
    clusters both vote on the single L3 clock through a shared SLAVE_L3
    node.
    
    The onecell table is also filled in registration order, so xlate only
    works when the node IDs happen to match that order. Size the table by
    the highest node ID, index it by ID, and skip slave creation when the
    node is already registered.
    
    icc_clk_set() also passes only the requesting node's bandwidth to
    clk_set_rate(), so the last writer wins. A low vote from one master can
    then lower the clock below what another master asked for. Before setting
    the rate, take the highest peak bandwidth across all nodes that share
    the clock.

> diff --git a/drivers/interconnect/icc-clk.c b/drivers/interconnect/icc-clk.c
> index 93c030608d3e0..567d071fade8e 100644
> --- a/drivers/interconnect/icc-clk.c
> +++ b/drivers/interconnect/icc-clk.c
> @@ -24,7 +24,10 @@ struct icc_clk_provider {
>  
>  static int icc_clk_set(struct icc_node *src, struct icc_node *dst)
>  {
> +	unsigned long rate = icc_units_to_bps(src->peak_bw);
> +	struct icc_provider *provider = src->provider;
>  	struct icc_clk_node *qn = src->data;
> +	struct icc_node *node;
>  	int ret;
>  
>  	if (!qn || !qn->clk)
>  		return 0;
>  
>  	if (!src->peak_bw) {
>  		if (qn->enabled)
>  			clk_disable_unprepare(qn->clk);
>  		qn->enabled = false;
>  
>  		return 0;
>  	}

[Severity: Medium]
Will this early return circumvent the newly added aggregation logic when a
master sets its bandwidth to zero?

When multiple masters share the same clock in an N:1 topology, bypassing the
rate aggregation loop due to an early return causes the underlying shared
clock to remain running at the previously requested high rate instead of
stepping down to the maximum of the remaining masters' votes. The clock stays
unnecessarily high until another node updates its vote.

>  
>  	if (!qn->enabled) {
>  		ret = clk_prepare_enable(qn->clk);
>  		if (ret)
>  			return ret;
>  		qn->enabled = true;
>  	}
>  
> -	return clk_set_rate(qn->clk, icc_units_to_bps(src->peak_bw));
> +	/*
> +	 * Multiple master nodes can share the same underlying clock (N:1
> +	 * topology, e.g. several CPU clusters voting on one L3 clock).
> +	 * Aggregate the peak bandwidth across all of them before setting
> +	 * the rate, otherwise the last caller wins and can undervote what
> +	 * another master already asked for.
> +	 */
> +	list_for_each_entry(node, &provider->nodes, node_list) {

[Severity: High]
Does this list iteration race with concurrent unbind?

icc_clk_set() executes as the provider->set callback holding only icc_bw_lock.
Concurrently, if an interconnect provider is unbound via sysfs,
icc_node_del() removes nodes from provider->nodes using list_del() while
holding only icc_lock:

drivers/interconnect/core.c:icc_node_del() {
    mutex_lock(&icc_lock);
    list_del(&node->node_list);
    mutex_unlock(&icc_lock);
}

Because the locks are disjoint, this list traversal can race with list_del(),
leading to iteration over poisoned pointers.

> +		struct icc_clk_node *n = node->data;
> +
> +		if (n && clk_is_match(n->clk, qn->clk))
> +			rate = max(rate, icc_units_to_bps(node->peak_bw));
> +	}
> +
> +	return clk_set_rate(qn->clk, rate);
>  }

[ ... ]

> @@ -126,7 +148,16 @@ struct icc_provider *icc_clk_register(struct device *dev,
>  		icc_node_add(node, provider);
>  		/* link to the next node, slave */
>  		icc_link_create(node, first_id + data[i].slave_id);
> -		onecell->nodes[j++] = node;
> +		onecell->nodes[data[i].master_id] = node;

[Severity: Medium]
Should the return value of icc_link_create() be checked here?

If icc_link_create() fails to allocate memory for the links, it returns
-ENOMEM. Because this return value is ignored, the interconnect graph links
might silently fail to form. This can break the master-to-slave interconnect
topology, leading to ignored bandwidth requests while driver initialization
appears to succeed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007-ipq9650_apss_clk-v1-0-fd4c2b3cbdea@oss.qualcomm.com?part=1