| Message ID | 20250111161429.51-2-quic_rlaggysh@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add EPSS L3 provider support on SA8775P SoC | expand |
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)? 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 >
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. This doesn't overcome the dependency on static ID. Drivers still have to manually lookup the resulting ID and use it to link the nodes. Instead ICC framework should be providing a completely dynamic solution: - icc_node_create() should get a completely dynamic counterpart. Use e.g. 1000000 as a dynamic start ID. - icc_link_create() shold get a counterpart which can create a link between two icc_node instances directly, without an additional lookup. You can check if your implementation is correct if you can refactor existing ICC drivers (e.g. icc-clk and/or icc-rpm to drop ID arrays completely). > > 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(+) >
Hi Raviteja,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Raviteja-Laggyshetty/interconnect-core-Add-dynamic-id-allocation-support/20250112-001756
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20250111161429.51-2-quic_rlaggysh%40quicinc.com
patch subject: [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support
config: arm-randconfig-r072-20250118 (https://download.01.org/0day-ci/archive/20250120/202501201530.UTAPd4lC-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project c23f2417dc5f6dc371afb07af5627ec2a9d373a0)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202501201530.UTAPd4lC-lkp@intel.com/
smatch warnings:
drivers/interconnect/core.c:889 icc_node_create_alloc_id() warn: inconsistent returns 'global &icc_lock'.
vim +889 drivers/interconnect/core.c
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 867 struct icc_node *icc_node_create_alloc_id(int start_id)
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 868 {
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 869 struct icc_node *node;
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 870 int id;
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 871
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 872 mutex_lock(&icc_lock);
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 873
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 874 node = kzalloc(sizeof(*node), GFP_KERNEL);
Do this allocation before taking the mutex_lock(&icc_lock). Otherwise
you'd have to unlock before returning.
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 875 if (!node)
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 876 return ERR_PTR(-ENOMEM);
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 877
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 878 id = idr_alloc(&icc_idr, node, start_id, 0, GFP_KERNEL);
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 879 if (id < 0) {
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 880 WARN(1, "%s: couldn't get idr\n", __func__);
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 881 kfree(node);
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 882 node = ERR_PTR(id);
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 883 goto out;
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 884 }
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 885 node->id = id;
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 886 out:
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 887 mutex_unlock(&icc_lock);
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 888
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 @889 return node;
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 890 }
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 >>
On 1/13/2025 1:44 PM, Dmitry Baryshkov 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. > > This doesn't overcome the dependency on static ID. Drivers still have to > manually lookup the resulting ID and use it to link the nodes. Instead > ICC framework should be providing a completely dynamic solution: > - icc_node_create() should get a completely dynamic counterpart. Use > e.g. 1000000 as a dynamic start ID. > - icc_link_create() shold get a counterpart which can create a link > between two icc_node instances directly, without an additional lookup. > Agreed, with current implementation, still there is dependency on IDs for linking the nodes. Instead of relying on node names for the links, array of struct pointers will be used, this will eliminate the need for ID lookup and avoids extra loops. Instead of providing counter part for the ICC framework APIs which involves duplication of most of the code, I will modify the existing icc_node_create, icc_link_create and icc_node_add APIs to support both static and dynamic IDs. > You can check if your implementation is correct if you can refactor > existing ICC drivers (e.g. icc-clk and/or icc-rpm to drop ID arrays > completely). > ok, I will check the implementation on icc-rpmh driver for sa8775p SoC. >> >> 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) +{ + 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) { }
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(+)