| Message ID | 20180207025607.22425-1-alistair@popple.id.au |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | core/device.c: Fix dt_find_compatible_node | expand |
On 07/02/18 13:56, Alistair Popple wrote: > dt_find_compatible_node() and dt_find_compatible_node_on_chip() are used to > find device nodes under a parent/root node with a given compatible > property. > > dt_next(root, prev) is used to walk the child nodes of the given parent and > takes two arguments - root contains the parent node to walk whilst prev > contains the previous child to search from so that it can be used as an > iterator over all children nodes. > > The first iteration of dt_find_compatible_node(root, prev) calls > dt_next(root, root) which is not a well defined operation as prev is > assumed to be child of the root node. The result is that when a node > contains no children it will start returning the parent nodes siblings > until it hits the top of the tree at which point a NULL derefence is > attempted when looking for the root nodes parent. > > Dereferencing NULL can result in undesirable data exceptions during system > boot and untimely non-hilarious system crashes. dt_next() should not be > called with prev == root. Instead we add a check to dt_next() such that > passing prev = NULL will cause it to start iterating from the first child > node (if any). > > Also add a unit test for this case to run-device.c. > > Signed-off-by: Alistair Popple <alistair@popple.id.au> This is both less buggy and clearer. Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > --- > core/device.c | 16 ++++++++++------ > core/test/run-device.c | 1 + > 2 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/core/device.c b/core/device.c > index fc1db568..194eaef3 100644 > --- a/core/device.c > +++ b/core/device.c > @@ -608,6 +608,12 @@ struct dt_node *dt_first(const struct dt_node *root) > struct dt_node *dt_next(const struct dt_node *root, > const struct dt_node *prev) > { > + if (!prev) > + prev = dt_first(root); > + > + if (!prev) > + return NULL; > + > /* Children? */ > if (!list_empty(&prev->children)) > return dt_first(prev); > @@ -719,10 +725,9 @@ struct dt_node *dt_find_compatible_node(struct dt_node *root, > struct dt_node *prev, > const char *compat) > { > - struct dt_node *node; > + struct dt_node *node = prev; > > - node = prev ? dt_next(root, prev) : root; > - for (; node; node = dt_next(root, node)) > + while ((node = dt_next(root, node))) > if (dt_node_is_compatible(node, compat)) > return node; > return NULL; > @@ -964,10 +969,9 @@ struct dt_node *dt_find_compatible_node_on_chip(struct dt_node *root, > const char *compat, > uint32_t chip_id) > { > - struct dt_node *node; > + struct dt_node *node = prev; > > - node = prev ? dt_next(root, prev) : root; > - for (; node; node = dt_next(root, node)) { > + while ((node = dt_next(root, node))) { > u32 cid = __dt_get_chip_id(node); > if (cid == chip_id && > dt_node_is_compatible(node, compat)) > diff --git a/core/test/run-device.c b/core/test/run-device.c > index 326be3ff..6d3842e6 100644 > --- a/core/test/run-device.c > +++ b/core/test/run-device.c > @@ -327,6 +327,7 @@ int main(void) > gc2 = dt_new(c1, "node-without-compatible"); > assert(__dt_find_property(gc2, "compatible") == NULL); > assert(!dt_node_is_compatible(gc2, "any-property")); > + assert(dt_find_compatible_node(gc2, NULL, "node-without-compatible") == NULL); > > assert(dt_find_compatible_node(root, NULL, "generic-fake-bus") == c2); > assert(dt_find_compatible_node(root, c2, "generic-fake-bus") == NULL); >
> This is both less buggy and clearer. I'm not convinced of this. It seems like it might somehow break NVLink (at least according to git bisect) so we should probably hold off merging this until I figure out what went wrong. Thanks. > Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > > > --- > > core/device.c | 16 ++++++++++------ > > core/test/run-device.c | 1 + > > 2 files changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/core/device.c b/core/device.c > > index fc1db568..194eaef3 100644 > > --- a/core/device.c > > +++ b/core/device.c > > @@ -608,6 +608,12 @@ struct dt_node *dt_first(const struct dt_node *root) > > struct dt_node *dt_next(const struct dt_node *root, > > const struct dt_node *prev) > > { > > + if (!prev) > > + prev = dt_first(root); > > + > > + if (!prev) > > + return NULL; > > + > > /* Children? */ > > if (!list_empty(&prev->children)) > > return dt_first(prev); > > @@ -719,10 +725,9 @@ struct dt_node *dt_find_compatible_node(struct dt_node *root, > > struct dt_node *prev, > > const char *compat) > > { > > - struct dt_node *node; > > + struct dt_node *node = prev; > > > > - node = prev ? dt_next(root, prev) : root; > > - for (; node; node = dt_next(root, node)) > > + while ((node = dt_next(root, node))) > > if (dt_node_is_compatible(node, compat)) > > return node; > > return NULL; > > @@ -964,10 +969,9 @@ struct dt_node *dt_find_compatible_node_on_chip(struct dt_node *root, > > const char *compat, > > uint32_t chip_id) > > { > > - struct dt_node *node; > > + struct dt_node *node = prev; > > > > - node = prev ? dt_next(root, prev) : root; > > - for (; node; node = dt_next(root, node)) { > > + while ((node = dt_next(root, node))) { > > u32 cid = __dt_get_chip_id(node); > > if (cid == chip_id && > > dt_node_is_compatible(node, compat)) > > diff --git a/core/test/run-device.c b/core/test/run-device.c > > index 326be3ff..6d3842e6 100644 > > --- a/core/test/run-device.c > > +++ b/core/test/run-device.c > > @@ -327,6 +327,7 @@ int main(void) > > gc2 = dt_new(c1, "node-without-compatible"); > > assert(__dt_find_property(gc2, "compatible") == NULL); > > assert(!dt_node_is_compatible(gc2, "any-property")); > > + assert(dt_find_compatible_node(gc2, NULL, "node-without-compatible") == NULL); > > > > assert(dt_find_compatible_node(root, NULL, "generic-fake-bus") == c2); > > assert(dt_find_compatible_node(root, c2, "generic-fake-bus") == NULL); > > > >
Alistair Popple <alistair@popple.id.au> writes: >> This is both less buggy and clearer. > > I'm not convinced of this. It seems like it might somehow break NVLink (at least > according to git bisect) so we should probably hold off merging this until I > figure out what went wrong. Thanks. Yeah, fixed booting on ZZ, but kind of enabled turbo-xstop mode if you actually had nvlinks (and loaded the driver) :)
diff --git a/core/device.c b/core/device.c index fc1db568..194eaef3 100644 --- a/core/device.c +++ b/core/device.c @@ -608,6 +608,12 @@ struct dt_node *dt_first(const struct dt_node *root) struct dt_node *dt_next(const struct dt_node *root, const struct dt_node *prev) { + if (!prev) + prev = dt_first(root); + + if (!prev) + return NULL; + /* Children? */ if (!list_empty(&prev->children)) return dt_first(prev); @@ -719,10 +725,9 @@ struct dt_node *dt_find_compatible_node(struct dt_node *root, struct dt_node *prev, const char *compat) { - struct dt_node *node; + struct dt_node *node = prev; - node = prev ? dt_next(root, prev) : root; - for (; node; node = dt_next(root, node)) + while ((node = dt_next(root, node))) if (dt_node_is_compatible(node, compat)) return node; return NULL; @@ -964,10 +969,9 @@ struct dt_node *dt_find_compatible_node_on_chip(struct dt_node *root, const char *compat, uint32_t chip_id) { - struct dt_node *node; + struct dt_node *node = prev; - node = prev ? dt_next(root, prev) : root; - for (; node; node = dt_next(root, node)) { + while ((node = dt_next(root, node))) { u32 cid = __dt_get_chip_id(node); if (cid == chip_id && dt_node_is_compatible(node, compat)) diff --git a/core/test/run-device.c b/core/test/run-device.c index 326be3ff..6d3842e6 100644 --- a/core/test/run-device.c +++ b/core/test/run-device.c @@ -327,6 +327,7 @@ int main(void) gc2 = dt_new(c1, "node-without-compatible"); assert(__dt_find_property(gc2, "compatible") == NULL); assert(!dt_node_is_compatible(gc2, "any-property")); + assert(dt_find_compatible_node(gc2, NULL, "node-without-compatible") == NULL); assert(dt_find_compatible_node(root, NULL, "generic-fake-bus") == c2); assert(dt_find_compatible_node(root, c2, "generic-fake-bus") == NULL);
dt_find_compatible_node() and dt_find_compatible_node_on_chip() are used to find device nodes under a parent/root node with a given compatible property. dt_next(root, prev) is used to walk the child nodes of the given parent and takes two arguments - root contains the parent node to walk whilst prev contains the previous child to search from so that it can be used as an iterator over all children nodes. The first iteration of dt_find_compatible_node(root, prev) calls dt_next(root, root) which is not a well defined operation as prev is assumed to be child of the root node. The result is that when a node contains no children it will start returning the parent nodes siblings until it hits the top of the tree at which point a NULL derefence is attempted when looking for the root nodes parent. Dereferencing NULL can result in undesirable data exceptions during system boot and untimely non-hilarious system crashes. dt_next() should not be called with prev == root. Instead we add a check to dt_next() such that passing prev = NULL will cause it to start iterating from the first child node (if any). Also add a unit test for this case to run-device.c. Signed-off-by: Alistair Popple <alistair@popple.id.au> --- core/device.c | 16 ++++++++++------ core/test/run-device.c | 1 + 2 files changed, 11 insertions(+), 6 deletions(-)