[dtc RFC PATCH] Enforce node name unit-address presence/absence

[dtc RFC PATCH] Enforce node name unit-address presence/absence

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

ePAPR 1.1 section 2.2.1.1 "Node Name Requirements" specifies that any
node that has a reg property must include a unit address in its name
with value matching the first entry in its reg property. Conversely, if
a node does not have a reg property, the node name must not include a
unit address.

Implement a check for this. The code doesn't validate the format of the
unit address; ePAPR implies this may vary from binding to binding, so
I'm not sure that it's possible to validate the value itself.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
This depends on my previous patch "Ensure all tests have matching reg
and unit address".

Note that this patch should not yet be applied; it will cause many real-
world *.dts files to fail to compile. Those need to be fixed first.
However, if/when that happens, this patch may be useful.
---
 checks.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/checks.c b/checks.c
index ee96a25..c80a055 100644
--- a/checks.c
+++ b/checks.c
@@ -287,9 +287,25 @@ NODE_ERROR(node_name_chars, PROPNODECHARS "@");
 static void check_node_name_format(struct check *c, struct node *dt,
 				   struct node *node)
 {
-	if (strchr(get_unitname(node), '@'))
+	const char *unitname;
+	struct property *prop;
+
+	unitname = get_unitname(node);
+
+	if (strchr(unitname, '@'))
 		FAIL(c, "Node %s has multiple '@' characters in name",
 		     node->fullpath);
+
+	prop = get_property(node, "reg");
+	if (prop) {
+		if (!unitname[0])
+			FAIL(c, "Node %s has a reg property, but no unit name",
+			    node->fullpath);
+	} else {
+		if (unitname[0])
+			FAIL(c, "Node %s has a unit name, but no reg property",
+			    node->fullpath);
+	}
 }
 NODE_ERROR(node_name_format, NULL, &node_name_chars);
 
-- 
1.8.1.5

Re: [dtc RFC PATCH] Enforce node name unit-address presence/absence

[Olof Johansson]

