2022-06-21 19:34:08

by Sudeep Holla

Subject: [PATCH v4 20/20] arch_topology: Warn that topology for nested clusters is not supported

We don't support the topology for clusters of CPU clusters while the
DT and ACPI bindings theoritcally support the same. Just warn about the
same so that it is clear to the users of arch_topology that the nested
clusters are not yet supported.

Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/base/arch_topology.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index ed1cb64a95aa..1c5fa7bbbd00 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -567,6 +567,8 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
if (c) {
leaf = false;
ret = parse_cluster(c, package_id, i, depth + 1);
+ if (depth > 1)
+ pr_warn("Topology for clusters of clusters not yet supported\n");
of_node_put(c);
if (ret != 0)
return ret;
--
2.36.1

2022-06-22 15:55:22

by Ionela Voinescu

Subject: Re: [PATCH v4 20/20] arch_topology: Warn that topology for nested clusters is not supported

Hi,

I just noticed this in a quick test.

On Tuesday 21 Jun 2022 at 20:20:34 (+0100), Sudeep Holla wrote:
> We don't support the topology for clusters of CPU clusters while the
> DT and ACPI bindings theoritcally support the same. Just warn about the
> same so that it is clear to the users of arch_topology that the nested
> clusters are not yet supported.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/base/arch_topology.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index ed1cb64a95aa..1c5fa7bbbd00 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -567,6 +567,8 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
> if (c) {
> leaf = false;
> ret = parse_cluster(c, package_id, i, depth + 1);
> + if (depth > 1)
> + pr_warn("Topology for clusters of clusters not yet supported\n");

I think the check should be for (depth > 0) or (depth >= 1).

We end up having depth = 2 when we have

cluster 0 {
//depth is 0
cluster 0 {
//depth is 1
cluster0 {
//depth is 2
...

I suppose we should warn about nested clusters from depth 1, right?

Thanks,
Ionela.

> of_node_put(c);
> if (ret != 0)
> return ret;
> --
> 2.36.1
>

2022-06-22 16:12:43

by Sudeep Holla

Subject: Re: [PATCH v4 20/20] arch_topology: Warn that topology for nested clusters is not supported

On Wed, Jun 22, 2022 at 04:06:29PM +0100, Ionela Voinescu wrote:
> Hi,
>
> I just noticed this in a quick test.
>
> On Tuesday 21 Jun 2022 at 20:20:34 (+0100), Sudeep Holla wrote:
> > We don't support the topology for clusters of CPU clusters while the
> > DT and ACPI bindings theoritcally support the same. Just warn about the
> > same so that it is clear to the users of arch_topology that the nested
> > clusters are not yet supported.
> >
> > Signed-off-by: Sudeep Holla <[email protected]>
> > ---
> > drivers/base/arch_topology.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> > index ed1cb64a95aa..1c5fa7bbbd00 100644
> > --- a/drivers/base/arch_topology.c
> > +++ b/drivers/base/arch_topology.c
> > @@ -567,6 +567,8 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
> > if (c) {
> > leaf = false;
> > ret = parse_cluster(c, package_id, i, depth + 1);
> > + if (depth > 1)
> > + pr_warn("Topology for clusters of clusters not yet supported\n");
>
> I think the check should be for (depth > 0) or (depth >= 1).
>
> We end up having depth = 2 when we have
>
> cluster 0 {
> //depth is 0
> cluster 0 {
> //depth is 1
> cluster0 {
> //depth is 2
> ...
>
> I suppose we should warn about nested clusters from depth 1, right?
>

You are absolutely correct. For some reason when I wrote this patch I
read the line above as depth++ instead of depth + 1. I was searching
for that now reading your reply just to realise that I misread it.

Thanks for catching this.

--
Regards,
Sudeep