* [PATCH V5 1/3] core/device: Add function to return child node using name at substring "@"
@ 2023-07-17 3:24 Athira Rajeev
2023-07-17 3:24 ` [PATCH V5 2/3] skiboot: Update IMC code to use dt_find_by_name_before_addr for checking dt nodes Athira Rajeev
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Athira Rajeev @ 2023-07-17 3:24 UTC (permalink / raw)
To: skiboot, dan, arbab, mpe, maddy
Cc: kjain, Athira Rajeev, disgoel, linuxppc-dev, mahesh
Add a function dt_find_by_name_before_addr() that returns the child node if
it matches till first occurrence 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_before_addr() function
and testcase for the same in core/test/run-device.c
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Reviewed-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
---
Changelog:
v4 -> v5:
- Addressed review comment from Reza and renamed
dt_find_by_name_substr to dt_find_by_name_before_addr
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 | 14 ++++++++++++++
include/device.h | 3 +++
3 files changed, 48 insertions(+)
diff --git a/core/device.c b/core/device.c
index b102dd97..a61a72b0 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_before_addr(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_before_addr(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..f1cb79bf 100644
--- a/core/test/run-device.c
+++ b/core/test/run-device.c
@@ -466,6 +466,20 @@ 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_before_addr */
+ 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_before_addr(root, "node@1") == addr1);
+ assert(dt_find_by_name_before_addr(root, "node0_") == NULL);
+ assert(dt_find_by_name_before_addr(root, "node0_1") == addr2);
+ assert(dt_find_by_name_before_addr(root, "node0@") == NULL);
+ assert(dt_find_by_name_before_addr(root, "node0_@") == NULL);
+ dt_free(root);
+
return 0;
}
diff --git a/include/device.h b/include/device.h
index 93fb90ff..f2402cc4 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_before_addr(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.39.3
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH V5 2/3] skiboot: Update IMC code to use dt_find_by_name_before_addr for checking dt nodes 2023-07-17 3:24 [PATCH V5 1/3] core/device: Add function to return child node using name at substring "@" Athira Rajeev @ 2023-07-17 3:24 ` Athira Rajeev 2023-07-17 3:24 ` [PATCH V5 3/3] skiboot: Update IMC PMU node names for power10 Athira Rajeev 2023-08-09 21:51 ` [PATCH V5 1/3] core/device: Add function to return child node using name at substring "@" Reza Arbab 2 siblings, 0 replies; 7+ messages in thread From: Athira Rajeev @ 2023-07-17 3:24 UTC (permalink / raw) To: skiboot, dan, arbab, mpe, maddy Cc: kjain, Athira Rajeev, disgoel, linuxppc-dev, mahesh 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_before_addr. Also, update the char array to use correct node names. Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com> --- v4 -> v5: - Addressed review comment from Reza and renamed dt_find_by_name_substr to dt_find_by_name_before_addr 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..a8198321 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_before_addr(dev, nest_pmus[i]); if (!target) continue; /* Remove the device node */ -- 2.39.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH V5 3/3] skiboot: Update IMC PMU node names for power10 2023-07-17 3:24 [PATCH V5 1/3] core/device: Add function to return child node using name at substring "@" Athira Rajeev 2023-07-17 3:24 ` [PATCH V5 2/3] skiboot: Update IMC code to use dt_find_by_name_before_addr for checking dt nodes Athira Rajeev @ 2023-07-17 3:24 ` Athira Rajeev 2023-08-09 21:58 ` Reza Arbab 2023-08-09 21:51 ` [PATCH V5 1/3] core/device: Add function to return child node using name at substring "@" Reza Arbab 2 siblings, 1 reply; 7+ messages in thread From: Athira Rajeev @ 2023-07-17 3:24 UTC (permalink / raw) To: skiboot, dan, arbab, mpe, maddy Cc: kjain, Athira Rajeev, disgoel, linuxppc-dev, mahesh 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 <atrajeev@linux.vnet.ibm.com> --- Changelog: v4 -> v5: - Addressed review comment from Reza and renamed dt_find_by_name_substr to dt_find_by_name_before_addr. Instead of using multiple variables for link name, reuse one variable. 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 | 196 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 186 insertions(+), 10 deletions(-) diff --git a/hw/imc.c b/hw/imc.c index a8198321..fd58058d 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 const char *nest_pmus_p9[] = { "powerbus0", "mcs0", "mcs1", @@ -103,6 +103,67 @@ static char const *nest_pmus[] = { /* reserved bits : 51 - 63 */ }; +static const char *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,129 @@ 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_before_addr(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_before_addr(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 name[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_before_addr(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(name, sizeof(name), "phb%d_%d", (i-35), j); + target = dt_find_by_name_before_addr(dev, name); + 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_before_addr(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(name, sizeof(name), "alink%1d", (7-i)); + target = dt_find_by_name_before_addr(dev, name); + if (target) + dt_free(target); + + snprintf(name, sizeof(name), "otl%1d_0", (7-i)); + target = dt_find_by_name_before_addr(dev, name); + if (target) + dt_free(target); + + snprintf(name, sizeof(name), "otl%1d_1", (7-i)); + target = dt_find_by_name_before_addr(dev, name); + if (target) + dt_free(target); + + break; + case 0x6: //alink configured and functional + + snprintf(name, sizeof(name), "xlink%1d", (7-i)); + target = dt_find_by_name_before_addr(dev, name); + if (target) + dt_free(target); + + snprintf(name, sizeof(name), "otl%1d_0", (7-i)); + target = dt_find_by_name_before_addr(dev, name); + if (target) + dt_free(target); + + snprintf(name, sizeof(name), "otl%1d_1", (7-i)); + target = dt_find_by_name_before_addr(dev, name); + if (target) + dt_free(target); + break; + + case 0x7: //CAPI configured and functional + snprintf(name, sizeof(name), "alink%1d", (7-i)); + target = dt_find_by_name_before_addr(dev, name); + if (target) + dt_free(target); + + snprintf(name, sizeof(name), "xlink%1d", (7-i)); + target = dt_find_by_name_before_addr(dev, name); + if (target) + dt_free(target); + break; + default: + snprintf(name, sizeof(name), "xlink%1d", (7-i)); + target = dt_find_by_name_before_addr(dev, name); + if (target) + dt_free(target); + + snprintf(name, sizeof(name), "alink%1d", (7-i)); + target = dt_find_by_name_before_addr(dev, name); + if (target) + dt_free(target); + + snprintf(name, sizeof(name), "otl%1d_0", (7-i)); + target = dt_find_by_name_before_addr(dev, name); + if (target) + dt_free(target); + + snprintf(name, sizeof(name), "otl%1d_1", (7-i)); + target = dt_find_by_name_before_addr(dev, name); + if (target) + dt_free(target); + break; + } } } -- 2.39.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH V5 3/3] skiboot: Update IMC PMU node names for power10 2023-07-17 3:24 ` [PATCH V5 3/3] skiboot: Update IMC PMU node names for power10 Athira Rajeev @ 2023-08-09 21:58 ` Reza Arbab 2023-09-11 5:01 ` Athira Rajeev 0 siblings, 1 reply; 7+ messages in thread From: Reza Arbab @ 2023-08-09 21:58 UTC (permalink / raw) To: Athira Rajeev; +Cc: maddy, dan, mahesh, kjain, skiboot, disgoel, linuxppc-dev On Mon, Jul 17, 2023 at 08:54:31AM +0530, Athira Rajeev wrote: >@@ -408,14 +469,129 @@ 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_before_addr(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)) { I think all these PPC_BITMASK(i, i) can be changed to PPC_BIT(i). >+ /* Check if the device node exists */ >+ target = dt_find_by_name_before_addr(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 name[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_before_addr(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(name, sizeof(name), "phb%d_%d", (i-35), j); >+ target = dt_find_by_name_before_addr(dev, name); >+ 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_before_addr(dev, nest_pmus_p10[i]); >+ if (!target) >+ continue; >+ /* Remove the device node */ >+ dt_free(target); >+ } >+ } -- Reza Arbab ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V5 3/3] skiboot: Update IMC PMU node names for power10 2023-08-09 21:58 ` Reza Arbab @ 2023-09-11 5:01 ` Athira Rajeev 0 siblings, 0 replies; 7+ messages in thread From: Athira Rajeev @ 2023-09-11 5:01 UTC (permalink / raw) To: Reza Arbab Cc: Madhavan Srinivasan, Dan Horák, Mahesh J Salgaonkar, Kajol Jain, skiboot, Disha Goel, linuxppc-dev > On 10-Aug-2023, at 3:28 AM, Reza Arbab <arbab@linux.ibm.com> wrote: > > On Mon, Jul 17, 2023 at 08:54:31AM +0530, Athira Rajeev wrote: >> @@ -408,14 +469,129 @@ 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_before_addr(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)) { > > I think all these PPC_BITMASK(i, i) can be changed to PPC_BIT(i). Hi Reza, Thanks for reviewing the changes. Yes. I will add the change in next version Thanks Athira > >> + /* Check if the device node exists */ >> + target = dt_find_by_name_before_addr(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 name[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_before_addr(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(name, sizeof(name), "phb%d_%d", (i-35), j); >> + target = dt_find_by_name_before_addr(dev, name); >> + 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_before_addr(dev, nest_pmus_p10[i]); >> + if (!target) >> + continue; >> + /* Remove the device node */ >> + dt_free(target); >> + } >> + } > > -- > Reza Arbab ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V5 1/3] core/device: Add function to return child node using name at substring "@" 2023-07-17 3:24 [PATCH V5 1/3] core/device: Add function to return child node using name at substring "@" Athira Rajeev 2023-07-17 3:24 ` [PATCH V5 2/3] skiboot: Update IMC code to use dt_find_by_name_before_addr for checking dt nodes Athira Rajeev 2023-07-17 3:24 ` [PATCH V5 3/3] skiboot: Update IMC PMU node names for power10 Athira Rajeev @ 2023-08-09 21:51 ` Reza Arbab 2023-09-11 5:15 ` Athira Rajeev 2 siblings, 1 reply; 7+ messages in thread From: Reza Arbab @ 2023-08-09 21:51 UTC (permalink / raw) To: Athira Rajeev; +Cc: maddy, dan, mahesh, kjain, skiboot, disgoel, linuxppc-dev Hi Athira, I still have a couple of the same questions I asked in v4. On Mon, Jul 17, 2023 at 08:54:29AM +0530, Athira Rajeev wrote: >Add a function dt_find_by_name_before_addr() that returns the child node if >it matches till first occurrence at "@" of a given name, otherwise NULL. Given this summary, I don't userstand the following: >+ assert(dt_find_by_name(root, "node@1") == addr1); >+ assert(dt_find_by_name(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_before_addr(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") == ???????); ^^^^^^^ -- Reza Arbab ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V5 1/3] core/device: Add function to return child node using name at substring "@" 2023-08-09 21:51 ` [PATCH V5 1/3] core/device: Add function to return child node using name at substring "@" Reza Arbab @ 2023-09-11 5:15 ` Athira Rajeev 0 siblings, 0 replies; 7+ messages in thread From: Athira Rajeev @ 2023-09-11 5:15 UTC (permalink / raw) To: Reza Arbab Cc: maddy, Dan Horák, mahesh, kjain, skiboot, disgoel, linuxppc-dev > On 10-Aug-2023, at 3:21 AM, Reza Arbab <arbab@linux.ibm.com> wrote: > > Hi Athira, > > I still have a couple of the same questions I asked in v4. > > On Mon, Jul 17, 2023 at 08:54:29AM +0530, Athira Rajeev wrote: >> Add a function dt_find_by_name_before_addr() that returns the child node if >> it matches till first occurrence at "@" of a given name, otherwise NULL. > > Given this summary, I don't userstand the following: > >> + assert(dt_find_by_name(root, "node@1") == addr1); >> + assert(dt_find_by_name(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: Hi Reza, Yes makes sense. dt_find_by_name can be removed in this test since its intention is to find device by name. I will remove these two checks. > >> +struct dt_node *dt_find_by_name_before_addr(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. Ok Reza > > 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") == ???????); > ^^^^^^^ In this case, dt_find_by_name_before_addr is not the right function to use. We have other functions like dt_find_by_name_addr that can be made use of. I will address other changes in next version Thanks Athira > > -- > Reza Arbab ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-09-11 5:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-07-17 3:24 [PATCH V5 1/3] core/device: Add function to return child node using name at substring "@" Athira Rajeev 2023-07-17 3:24 ` [PATCH V5 2/3] skiboot: Update IMC code to use dt_find_by_name_before_addr for checking dt nodes Athira Rajeev 2023-07-17 3:24 ` [PATCH V5 3/3] skiboot: Update IMC PMU node names for power10 Athira Rajeev 2023-08-09 21:58 ` Reza Arbab 2023-09-11 5:01 ` Athira Rajeev 2023-08-09 21:51 ` [PATCH V5 1/3] core/device: Add function to return child node using name at substring "@" Reza Arbab 2023-09-11 5:15 ` Athira Rajeev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).