On Wed, Sep 18, 2013 at 1:23 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> ePAPR 1.1 section 2.2.1.1 "Node Name Requirements" specifies that any
> node that has a reg property must include a unit address in its name
> with value matching the first entry in its reg property. Conversely, if
> a node does not have a reg property, the node name must not include a
> unit address.
>
> Implement a check for this. The code doesn't validate the format of the
> unit address; ePAPR implies this may vary from binding to binding, so
> I'm not sure that it's possible to validate the value itself.
>
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> This depends on my previous patch "Ensure all tests have matching reg
> and unit address".
>
> Note that this patch should not yet be applied; it will cause many real-
> world *.dts files to fail to compile. Those need to be fixed first.
> However, if/when that happens, this patch may be useful.
> ---
>  checks.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/checks.c b/checks.c
> index ee96a25..c80a055 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -287,9 +287,25 @@ NODE_ERROR(node_name_chars, PROPNODECHARS "@");
>  static void check_node_name_format(struct check *c, struct node *dt,
>                                    struct node *node)
>  {
> -       if (strchr(get_unitname(node), '@'))
> +       const char *unitname;
> +       struct property *prop;
> +
> +       unitname = get_unitname(node);
> +
> +       if (strchr(unitname, '@'))
>                 FAIL(c, "Node %s has multiple '@' characters in name",
>                      node->fullpath);
> +
> +       prop = get_property(node, "reg");
> +       if (prop) {
> +               if (!unitname[0])
> +                       FAIL(c, "Node %s has a reg property, but no unit name",
> +                           node->fullpath);
> +       } else {
> +               if (unitname[0])
> +                       FAIL(c, "Node %s has a unit name, but no reg property",
> +                           node->fullpath);

These checks are very useful, even though they might sort of cross
over the domain to what a dtc linter would do instead of the compiler.

Anyway, I think it'd be better to produce warnings than errors for
this. That way we could also merge it now while the trees are fixed
up.

Also, maybe warn for @0x<foo>, which is another unpreferred syntax, it
should just be @<foo> (with foo being in hex).


-Olof
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [dtc RFC PATCH] Enforce node name unit-address presence/absence

[Stephen Warren]

On 09/18/2013 02:41 PM, Olof Johansson wrote:
> On Wed, Sep 18, 2013 at 1:23 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> ePAPR 1.1 section 2.2.1.1 "Node Name Requirements" specifies that any
>> node that has a reg property must include a unit address in its name
>> with value matching the first entry in its reg property. Conversely, if
>> a node does not have a reg property, the node name must not include a
>> unit address.
>>
>> Implement a check for this. The code doesn't validate the format of the
>> unit address; ePAPR implies this may vary from binding to binding, so
>> I'm not sure that it's possible to validate the value itself.
...
> Anyway, I think it'd be better to produce warnings than errors for
> this. That way we could also merge it now while the trees are fixed
> up.

Yes, that makes sense.

> Also, maybe warn for @0x<foo>, which is another unpreferred syntax, it
> should just be @<foo> (with foo being in hex).

ePAPR doesn't seem to disallow that; it explicitly says that the
unit-address consists of the characters from table 2-1, which is the
same table of characters used for the node name itself. However, it does
state that the binding for a particular bus may impose additional
restrictions; should I implement such a check but limit it to the root
node or specific known bus types? That would require explicitly
whitelisting the check for a lot of bus types, given that each I2C/...
controller binding is a bus type...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [dtc RFC PATCH] Enforce node name unit-address presence/absence

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2349 bytes --]

On Wed, Sep 18, 2013 at 05:02:20PM -0600, Stephen Warren wrote:
> On 09/18/2013 02:41 PM, Olof Johansson wrote:
> > On Wed, Sep 18, 2013 at 1:23 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> >> From: Stephen Warren <swarren@nvidia.com>
> >>
> >> ePAPR 1.1 section 2.2.1.1 "Node Name Requirements" specifies that any
> >> node that has a reg property must include a unit address in its name
> >> with value matching the first entry in its reg property. Conversely, if
> >> a node does not have a reg property, the node name must not include a
> >> unit address.
> >>
> >> Implement a check for this. The code doesn't validate the format of the
> >> unit address; ePAPR implies this may vary from binding to binding, so
> >> I'm not sure that it's possible to validate the value itself.
> ...
> > Anyway, I think it'd be better to produce warnings than errors for
> > this. That way we could also merge it now while the trees are fixed
> > up.
> 
> Yes, that makes sense.
> 
> > Also, maybe warn for @0x<foo>, which is another unpreferred syntax, it
> > should just be @<foo> (with foo being in hex).
> 
> ePAPR doesn't seem to disallow that; it explicitly says that the
> unit-address consists of the characters from table 2-1, which is the
> same table of characters used for the node name itself. However, it does
> state that the binding for a particular bus may impose additional
> restrictions; should I implement such a check but limit it to the root
> node or specific known bus types? That would require explicitly
> whitelisting the check for a lot of bus types, given that each I2C/...
> controller binding is a bus type...

Yeah, I think that's the wrong approach.  Instead I think we need a
table of bus type -> unit address validation functions.  That way we
can start with the common ones - plain memory address, PCI, USB and
I2C, then add more as we need them.

I actually started implementing this once, but I seem to have lost the
patch.

One thing to bear in mind if you do have a crack at this - the correct
encoding of reg -> unit address isn't always unique, although it
usually 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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [dtc RFC PATCH] Enforce node name unit-address presence/absence

[Benjamin Herrenschmidt]

On Wed, 2013-09-18 at 13:41 -0700, Olof Johansson wrote:
> Also, maybe warn for @0x<foo>, which is another unpreferred syntax, it
> should just be @<foo> (with foo being in hex).

It can also bee @foo,bar, it doesn *have* to match the exact content of
the reg property first entry. In fact it's not uncommon to use a 64-bit
value here on 64-bit processors and pci uses a different encoding scheme
(on real OFW at least) where they typically do @dev,fn

Yes, the fact that the unit address is not something deterministic is
and has always been a major PITA though.

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [dtc RFC PATCH] Enforce node name unit-address presence/absence

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3529 bytes --]

