allow using labels with fdtget/fdtput #89

Villemoes · 2023-03-09T13:36:09Z

It can be convenient and more readable to use &label syntax with fdtget and fdtput, rather than having to know the full path. This small series adds that support by hooking into fdt_path_offset_namelen() in the same place where we already allow the first component to be an alias. Of course this only works for blobs that have been built with -@, but it shouldn't affect existing use cases.

Villemoes · 2023-04-11T08:24:02Z

Any comments?

The CI errors don't seem to be related to these changes, so I don't know if I'm supposed to do anything about them.

dgibson · 2023-04-11T09:46:12Z

libfdt/fdt_ro.c

@@ -466,6 +466,17 @@ const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
 	return prop->data;


Hmm.. I think I need a bit more convincing that this is sufficiently widely useful to be worth adding. It is, after all, just a wrapper over two consecutive calls.

OK, I wasn't really sure. I thought there would be plenty of potential users in the U-Boot code base, but now that I actually try to find them, they do exist but not as abundant as I thought. I'll probably still add it as a static helper to implement the fdt_get_(alias/symbol)_namelen.

dgibson · 2023-04-11T09:50:33Z

libfdt/fdt_ro.c

@@ -247,6 +247,12 @@ int fdt_subnode_offset(const void *fdt, int parentoffset,
 	return fdt_subnode_offset_namelen(fdt, parentoffset, name, strlen(name));
 }

