[PATCH] Remove bogus errors from check_chosen.

[PATCH] Remove bogus errors from check_chosen.

[Scott Wood]

It is not an error for /chosen (or any of its children) to be missing.

It is not a requirement that the output of dtc be a complete, valid
device tree, as it may be intended that the dtb be passed through boot
code that will complete it.  Thus, do not complain.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 livetree.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/livetree.c b/livetree.c
index 45642dc..ce73f50 100644
--- a/livetree.c
+++ b/livetree.c
@@ -588,13 +588,8 @@ static int check_chosen(struct node *root)
 	int ok = 1;
 
 	chosen = get_subnode(root, "chosen");
-	if (! chosen) {
-		ERRMSG("Missing /chosen node\n");
-		return 0;
-	}
-
-	CHECK_HAVE_WARN_STRING(chosen, "bootargs");
-	CHECK_HAVE_WARN_STRING(chosen, "linux,stdout-path");
+	if (!chosen)
+		return ok;
 
         /* give warning for obsolete interrupt-controller property */
 	do {
-- 
1.5.0.3

Re: [PATCH] Remove bogus errors from check_chosen.

[Scott Wood]

On Thu, Mar 22, 2007 at 11:11:09AM -0500, Scott Wood wrote:
> It is not an error for /chosen (or any of its children) to be missing.
> 
> It is not a requirement that the output of dtc be a complete, valid
> device tree, as it may be intended that the dtb be passed through boot
> code that will complete it.  Thus, do not complain.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
>  livetree.c |    9 ++-------
>  1 files changed, 2 insertions(+), 7 deletions(-)

Obviously, this is a DTC patch, and not a kernel patch.
I forgot to label it as such.

-Scott

Re: [PATCH] Remove bogus errors from check_chosen.

[David Gibson]

On Thu, Mar 22, 2007 at 11:11:09AM -0500, Scott Wood wrote:
> It is not an error for /chosen (or any of its children) to be missing.
> 
> It is not a requirement that the output of dtc be a complete, valid
> device tree, as it may be intended that the dtb be passed through boot
> code that will complete it.  Thus, do not complain.

I'm not 100% comfortable with this patch; I'd like for dtc to have the
facility to do more-or-less complete tree validation.  However, as you
point out it's really irritating right now, so I think removing the
check is the best approach for now.  If I ever get around to
implementing a decent error/warning handling framework for dtc, then
more complete checking can go back in under the right circumstances.
 
> Signed-off-by: Scott Wood <scottwood@freescale.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

-- 
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

Re: [PATCH] Remove bogus errors from check_chosen.

[Scott Wood]

On Fri, Mar 23, 2007 at 02:31:58PM +1100, David Gibson wrote:
> I'm not 100% comfortable with this patch; I'd like for dtc to have the
> facility to do more-or-less complete tree validation.

That'd be nice as long as it's optional; it should be possible to do a
low level dts->dtb transformation without the tool assuming that the
output is intended to be a final, valid tree with the specific bindings
that dtc knowns about (or even an OF-ish tree at all).

-Scott

Re: [PATCH] Remove bogus errors from check_chosen.

[Jerry Van Baren]

Scott Wood wrote:
> On Fri, Mar 23, 2007 at 02:31:58PM +1100, David Gibson wrote:
>> I'm not 100% comfortable with this patch; I'd like for dtc to have the
>> facility to do more-or-less complete tree validation.
> 
> That'd be nice as long as it's optional; it should be possible to do a
> low level dts->dtb transformation without the tool assuming that the
> output is intended to be a final, valid tree with the specific bindings
> that dtc knowns about (or even an OF-ish tree at all).
> 
> -Scott

It needs a -Wall ;-)

The -q[q[q]] I added cuts down/eliminates the moaning, but you still 
have to -f force the output which doesn't match what Scott is advocating 
if I understand him.

The gcc -Wall is backwards to what we should have, perhaps adding --no* 
options like:
   --nochosen
   --nocpu
   --noarmyboots
to tell dtc that lonely barefoot blobs are OK.

gvb

Re: [PATCH] Remove bogus errors from check_chosen.

[Scott Wood]

