Re: [PATCH V8 2/7] interconnect: core: Add dynamic id allocation support

[Raviteja Laggyshetty]

On 2/17/2025 6:32 AM, Dmitry Baryshkov wrote:
> On Sun, Feb 16, 2025 at 10:08:51PM +0530, Raviteja Laggyshetty wrote:
>>
>>
>> On 2/10/2025 4:20 PM, Dmitry Baryshkov wrote:
>>> On Wed, Feb 05, 2025 at 06:27:38PM +0000, Raviteja Laggyshetty wrote:
>>>> The current interconnect framework relies on static IDs for node
>>>> creation and registration, which limits topologies with multiple
>>>> instances of the same interconnect provider. To address this, update
>>>> the interconnect framework APIs icc_node_create() and icc_link_create()
>>>> APIs to dynamically allocate IDs for interconnect nodes during creation.
>>>> This change removes the dependency on static IDs, allowing multiple
>>>> instances of the same hardware, such as EPSS L3.
>>>>
>>>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@xxxxxxxxxxx>
>>>> ---
>>>>  drivers/interconnect/core.c | 13 ++++++++++++-
>>>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
>>>> index 9d5404a07e8a..40700246f1b6 100644
>>>> --- a/drivers/interconnect/core.c
>>>> +++ b/drivers/interconnect/core.c
>>>> @@ -20,6 +20,8 @@
>>>>  
>>>>  #include "internal.h"
>>>>  
>>>> +#define ICC_DYN_ID_START 10000
>>>> +
>>>>  #define CREATE_TRACE_POINTS
>>>>  #include "trace.h"
>>>>  
>>>> @@ -826,7 +828,12 @@ static struct icc_node *icc_node_create_nolock(int id)
>>>>  	if (!node)
>>>>  		return ERR_PTR(-ENOMEM);
>>>>  
>>>> -	id = idr_alloc(&icc_idr, node, id, id + 1, GFP_KERNEL);
>>>> +	/* negative id indicates dynamic id allocation */
>>>> +	if (id < 0)
>>>
>>> Nit: I think it might be better to add an explicit define for that and
>>> to decline all other negatdive values. Please leave us some room for
>>> future expansion.
>>>
>> Do you mean to replace the value of ALLOC_DYN_ID from -1 to some
>> positive value like 100000 and to use it as initial ID for the nodes
>> requiring the dynamic allocation ? This explicit define can be used as
>> check for dynamic allocation and also as argument to idr_alloc min value
>> argument. Is my interpretation of the comment correct ?
> 
> No, it is not. I asked to add an explicit define for -1 in the ICC
> framework and make icc_node_create_nolock() reject all other negative
> values.

Understood, will make the change as suggested.
> 
>>
>>>> +		id = idr_alloc(&icc_idr, node, ICC_DYN_ID_START, 0, GFP_KERNEL);
>>>> +	else
>>>> +		id = idr_alloc(&icc_idr, node, id, id + 1, GFP_KERNEL);
>>>> +
>>>>  	if (id < 0) {
>>>>  		WARN(1, "%s: couldn't get idr\n", __func__);
>>>>  		kfree(node);
>>>> @@ -962,6 +969,10 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider)
>>>>  	node->avg_bw = node->init_avg;
>>>>  	node->peak_bw = node->init_peak;
>>>>  
>>>> +	if (node->id >= ICC_DYN_ID_START)
>>>> +		node->name = devm_kasprintf(provider->dev, GFP_KERNEL, "%s@%s",
>>>> +					    node->name, dev_name(provider->dev));
>>>> +
>>>>  	if (node->avg_bw || node->peak_bw) {
>>>>  		if (provider->pre_aggregate)
>>>>  			provider->pre_aggregate(node);
>>>> -- 
>>>> 2.39.2
>>>>
>>>
>>
>