+static const char *fdt_get_symbol_namelen(const void *fdt,


This seems sufficiently useful that it would be worth making public.

OK. Will refactor a bit so it gets added by itself then, and then used in fdt_path_offset_namelen().

dgibson · 2023-04-11T09:51:04Z

libfdt/fdt_ro.c

 	if (*path != '/') {
 		const char *q = memchr(path, '/', end - p);

 		if (!q)
 			q = end;

-		p = fdt_get_alias_namelen(fdt, p, q - p);
+		if (*p == '&')


I'd like to see the new syntax described in the header comment for fdt_path_offset_namelen().

dgibson · 2023-04-11T09:53:39Z

tests/run_tests.sh

@@ -859,7 +859,7 @@ dtbs_equal_tests () {
 fdtget_tests () {
    dts=label01.dts
    dtb=$dts.fdtget.test.dtb
-    run_dtc_test -O dtb -o $dtb "$SRCDIR/$dts"
+    run_dtc_test -@ -O dtb -o $dtb "$SRCDIR/$dts"


Obviously you need to add -@ to test the new functionality. However, this means you're no longer testing the old functionality on a tree that's not compiled with -@. I'd prefer to extend this so that both the -@ and not -@ cases are exercised.

Good point. I'll see if I can figure out an easy way to do these tests both ways, and then either omit the new tests when not built with -@, or turn them into expected errors.

dgibson · 2023-04-11T09:55:10Z

Sorry, I'm finding it very difficult these days to find even minimal time to maintain dtc.

The concept for these patches looks good, I've put in a review for some of the details.

Villemoes · 2023-04-12T21:12:10Z

Branch updated. I think this should address the comments, but I may have been too verbose in the fdt_path_offset documentation update.

ukleinek

In general I like this series. Only a few little comments in the diffs.

The PR I added a few hours ago could benefit from the functions that are introduced here. I hesitate to base my work on top of this one, but once this PR is in shape and goes in I can rework my commits..

ukleinek · 2023-04-26T22:31:59Z

libfdt/fdt_ro.c

+		if (!can_assume(VALID_INPUT) && *p != '/')
+			return -FDT_ERR_BADVALUE;
+


Would it make sense to write:

/aliases { self = "&somelabel"; }

? With the previous commit it would work I think. I think I would have picked -EFDT_ERR_BADPATH as error code.

Would it make sense to write:

/aliases { self = "&somelabel"; }

? With the previous commit it would work I think.

I can't see the use case. For this to resolve correctly, the dtb would have to have been built with -@, and then the somelabel property would still contain the absolute path, so there's no way this could even make the dtb smaller. So no, I would consider such a .dts file malformed, and I don't see any reason we should allow anything but absolute paths on the RHS of alias and symbol properties.

I think I would have picked -EFDT_ERR_BADPATH as error code.

I believe my rationale in the commit message makes sense, and I certainly don't want to overload FDT_ERR_BADPATH more when there's such a fitting new error code to use.

For this to resolve correctly, the dtb would have to have been built with -@, and then the somelabel property would still contain the absolute path, so there's no way this could even make the dtb smaller. So no, I would consider such a .dts file malformed, and I don't see any reason we should allow anything but absolute paths on the RHS of alias and symbol properties.

It can be smaller; with the following dts in test.dts

/dts-v1/; / { aliases { sl1 = "&somelabel"; sl2 = &somelabel; }; somelabel: somenodewithalongname { someproperty = "somevalue"; }; };

and

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c index d30c757180b7..c06c5d24d959 100644 --- a/libfdt/fdt_ro.c +++ b/libfdt/fdt_ro.c @@ -269,8 +269,10 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen) if (!p) return -FDT_ERR_BADPATH; +#if 0 if (!can_assume(VALID_INPUT) && *p != '/') return -FDT_ERR_BADVALUE; +#endif offset = fdt_path_offset(fdt, p);

$ dtc -@ -I dts -O dtb test.dts > test.dtb test.dts:5.3-22: Warning (alias_paths): /aliases:sl1: aliases property is not a valid node (&somelabel) $ fdtget test.dtb sl1 someproperty somevalue

So it works fine and the sl1 alias is shorter than the equivalent sl2 one.

For this to resolve correctly, the dtb would have to have been built with -@, and then the somelabel property would still contain the absolute path, so there's no way this could even make the dtb smaller. So no, I would consider such a .dts file malformed, and I don't see any reason we should allow anything but absolute paths on the RHS of alias and symbol properties.

It can be smaller; with the following dts in test.dts

Yes, of course, if the base assumption is that the dts will be built with -@ regardless, so the absolute path will be listed under the symbols node, of course the string &somelabel will be shorter than the having the value of sl1 be another copy of that absolute path.

However, that is entirely besides the point. I still completely fail to see the actual use case, or why we should allow aliases or symbols to resolve to anything but an absolute path, and stand by my "I would consider such a dts malformed". And current dtc obviously agrees with me, per the warning output above.

I think I would have picked -EFDT_ERR_BADPATH as error code.

I believe my rationale in the commit message makes sense, and I certainly don't want to overload FDT_ERR_BADPATH more when there's such a fitting new error code to use.

Hm, to sum up, with

/dts-v1/; / { aliases { sa1 = &somelabel; sa2 = "/nonexisting"; sa3 = "invalid"; }; somelabel: somenodewithalongname { someproperty = "somevalue"; }; };

we get for fdtget test.dtb sa$i someproperty respectively:

i = 1, success: "somevalue"

i = 2, error -FDT_ERR_NOTFOUND

i = 3, error -FDT_ERR_BADVALUE

i = 4, error -FDT_ERR_BADPATH

Looking at it from a user's perspective: The user asks for a property at the path "sa3". Replying with

FDT_ERR_BADPATH: Function was passed a badly formatted path

seems right to me and also matches the behavior of fdtget before your changes(!). However replying with

FDT_ERR_BADVALUE: Device tree has a property with an unexpected value.

(while being correct) is a diagnosis about the property /aliases/sa3 and not about the path "sa3" that the user provided and so only something like an implementation detail.

I'm not authoritative here, so I'll stop arguing here and let @dgibson think and decide about that.

I don't think we should extend the semantics of /aliases. That's been around since the ancient history of OF, and I think extending it is of very little use here. Noting that you can already do:

/ { aliases { sa = &label; } }

Where dtc will expand that to the full path.

I'm not 100% sure, but I believe it's also valid for aliases to reference other aliases. I couldn't quickly find absolute confirmation in IEEE1275, but I think this was valid and used in practice in the old OF days. For example:

/ { aliases { sa1 = "/foo/bar"; sa2 = "sa1/baz"; }; }

I'm not 100% sure, but I believe it's also valid for aliases to reference other aliases. I couldn't quickly find absolute confirmation in IEEE1275, but I think this was valid and used in practice in the old OF days. For example:

/ { aliases { sa1 = "/foo/bar"; sa2 = "sa1/baz"; }; }

Well, if that's really the case, then of course my sanity check patch should be removed. It's really independent of the rest of the series, but was just something I noticed while I was in the area. It does mean that a client program (which very much can be a bootloader or kernel) can end up in an infinite loop/stack overflow if the dtb is broken or malicious, which is what I wanted to prevent.

However, the fact that dtc today would warn about this suggests that's it's probably not in common use.

$ dtc -I dts -O dtb -o test.dtb test.dts test.dts:6.17-33: Warning (alias_paths): /aliases:sa2: aliases property is not a valid node (sa1/baz)

That was added back in 2017. The commit message says "Add checks for aliases node that all properties follow alias naming convention and the values are a valid path.", but I guess it's then a little ambiguous what a valid path means in this context. Certainly the sanity checker itself doesn't do alias resolving.

I'm not 100% sure, but I believe it's also valid for aliases to reference other aliases. I couldn't quickly find absolute confirmation in IEEE1275, but I think this was valid and used in practice in the old OF days. For example:

/ { aliases { sa1 = "/foo/bar"; sa2 = "sa1/baz"; }; }

Well, if that's really the case, then of course my sanity check patch should be removed.

Ah, but https://www.devicetree.org/specifications/ is quite clear:

Each property of the ``/aliases`` node defines an alias. The property name specifies the alias name. The property value specifies the full path to a node in the devicetree.

and

An alias value is a device path and is encoded as a string. The value represents the full path to a node, but the path does not need to refer to a leaf node.

That text seems to be essentially unchanged since it first appeared in that git repo.

I'm not 100% sure, but I believe it's also valid for aliases to reference other aliases. I couldn't quickly find absolute confirmation in IEEE1275, but I think this was valid and used in practice in the old OF days. For example:

/ { aliases { sa1 = "/foo/bar"; sa2 = "sa1/baz"; }; }

Well, if that's really the case, then of course my sanity check patch should be removed.

Ah, but https://www.devicetree.org/specifications/ is quite clear:

Eh.. maybe..

Each property of the ``/aliases`` node defines an alias. The property name specifies the alias name. The property value specifies the full path to a node in the devicetree.

and

An alias value is a device path and is encoded as a string. The value represents the full path to a node, but the path does not need to refer to a leaf node.

So, both of these look to be paraphrased from IEEE1275. The ambiguity, such as it is, is in the meaning of "full path". 1275 uses the term device-path in the equivalent place, and I think that term may allow the first component to be an alias, rather than /, though I'm having trouble pinning down something which says that one way or another. I do see device-path used in other contexts of 1275 where I'm pretty certain aliases are allowed (e.g. the value of the bootpath property of /chosen).

That text seems to be essentially unchanged since it first appeared in that git repo.

As you'd expect for something more or less inherited from 1275.

The ambiguity, such as it is, is in the meaning of "full path".

Alright, I've pushed a version without that sanity check (and with s/nodename/path/).

ukleinek · 2023-04-26T22:43:19Z

libfdt/fdt_ro.c

+static const void *fdt_path_getprop_namelen(const void *fdt, const char *node,
+					    const char *name, int namelen,
+					    int *lenp)


I think the parameter naming is not optimal. node isn't a struct node but the path name of the node to lookup name in. Maybe use s/node/nodename/ and s/name/propname/?

Ack, grepping clearly shows that we only have const char *nodename and const char *node_name. And propname is also not unheard of, and makes sense here where there's also a nodename.

path would be better, matching fdt_path_offset().

ukleinek · 2023-04-26T22:50:34Z

libfdt/libfdt.h

+ * first component is treated as an alias (or, if it begins with '&',
+ * the rest is treated as a symbol).  That is, the property by that
+ * name is looked up in the /aliases (or /__symbols__) node, and the
+ * value of that property used in place of that first component.
 *


Something that is missing here is that the components in path don't need to specify the unit address ...

Huh? Try looking at the commit (or the "improve documentation for fdt_path_offset()") with -U10. Then you'll see that that is right there in the context:

--- a/libfdt/libfdt.h +++ b/libfdt/libfdt.h @@ -517,24 +517,49 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen); * @fdt: pointer to the device tree blob * @path: full path of the node to locate * * fdt_path_offset() finds a node of a given path in the device tree. * Each path component may omit the unit address portion, but the * results of this are undefined if any such path component is * ambiguous (that is if there are multiple nodes at the relevant * level matching the given component, differentiated only by unit * address). * + * If the path is not absolute (i.e. does not begin with '/'), the + * first component is treated as an alias. That is, the property by + * that name is looked up in the /aliases node, and the value of that + * property used in place of that first component.

Unless I'm misunderstanding what you were trying to say, I don't think there's anything to fix.

Ah indeed. It's documented and I missed that. While working on dtc for my change I stumbled about this "feature" that is inherited to e.g. fdt_overlay_target_offset() which is surprising at least. (If you're subscribed to [email protected], see my mail "Bug or feature? /somenode overwrites /somenode@17 in fdt-overlay" with Message-Id [email protected], I didn't find it in a web archive.)

(If you're subscribed to [email protected], see my mail "Bug or feature? /somenode overwrites /somenode@17 in fdt-overlay" with Message-Id [email protected], I didn't find it in a web archive.)

I'm not subscribed, but it seems that that ML should be archived on lore.kernel.org, and if it isn't already (I can't find it), someone should ask helpdesk to do it (see https://korg.docs.kernel.org/lore.html).

ukleinek · 2023-04-26T22:53:41Z

libfdt/libfdt.h

@@ -547,12 +548,17 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen);
 *
 *   /soc@0/i2c@30a40000/eeprom@52
 *   i2c2/eeprom@52
+ *   &foo/eeprom@52
+ *   &bar


... so (as /soc@0 is the only soc@???? in / and /soc@0/i2c@30a40000 is the only i2c@??? in /soc@0, and eeprom@52 is also unique,

/soc/i2c/eeprom

is another path to that node.

I'm unsure if this is a bug of a feature though.

Well, in that very minimal example, yes, and presumably soc@0 is always the only soc node, but in real life, there'd very likely be several i2c@... nodes, and I don't see the point of confusing this example with the subtleties from that ambiguity, which has nothing to do with the feature I'm implementing.

dgibson · 2023-04-29T06:52:20Z

On Fri, Apr 28, 2023 at 01:15:23AM -0700, Villemoes wrote: @Villemoes commented on this pull request. > + if (!can_assume(VALID_INPUT) && *p != '/') + return -FDT_ERR_BADVALUE; + > > I'm not 100% sure, but I believe it's also valid for aliases to reference other aliases. I couldn't quickly find absolute confirmation in IEEE1275, but I think this was valid and used in practice in the old OF days. For example: > > ``` > > / { > > aliases { > > sa1 = "/foo/bar"; > > sa2 = "sa1/baz"; > > }; > > } > > ``` > > Well, if that's really the case, then of course my sanity check patch should be removed. Ah, but https://www.devicetree.org/specifications/ is quite clear:

Eh.. not necessarily.

``` Each property of the ``/aliases`` node defines an alias. The property name specifies the alias name. The property value specifies the full path to a node in the devicetree. ``` and ``` An alias value is a device path and is encoded as a string. The value represents the full path to a node, but the path does not need to refer to a leaf node. ```

So, both of these look to be paraphrased from IEEE1275. The ambiguity, such as it is, is in the meaning of "full path". 1275 uses the term "device-path" in the equivalent place, and I think that term may allow the first component to be an alias, rather than '/', though I'm having trouble pinning down something which says that one way or another. I do see "device-path" used in other contexts of 1275 where I'm pretty certain aliases are allowed (e.g. the value of the 'bootpath' property of /chosen).

That text seems to be essentially unchanged since it first appeared in that git repo.

As you'd expect for something more or less inherited from 1275.

…

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

dgibson · 2023-05-03T13:52:10Z

I've merged the first 4 patches. I need to think a bit more about the remaining two.

When using the CLI tools fdtget and fdtput, it can be convenient to refer to a node by a label rather than having to know the full path from the root. Signed-off-by: Rasmus Villemoes <[email protected]>

We obviously need to build some dtbs with -@ to do these tests. In order to still test all the old functionality on blobs built without -@, simply run all fdtget/fdtput tests twice, with/without -@, and for the few new test cases that rely on -@, check that they fail when the blob is not built with -@. Signed-off-by: Rasmus Villemoes <[email protected]>

Villemoes · 2024-10-02T08:41:09Z

I have rebased the last two patches on top of current main. Can they be considered for inclusion?

Villemoes force-pushed the label-path branch from 9304943 to 77b6425 Compare March 9, 2023 13:46

dgibson reviewed Apr 11, 2023

View reviewed changes

Villemoes force-pushed the label-path branch from 77b6425 to ce4e9ef Compare April 12, 2023 20:43

ukleinek suggested changes Apr 26, 2023

View reviewed changes

Villemoes force-pushed the label-path branch 2 times, most recently from e2fc8e9 to f0ec5b1 Compare April 27, 2023 07:09

Villemoes force-pushed the label-path branch from f0ec5b1 to 02592b5 Compare April 29, 2023 09:18

dgibson force-pushed the main branch from 83d011c to ae97d97 Compare March 6, 2024 04:48

Rasmus Villemoes added 2 commits October 2, 2024 10:04

allow paths in fdt_path_offset to begin with a &label reference

f015453

When using the CLI tools fdtget and fdtput, it can be convenient to refer to a node by a label rather than having to know the full path from the root. Signed-off-by: Rasmus Villemoes <[email protected]>

Villemoes force-pushed the label-path branch from 02592b5 to 72c72c5 Compare October 2, 2024 08:16

Villemoes closed this Oct 7, 2024

Villemoes deleted the label-path branch October 7, 2024 14:05

Villemoes mentioned this pull request Oct 7, 2024

allow using labels with fdtget/fdtput #150

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow using labels with fdtget/fdtput #89

allow using labels with fdtget/fdtput #89

Villemoes commented Mar 9, 2023

Villemoes commented Apr 11, 2023

dgibson Apr 11, 2023

Villemoes Apr 12, 2023

dgibson Apr 11, 2023

Villemoes Apr 12, 2023

dgibson Apr 11, 2023

dgibson Apr 11, 2023

Villemoes Apr 12, 2023

dgibson commented Apr 11, 2023

Villemoes commented Apr 12, 2023

ukleinek left a comment

ukleinek Apr 26, 2023

Villemoes Apr 27, 2023

ukleinek Apr 27, 2023

Villemoes Apr 27, 2023

ukleinek Apr 27, 2023

dgibson Apr 28, 2023 •

edited

Loading

Villemoes Apr 28, 2023

Villemoes Apr 28, 2023

dgibson Apr 29, 2023

Villemoes Apr 29, 2023

ukleinek Apr 26, 2023

Villemoes Apr 27, 2023

dgibson Apr 28, 2023

ukleinek Apr 26, 2023

Villemoes Apr 27, 2023

ukleinek Apr 27, 2023

Villemoes Apr 27, 2023

ukleinek Apr 26, 2023

Villemoes Apr 27, 2023

dgibson commented Apr 29, 2023 via email

dgibson commented May 3, 2023

Villemoes commented Oct 2, 2024

		@@ -466,6 +466,17 @@ const void fdt_getprop_namelen(const void fdt, int nodeoffset,
		return prop->data;

		if (!can_assume(VALID_INPUT) && *p != '/')
		return -FDT_ERR_BADVALUE;

allow using labels with fdtget/fdtput #89

allow using labels with fdtget/fdtput #89

Conversation

Villemoes commented Mar 9, 2023

Villemoes commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgibson commented Apr 11, 2023

Villemoes commented Apr 12, 2023

ukleinek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgibson Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgibson commented Apr 29, 2023 via email

dgibson commented May 3, 2023

Villemoes commented Oct 2, 2024

dgibson Apr 28, 2023 •

edited

Loading