| Message ID | 20250205182743.915-3-quic_rlaggysh@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Add EPSS L3 provider support on SA8775P SoC | expand |
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@quicinc.com> > --- > 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. > + 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 >
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@quicinc.com> >> --- >> 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 ? >> + 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 >> >
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@quicinc.com> > >> --- > >> 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. > > >> + 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 > >> > > >
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@quicinc.com> >>>> --- >>>> 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 >>>> >>> >> >
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) + 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);
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@quicinc.com> --- drivers/interconnect/core.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)