On Wed, Sep 18, 2013 at 01:41:16PM -0700, Olof Johansson wrote:
> On Wed, Sep 18, 2013 at 1:23 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> > From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >
> > ePAPR 1.1 section 2.2.1.1 "Node Name Requirements" specifies that any
> > node that has a reg property must include a unit address in its name
> > with value matching the first entry in its reg property. Conversely, if
> > a node does not have a reg property, the node name must not include a
> > unit address.
> >
> > Implement a check for this. The code doesn't validate the format of the
> > unit address; ePAPR implies this may vary from binding to binding, so
> > I'm not sure that it's possible to validate the value itself.
> >
> > Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> > This depends on my previous patch "Ensure all tests have matching reg
> > and unit address".
> >
> > Note that this patch should not yet be applied; it will cause many real-
> > world *.dts files to fail to compile. Those need to be fixed first.
> > However, if/when that happens, this patch may be useful.
> > ---
> >  checks.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/checks.c b/checks.c
> > index ee96a25..c80a055 100644
> > --- a/checks.c
> > +++ b/checks.c
> > @@ -287,9 +287,25 @@ NODE_ERROR(node_name_chars, PROPNODECHARS "@");
> >  static void check_node_name_format(struct check *c, struct node *dt,
> >                                    struct node *node)
> >  {
> > -       if (strchr(get_unitname(node), '@'))
> > +       const char *unitname;
> > +       struct property *prop;
> > +
> > +       unitname = get_unitname(node);
> > +
> > +       if (strchr(unitname, '@'))
> >                 FAIL(c, "Node %s has multiple '@' characters in name",
> >                      node->fullpath);
> > +
> > +       prop = get_property(node, "reg");
> > +       if (prop) {
> > +               if (!unitname[0])
> > +                       FAIL(c, "Node %s has a reg property, but no unit name",
> > +                           node->fullpath);
> > +       } else {
> > +               if (unitname[0])
> > +                       FAIL(c, "Node %s has a unit name, but no reg property",
> > +                           node->fullpath);
> 
> These checks are very useful, even though they might sort of cross
> over the domain to what a dtc linter would do instead of the compiler.

So, I think inside the compiler is the best place for a linter anyway
- that way people will actually run it.

The entire checks infrastructure was built specifically to allow
linting inside the compiler - I just never had time to implement many
checks beyond the basics.

> Anyway, I think it'd be better to produce warnings than errors for
> this. That way we could also merge it now while the trees are fixed
> up.

Yes, I agree.

> Also, maybe warn for @0x<foo>, which is another unpreferred syntax, it
> should just be @<foo> (with foo being in hex).

Well.. that comes to validating the contents of the unit address.  And
as Ben points out that needs per bus type information to know how
they're conventionally formatted.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [dtc RFC PATCH] Enforce node name unit-address presence/absence

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2607 bytes --]

On Wed, Sep 18, 2013 at 02:23:56PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> ePAPR 1.1 section 2.2.1.1 "Node Name Requirements" specifies that any
> node that has a reg property must include a unit address in its name
> with value matching the first entry in its reg property. Conversely, if
> a node does not have a reg property, the node name must not include a
> unit address.
> 
> Implement a check for this. The code doesn't validate the format of the
> unit address; ePAPR implies this may vary from binding to binding, so
> I'm not sure that it's possible to validate the value itself.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> This depends on my previous patch "Ensure all tests have matching reg
> and unit address".
> 
> Note that this patch should not yet be applied; it will cause many real-
> world *.dts files to fail to compile. Those need to be fixed first.
> However, if/when that happens, this patch may be useful.
> ---
>  checks.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/checks.c b/checks.c
> index ee96a25..c80a055 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -287,9 +287,25 @@ NODE_ERROR(node_name_chars, PROPNODECHARS "@");
>  static void check_node_name_format(struct check *c, struct node *dt,
>  				   struct node *node)
>  {
> -	if (strchr(get_unitname(node), '@'))
> +	const char *unitname;
> +	struct property *prop;
> +
> +	unitname = get_unitname(node);
> +
> +	if (strchr(unitname, '@'))
>  		FAIL(c, "Node %s has multiple '@' characters in name",
>  		     node->fullpath);
> +
> +	prop = get_property(node, "reg");
> +	if (prop) {
> +		if (!unitname[0])
> +			FAIL(c, "Node %s has a reg property, but no unit name",
> +			    node->fullpath);
> +	} else {
> +		if (unitname[0])
> +			FAIL(c, "Node %s has a unit name, but no reg property",
> +			    node->fullpath);
> +	}
>  }
>  NODE_ERROR(node_name_format, NULL, &node_name_chars);

I'd prefer to see this implemented as a new check, rather than
extending node_name_format.  It will be a bit more verbose, but it
keeps the low-level syntactic check seperate from the higher-level
semantic / linting check.

It also allows it to be configured as a warning seperately from the
simpler check.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]