On Fri, Mar 23, 2007 at 11:21:27AM -0400, Jerry Van Baren wrote:
> The -q[q[q]] I added cuts down/eliminates the moaning, but you still 
> have to -f force the output which doesn't match what Scott is advocating 
> if I understand him.
> 
> The gcc -Wall is backwards to what we should have, perhaps adding --no* 
> options like:
>   --nochosen
>   --nocpu
>   --noarmyboots
> to tell dtc that lonely barefoot blobs are OK.

There should also be a --no-validate option to turn everything off, if
the user is confident that the tree is right and doesn't want to get
broken by new dtcs that have new error categories (or if the user is
using dtc to compile some other type of tree than an OF-ish device tree).

In other words, there should be a clear separation between the structural
layer and the semantic layer, even if the same binary can do both.

-Scott

Re: [PATCH] Remove bogus errors from check_chosen.

[Jerry Van Baren]

Scott Wood wrote:
> On Fri, Mar 23, 2007 at 11:21:27AM -0400, Jerry Van Baren wrote:
>> The -q[q[q]] I added cuts down/eliminates the moaning, but you still 
>> have to -f force the output which doesn't match what Scott is advocating 
>> if I understand him.
>>
>> The gcc -Wall is backwards to what we should have, perhaps adding --no* 
>> options like:
>>   --nochosen
>>   --nocpu
>>   --noarmyboots
>> to tell dtc that lonely barefoot blobs are OK.
> 
> There should also be a --no-validate option to turn everything off, if
> the user is confident that the tree is right and doesn't want to get
> broken by new dtcs that have new error categories (or if the user is
> using dtc to compile some other type of tree than an OF-ish device tree).
> 
> In other words, there should be a clear separation between the structural
> layer and the semantic layer, even if the same binary can do both.
> 
> -Scott

Unless I'm missing an error scenario (entirely possible), --no-validate 
is nothing more than
   -f -qqq
which forces the output and quiets the moaning.  Just not as pretty...

I would still advocate being able to disable individual warning/errors. 
For instance, for u-boot, the chosen (and other) nodes are generated by 
u-boot so it is OK to not have them in the source.  If something new 
comes along that _is_ needed, and I would feel better if only the chosen 
node warning was suppressed and dtc _would_ complain about the new 
missing node.

Either way you pick it there will be hypothetical, and probably 
eventually real, scenarios the that wrong choice will be made.

gvb

Re: [PATCH] Remove bogus errors from check_chosen.

[David Gibson]

On Fri, Mar 23, 2007 at 11:48:30AM -0400, Jerry Van Baren wrote:
> Scott Wood wrote:
> > On Fri, Mar 23, 2007 at 11:21:27AM -0400, Jerry Van Baren wrote:
> >> The -q[q[q]] I added cuts down/eliminates the moaning, but you still 
> >> have to -f force the output which doesn't match what Scott is advocating 
> >> if I understand him.
> >>
> >> The gcc -Wall is backwards to what we should have, perhaps adding --no* 
> >> options like:
> >>   --nochosen
> >>   --nocpu
> >>   --noarmyboots
> >> to tell dtc that lonely barefoot blobs are OK.
> > 
> > There should also be a --no-validate option to turn everything off, if
> > the user is confident that the tree is right and doesn't want to get
> > broken by new dtcs that have new error categories (or if the user is
> > using dtc to compile some other type of tree than an OF-ish device tree).
> > 
> > In other words, there should be a clear separation between the structural
> > layer and the semantic layer, even if the same binary can do both.
> > 
> > -Scott
> 
> Unless I'm missing an error scenario (entirely possible), --no-validate 
> is nothing more than
>    -f -qqq
> which forces the output and quiets the moaning.  Just not as pretty...
> 
> I would still advocate being able to disable individual warning/errors. 
> For instance, for u-boot, the chosen (and other) nodes are generated by 
> u-boot so it is OK to not have them in the source.  If something new 
> comes along that _is_ needed, and I would feel better if only the chosen 
> node warning was suppressed and dtc _would_ complain about the new 
> missing node.

