Re: [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support

[Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>]

On 1/12/2025 2:27 AM, Bjorn Andersson wrote:
> On Sat, Jan 11, 2025 at 04:14:25PM +0000, Raviteja Laggyshetty wrote:
>> Current interconnect framework is based on static IDs for creating node
>> and registering with framework. This becomes a limitation for topologies
>> where there are multiple instances of same interconnect provider. Add
>> icc_node_create_alloc_id() API to create icc node with dynamic id, this
>> will help to overcome the dependency on static IDs.
>>
>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>> ---
>>  drivers/interconnect/core.c           | 32 +++++++++++++++++++++++++++
>>  include/linux/interconnect-provider.h |  6 +++++
>>  2 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
>> index 9d5404a07e8a..0b7093eb51af 100644
>> --- a/drivers/interconnect/core.c
>> +++ b/drivers/interconnect/core.c
>> @@ -858,6 +858,38 @@ struct icc_node *icc_node_create(int id)
>>  }
>>  EXPORT_SYMBOL_GPL(icc_node_create);
>>  
>> +/**
>> + * icc_node_create_alloc_id() - create node and dynamically allocate id
>> + * @start_id: min id to be allocated
>> + *
>> + * Return: icc_node pointer on success, or ERR_PTR() on error
>> + */
>> +struct icc_node *icc_node_create_alloc_id(int start_id)
> 
> By having clients pass in start_id, you distribute the decision of what
> a "good number" is across multiple parts of the system (or you have
> clients relying on getting [start_id, start_id + N) back).
> 
> Wouldn't it be better to hide that choice in one place (inside the icc
> framework)?
> 

Yes, inline to Dmitry's suggestion I will be moving the start_id to
framework and all dynamic allocations will start from 10000.

> Regards,
> Bjorn
> 
>> +{
>> +	struct icc_node *node;
>> +	int id;
>> +
>> +	mutex_lock(&icc_lock);
>> +
>> +	node = kzalloc(sizeof(*node), GFP_KERNEL);
>> +	if (!node)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	id = idr_alloc(&icc_idr, node, start_id, 0, GFP_KERNEL);
>> +	if (id < 0) {
>> +		WARN(1, "%s: couldn't get idr\n", __func__);
>> +		kfree(node);
>> +		node = ERR_PTR(id);
>> +		goto out;
>> +	}
>> +	node->id = id;
>> +out:
>> +	mutex_unlock(&icc_lock);
>> +
>> +	return node;
>> +}
>> +EXPORT_SYMBOL_GPL(icc_node_create_alloc_id);
>> +
>>  /**
>>   * icc_node_destroy() - destroy a node
>>   * @id: node id
>> diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
>> index f5aef8784692..4fc7a5884374 100644
>> --- a/include/linux/interconnect-provider.h
>> +++ b/include/linux/interconnect-provider.h
>> @@ -117,6 +117,7 @@ struct icc_node {
>>  int icc_std_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
>>  		      u32 peak_bw, u32 *agg_avg, u32 *agg_peak);
>>  struct icc_node *icc_node_create(int id);
>> +struct icc_node *icc_node_create_alloc_id(int start_id);
>>  void icc_node_destroy(int id);
>>  int icc_link_create(struct icc_node *node, const int dst_id);
>>  void icc_node_add(struct icc_node *node, struct icc_provider *provider);
>> @@ -141,6 +142,11 @@ static inline struct icc_node *icc_node_create(int id)
>>  	return ERR_PTR(-ENOTSUPP);
>>  }
>>  
>> +static inline struct icc_node *icc_node_create_alloc_id(int start_id)
>> +{
>> +	return ERR_PTR(-EOPNOTSUPP);
>> +}
>> +
>>  static inline void icc_node_destroy(int id)
>>  {
>>  }
>> -- 
>> 2.39.2
>>