* [PATCH V2 1/3] core/device: Add function to return child node using name at substring "@"
@ 2023-01-18 5:44 Athira Rajeev
2023-01-18 5:44 ` [PATCH V2 2/3] skiboot: Update IMC code to use dt_find_by_name_substr for checking dt nodes Athira Rajeev
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Athira Rajeev @ 2023-01-18 5:44 UTC (permalink / raw)
To: skiboot, dan, mpe, maddy; +Cc: kjain, disgoel, linuxppc-dev, mahesh
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 <atrajeev@linux.vnet.ibm.com>
---
Changelog:
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 | 18 ++++++++++++++++++
core/test/run-device.c | 11 +++++++++++
include/device.h | 3 +++
3 files changed, 32 insertions(+)
diff --git a/core/device.c b/core/device.c
index 2de37c74..df3a5775 100644
--- a/core/device.c
+++ b/core/device.c
@@ -395,6 +395,24 @@ 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 *pos;
+
+ list_for_each(&root->children, child, list) {
+ pos = strchr(child->name, '@');
+ if (!strncmp(child->name, name, pos - child->name))
+ return child;
+
+ match = dt_find_by_name_substr(child, name);
+ if (match)
+ return match;
+ }
+
+ 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..0e463e58 100644
--- a/core/test/run-device.c
+++ b/core/test/run-device.c
@@ -466,6 +466,17 @@ 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);
+ 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.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH V2 2/3] skiboot: Update IMC code to use dt_find_by_name_substr for checking dt nodes
2023-01-18 5:44 [PATCH V2 1/3] core/device: Add function to return child node using name at substring "@" Athira Rajeev
@ 2023-01-18 5:44 ` Athira Rajeev
2023-01-18 5:44 ` [PATCH V2 3/3] skiboot: Update IMC PMU node names for power10 Athira Rajeev
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Athira Rajeev @ 2023-01-18 5:44 UTC (permalink / raw)
To: skiboot, dan, mpe, maddy; +Cc: kjain, 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_substr. Also, update the char array
to use correct node names with suffix "@"
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
Changelog:
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.
Hence used existing nest_pmus char array to
update node names with "@" suffix
hw/imc.c | 104 +++++++++++++++++++++++++++----------------------------
1 file changed, 52 insertions(+), 52 deletions(-)
diff --git a/hw/imc.c b/hw/imc.c
index 97e0809f..805e6cc1 100644
--- a/hw/imc.c
+++ b/hw/imc.c
@@ -50,57 +50,57 @@ static unsigned int *htm_scom_index;
* nest_pmus[] is an array containing all the possible nest IMC PMU node names.
*/
static char const *nest_pmus[] = {
- "powerbus0",
- "mcs0",
- "mcs1",
- "mcs2",
- "mcs3",
- "mcs4",
- "mcs5",
- "mcs6",
- "mcs7",
- "mba0",
- "mba1",
- "mba2",
- "mba3",
- "mba4",
- "mba5",
- "mba6",
- "mba7",
- "cen0",
- "cen1",
- "cen2",
- "cen3",
- "cen4",
- "cen5",
- "cen6",
- "cen7",
- "xlink0",
- "xlink1",
- "xlink2",
- "mcd0",
- "mcd1",
- "phb0",
- "phb1",
- "phb2",
- "phb3",
- "phb4",
- "phb5",
- "nx",
- "capp0",
- "capp1",
- "vas",
- "int",
- "alink0",
- "alink1",
- "alink2",
- "alink3",
- "nvlink0",
- "nvlink1",
- "nvlink2",
- "nvlink3",
- "nvlink4",
- "nvlink5",
+ "powerbus0@",
+ "mcs0@",
+ "mcs1@",
+ "mcs2@",
+ "mcs3@",
+ "mcs4@",
+ "mcs5@",
+ "mcs6@",
+ "mcs7@",
+ "mba0@",
+ "mba1@",
+ "mba2@",
+ "mba3@",
+ "mba4@",
+ "mba5@",
+ "mba6@",
+ "mba7@",
+ "centaur0@",
+ "centaur1@",
+ "centaur2@",
+ "centaur3@",
+ "centaur4@",
+ "centaur5@",
+ "centaur6@",
+ "centaur7@",
+ "xlink0@",
+ "xlink1@",
+ "xlink2@",
+ "mcd0@",
+ "mcd1@",
+ "phb0@",
+ "phb1@",
+ "phb2@",
+ "phb3@",
+ "phb4@",
+ "phb5@",
+ "nx@",
+ "capp0@",
+ "capp1@",
+ "vas@",
+ "int@",
+ "alink0@",
+ "alink1@",
+ "alink2@",
+ "alink3@",
+ "nvlink0@",
+ "nvlink1@",
+ "nvlink2@",
+ "nvlink3@",
+ "nvlink4@",
+ "nvlink5@",
/* reserved bits : 51 - 63 */
};
@@ -412,7 +412,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.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH V2 3/3] skiboot: Update IMC PMU node names for power10
2023-01-18 5:44 [PATCH V2 1/3] core/device: Add function to return child node using name at substring "@" Athira Rajeev
2023-01-18 5:44 ` [PATCH V2 2/3] skiboot: Update IMC code to use dt_find_by_name_substr for checking dt nodes Athira Rajeev
@ 2023-01-18 5:44 ` Athira Rajeev
2023-01-30 5:47 ` [Skiboot] [PATCH V2 1/3] core/device: Add function to return child node using name at substring "@" Athira Rajeev
2023-01-30 9:44 ` Mahesh J Salgaonkar
3 siblings, 0 replies; 6+ messages in thread
From: Athira Rajeev @ 2023-01-18 5:44 UTC (permalink / raw)
To: skiboot, dan, mpe, maddy; +Cc: kjain, 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:
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 805e6cc1..20bc51b4 100644
--- a/hw/imc.c
+++ b/hw/imc.c
@@ -49,7 +49,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@",
@@ -104,6 +104,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.
@@ -371,7 +432,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;
@@ -409,14 +470,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.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Skiboot] [PATCH V2 1/3] core/device: Add function to return child node using name at substring "@"
2023-01-18 5:44 [PATCH V2 1/3] core/device: Add function to return child node using name at substring "@" Athira Rajeev
2023-01-18 5:44 ` [PATCH V2 2/3] skiboot: Update IMC code to use dt_find_by_name_substr for checking dt nodes Athira Rajeev
2023-01-18 5:44 ` [PATCH V2 3/3] skiboot: Update IMC PMU node names for power10 Athira Rajeev
@ 2023-01-30 5:47 ` Athira Rajeev
2023-01-30 9:44 ` Mahesh J Salgaonkar
3 siblings, 0 replies; 6+ messages in thread
From: Athira Rajeev @ 2023-01-30 5:47 UTC (permalink / raw)
To: skiboot, Dan Horák, Michael Ellerman, Madhavan Srinivasan
Cc: Kajol Jain, disgoel, linuxppc-dev
> On 18-Jan-2023, at 11:14 AM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> 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 <atrajeev@linux.vnet.ibm.com>
> ---
> Changelog:
> 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.
Hi Dan,
Looking for review comments for this V2 patch set. Please share your feedback
Thanks
Athira
>
> core/device.c | 18 ++++++++++++++++++
> core/test/run-device.c | 11 +++++++++++
> include/device.h | 3 +++
> 3 files changed, 32 insertions(+)
>
> diff --git a/core/device.c b/core/device.c
> index 2de37c74..df3a5775 100644
> --- a/core/device.c
> +++ b/core/device.c
> @@ -395,6 +395,24 @@ 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 *pos;
> +
> + list_for_each(&root->children, child, list) {
> + pos = strchr(child->name, '@');
> + if (!strncmp(child->name, name, pos - child->name))
> + return child;
> +
> + match = dt_find_by_name_substr(child, name);
> + if (match)
> + return match;
> + }
> +
> + 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..0e463e58 100644
> --- a/core/test/run-device.c
> +++ b/core/test/run-device.c
> @@ -466,6 +466,17 @@ 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);
> + 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.27.0
>
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2 1/3] core/device: Add function to return child node using name at substring "@"
2023-01-18 5:44 [PATCH V2 1/3] core/device: Add function to return child node using name at substring "@" Athira Rajeev
` (2 preceding siblings ...)
2023-01-30 5:47 ` [Skiboot] [PATCH V2 1/3] core/device: Add function to return child node using name at substring "@" Athira Rajeev
@ 2023-01-30 9:44 ` Mahesh J Salgaonkar
2023-02-02 17:06 ` Athira Rajeev
3 siblings, 1 reply; 6+ messages in thread
From: Mahesh J Salgaonkar @ 2023-01-30 9:44 UTC (permalink / raw)
To: Athira Rajeev; +Cc: maddy, dan, kjain, skiboot, disgoel, linuxppc-dev
On 2023-01-18 11:14:50 Wed, 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.
> 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 <atrajeev@linux.vnet.ibm.com>
> ---
> Changelog:
> 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 | 18 ++++++++++++++++++
> core/test/run-device.c | 11 +++++++++++
> include/device.h | 3 +++
> 3 files changed, 32 insertions(+)
>
> diff --git a/core/device.c b/core/device.c
> index 2de37c74..df3a5775 100644
> --- a/core/device.c
> +++ b/core/device.c
> @@ -395,6 +395,24 @@ 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 *pos;
> +
> + list_for_each(&root->children, child, list) {
> + pos = strchr(child->name, '@');
> + if (!strncmp(child->name, name, pos - child->name))
Shouldn't we care about string length of substring to be checked before
comparision ? The code assumes that it is always within the limit of
position of '@' in node name string. Hence, it returns a wrong node
whose name partially matches with substring passed.
e.g.
With following two nodes in deviec tree (as per your test):
/node@1
/node0_1@2
the substring 'node0', 'node0@' and 'node0_@' all matches with 'node@1' device
tree node.
Is this expected ?
Also, what do you expect dt_find_by_name_substr() to return for string
like 'node0' and 'node0_' ? NULL or node '/node0_1@2' ?
> + return child;
> +
> + match = dt_find_by_name_substr(child, name);
> + if (match)
> + return match;
> + }
> +
> + 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..0e463e58 100644
> --- a/core/test/run-device.c
> +++ b/core/test/run-device.c
> @@ -466,6 +466,17 @@ 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);
Below additional tests are failing:
assert(dt_find_by_name_substr(root, "node0@") == NULL);
assert(dt_find_by_name_substr(root, "node0_@") == NULL);
Maybe we should add few more test checks for "node0" and "node0_" as well.
Thanks,
-Mahesh.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2 1/3] core/device: Add function to return child node using name at substring "@"
2023-01-30 9:44 ` Mahesh J Salgaonkar
@ 2023-02-02 17:06 ` Athira Rajeev
0 siblings, 0 replies; 6+ messages in thread
From: Athira Rajeev @ 2023-02-02 17:06 UTC (permalink / raw)
To: mahesh
Cc: Madhavan Srinivasan, Dan Horák, Kajol Jain, skiboot,
disgoel, linuxppc-dev
> On 30-Jan-2023, at 3:14 PM, Mahesh J Salgaonkar <mahesh@linux.ibm.com> wrote:
>
> On 2023-01-18 11:14:50 Wed, 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.
>> 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 <atrajeev@linux.vnet.ibm.com>
>> ---
>> Changelog:
>> 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 | 18 ++++++++++++++++++
>> core/test/run-device.c | 11 +++++++++++
>> include/device.h | 3 +++
>> 3 files changed, 32 insertions(+)
>>
>> diff --git a/core/device.c b/core/device.c
>> index 2de37c74..df3a5775 100644
>> --- a/core/device.c
>> +++ b/core/device.c
>> @@ -395,6 +395,24 @@ 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 *pos;
>> +
>> + list_for_each(&root->children, child, list) {
>> + pos = strchr(child->name, '@');
>> + if (!strncmp(child->name, name, pos - child->name))
>
> Shouldn't we care about string length of substring to be checked before
> comparision ? The code assumes that it is always within the limit of
> position of '@' in node name string. Hence, it returns a wrong node
> whose name partially matches with substring passed.
>
> e.g.
> With following two nodes in deviec tree (as per your test):
> /node@1
> /node0_1@2
>
> the substring 'node0', 'node0@' and 'node0_@' all matches with 'node@1' device
> tree node.
> Is this expected ?
Hi Mahesh,
Thanks for reviewing and pointing out.
As you pointed, currently it also returns if name partially matches with substring which is not expected.
I willer-work on changes and post a V3
>
> Also, what do you expect dt_find_by_name_substr() to return for string
> like 'node0' and 'node0_' ? NULL or node '/node0_1@2' ?
>
>> + return child;
>> +
>> + match = dt_find_by_name_substr(child, name);
>> + if (match)
>> + return match;
>> + }
>> +
>> + 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..0e463e58 100644
>> --- a/core/test/run-device.c
>> +++ b/core/test/run-device.c
>> @@ -466,6 +466,17 @@ 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);
>
> Below additional tests are failing:
>
> assert(dt_find_by_name_substr(root, "node0@") == NULL);
> assert(dt_find_by_name_substr(root, "node0_@") == NULL);
>
> Maybe we should add few more test checks for "node0" and "node0_" as well.
Sure, I will fix this in V3
Thanks
Athira
>
> Thanks,
> -Mahesh.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-02-02 17:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-18 5:44 [PATCH V2 1/3] core/device: Add function to return child node using name at substring "@" Athira Rajeev
2023-01-18 5:44 ` [PATCH V2 2/3] skiboot: Update IMC code to use dt_find_by_name_substr for checking dt nodes Athira Rajeev
2023-01-18 5:44 ` [PATCH V2 3/3] skiboot: Update IMC PMU node names for power10 Athira Rajeev
2023-01-30 5:47 ` [Skiboot] [PATCH V2 1/3] core/device: Add function to return child node using name at substring "@" Athira Rajeev
2023-01-30 9:44 ` Mahesh J Salgaonkar
2023-02-02 17:06 ` 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).