Absolutely, this is what I want.  It requires a non-trivial amount of
extra infrastructure to implement though, which is why I haven't
gotten to it.  Because some tests rely on earlier tests to work at all
(i.e. to not SEGV), if we don't bomb out on the first error, we need
to actually keep track of which tests have passed and which tests rely
on which earlier tests.

-- 
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

Re: [PATCH] Remove bogus errors from check_chosen.

[David Gibson]

On Fri, Mar 23, 2007 at 10:36:09AM -0500, Scott Wood wrote:
> On Fri, Mar 23, 2007 at 11:21:27AM -0400, Jerry Van Baren wrote:
> > The -q[q[q]] I added cuts down/eliminates the moaning, but you still 
> > have to -f force the output which doesn't match what Scott is advocating 
> > if I understand him.
> > 
> > The gcc -Wall is backwards to what we should have, perhaps adding --no* 
> > options like:
> >   --nochosen
> >   --nocpu
> >   --noarmyboots
> > to tell dtc that lonely barefoot blobs are OK.
> 
> There should also be a --no-validate option to turn everything off, if
> the user is confident that the tree is right and doesn't want to get
> broken by new dtcs that have new error categories (or if the user is
> using dtc to compile some other type of tree than an OF-ish device tree).

That's what -f is.

-- 
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

Re: [PATCH] Remove bogus errors from check_chosen.

[Jon Loeliger]

On Fri, 2007-03-23 at 10:05, Scott Wood wrote:
> On Fri, Mar 23, 2007 at 02:31:58PM +1100, David Gibson wrote:
> > I'm not 100% comfortable with this patch; I'd like for dtc to have the
> > facility to do more-or-less complete tree validation.
> 
> That'd be nice as long as it's optional; it should be possible to do a
> low level dts->dtb transformation without the tool assuming that the
> output is intended to be a final, valid tree with the specific bindings
> that dtc knowns about (or even an OF-ish tree at all).
> 
> -Scott

So, how about adding a "--complete" or "--final"
sort of flag an having it:

    - Enforce presence of /chosen
    - Disallowing the proposed [ ? ? ? ? ] indicators
    - Uh, other stricter checking...

jdl

Re: [PATCH] Remove bogus errors from check_chosen.

[David Gibson]

On Fri, Mar 23, 2007 at 11:03:13AM -0500, Jon Loeliger wrote:
> On Fri, 2007-03-23 at 10:05, Scott Wood wrote:
> > On Fri, Mar 23, 2007 at 02:31:58PM +1100, David Gibson wrote:
> > > I'm not 100% comfortable with this patch; I'd like for dtc to have the
> > > facility to do more-or-less complete tree validation.
> > 
> > That'd be nice as long as it's optional; it should be possible to do a
> > low level dts->dtb transformation without the tool assuming that the
> > output is intended to be a final, valid tree with the specific bindings
> > that dtc knowns about (or even an OF-ish tree at all).
> > 
> > -Scott
> 
> So, how about adding a "--complete" or "--final"
> sort of flag an having it:
> 
>     - Enforce presence of /chosen
>     - Disallowing the proposed [ ? ? ? ? ] indicators
>     - Uh, other stricter checking...

I was thinking of having three basic levels of checking:

	- syntactic structure
Checks on the structure of the flat tree: duplicate property / node
names, invalid characters in names and other such basic errors.  This
would be on, and fatal by default.

	- semantic structure
Checks on the content of nodes: linux,phandle #a and #s properties
have the right size, reg properties match the corresponding #a and #s
nodes, properties which reference phandles contain a valid phandle,
properties supposed to contain strings actually do.  This would be on
by default.  It would be fatal by default when generating a dtb or asm
from dts, but not in the other direction.

	- kernel requirements
Checks this is a complete tree with all the bits that the kernel
needs: /chosen is present, /memory is present and valid, cpu nodes
have the required cache size information etc.  This would only be on
if requested.

-- 
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

Re: [PATCH] Remove bogus errors from check_chosen.

[Jon Loeliger]

So, like, the other day Scott Wood mumbled:
> It is not an error for /chosen (or any of its children) to be missing.
> 
> It is not a requirement that the output of dtc be a complete, valid
> device tree, as it may be intended that the dtb be passed through boot
> code that will complete it.  Thus, do not complain.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>

Applied.

Thanks,
jdl