[PATCH V4 1/3] core/device: Add function to return child node using name at substring "@"

Subsystems: the rest

STALE1255d REVIEWED: 2 (2M)

1 review trailer.

7 messages, 2 authors, 2023-04-03 · open the first message on its own page

[PATCH V4 1/3] core/device: Add function to return child node using name at substring "@"

From: Athira Rajeev <hidden>
Date: 2023-03-06 03:40:28

Add a function dt_find_by_name_substr() that returns the child node if
it matches till first occurence at "@" of a given name, otherwise NULL.
This is helpful for cases with node name like: "name@addr". In
scenarios where nodes are added with "name@addr" format and if the
value of "addr" is not known, that node can't be matched with node
name or addr. Hence matching with substring as node name will return
the expected result. Patch adds dt_find_by_name_substr() function
and testcase for the same in core/test/run-device.c

Signed-off-by: Athira Rajeev <redacted>
Reviewed-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
---
Changelog:
v3 -> v4:
- Addressed review comment from Mahesh and added his Reviewed-by.

v2 -> v3:
- After review comments from Mahesh, fixed the code
  to consider string upto "@" for both input node name
  as well as child node name. V2 version was comparing
  input node name and child node name upto string length
  of child name. But this will return wrong node if input
  name is larger than child name. Because it will match
  as substring for child name.
  https://lists.ozlabs.org/pipermail/skiboot/2023-January/018596.html

v1 -> v2:
- Addressed review comment from Dan to update
  the utility funtion to search and compare
  upto "@". Renamed it as dt_find_by_name_substr.

 core/device.c          | 31 +++++++++++++++++++++++++++++++
 core/test/run-device.c | 15 +++++++++++++++
 include/device.h       |  3 +++
 3 files changed, 49 insertions(+)
diff --git a/core/device.c b/core/device.c
index b102dd97..6b457ec4 100644
--- a/core/device.c
+++ b/core/device.c
@@ -395,6 +395,37 @@ struct dt_node *dt_find_by_name(struct dt_node *root, const char *name)
 }
 
 
+struct dt_node *dt_find_by_name_substr(struct dt_node *root, const char *name)
+{
+	struct dt_node *child, *match;
+	char *node, *child_node = NULL;
+
+	node = strdup(name);
+	if (!node)
+		return NULL;
+	node = strtok(node, "@");
+	list_for_each(&root->children, child, list) {
+		child_node = strdup(child->name);
+		if (!child_node)
+			goto err;
+		child_node = strtok(child_node, "@");
+		if (!strcmp(child_node, node)) {
+			free(child_node);
+			free(node);
+			return child;
+		}
+
+		match = dt_find_by_name_substr(child, name);
+		if (match)
+			return match;
+	}
+
+	free(child_node);
+err:
+	free(node);
+	return NULL;
+}
+
 struct dt_node *dt_new_check(struct dt_node *parent, const char *name)
 {
 	struct dt_node *node = dt_find_by_name(parent, name);
diff --git a/core/test/run-device.c b/core/test/run-device.c
index 4a12382b..6997452e 100644
--- a/core/test/run-device.c
+++ b/core/test/run-device.c
@@ -466,6 +466,21 @@ int main(void)
 	new_prop_ph = dt_prop_get_u32(ut2, "something");
 	assert(!(new_prop_ph == ev1_ph));
 	dt_free(subtree);
+
+	/* Test dt_find_by_name_substr */
+	root = dt_new_root("");
+	addr1 = dt_new_addr(root, "node", 0x1);
+	addr2 = dt_new_addr(root, "node0_1", 0x2);
+	assert(dt_find_by_name(root, "node@1") == addr1);
+	assert(dt_find_by_name(root, "node0_1@2") == addr2);
+	assert(dt_find_by_name_substr(root, "node@1") == addr1);
+	assert(dt_find_by_name_substr(root, "node0_1@2") == addr2);
+	assert(dt_find_by_name_substr(root, "node0_") == NULL);
+	assert(dt_find_by_name_substr(root, "node0_1") == addr2);
+	assert(dt_find_by_name_substr(root, "node0@") == NULL);
+	assert(dt_find_by_name_substr(root, "node0_@") == NULL);
+	dt_free(root);
+
 	return 0;
 }
 
