| Message ID | 550A4382.4030209@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 18, 2015 at 10:33 PM, Frank Rowand <frowand.list@gmail.com> wrote: > From: Frank Rowand <frank.rowand@sonymobile.com> > > Add /chosen/dtb-node binding. Why? Please write better commit messages. > > Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com> > --- > Documentation/devicetree/bindings/chosen.txt | 37 +++++++++++ > > Index: b/Documentation/devicetree/bindings/chosen.txt > =================================================================== > --- a/Documentation/devicetree/bindings/chosen.txt > +++ b/Documentation/devicetree/bindings/chosen.txt > @@ -46,6 +46,43 @@ on PowerPC "stdout" if "stdout-path" is > should only use the "stdout-path" property. > > > +dtb-info node > +---------------- > + > +Information that describes where the device tree blob (DTB) came from and the > +environment it was created in. > + > +This node is normally created by including arch/arm/boot/dts/skeleton.dtsi, > +which includes include/dt-bindings/version.dtsi. > + > +Properties: > + > +version > + The version of the DTB. This is analagous to the linux kernel version. > + > + This is a format free field intended for human consumption. User space > + programs should not have any expections about this property. > + > + The DTB number in this property is incremented each time a make that > + creates one or more DTBs is invoked. If the make creates multiple > + DTBs then this number is only incremented once. > + > + The DTB number is stored in file .version_dtb. > + > +version-linux > + The version of the linux kernel most recently built in the source > + control system that contains the source used to build the DTB. > + > + The linux kernel version number is not incremented for a make that > + creates a DTB. > + > +dtb-path > + The build directory relative path of the DTB. > + > +dts-path > + The absolute path of the .dts file compiled to create the DTB. So these become an ABI and we can never change the directory structure? The problem with informational fields is someone, somewhere will rely on them and then we are stuck with them. Look at /proc/cpuinfo. Rob
On Thu, Mar 19, 2015 at 03:33:22AM +0000, Frank Rowand wrote: > From: Frank Rowand <frank.rowand@sonymobile.com> > > Add /chosen/dtb-node binding. Why? It doesn't matter what the cover says, the commit message should have a rationale. Who needs this information, and when do they need it? Why is the existing information insufficient? > Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com> > --- > Documentation/devicetree/bindings/chosen.txt | 37 +++++++++++ > > Index: b/Documentation/devicetree/bindings/chosen.txt > =================================================================== > --- a/Documentation/devicetree/bindings/chosen.txt > +++ b/Documentation/devicetree/bindings/chosen.txt > @@ -46,6 +46,43 @@ on PowerPC "stdout" if "stdout-path" is > should only use the "stdout-path" property. > > > +dtb-info node > +---------------- > + > +Information that describes where the device tree blob (DTB) came from and the > +environment it was created in. > + > +This node is normally created by including arch/arm/boot/dts/skeleton.dtsi, > +which includes include/dt-bindings/version.dtsi. > + > +Properties: > + > +version > + The version of the DTB. This is analagous to the linux kernel version. > + > + This is a format free field intended for human consumption. User space > + programs should not have any expections about this property. I doubt that this would stay the case for long were this property added. > + The DTB number in this property is incremented each time a make that > + creates one or more DTBs is invoked. If the make creates multiple > + DTBs then this number is only incremented once. > + > + The DTB number is stored in file .version_dtb. This is irrelevant to the binding, and as you mention above, you can make no expectations about this property. I'm not sure I see the point in adding a property which is not well-defined and not guarnateed to be in any way stable. > + > +version-linux > + The version of the linux kernel most recently built in the source > + control system that contains the source used to build the DTB. > + > + The linux kernel version number is not incremented for a make that > + creates a DTB. ...so if I build a DTB outside of a linux source tree I don't get to describe that? > +dtb-path > + The build directory relative path of the DTB. > + > +dts-path > + The absolute path of the .dts file compiled to create the DTB. Why do you need the DTB path? Why do these differ w.r.t. absolute/relative? Why would you _ever_ need an absolute path!? Mark.
On 3/19/2015 6:23 AM, Rob Herring wrote: > On Wed, Mar 18, 2015 at 10:33 PM, Frank Rowand <frowand.list@gmail.com> wrote: >> From: Frank Rowand <frank.rowand@sonymobile.com> >> >> Add /chosen/dtb-node binding. > > Why? Please write better commit messages. Will update. > >> >> Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com> >> --- >> Documentation/devicetree/bindings/chosen.txt | 37 +++++++++++ >> >> Index: b/Documentation/devicetree/bindings/chosen.txt >> =================================================================== >> --- a/Documentation/devicetree/bindings/chosen.txt >> +++ b/Documentation/devicetree/bindings/chosen.txt >> @@ -46,6 +46,43 @@ on PowerPC "stdout" if "stdout-path" is >> should only use the "stdout-path" property. >> >> >> +dtb-info node >> +---------------- >> + >> +Information that describes where the device tree blob (DTB) came from and the >> +environment it was created in. >> + >> +This node is normally created by including arch/arm/boot/dts/skeleton.dtsi, >> +which includes include/dt-bindings/version.dtsi. >> + >> +Properties: >> + >> +version >> + The version of the DTB. This is analagous to the linux kernel version. >> + >> + This is a format free field intended for human consumption. User space >> + programs should not have any expections about this property. >> + >> + The DTB number in this property is incremented each time a make that >> + creates one or more DTBs is invoked. If the make creates multiple >> + DTBs then this number is only incremented once. >> + >> + The DTB number is stored in file .version_dtb. >> + >> +version-linux >> + The version of the linux kernel most recently built in the source >> + control system that contains the source used to build the DTB. >> + >> + The linux kernel version number is not incremented for a make that >> + creates a DTB. >> + >> +dtb-path >> + The build directory relative path of the DTB. >> + >> +dts-path >> + The absolute path of the .dts file compiled to create the DTB. > > So these become an ABI and we can never change the directory structure? Nope. This is describing where the dtb and the dts were on the build host when the dtb was built. The computer where the dtb is currently located when this information is useful (on the "target") is probably not even the same computer the dtb was built on. Mark Rutland asks a lot more questions about this so I'll answer some more in reply to his comments. > > The problem with informational fields is someone, somewhere will rely > on them and then we are stuck with them. Look at /proc/cpuinfo. > > Rob >
On 3/19/2015 6:49 AM, Mark Rutland wrote: > On Thu, Mar 19, 2015 at 03:33:22AM +0000, Frank Rowand wrote: >> From: Frank Rowand <frank.rowand@sonymobile.com> >> >> Add /chosen/dtb-node binding. > > Why? It doesn't matter what the cover says, the commit message should > have a rationale. > > Who needs this information, and when do they need it? > > Why is the existing information insufficient? Will update and add better information. > >> Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com> >> --- >> Documentation/devicetree/bindings/chosen.txt | 37 +++++++++++ >> >> Index: b/Documentation/devicetree/bindings/chosen.txt >> =================================================================== >> --- a/Documentation/devicetree/bindings/chosen.txt >> +++ b/Documentation/devicetree/bindings/chosen.txt >> @@ -46,6 +46,43 @@ on PowerPC "stdout" if "stdout-path" is >> should only use the "stdout-path" property. >> >> >> +dtb-info node >> +---------------- >> + >> +Information that describes where the device tree blob (DTB) came from and the >> +environment it was created in. >> + >> +This node is normally created by including arch/arm/boot/dts/skeleton.dtsi, >> +which includes include/dt-bindings/version.dtsi. >> + >> +Properties: >> + >> +version >> + The version of the DTB. This is analagous to the linux kernel version. >> + >> + This is a format free field intended for human consumption. User space >> + programs should not have any expections about this property. > > I doubt that this would stay the case for long were this property added. > >> + The DTB number in this property is incremented each time a make that >> + creates one or more DTBs is invoked. If the make creates multiple >> + DTBs then this number is only incremented once. >> + >> + The DTB number is stored in file .version_dtb. > > This is irrelevant to the binding, and as you mention above, you can > make no expectations about this property. OK, I'll remove the reference to .version_dtb. > > I'm not sure I see the point in adding a property which is not > well-defined and not guarnateed to be in any way stable. This binding is kind of an odd ball to me. It is clearly _not_ describing hardware, which is really the central point of the dtb. But the chosen node is allowed to cover policy things like the boot command line. So I would encourage a slightly unusual mind set when reviewing this binding (not that I really know what that mind set is!). The answer to the specific question of why add a "not well-defined" binding is the same answer as to why have a kernel version string. There was a proposal to remove the kernel version string and that was quickly dropped. I should have written more about why this is useful. This binding provides the provenance of the DTB. What source (and version of the source) and version of the compiler (dtc) was used to make the DTB. If I want to debug DTB related issues, read the source that was used to create the DTB, or slightly modify the DTB - where is the source and what is the version of it in the source control system. When building and installing a DTB, did the version that I wanted to install on the target actually get on the target. > >> + >> +version-linux >> + The version of the linux kernel most recently built in the source >> + control system that contains the source used to build the DTB. >> + >> + The linux kernel version number is not incremented for a make that >> + creates a DTB. > > ...so if I build a DTB outside of a linux source tree I don't get to > describe that? I will make the name more generic. > >> +dtb-path >> + The build directory relative path of the DTB. >> + >> +dts-path >> + The absolute path of the .dts file compiled to create the DTB. > > Why do you need the DTB path? Paranoia. Even if not probable, one could build different DTBs from the same .dts. > > Why do these differ w.r.t. absolute/relative? Those are the forms of the path that are present in the build system. If there is a good reason to change one of them to the other form, I could add the extra complexity to massage the path. > > Why would you _ever_ need an absolute path!? The absolute path tells you which source repository contained the source. > > Mark. >
On Thu, Mar 19, 2015 at 6:02 PM, Frank Rowand <frowand.list@gmail.com> wrote: >> Why would you _ever_ need an absolute path!? > > The absolute path tells you which source repository contained the source. Really? Usually it's just gonna be <something>/linux-2.6... IMHO it leaks information (the <something>) you don't want to be in final build, like login names and file server names. We already have way to many uses of __FILE__ in the kernel source tree. Unfortunately there's no replacement macro with just the relative path, against the topdir of the source tree. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 3/18/2015 8:33 PM, Frank Rowand wrote: > From: Frank Rowand <frank.rowand@sonymobile.com> > > Add /chosen/dtb-node binding. > > Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com> > --- > Documentation/devicetree/bindings/chosen.txt | 37 +++++++++++ > > Index: b/Documentation/devicetree/bindings/chosen.txt > =================================================================== > --- a/Documentation/devicetree/bindings/chosen.txt > +++ b/Documentation/devicetree/bindings/chosen.txt > @@ -46,6 +46,43 @@ on PowerPC "stdout" if "stdout-path" is > should only use the "stdout-path" property. > > > +dtb-info node > +---------------- > + > +Information that describes where the device tree blob (DTB) came from and the > +environment it was created in. I left out optional vs required. The next version will state that the dtb-info node and each separate property within it are optional. > + > +This node is normally created by including arch/arm/boot/dts/skeleton.dtsi, > +which includes include/dt-bindings/version.dtsi. > + > +Properties: > + > +version > + The version of the DTB. This is analagous to the linux kernel version. > + > + This is a format free field intended for human consumption. User space > + programs should not have any expections about this property. > + > + The DTB number in this property is incremented each time a make that > + creates one or more DTBs is invoked. If the make creates multiple > + DTBs then this number is only incremented once. > + > + The DTB number is stored in file .version_dtb. > + > +version-linux > + The version of the linux kernel most recently built in the source > + control system that contains the source used to build the DTB. > + > + The linux kernel version number is not incremented for a make that > + creates a DTB. > + > +dtb-path > + The build directory relative path of the DTB. > + > +dts-path > + The absolute path of the .dts file compiled to create the DTB. > + > + > Properties documented in other bindings > --------------------------------------- > #address-cells video/simple-framebuffer-sunxi.txt >
On Thu, Mar 19, 2015 at 08:23:29AM -0500, Rob Herring wrote: > On Wed, Mar 18, 2015 at 10:33 PM, Frank Rowand <frowand.list@gmail.com> wrote: > > +version > > + The version of the DTB. This is analagous to the linux kernel version. > > + > > + This is a format free field intended for human consumption. User space > > + programs should not have any expections about this property. > > + > > + The DTB number in this property is incremented each time a make that > > + creates one or more DTBs is invoked. If the make creates multiple > > + DTBs then this number is only incremented once. > > + > > + The DTB number is stored in file .version_dtb. > > + > > +version-linux > > + The version of the linux kernel most recently built in the source > > + control system that contains the source used to build the DTB. > > + > > + The linux kernel version number is not incremented for a make that > > + creates a DTB. > > + > > +dtb-path > > + The build directory relative path of the DTB. > > + > > +dts-path > > + The absolute path of the .dts file compiled to create the DTB. > > So these become an ABI and we can never change the directory structure? > > The problem with informational fields is someone, somewhere will rely > on them and then we are stuck with them. Look at /proc/cpuinfo. There's a bigger problem: including the Linux kernel specific version means that we will _never_ be able to move them out of the kernel. Moreover, what it means is that people can then write code to test what the dtb version is, and we'll end up with the dtb version being tested to determine various features. Since the design goal of DTB is to describe the hardware, including the Linux kernel version is totally against that: the Linux kernel version should be totally irrelevant. What's more relevant would be a _hardware_ version field, but even that's questionable... And yes, you are right - anything like this which we add immediately becames a user ABI which becomes *very* difficult to change later, so in order to accept anything like this, we have to be absolutely sure that this is something we really really really really want to do. Let me put it another way: once this is accepted, it will probably be impossible to ever change it. ... just like Bogomips in /proc/cpuinfo.
> And yes, you are right - anything like this which we add immediately > becames a user ABI which becomes *very* difficult to change later, so > in order to accept anything like this, we have to be absolutely sure > that this is something we really really really really want to do. Let > me put it another way: once this is accepted, it will probably be > impossible to ever change it. Agreed. Having debugged issues with bad DTBs in the past without having this information, I don't see it as being majorly helpful -- the DTB is relatively simple and can be decompiled. You don't gain much from being able to read which file it was built from on a random server you don't have access to, and if you do have access to the source it's relatively easy to discover the file it was built from. So unless there's a really compelling case for this, I don't see the point in having it. Mark.
On 3/19/2015 11:41 AM, Russell King - ARM Linux wrote: > On Thu, Mar 19, 2015 at 08:23:29AM -0500, Rob Herring wrote: >> On Wed, Mar 18, 2015 at 10:33 PM, Frank Rowand <frowand.list@gmail.com> wrote: >>> +version >>> + The version of the DTB. This is analagous to the linux kernel version. >>> + >>> + This is a format free field intended for human consumption. User space >>> + programs should not have any expections about this property. >>> + >>> + The DTB number in this property is incremented each time a make that >>> + creates one or more DTBs is invoked. If the make creates multiple >>> + DTBs then this number is only incremented once. >>> + >>> + The DTB number is stored in file .version_dtb. >>> + >>> +version-linux >>> + The version of the linux kernel most recently built in the source >>> + control system that contains the source used to build the DTB. >>> + >>> + The linux kernel version number is not incremented for a make that >>> + creates a DTB. >>> + >>> +dtb-path >>> + The build directory relative path of the DTB. >>> + >>> +dts-path >>> + The absolute path of the .dts file compiled to create the DTB. >> >> So these become an ABI and we can never change the directory structure? >> >> The problem with informational fields is someone, somewhere will rely >> on them and then we are stuck with them. Look at /proc/cpuinfo. > > There's a bigger problem: including the Linux kernel specific version > means that we will _never_ be able to move them out of the kernel. As I failed to properly document, but have said will be in the next version, the dtb-info node and all of the properties are optional. As I replied to Mark, I will change version-linux to a more generic name and meaning in the next version. The property could be missing, it could be about BSD, it could be about uboot. Whatever. > > Moreover, what it means is that people can then write code to test > what the dtb version is, and we'll end up with the dtb version being > tested to determine various features. What??? Why would we ever accept code that tested the dtb version instead of the compatible strings and properties in device nodes? The dtb version is a meaningless number just like the kernel version number in /proc/version is a meaningless number. It starts at 1 (and can be reset to 1 anytime the person controlling the build environment chooses to for any random reason). The dtb version number only makes sense in a local context to check which build of an object actually got onto the target. I could be creating some confusion here though. The dtb version string also includes the current commit of the source code repository, in exactly the same manner as the kernel version string does. > > Since the design goal of DTB is to describe the hardware, including > the Linux kernel version is totally against that: the Linux kernel > version should be totally irrelevant. What's more relevant would be > a _hardware_ version field, but even that's questionable... > > And yes, you are right - anything like this which we add immediately > becames a user ABI which becomes *very* difficult to change later, so > in order to accept anything like this, we have to be absolutely sure > that this is something we really really really really want to do. Let > me put it another way: once this is accepted, it will probably be > impossible to ever change it. Would it be more palatable if including the dtb-info node was conditional on some debug config option? > > ... just like Bogomips in /proc/cpuinfo. >
> > I'm not sure I see the point in adding a property which is not > > well-defined and not guarnateed to be in any way stable. > > This binding is kind of an odd ball to me. It is clearly _not_ describing > hardware, which is really the central point of the dtb. But the chosen > node is allowed to cover policy things like the boot command line. Sure, but this has nothing to do with policy regarding the HW. This is entirely to do with the build environment of the DTB, which in general you don't need to know. > If I want to debug DTB related issues, read the source that was used > to create the DTB, or slightly modify the DTB - where is the source > and what is the version of it in the source control system. That's only useful if you have access to the machine that the source is on, access to the repo on said machine, and so on. You can easily slightly modify a DTB by decompiling and recompiling it, as bootloaders do to inject /chosen/bootargs. Admittedly this can be painful, but at least you know exactly what was changed, because you know which DTB you started with. Consider what modification by other agents means for provenance of the DTB. We've already been bitten by bootloaders messing with the DTB [1], and simple loaders can change things substantially [2,3], and won't update any provenance binding. > When building and installing a DTB, did the version that I wanted to > install on the target actually get on the target. You can already check the hash if you want to check that you copies the correct version; which is better than trusting an arbitrary string, because if anything fiddles with the DTB it will change. [...] > >> +dtb-path > >> + The build directory relative path of the DTB. > >> + > >> +dts-path > >> + The absolute path of the .dts file compiled to create the DTB. > > > > Why do you need the DTB path? > > Paranoia. Even if not probable, one could build different DTBs from > the same .dts. One could also build the same DTB from two different dts files, no? You could build the same dtb from the same dts, but with some arbitrary things changed, such that the at either end of the process is irrelevant. Personally, I end up doing this a fair amount when testing. > > Why do these differ w.r.t. absolute/relative? > > Those are the forms of the path that are present in the build > system. If there is a good reason to change one of them to the > other form, I could add the extra complexity to massage the path. > > > > > Why would you _ever_ need an absolute path!? > > The absolute path tells you which source repository contained the source. Except that the absolute path is realtive to the machine it was built on, so doesn't actually help. On various machines I have folders called /home/mark/src/linux. These are not the same folder. If I gave you a DTB that told you it was from /home/mark/src/linux/arch/arm64/boot/dts/vendor/foo.dts, you would have difficulty acquiring that precise file. As far as I can tell, this binding just allows you to place a meaningless set of strings in the DTB, that don't actually tell you anything useful. Mark. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/329938.html [2] https://git.kernel.org/cgit/linux/kernel/git/mark/boot-wrapper-aarch64.git/ [3] https://git.kernel.org/cgit/linux/kernel/git/mark/boot-wrapper-aarch64.git/commit/?id=e4ae51a1c128ccaac01bdc834692fd15c3a3c8de
On Thu, Mar 19, 2015 at 12:01:42PM -0700, Frank Rowand wrote: > On 3/19/2015 11:41 AM, Russell King - ARM Linux wrote: > What??? Why would we ever accept code that tested the dtb version > instead of the compatible strings and properties in device nodes? > The dtb version is a meaningless number just like the kernel version > number in /proc/version is a meaningless number. It starts at 1 (and > can be reset to 1 anytime the person controlling the build environment > chooses to for any random reason). The dtb version number only makes > sense in a local context to check which build of an object actually > got onto the target. I think you need to learn a lesson here. I rotfled at your "just like the kernel version number" comment, because we already have code in the kernel to map 3.x and 4.x kernels to a 2.60.x number because userspace breaks with 3.x and 4.x version numbers. I'm quite sure that someone made the exact same argument about kernel version numbers that you're making above about versioning dtb stuff. At the end of the day, if userspace starts abusing an API that we've provided in good faith, and we change something such as the version information which results in userspace breaking - even if userspace is doing something that's wrong according to how we've defined it, it's still our problem to fix. No userspace regressions when upgrading. That's the rule. Don't bother trying to argue against this - you can't. We will not accept any argument (no matter how valid) which will result in userspace breaking upon an upgrade. You must be *absolutely* *sure* that you want to export this information, and that you are absolutely happy with the consequences which would occur should userspace then start using this information in a way which you did not intend, which could very well block you from ever being able to change the version number from a prescribed "this version number makes userspace work" value.
On 3/19/2015 12:32 PM, Russell King - ARM Linux wrote: > On Thu, Mar 19, 2015 at 12:01:42PM -0700, Frank Rowand wrote: >> On 3/19/2015 11:41 AM, Russell King - ARM Linux wrote: >> What??? Why would we ever accept code that tested the dtb version >> instead of the compatible strings and properties in device nodes? >> The dtb version is a meaningless number just like the kernel version >> number in /proc/version is a meaningless number. It starts at 1 (and >> can be reset to 1 anytime the person controlling the build environment >> chooses to for any random reason). The dtb version number only makes >> sense in a local context to check which build of an object actually >> got onto the target. > > I think you need to learn a lesson here. I rotfled at your "just like > the kernel version number" comment, because we already have code in the > kernel to map 3.x and 4.x kernels to a 2.60.x number because userspace > breaks with 3.x and 4.x version numbers. I am aware of that and totally agree that it is on point. > > I'm quite sure that someone made the exact same argument about kernel > version numbers that you're making above about versioning dtb stuff. > > At the end of the day, if userspace starts abusing an API that we've > provided in good faith, and we change something such as the version > information which results in userspace breaking - even if userspace is > doing something that's wrong according to how we've defined it, it's > still our problem to fix. No userspace regressions when upgrading. > That's the rule. And I totally agree with that. > > Don't bother trying to argue against this - you can't. We will not accept > any argument (no matter how valid) which will result in userspace breaking > upon an upgrade. No argument. But I am not arguing for that. > > You must be *absolutely* *sure* that you want to export this information, > and that you are absolutely happy with the consequences which would occur > should userspace then start using this information in a way which you did > not intend, which could very well block you from ever being able to change > the version number from a prescribed "this version number makes userspace > work" value. I understand the concern you are expressing. And I agree it is an issue to be concerned about and not dismissed. But I also think that the concern is mis-characterizing the "DTB version". To pick on the example in patch 0, an analogous Linux version is "#5" (not "4.0.0"): Linux version 4.0.0-rc4-dirty (frank@buildhost) (gcc version 4.6.x-google 20120106 (prerelease) (GCC) ) #5 SMP PREEMPT Wed Mar 18 20:04:48 PDT 2015 and the proposed DTB version is "#4": DTB version 4.0.0-rc4-dirty (frank@buildhost) (DTC 1.4.0-dirty) #4 Wed Mar 18 20:04:11 PDT 2015 I don't think the concern holds for "#5" and "#4". I will concede that there is something unique in the proposed DTB version - the source code system commit version number (in this example "4.0.0-rc4-dirty" from git).
On 3/19/2015 12:12 PM, Mark Rutland wrote: >>> I'm not sure I see the point in adding a property which is not >>> well-defined and not guarnateed to be in any way stable. >> >> This binding is kind of an odd ball to me. It is clearly _not_ describing >> hardware, which is really the central point of the dtb. But the chosen >> node is allowed to cover policy things like the boot command line. > > Sure, but this has nothing to do with policy regarding the HW. This is > entirely to do with the build environment of the DTB, which in general > you don't need to know. > >> If I want to debug DTB related issues, read the source that was used >> to create the DTB, or slightly modify the DTB - where is the source >> and what is the version of it in the source control system. > > That's only useful if you have access to the machine that the source is > on, access to the repo on said machine, and so on. Consider substituting the Linux kernel for the DTB and make the same statement to see if that sounds like a concern. Then consider that in many cases a DTB could come from a Linux or Android distribution, just like many Linux kernels do. My understanding is that the sources in the kernel tree used to make DTBs are gpl v2 and thus just as available as the source to a Linux kernel would be. > > You can easily slightly modify a DTB by decompiling and recompiling it, > as bootloaders do to inject /chosen/bootargs. Admittedly this can be > painful, but at least you know exactly what was changed, because you > know which DTB you started with. Yes, there are circumstances where I might find that the best way to work on a problem. I also often use debuggers to show me an assembly display when debugging a kernel problem. But I sometimes prefer to start at the source code level. > > Consider what modification by other agents means for provenance of the > DTB. We've already been bitten by bootloaders messing with the DTB [1], > and simple loaders can change things substantially [2,3], and won't > update any provenance binding. Yes, bootloaders can cause issues. This patch set is not intended to deal with those problems. btw, thank you for the references, concrete examples help a lot (and they are interesting examples)! > >> When building and installing a DTB, did the version that I wanted to >> install on the target actually get on the target. > > You can already check the hash if you want to check that you copies the > correct version; which is better than trusting an arbitrary string, > because if anything fiddles with the DTB it will change. I'm confused. If the bootloader fiddles with the DTB then I can not compare a hash of the DTB from the build host to a hash of the DTB on the target. Of course the bootloader can also fiddle with the dtb-info if it wants to . Bootloaders do what they want to do. :-( > > [...] > >>>> +dtb-path >>>> + The build directory relative path of the DTB. >>>> + >>>> +dts-path >>>> + The absolute path of the .dts file compiled to create the DTB. >>> >>> Why do you need the DTB path? >> >> Paranoia. Even if not probable, one could build different DTBs from >> the same .dts. > > One could also build the same DTB from two different dts files, no? > > You could build the same dtb from the same dts, but with some arbitrary > things changed, such that the at either end of the process is > irrelevant. Personally, I end up doing this a fair amount when testing. Exactly. And the dtb version number will change when the DTB is recompiled. > >>> Why do these differ w.r.t. absolute/relative? >> >> Those are the forms of the path that are present in the build >> system. If there is a good reason to change one of them to the >> other form, I could add the extra complexity to massage the path. >> >>> >>> Why would you _ever_ need an absolute path!? >> >> The absolute path tells you which source repository contained the source. > > Except that the absolute path is realtive to the machine it was built > on, so doesn't actually help. Different use cases for different people. In all cases the absolute path includes the relative path, so it is just a case of extra information. The relative portion of the path is still useful for anyone who wants to know what .dts was used in the build, even if they don't have access to the build machine. (They do have access to the gpl v2 source code, which will have at least the relative portion of the path because that is needed to build the DTB from the .dts.) The extra information in the absolute path could be useful to a build engineer, a distro engineer, or a support person for a distro. Or could just be useless extra info. > > On various machines I have folders called /home/mark/src/linux. These > are not the same folder. If I gave you a DTB that told you it was from > /home/mark/src/linux/arch/arm64/boot/dts/vendor/foo.dts, you would have > difficulty acquiring that precise file. If you distributed the DTB to me under the gpl v2 then you have also either distributed the source to me or offered to make it available to me. > > As far as I can tell, this binding just allows you to place a > meaningless set of strings in the DTB, that don't actually tell you > anything useful. > > Mark. > > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/329938.html > [2] https://git.kernel.org/cgit/linux/kernel/git/mark/boot-wrapper-aarch64.git/ > [3] https://git.kernel.org/cgit/linux/kernel/git/mark/boot-wrapper-aarch64.git/commit/?id=e4ae51a1c128ccaac01bdc834692fd15c3a3c8de >
> > You must be *absolutely* *sure* that you want to export this information, > > and that you are absolutely happy with the consequences which would occur > > should userspace then start using this information in a way which you did > > not intend, which could very well block you from ever being able to change > > the version number from a prescribed "this version number makes userspace > > work" value. > > I understand the concern you are expressing. And I agree it is an issue to > be concerned about and not dismissed. But I also think that the concern is > mis-characterizing the "DTB version". To pick on the example in patch 0, > an analogous Linux version is "#5" (not "4.0.0"): > > Linux version 4.0.0-rc4-dirty (frank@buildhost) (gcc version 4.6.x-google 20120106 (prerelease) (GCC) ) #5 SMP PREEMPT Wed Mar 18 20:04:48 PDT 2015 > > and the proposed DTB version is "#4": > > DTB version 4.0.0-rc4-dirty (frank@buildhost) (DTC 1.4.0-dirty) #4 Wed Mar 18 20:04:11 PDT 2015 > > I don't think the concern holds for "#5" and "#4". > > I will concede that there is something unique in the proposed DTB version - > the source code system commit version number (in this example "4.0.0-rc4-dirty" > from git). The problem that Russell is describing is that regardless of the origin and intended purpose of the value, some consumer of the value will decide that some arbitrary value means something special to them (even if it does not), and when this changes thigns will break. So in that respect, it doesn't matter where the value came from or what you intend it to mean; it will almost certainly be abused. We try to avoid introducing fragile interfaces like these. Mark.
> > You can already check the hash if you want to check that you copies the > > correct version; which is better than trusting an arbitrary string, > > because if anything fiddles with the DTB it will change. > > I'm confused. If the bootloader fiddles with the DTB then I can not > compare a hash of the DTB from the build host to a hash of the DTB > on the target. Of course the bootloader can also fiddle with the > dtb-info if it wants to . Bootloaders do what they want to do. :-( This is a fundamental issue with the approach: if bootloaders mess with the DTB, then the provenance information is at best misleading. If bootloaders don't mess with the DTB, then a hash gives you reliable provenance information, provided you have a DB of the hashes of DTBs you've shipped. If you're already in the habit of distributing binaries that should be fine, and if you're not then it presumably doesn't matter anyway. > >>>> +dtb-path > >>>> + The build directory relative path of the DTB. > >>>> + > >>>> +dts-path > >>>> + The absolute path of the .dts file compiled to create the DTB. > >>> > >>> Why do you need the DTB path? > >> > >> Paranoia. Even if not probable, one could build different DTBs from > >> the same .dts. > > > > One could also build the same DTB from two different dts files, no? > > > > You could build the same dtb from the same dts, but with some arbitrary > > things changed, such that the at either end of the process is > > irrelevant. Personally, I end up doing this a fair amount when testing. > > Exactly. And the dtb version number will change when the DTB is > recompiled. Consider having to work around stale object issues in the build system with git clean -fdx or similar. While unfortunate, this is sometimes necessary. Due to this, I build two different DTBs with exactly the same "version". If you need to tell these apart, you can do that by hash. You don't need the "version" to do so. (and yes, I am aware that the same is true of the kernel). > >>> Why do these differ w.r.t. absolute/relative? > >> > >> Those are the forms of the path that are present in the build > >> system. If there is a good reason to change one of them to the > >> other form, I could add the extra complexity to massage the path. > >> > >>> > >>> Why would you _ever_ need an absolute path!? > >> > >> The absolute path tells you which source repository contained the source. > > > > Except that the absolute path is realtive to the machine it was built > > on, so doesn't actually help. > > Different use cases for different people. Given the reservations that have been expressed so far, it sounds like the set of negative use cases is larger than the set of positives. > In all cases the absolute path includes the relative path, so it is > just a case of extra information. The relative portion of the path > is still useful for anyone who wants to know what .dts was used in > the build, even if they don't have access to the build machine. > (They do have access to the gpl v2 source code, which will have > at least the relative portion of the path because that is needed > to build the DTB from the .dts.) > > The extra information in the absolute path could be useful to a > build engineer, a distro engineer, or a support person for a distro. > Or could just be useless extra info. I very much suspect it will end up being the latter. For example I've seen various iterations of certain FW ship with all-zero ID numbers, for which the use-cases of said ID numbers are exactly those you mention above. As far as I can see, the only consistent and sane way to identify a binary object is by its hash, which for obvious reasons cannot be encoded within itself (though can be computed consistently). > > On various machines I have folders called /home/mark/src/linux. These > > are not the same folder. If I gave you a DTB that told you it was from > > /home/mark/src/linux/arch/arm64/boot/dts/vendor/foo.dts, you would have > > difficulty acquiring that precise file. > > If you distributed the DTB to me under the gpl v2 then you have also > either distributed the source to me or offered to make it available > to me. That being the case you don't care about the fields; we've established that they're meaningless to the end user. Were I to have distributed files to you, I would have archived the relevant build (with source, config, and so on). I can look up what I've distributed to you and hand you the source without needing additional properties in the DTB. Mark.
Index: b/Documentation/devicetree/bindings/chosen.txt =================================================================== --- a/Documentation/devicetree/bindings/chosen.txt +++ b/Documentation/devicetree/bindings/chosen.txt @@ -46,6 +46,43 @@ on PowerPC "stdout" if "stdout-path" is should only use the "stdout-path" property. +dtb-info node +---------------- + +Information that describes where the device tree blob (DTB) came from and the +environment it was created in. + +This node is normally created by including arch/arm/boot/dts/skeleton.dtsi, +which includes include/dt-bindings/version.dtsi. + +Properties: + +version + The version of the DTB. This is analagous to the linux kernel version. + + This is a format free field intended for human consumption. User space + programs should not have any expections about this property. + + The DTB number in this property is incremented each time a make that + creates one or more DTBs is invoked. If the make creates multiple + DTBs then this number is only incremented once. + + The DTB number is stored in file .version_dtb. + +version-linux + The version of the linux kernel most recently built in the source + control system that contains the source used to build the DTB. + + The linux kernel version number is not incremented for a make that + creates a DTB. + +dtb-path + The build directory relative path of the DTB. + +dts-path + The absolute path of the .dts file compiled to create the DTB. + + Properties documented in other bindings --------------------------------------- #address-cells video/simple-framebuffer-sunxi.txt