diff --git a/include/device.h b/include/device.h
index 93fb90ff..b6a1a813 100644
--- a/include/device.h
+++ b/include/device.h
@@ -184,6 +184,9 @@ struct dt_node *dt_find_by_path(struct dt_node *root, const char *path);
 /* Find a child node by name */
 struct dt_node *dt_find_by_name(struct dt_node *root, const char *name);
 
+/* Find a child node by name and substring */
+struct dt_node *dt_find_by_name_substr(struct dt_node *root, const char *name);
+
 /* Find a node by phandle */
 struct dt_node *dt_find_by_phandle(struct dt_node *root, u32 phandle);
 
-- 
2.31.1

[PATCH V4 2/3] skiboot: Update IMC code to use dt_find_by_name_substr for checking dt nodes

From: Athira Rajeev <hidden>
Date: 2023-03-06 03:41:31

The nest IMC (In Memory Collection) Performance Monitoring
Unit(PMU) node names are saved in nest_pmus[] array in the
"hw/imc.c" IMC code. Not all the IMC PMUs listed in the device
tree may be available. Nest IMC PMU names along with their
bit values is represented in imc availability vector.
The nest_pmus[] array is used to remove the unavailable nodes
by checking this vector.

To check node availability, code was using "dt_find_by_substr".
But since the node names have format like: "name@offset",
dt_find_by_name doesn't return the expected result. Fix this
by using dt_find_by_name_substr. Also, update the char array
to use correct node names.

Signed-off-by: Athira Rajeev <redacted>
---
Changelog:
v3 -> v4:
- Addressed review comment from Mahesh and added his Reviewed-by
  for patch 1.

v2 -> v3:
- After review comments from Mahesh, fixed the code
  to consider string upto "@" for both input node name
  as well as child node name. V2 version was comparing
  input node name and child node name upto string length
  of child name. But this will return wrong node if input
  name is larger than child name. Because it will match
  as substring for child name.
  https://lists.ozlabs.org/pipermail/skiboot/2023-January/018596.html

v1 -> v2:
- Addressed review comment from Dan to update
  the utility funtion to search and compare
  upto "@". Renamed it as dt_find_by_name_substr.

 hw/imc.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/hw/imc.c b/hw/imc.c
index cbd68edc..55f53cce 100644
--- a/hw/imc.c
+++ b/hw/imc.c
@@ -66,14 +66,14 @@ static char const *nest_pmus[] = {
 	"mba5",
 	"mba6",
 	"mba7",
-	"cen0",
-	"cen1",
-	"cen2",
-	"cen3",
-	"cen4",
-	"cen5",
-	"cen6",
-	"cen7",
+	"centaur0",
+	"centaur1",
+	"centaur2",
+	"centaur3",
+	"centaur4",
+	"centaur5",
+	"centaur6",
+	"centaur7",
 	"xlink0",
 	"xlink1",
 	"xlink2",
@@ -411,7 +411,7 @@ static void disable_unavailable_units(struct dt_node *dev)
 	for (i = 0; i < ARRAY_SIZE(nest_pmus); i++) {
 		if (!(PPC_BITMASK(i, i) & avl_vec)) {
 			/* Check if the device node exists */
-			target = dt_find_by_name(dev, nest_pmus[i]);
+			target = dt_find_by_name_substr(dev, nest_pmus[i]);
 			if (!target)
 				continue;
 			/* Remove the device node */
-- 
2.31.1

[PATCH V4 3/3] skiboot: Update IMC PMU node names for power10

From: Athira Rajeev <hidden>
Date: 2023-03-06 03:42:42

The nest IMC (In Memory Collection) Performance Monitoring
Unit(PMU) node names are saved as "struct nest_pmus_struct"
in the "hw/imc.c" IMC code. Not all the IMC PMUs listed in
the device tree may be available. Nest IMC PMU names along with
their bit values is represented in imc availability vector.
This struct is used to remove the unavailable nodes by checking
this vector.

For power10, the imc_chip_avl_vector ie, imc availability vector
( which is a part of the IMC control block structure ), has
change in mapping of units and bit positions. Hence rename the
existing nest_pmus array to nest_pmus_p9 and add entry for power10
as nest_pmus_p10.

Also the avl_vector has another change in bit positions 11:34. These
bit positions tells the availability of Xlink/Alink/CAPI. There
are total 8 links and three bit field combination says which link
is available. Patch implements all these change to handle
nest_pmus_p10.

Signed-off-by: Athira Rajeev <redacted>
---
Changelog:
v3 -> v4:
- Addressed review comment from Mahesh and added his Reviewed-by
  for patch 1.

v2 -> v3:
- After review comments from Mahesh, fixed the code
  to consider string upto "@" for both input node name
  as well as child node name. V2 version was comparing
  input node name and child node name upto string length
  of child name. But this will return wrong node if input
  name is larger than child name. Because it will match
  as substring for child name.
  https://lists.ozlabs.org/pipermail/skiboot/2023-January/018596.html

v1 -> v2:
- Addressed review comment from Dan to update
  the utility funtion to search and compare
  upto "@". Renamed it as dt_find_by_name_substr.

 hw/imc.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 185 insertions(+), 10 deletions(-)
diff --git a/hw/imc.c b/hw/imc.c
index 55f53cce..3e14b508 100644
--- a/hw/imc.c
+++ b/hw/imc.c
@@ -48,7 +48,7 @@ static unsigned int *htm_scom_index;
  * imc_chip_avl_vector(in struct imc_chip_cb, look at include/imc.h).
  * nest_pmus[] is an array containing all the possible nest IMC PMU node names.
  */
-static char const *nest_pmus[] = {
+static char const *nest_pmus_p9[] = {
 	"powerbus0",
 	"mcs0",
 	"mcs1",
@@ -103,6 +103,67 @@ static char const *nest_pmus[] = {
 	/* reserved bits : 51 - 63 */
 };
 
+static char const *nest_pmus_p10[] = {
+	"pb",
+	"mcs0",
+	"mcs1",
+	"mcs2",
+	"mcs3",
+	"mcs4",
+	"mcs5",
+	"mcs6",
+	"mcs7",
+	"pec0",
+	"pec1",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"NA",
+	"phb0",
+	"phb1",
+	"phb2",
+	"phb3",
+	"phb4",
+	"phb5",
+	"ocmb0",
+	"ocmb1",
+	"ocmb2",
+	"ocmb3",
+	"ocmb4",
+	"ocmb5",
+	"ocmb6",
+	"ocmb7",
+	"ocmb8",
+	"ocmb9",
+	"ocmb10",
+	"ocmb11",
+	"ocmb12",
+	"ocmb13",
+	"ocmb14",
+	"ocmb15",
+	"nx",
+};
+
 /*
  * Due to Nest HW/OCC restriction, microcode will not support individual unit
  * events for these nest units mcs0, mcs1 ... mcs7 in the accumulation mode.
@@ -370,7 +431,7 @@ static void disable_unavailable_units(struct dt_node *dev)
 	uint64_t avl_vec;
 	struct imc_chip_cb *cb;
 	struct dt_node *target;
-	int i;
+	int i, j;
 	bool disable_all_nests = false;
 	struct proc_chip *chip;
 
@@ -408,14 +469,128 @@ static void disable_unavailable_units(struct dt_node *dev)
 			avl_vec = (0xffULL) << 56;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(nest_pmus); i++) {
-		if (!(PPC_BITMASK(i, i) & avl_vec)) {
-			/* Check if the device node exists */
-			target = dt_find_by_name_substr(dev, nest_pmus[i]);
-			if (!target)
-				continue;
-			/* Remove the device node */
-			dt_free(target);
+	if (proc_gen == proc_gen_p9) {
+		for (i = 0; i < ARRAY_SIZE(nest_pmus_p9); i++) {
+			if (!(PPC_BITMASK(i, i) & avl_vec)) {
+				/* Check if the device node exists */
+				target = dt_find_by_name_substr(dev, nest_pmus_p9[i]);
+				if (!target)
+					continue;
+				/* Remove the device node */
+				dt_free(target);
+			}
+		}
+	} else if (proc_gen == proc_gen_p10) {
+		int val;
+		char al[8], xl[8], otl[8], phb[8];
+		for (i = 0; i < 11; i++) {
+			if (!(PPC_BITMASK(i, i) & avl_vec)) {
+				/* Check if the device node exists */
+				target = dt_find_by_name_substr(dev, nest_pmus_p10[i]);
+				if (!target)
+					continue;
+				/* Remove the device node */
+				dt_free(target);
+			}
+		}
+
+		for (i = 35; i < 41; i++) {
+			if (!(PPC_BITMASK(i, i) & avl_vec)) {
+				/* Check if the device node exists for phb */
+				for (j = 0; j < 3; j++) {
+					snprintf(phb, sizeof(phb), "phb%d_%d", (i-35), j);
+					target = dt_find_by_name_substr(dev, phb);
+					if (!target)
+						continue;
+					/* Remove the device node */
+					dt_free(target);
+				}
+			}
+		}
+
+		for (i = 41; i < 58; i++) {
+			if (!(PPC_BITMASK(i, i) & avl_vec)) {
+				/* Check if the device node exists */
+				target = dt_find_by_name_substr(dev, nest_pmus_p10[i]);
+				if (!target)
+					continue;
+				/* Remove the device node */
+				dt_free(target);
+			}
+		}
+
+		for (i=0; i<8; i++) {
+			val = ((avl_vec & (0x7ULL << (29 + (3 * i)))) >> (29 + (3 * i)));
+			switch (val) {
+			case 0x5: //xlink configured and functional
+
+				snprintf(al, sizeof(al), "alink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, al);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl, sizeof(otl),"otl%1d_0",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl,sizeof(otl),"otl%1d_1",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+
+				break;
+			case 0x6: //alink configured and functional
+
+				snprintf(xl,sizeof(xl),"xlink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, xl);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl,sizeof(otl),"otl%1d_0",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl,sizeof(otl),"otl%1d_1",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+				break;
+
+			case 0x7: //CAPI configured and functional
+				snprintf(al,sizeof(al),"alink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, al);
+				if (target)
+					dt_free(target);
+
+				snprintf(xl,sizeof(xl),"xlink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, xl);
+				if (target)
+					dt_free(target);
+				break;
+			default:
+				snprintf(xl,sizeof(xl),"xlink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, xl);
+				if (target)
+					dt_free(target);
+
+				snprintf(al,sizeof(al),"alink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, al);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl,sizeof(otl),"otl%1d_0",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl,sizeof(otl),"otl%1d_1",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+				break;
+			}
 		}
 	}
 
-- 
2.31.1

Re: [PATCH V4 1/3] core/device: Add function to return child node using name at substring "@"

From: Athira Rajeev <hidden>
Date: 2023-03-10 06:21:10

On 06-Mar-2023, at 9:09 AM, Athira Rajeev [off-list ref] wrote:

Add a function dt_find_by_name_substr() that returns the child node if
it matches till first occurence at "@" of a given name, otherwise NULL.
This is helpful for cases with node name like: "name@addr". In
scenarios where nodes are added with "name@addr" format and if the
value of "addr" is not known, that node can't be matched with node
name or addr. Hence matching with substring as node name will return
the expected result. Patch adds dt_find_by_name_substr() function
and testcase for the same in core/test/run-device.c

Signed-off-by: Athira Rajeev <redacted>
Reviewed-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
---
Changelog:
v3 -> v4:
- Addressed review comment from Mahesh and added his Reviewed-by.
Hi,

Looking for review comments. Please share feedback.

Thanks
Athira
quoted hunk
v2 -> v3:
- After review comments from Mahesh, fixed the code
 to consider string upto "@" for both input node name
 as well as child node name. V2 version was comparing
 input node name and child node name upto string length
 of child name. But this will return wrong node if input
 name is larger than child name. Because it will match
 as substring for child name.
 https://lists.ozlabs.org/pipermail/skiboot/2023-January/018596.html

v1 -> v2:
- Addressed review comment from Dan to update
 the utility funtion to search and compare
 upto "@". Renamed it as dt_find_by_name_substr.

core/device.c          | 31 +++++++++++++++++++++++++++++++
core/test/run-device.c | 15 +++++++++++++++
include/device.h       |  3 +++
3 files changed, 49 insertions(+)
diff --git a/core/device.c b/core/device.c
index b102dd97..6b457ec4 100644
--- a/core/device.c
+++ b/core/device.c
@@ -395,6 +395,37 @@ struct dt_node *dt_find_by_name(struct dt_node *root, const char *name)
}


+struct dt_node *dt_find_by_name_substr(struct dt_node *root, const char *name)
+{
+	struct dt_node *child, *match;
+	char *node, *child_node = NULL;
+
+	node = strdup(name);
+	if (!node)
+		return NULL;
+	node = strtok(node, "@");
+	list_for_each(&root->children, child, list) {
+		child_node = strdup(child->name);
+		if (!child_node)
+			goto err;
+		child_node = strtok(child_node, "@");
+		if (!strcmp(child_node, node)) {
+			free(child_node);
+			free(node);
+			return child;
+		}
+
+		match = dt_find_by_name_substr(child, name);
+		if (match)
+			return match;
+	}
+
+	free(child_node);
+err:
+	free(node);
+	return NULL;
+}
+
struct dt_node *dt_new_check(struct dt_node *parent, const char *name)
{
	struct dt_node *node = dt_find_by_name(parent, name);
diff --git a/core/test/run-device.c b/core/test/run-device.c
index 4a12382b..6997452e 100644
--- a/core/test/run-device.c
+++ b/core/test/run-device.c
@@ -466,6 +466,21 @@ int main(void)
	new_prop_ph = dt_prop_get_u32(ut2, "something");
	assert(!(new_prop_ph == ev1_ph));
	dt_free(subtree);
+
+	/* Test dt_find_by_name_substr */
+	root = dt_new_root("");
+	addr1 = dt_new_addr(root, "node", 0x1);
+	addr2 = dt_new_addr(root, "node0_1", 0x2);
+	assert(dt_find_by_name(root, "node@1") == addr1);
+	assert(dt_find_by_name(root, "node0_1@2") == addr2);
+	assert(dt_find_by_name_substr(root, "node@1") == addr1);
+	assert(dt_find_by_name_substr(root, "node0_1@2") == addr2);
+	assert(dt_find_by_name_substr(root, "node0_") == NULL);
+	assert(dt_find_by_name_substr(root, "node0_1") == addr2);
+	assert(dt_find_by_name_substr(root, "node0@") == NULL);
+	assert(dt_find_by_name_substr(root, "node0_@") == NULL);
+	dt_free(root);
+
	return 0;
}
diff --git a/include/device.h b/include/device.h
index 93fb90ff..b6a1a813 100644
--- a/include/device.h
+++ b/include/device.h
@@ -184,6 +184,9 @@ struct dt_node *dt_find_by_path(struct dt_node *root, const char *path);
/* Find a child node by name */
struct dt_node *dt_find_by_name(struct dt_node *root, const char *name);

+/* Find a child node by name and substring */
+struct dt_node *dt_find_by_name_substr(struct dt_node *root, const char *name);
+
/* Find a node by phandle */
struct dt_node *dt_find_by_phandle(struct dt_node *root, u32 phandle);

-- 
2.31.1

Re: [Skiboot] [PATCH V4 1/3] core/device: Add function to return child node using name at substring "@"

From: Reza Arbab <hidden>
Date: 2023-03-22 20:14:11

Hi Athira,

On Mon, Mar 06, 2023 at 09:09:11AM +0530, Athira Rajeev wrote:
Add a function dt_find_by_name_substr() that returns the child node if
it matches till first occurence at "@" of a given name, otherwise NULL.
Given this summary, I don't understand the following:
+	assert(dt_find_by_name_substr(root, "node@1") == addr1);
+	assert(dt_find_by_name_substr(root, "node0_1@2") == addr2);
Is this behavior required? I don't think it makes sense to call this 
function with a second argument containing '@', so I wouldn't expect it 
to match anything in these cases. The function seems to specifically 
enable it:
+struct dt_node *dt_find_by_name_substr(struct dt_node *root, const char *name)
+{
[snip]
+      node = strdup(name);
+      if (!node)
+              return NULL;
+      node = strtok(node, "@");
Seems like you could get rid of this and just use name as-is.

I was curious about something else; say we have 'node@1' and 'node@2'.  
Is there an expectation of which it should match?

     addr1 = dt_new_addr(root, "node", 0x1);
     addr2 = dt_new_addr(root, "node", 0x2);
     assert(dt_find_by_name_substr(root, "node") == ???????);
                                                    ^^^^^^^
+/* Find a child node by name and substring */
+struct dt_node *dt_find_by_name_substr(struct dt_node *root, const char *name);
I think this name fit better in previous versions of the patch, but 
since you're specifically looking for '@' now, maybe call it something 
like dt_find_by_name_before_addr?

-- 
Reza Arbab

Re: [Skiboot] [PATCH V4 3/3] skiboot: Update IMC PMU node names for power10

From: Reza Arbab <hidden>
Date: 2023-03-22 20:45:21

On Mon, Mar 06, 2023 at 09:09:13AM +0530, Athira Rajeev wrote:
+	} else if (proc_gen == proc_gen_p10) {
+		int val;
+		char al[8], xl[8], otl[8], phb[8];
Are four different variables needed here? I think you could just reuse 
one temporary variable.

		char name[8];
+		for (i=0; i<8; i++) {
+			val = ((avl_vec & (0x7ULL << (29 + (3 * i)))) >> (29 + (3 * i)));
+			switch (val) {
+			case 0x5: //xlink configured and functional
+
+				snprintf(al, sizeof(al), "alink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, al);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl, sizeof(otl),"otl%1d_0",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl,sizeof(otl),"otl%1d_1",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+
+				break;
+			case 0x6: //alink configured and functional
+
+				snprintf(xl,sizeof(xl),"xlink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, xl);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl,sizeof(otl),"otl%1d_0",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl,sizeof(otl),"otl%1d_1",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+				break;
+
+			case 0x7: //CAPI configured and functional
+				snprintf(al,sizeof(al),"alink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, al);
+				if (target)
+					dt_free(target);
+
+				snprintf(xl,sizeof(xl),"xlink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, xl);
+				if (target)
+					dt_free(target);
+				break;
+			default:
+				snprintf(xl,sizeof(xl),"xlink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, xl);
+				if (target)
+					dt_free(target);
+
+				snprintf(al,sizeof(al),"alink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, al);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl,sizeof(otl),"otl%1d_0",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl,sizeof(otl),"otl%1d_1",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+				break;
As far as I know skiboot follows the kernel coding style. Would you mind 
fixing up the minor style nits checkpatch.pl reports for this patch?

-- 
Reza Arbab

Re: [Skiboot] [PATCH V4 3/3] skiboot: Update IMC PMU node names for power10

From: Athira Rajeev <hidden>
Date: 2023-04-03 04:05:39

On 23-Mar-2023, at 2:14 AM, Reza Arbab [off-list ref] wrote:

On Mon, Mar 06, 2023 at 09:09:13AM +0530, Athira Rajeev wrote:
quoted
+	} else if (proc_gen == proc_gen_p10) {
+		int val;
+		char al[8], xl[8], otl[8], phb[8];
Are four different variables needed here? I think you could just reuse one temporary variable.

		char name[8];
Hi Reza,

Thanks for checking the patch. I will send next version with this change
quoted
+		for (i=0; i<8; i++) {
+			val = ((avl_vec & (0x7ULL << (29 + (3 * i)))) >> (29 + (3 * i)));
+			switch (val) {
+			case 0x5: //xlink configured and functional
+
+				snprintf(al, sizeof(al), "alink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, al);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl, sizeof(otl),"otl%1d_0",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl,sizeof(otl),"otl%1d_1",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+
+				break;
+			case 0x6: //alink configured and functional
+
+				snprintf(xl,sizeof(xl),"xlink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, xl);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl,sizeof(otl),"otl%1d_0",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl,sizeof(otl),"otl%1d_1",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+				break;
+
+			case 0x7: //CAPI configured and functional
+				snprintf(al,sizeof(al),"alink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, al);
+				if (target)
+					dt_free(target);
+
+				snprintf(xl,sizeof(xl),"xlink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, xl);
+				if (target)
+					dt_free(target);
+				break;
+			default:
+				snprintf(xl,sizeof(xl),"xlink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, xl);
+				if (target)
+					dt_free(target);
+
+				snprintf(al,sizeof(al),"alink%1d",(7-i));
+				target = dt_find_by_name_substr(dev, al);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl,sizeof(otl),"otl%1d_0",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+
+				snprintf(otl,sizeof(otl),"otl%1d_1",(7-i));
+				target = dt_find_by_name_substr(dev, otl);
+				if (target)
+					dt_free(target);
+				break;
As far as I know skiboot follows the kernel coding style. Would you mind fixing up the minor style nits checkpatch.pl reports for this patch?
Sure 

Thanks
Athira
-- 
Reza Arbab

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help