[PATCH 0/2] DTC unit-address checks

[PATCH 0/2] DTC unit-address checks

[Rob Herring]

I've been much more active in reviewing bindings lately and a few things 
are getting old repeating. One is unit-address issues. I happened to be 
looking at the old dtc C schema today and found this old patch[1] from 
Stephen which never got merged. It derailed into how to updated ePAPR to 
clarify for unit-address with ranges property, but didn't seem to have 
any other objections. So, I've refreshed Stephen's patch and skip the 
check if non-empty ranges is present.

I also added a 2nd patch with checks for leading '0x' or 0s. This should 
never be valid. Also, I think Stephen's comment about checking the 
address value being difficult is wrong. We should be able to check that 
as any bus specific unit addresses should contain commas.

On a build of all dtbs, ARM has 36K warnings! The good news is arm64 is 
*only* 400 or so. I've skimmed thru them and they all looked valid to 
me.

Any objections to this series will result in getting added as a DT 
binding maintainer. :)

Rob

[1] https://lkml.org/lkml/2013/9/19/301

Rob Herring (1):
  Warn on node name unit-addresses with '0x' or leading 0s

Stephen Warren (1):
  Warn on node name unit-address presence/absence mismatch

 checks.c                        | 38 ++++++++++++++++++++++++++++++++++++--
 tests/reg-without-unit-addr.dts | 10 ++++++++++
 tests/run_tests.sh              |  4 ++++
 tests/unit-addr-leading-0s.dts  | 10 ++++++++++
 tests/unit-addr-leading-0x.dts  | 10 ++++++++++
 tests/unit-addr-without-reg.dts |  9 +++++++++
 6 files changed, 79 insertions(+), 2 deletions(-)
 create mode 100644 tests/reg-without-unit-addr.dts
 create mode 100644 tests/unit-addr-leading-0s.dts
 create mode 100644 tests/unit-addr-leading-0x.dts
 create mode 100644 tests/unit-addr-without-reg.dts

-- 
2.5.0

[PATCH 1/2] Warn on node name unit-address presence/absence mismatch

[Rob Herring]

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. Also allow ranges property as it is deemed valid, but ePAPR
is not clear about it.

Implement a check for this. The code doesn't validate the format of the
unit address; ePAPR implies this may vary from (containing bus) binding
to binding, so doing so would be much more complex.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[robh: also allow non-empty ranges]
Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c                        | 28 ++++++++++++++++++++++++++--
 tests/reg-without-unit-addr.dts | 10 ++++++++++
 tests/run_tests.sh              |  2 ++
 tests/unit-addr-without-reg.dts |  9 +++++++++
 4 files changed, 47 insertions(+), 2 deletions(-)
 create mode 100644 tests/reg-without-unit-addr.dts
 create mode 100644 tests/unit-addr-without-reg.dts

diff --git a/checks.c b/checks.c
index 0c03ac9..4001b8c 100644
--- a/checks.c
+++ b/checks.c
@@ -294,6 +294,30 @@ static void check_node_name_format(struct check *c, struct node *dt,
 }
 NODE_ERROR(node_name_format, NULL, &node_name_chars);
 
+static void check_unit_address_vs_reg(struct check *c, struct node *dt,
+			     struct node *node)
+{
+	const char *unitname = get_unitname(node);
+	struct property *prop = get_property(node, "reg");
+
+	if (!prop) {
+		prop = get_property(node, "ranges");
+		if (prop && !prop->val.len)
+			prop = NULL;
+	}
+
+	if (prop) {
+		if (!unitname[0])
+			FAIL(c, "Node %s has a reg or ranges 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_WARNING(unit_address_vs_reg, NULL);
+
 static void check_property_name_chars(struct check *c, struct node *dt,
 				      struct node *node, struct property *prop)
 {
@@ -654,8 +678,8 @@ TREE_WARNING(obsolete_chosen_interrupt_controller, NULL);
 
 static struct check *check_table[] = {
 	&duplicate_node_names, &duplicate_property_names,
-	&node_name_chars, &node_name_format, &property_name_chars,
-	&name_is_string, &name_properties,
+	&node_name_chars, &node_name_format, &unit_address_vs_reg,
+	&property_name_chars, &name_is_string, &name_properties,
 
 	&duplicate_label,
 
diff --git a/tests/reg-without-unit-addr.dts b/tests/reg-without-unit-addr.dts
new file mode 100644
index 0000000..aaf8af7
--- /dev/null
+++ b/tests/reg-without-unit-addr.dts
@@ -0,0 +1,10 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	node {
+		reg = <0 1>;
+	};
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 8863c9a..4b7a131 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -443,6 +443,8 @@ dtc_tests () {
     check_tests reg-ranges-root.dts reg_format ranges_format
     check_tests default-addr-size.dts avoid_default_addr_size
     check_tests obsolete-chosen-interrupt-controller.dts obsolete_chosen_interrupt_controller
+    check_tests reg-without-unit-addr.dts unit_address_vs_reg
+    check_tests unit-addr-without-reg.dts unit_address_vs_reg
     run_sh_test dtc-checkfails.sh node_name_chars -- -I dtb -O dtb bad_node_char.dtb
     run_sh_test dtc-checkfails.sh node_name_format -- -I dtb -O dtb bad_node_format.dtb
     run_sh_test dtc-checkfails.sh prop_name_chars -- -I dtb -O dtb bad_prop_char.dtb
diff --git a/tests/unit-addr-without-reg.dts b/tests/unit-addr-without-reg.dts
new file mode 100644
index 0000000..ac786eb
--- /dev/null
+++ b/tests/unit-addr-without-reg.dts
@@ -0,0 +1,9 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	node@1 {
+	};
+};
-- 
2.5.0

Re: [PATCH 1/2] Warn on node name unit-address presence/absence mismatch

[David Gibson]

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

On Thu, Feb 11, 2016 at 02:46:58PM -0600, Rob Herring 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. Also allow ranges property as it is deemed valid, but ePAPR
> is not clear about it.
> 
> Implement a check for this. The code doesn't validate the format of the
> unit address; ePAPR implies this may vary from (containing bus) binding
> to binding, so doing so would be much more complex.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> [robh: also allow non-empty ranges]
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

I've applied this to master.

[snip]
> @@ -654,8 +678,8 @@ TREE_WARNING(obsolete_chosen_interrupt_controller, NULL);
>  
>  static struct check *check_table[] = {
>  	&duplicate_node_names, &duplicate_property_names,
> -	&node_name_chars, &node_name_format, &property_name_chars,
> -	&name_is_string, &name_properties,
> +	&node_name_chars, &node_name_format, &unit_address_vs_reg,
> +	&property_name_chars, &name_is_string, &name_properties,
>  

However, I've moved the new test around in check_table.  It's original
test placed it next to very basic structural tests, further down with
more semantic tests makes more sense.

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/2] Warn on node name unit-addresses with '0x' or leading 0s

[Rob Herring]

Node name unit-addresses should never begin with 0x or leading 0s
regardless of whether they have a bus specific address (i.e. one with
commas) or not. Add warnings to check for these cases.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c                       | 10 ++++++++++
 tests/run_tests.sh             |  2 ++
 tests/unit-addr-leading-0s.dts | 10 ++++++++++
 tests/unit-addr-leading-0x.dts | 10 ++++++++++
 4 files changed, 32 insertions(+)
 create mode 100644 tests/unit-addr-leading-0s.dts
 create mode 100644 tests/unit-addr-leading-0x.dts

diff --git a/checks.c b/checks.c
index 4001b8c..300ab43 100644
--- a/checks.c
+++ b/checks.c
@@ -310,6 +310,16 @@ static void check_unit_address_vs_reg(struct check *c, struct node *dt,
 		if (!unitname[0])
 			FAIL(c, "Node %s has a reg or ranges property, but no unit name",
 			    node->fullpath);
+
+		if (strstr(unitname, "0x") == unitname) {
+			FAIL(c, "Node %s unit name should not have leading \"0x\"",
+			    node->fullpath);
+			/* skip over 0x for next test */
+			unitname += 2;
+		}
+		if (unitname[0] == '0' && isxdigit(unitname[1]))
+			FAIL(c, "Node %s unit name should not have leading 0s",
+			    node->fullpath);
 	} else {
 		if (unitname[0])
 			FAIL(c, "Node %s has a unit name, but no reg property",
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 4b7a131..502caa6 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -445,6 +445,8 @@ dtc_tests () {
     check_tests obsolete-chosen-interrupt-controller.dts obsolete_chosen_interrupt_controller
     check_tests reg-without-unit-addr.dts unit_address_vs_reg
     check_tests unit-addr-without-reg.dts unit_address_vs_reg
+    check_tests unit-addr-leading-0x.dts unit_address_vs_reg
+    check_tests unit-addr-leading-0s.dts unit_address_vs_reg
     run_sh_test dtc-checkfails.sh node_name_chars -- -I dtb -O dtb bad_node_char.dtb
     run_sh_test dtc-checkfails.sh node_name_format -- -I dtb -O dtb bad_node_format.dtb
     run_sh_test dtc-checkfails.sh prop_name_chars -- -I dtb -O dtb bad_prop_char.dtb
diff --git a/tests/unit-addr-leading-0s.dts b/tests/unit-addr-leading-0s.dts
new file mode 100644
index 0000000..7c8e2ce
--- /dev/null
+++ b/tests/unit-addr-leading-0s.dts
@@ -0,0 +1,10 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	node@001 {
+		reg = <1 0>;
+	};
+};
diff --git a/tests/unit-addr-leading-0x.dts b/tests/unit-addr-leading-0x.dts
new file mode 100644
index 0000000..7ed7254
--- /dev/null
+++ b/tests/unit-addr-leading-0x.dts
@@ -0,0 +1,10 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	node@0x1 {
+		reg = <1 0>;
+	};
+};
-- 
2.5.0

Re: [PATCH 2/2] Warn on node name unit-addresses with '0x' or leading 0s

[David Gibson]

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

On Thu, Feb 11, 2016 at 02:46:59PM -0600, Rob Herring wrote:
> Node name unit-addresses should never begin with 0x or leading 0s
> regardless of whether they have a bus specific address (i.e. one with
> commas) or not. Add warnings to check for these cases.

Hmm.. I'm pretty sure that's true in practice, but it's not true in
theory.  A bus could define it's unit address format just about
however it wants, including with leading 0s.

I think a better approach would be to add a test specific to
simple-bus devices (by looking at compatible on the parent) that fully
checks the unit address.

From there we can start adding tests for other bus types.

> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  checks.c                       | 10 ++++++++++
>  tests/run_tests.sh             |  2 ++
>  tests/unit-addr-leading-0s.dts | 10 ++++++++++
>  tests/unit-addr-leading-0x.dts | 10 ++++++++++
>  4 files changed, 32 insertions(+)
>  create mode 100644 tests/unit-addr-leading-0s.dts
>  create mode 100644 tests/unit-addr-leading-0x.dts
> 
> diff --git a/checks.c b/checks.c
> index 4001b8c..300ab43 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -310,6 +310,16 @@ static void check_unit_address_vs_reg(struct check *c, struct node *dt,
>  		if (!unitname[0])
>  			FAIL(c, "Node %s has a reg or ranges property, but no unit name",
>  			    node->fullpath);
> +
> +		if (strstr(unitname, "0x") == unitname) {
> +			FAIL(c, "Node %s unit name should not have leading \"0x\"",
> +			    node->fullpath);
> +			/* skip over 0x for next test */
> +			unitname += 2;
> +		}
> +		if (unitname[0] == '0' && isxdigit(unitname[1]))
> +			FAIL(c, "Node %s unit name should not have leading 0s",
> +			    node->fullpath);

I'd also prefer to see these extensions (or ones like them) as
separate checjs from the basic "is unit address present" test.  Makes
the output easier to interpret and allows easier control of which
checks are active.

>  	} else {
>  		if (unitname[0])
>  			FAIL(c, "Node %s has a unit name, but no reg property",
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 4b7a131..502caa6 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -445,6 +445,8 @@ dtc_tests () {
>      check_tests obsolete-chosen-interrupt-controller.dts obsolete_chosen_interrupt_controller
>      check_tests reg-without-unit-addr.dts unit_address_vs_reg
>      check_tests unit-addr-without-reg.dts unit_address_vs_reg
> +    check_tests unit-addr-leading-0x.dts unit_address_vs_reg
> +    check_tests unit-addr-leading-0s.dts unit_address_vs_reg
>      run_sh_test dtc-checkfails.sh node_name_chars -- -I dtb -O dtb bad_node_char.dtb
>      run_sh_test dtc-checkfails.sh node_name_format -- -I dtb -O dtb bad_node_format.dtb
>      run_sh_test dtc-checkfails.sh prop_name_chars -- -I dtb -O dtb bad_prop_char.dtb
> diff --git a/tests/unit-addr-leading-0s.dts b/tests/unit-addr-leading-0s.dts
> new file mode 100644
> index 0000000..7c8e2ce
> --- /dev/null
> +++ b/tests/unit-addr-leading-0s.dts
> @@ -0,0 +1,10 @@
> +/dts-v1/;
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	node@001 {
> +		reg = <1 0>;
> +	};
> +};
> diff --git a/tests/unit-addr-leading-0x.dts b/tests/unit-addr-leading-0x.dts
> new file mode 100644
> index 0000000..7ed7254
> --- /dev/null
> +++ b/tests/unit-addr-leading-0x.dts
> @@ -0,0 +1,10 @@
> +/dts-v1/;
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	node@0x1 {
> +		reg = <1 0>;
> +	};
> +};

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] Warn on node name unit-addresses with '0x' or leading 0s

[Rob Herring]

On Thu, Feb 18, 2016 at 11:07 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Thu, Feb 11, 2016 at 02:46:59PM -0600, Rob Herring wrote:
>> Node name unit-addresses should never begin with 0x or leading 0s
>> regardless of whether they have a bus specific address (i.e. one with
>> commas) or not. Add warnings to check for these cases.
>
> Hmm.. I'm pretty sure that's true in practice, but it's not true in
> theory.  A bus could define it's unit address format just about
> however it wants, including with leading 0s.

Only if it is not reviewed... This whole check is about what best
practices are, not what is possible.

> I think a better approach would be to add a test specific to
> simple-bus devices (by looking at compatible on the parent) that fully
> checks the unit address.
>
> From there we can start adding tests for other bus types.

simple-bus is easy enough, but then next up would be I2C and SPI. We
can't generically tell if a node is on I2C or SPI bus. If we do have
some bus with wacky addresses, it should definitely have a bus
compatible and then we can simply exclude it from the check.

Another option would be skipping the test if there are any commas (or
periods, etc.) in the unit address. That's pretty rare to begin with
and a single number is pretty much not a bus specific unit-address.

Rob

Re: [PATCH 2/2] Warn on node name unit-addresses with '0x' or leading 0s

[David Gibson]

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

On Mon, Feb 22, 2016 at 10:51:46AM -0600, Rob Herring wrote:
> On Thu, Feb 18, 2016 at 11:07 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Thu, Feb 11, 2016 at 02:46:59PM -0600, Rob Herring wrote:
> >> Node name unit-addresses should never begin with 0x or leading 0s
> >> regardless of whether they have a bus specific address (i.e. one with
> >> commas) or not. Add warnings to check for these cases.
> >
> > Hmm.. I'm pretty sure that's true in practice, but it's not true in
> > theory.  A bus could define it's unit address format just about
> > however it wants, including with leading 0s.
> 
> Only if it is not reviewed... This whole check is about what best
> practices are, not what is possible.

Hmm.  dtc checks are really about checking for best practice at the
level of individual dts files, though, not bindings.

> > I think a better approach would be to add a test specific to
> > simple-bus devices (by looking at compatible on the parent) that fully
> > checks the unit address.
> >
> > From there we can start adding tests for other bus types.
> 
> simple-bus is easy enough,

So, start with that, then tackle the next problem.

> but then next up would be I2C and SPI. We
> can't generically tell if a node is on I2C or SPI bus.

Why not?  Or perhaps.. how generically do you need?  I think having a
big list of i2c / spi controllers would be acceptable here, if not
ideal.

> If we do have
> some bus with wacky addresses, it should definitely have a bus
> compatible and then we can simply exclude it from the check.
> 
> Another option would be skipping the test if there are any commas (or
> periods, etc.) in the unit address. That's pretty rare to begin with
> and a single number is pretty much not a bus specific unit-address.

Um.. no.. there are definitely bus types that don't typically use
commas.  ISA, for one.

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] Warn on node name unit-addresses with '0x' or leading 0s

[Rob Herring]

On Mon, Feb 22, 2016 at 11:47 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Mon, Feb 22, 2016 at 10:51:46AM -0600, Rob Herring wrote:
>> On Thu, Feb 18, 2016 at 11:07 PM, David Gibson
>> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Thu, Feb 11, 2016 at 02:46:59PM -0600, Rob Herring wrote:
>> >> Node name unit-addresses should never begin with 0x or leading 0s
>> >> regardless of whether they have a bus specific address (i.e. one with
>> >> commas) or not. Add warnings to check for these cases.
>> >
>> > Hmm.. I'm pretty sure that's true in practice, but it's not true in
>> > theory.  A bus could define it's unit address format just about
>> > however it wants, including with leading 0s.
>>
>> Only if it is not reviewed... This whole check is about what best
>> practices are, not what is possible.
>
> Hmm.  dtc checks are really about checking for best practice at the
> level of individual dts files, though, not bindings.

Checking simple-bus specifically would be checking a binding.

>> > I think a better approach would be to add a test specific to
>> > simple-bus devices (by looking at compatible on the parent) that fully
>> > checks the unit address.
>> >
>> > From there we can start adding tests for other bus types.
>>
>> simple-bus is easy enough,
>
> So, start with that, then tackle the next problem.
>
>> but then next up would be I2C and SPI. We
>> can't generically tell if a node is on I2C or SPI bus.
>
> Why not?  Or perhaps.. how generically do you need?  I think having a
> big list of i2c / spi controllers would be acceptable here, if not
> ideal.

So someone adds a new controller, puts crap in for unit addresses, and
then no warnings until that compatible string is added to dtc. And I'm
still left spending my time in reviews telling them to fix this
trivial crap.

That's roughly 60 I2C controllers (families, so multiple compatible
strings each) plus similar number of SPI controllers, OF-graph
binding, and random other things where reg gets used.

>
>> If we do have
>> some bus with wacky addresses, it should definitely have a bus
>> compatible and then we can simply exclude it from the check.
>>
>> Another option would be skipping the test if there are any commas (or
>> periods, etc.) in the unit address. That's pretty rare to begin with
>> and a single number is pretty much not a bus specific unit-address.
>
> Um.. no.. there are definitely bus types that don't typically use
> commas.  ISA, for one.

All the cases of ISA in the kernel tree at least would pass this test.
But we could either blacklist ISA or skip if any non-hex characters
are present.

BTW, my next patch is stricter node and property name checks on the
use of '#', '?', '.', '+', '*', and '_'. So if you don't think these
kinds to checks belong in dtc, then tell me and suggest how we should
check for this.

Rob

Re: [PATCH 2/2] Warn on node name unit-addresses with '0x' or leading 0s

[David Gibson]

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

On Tue, Feb 23, 2016 at 08:35:46AM -0600, Rob Herring wrote:
> On Mon, Feb 22, 2016 at 11:47 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Mon, Feb 22, 2016 at 10:51:46AM -0600, Rob Herring wrote:
> >> On Thu, Feb 18, 2016 at 11:07 PM, David Gibson
> >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >> > On Thu, Feb 11, 2016 at 02:46:59PM -0600, Rob Herring wrote:
> >> >> Node name unit-addresses should never begin with 0x or leading 0s
> >> >> regardless of whether they have a bus specific address (i.e. one with
> >> >> commas) or not. Add warnings to check for these cases.
> >> >
> >> > Hmm.. I'm pretty sure that's true in practice, but it's not true in
> >> > theory.  A bus could define it's unit address format just about
> >> > however it wants, including with leading 0s.
> >>
> >> Only if it is not reviewed... This whole check is about what best
> >> practices are, not what is possible.
> >
> > Hmm.  dtc checks are really about checking for best practice at the
> > level of individual dts files, though, not bindings.
> 
> Checking simple-bus specifically would be checking a binding.

Sorry, I wasn't clear.  dtc checking the dts against a binding is
fine, but checking the sanity of the binding itself is beyond its
scope.

> >> > I think a better approach would be to add a test specific to
> >> > simple-bus devices (by looking at compatible on the parent) that fully
> >> > checks the unit address.
> >> >
> >> > From there we can start adding tests for other bus types.
> >>
> >> simple-bus is easy enough,
> >
> > So, start with that, then tackle the next problem.
> >
> >> but then next up would be I2C and SPI. We
> >> can't generically tell if a node is on I2C or SPI bus.
> >
> > Why not?  Or perhaps.. how generically do you need?  I think having a
> > big list of i2c / spi controllers would be acceptable here, if not
> > ideal.
> 
> So someone adds a new controller, puts crap in for unit addresses, and
> then no warnings until that compatible string is added to dtc. And I'm
> still left spending my time in reviews telling them to fix this
> trivial crap.
> 
> That's roughly 60 I2C controllers (families, so multiple compatible
> strings each) plus similar number of SPI controllers, OF-graph
> binding, and random other things where reg gets used.

Ah, I see.

Ok, I guess we do need to have an option for a "fallback" scheme for
unit addresses (i.e. hex) for bus types we don't specifically
recognize.  But I'd still like the logic to be:
      if (known bus type)
           check against format for this bus type
      else
           check against fallback format

Rather than putting the second test in with a hacked up set of
exclusions.

To do this nicely, I think the best way will be to add a bus_type
field to the node structure in dtc, and have it populated (with an
option for "unknown") in an early check pass, that later unit address
tests can references as a prereq.

Pointer to a struct with unit address formatting functions, with NULL
for unknown is the obvious choice to me for bus_type.

> >> If we do have
> >> some bus with wacky addresses, it should definitely have a bus
> >> compatible and then we can simply exclude it from the check.
> >>
> >> Another option would be skipping the test if there are any commas (or
> >> periods, etc.) in the unit address. That's pretty rare to begin with
> >> and a single number is pretty much not a bus specific unit-address.
> >
> > Um.. no.. there are definitely bus types that don't typically use
> > commas.  ISA, for one.
> 
> All the cases of ISA in the kernel tree at least would pass this test.
> But we could either blacklist ISA or skip if any non-hex characters
> are present.
> 
> BTW, my next patch is stricter node and property name checks on the
> use of '#', '?', '.', '+', '*', and '_'. So if you don't think these
> kinds to checks belong in dtc, then tell me and suggest how we should
> check for this.

That sounds reasonable on the face of it.

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] Warn on node name unit-addresses with '0x' or leading 0s

[Rob Herring]

On Wed, Feb 24, 2016 at 11:44:56AM +1100, David Gibson wrote:
> On Tue, Feb 23, 2016 at 08:35:46AM -0600, Rob Herring wrote:
> > On Mon, Feb 22, 2016 at 11:47 PM, David Gibson
> > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > On Mon, Feb 22, 2016 at 10:51:46AM -0600, Rob Herring wrote:
> > >> On Thu, Feb 18, 2016 at 11:07 PM, David Gibson
> > >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > >> > On Thu, Feb 11, 2016 at 02:46:59PM -0600, Rob Herring wrote:
> > >> >> Node name unit-addresses should never begin with 0x or leading 0s
> > >> >> regardless of whether they have a bus specific address (i.e. one with
> > >> >> commas) or not. Add warnings to check for these cases.
> > >> >
> > >> > Hmm.. I'm pretty sure that's true in practice, but it's not true in
> > >> > theory.  A bus could define it's unit address format just about
> > >> > however it wants, including with leading 0s.
> > >>
> > >> Only if it is not reviewed... This whole check is about what best
> > >> practices are, not what is possible.
> > >
> > > Hmm.  dtc checks are really about checking for best practice at the
> > > level of individual dts files, though, not bindings.
> > 
> > Checking simple-bus specifically would be checking a binding.
> 
> Sorry, I wasn't clear.  dtc checking the dts against a binding is
> fine, but checking the sanity of the binding itself is beyond its
> scope.
> 
> > >> > I think a better approach would be to add a test specific to
> > >> > simple-bus devices (by looking at compatible on the parent) that fully
> > >> > checks the unit address.
> > >> >
> > >> > From there we can start adding tests for other bus types.
> > >>
> > >> simple-bus is easy enough,
> > >
> > > So, start with that, then tackle the next problem.
> > >
> > >> but then next up would be I2C and SPI. We
> > >> can't generically tell if a node is on I2C or SPI bus.
> > >
> > > Why not?  Or perhaps.. how generically do you need?  I think having a
> > > big list of i2c / spi controllers would be acceptable here, if not
> > > ideal.
> > 
> > So someone adds a new controller, puts crap in for unit addresses, and
> > then no warnings until that compatible string is added to dtc. And I'm
> > still left spending my time in reviews telling them to fix this
> > trivial crap.
> > 
> > That's roughly 60 I2C controllers (families, so multiple compatible
> > strings each) plus similar number of SPI controllers, OF-graph
> > binding, and random other things where reg gets used.
> 
> Ah, I see.
> 
> Ok, I guess we do need to have an option for a "fallback" scheme for
> unit addresses (i.e. hex) for bus types we don't specifically
> recognize.  But I'd still like the logic to be:
>       if (known bus type)
>            check against format for this bus type
>       else
>            check against fallback format
> 
> Rather than putting the second test in with a hacked up set of
> exclusions.

Okay, makes sense.

Do you think we still need simple-bus as an explicit type given the 
check is the same as the default? Might be useful to have if we want to 
add some checks that address translations work.

> To do this nicely, I think the best way will be to add a bus_type
> field to the node structure in dtc, and have it populated (with an
> option for "unknown") in an early check pass, that later unit address
> tests can references as a prereq.
> 
> Pointer to a struct with unit address formatting functions, with NULL
> for unknown is the obvious choice to me for bus_type.

So, something like this for the first stage:

static bool pci_bus_check_is_type(struct node *node)
{
	struct property *prop;
	
	if (!node || !node->parent)
		return false;

	prop = get_property(node->parent, "device_type");
	if (!prop)
		return false;
		
	if (strcmp(prop->val, "pci") == 0)
		return true;
		
	return false;
}

static void pci_bus_check_unit_address()
{

}

struct bus_type_fns {
	.check_is_type = pci_bus_check_is_type,
	.check_unit_address = pci_bus_check_unit_address,
} pci_bus_fns;

struct bus_type_fns * {
	&pci_bus_fns,
	NULL
} bus_types;

static void fixup_bus_type(struct check *c, struct node *root,
				  struct node *node)
{
	struct bus_type_fns **bus;
	
	for (bus = bus_types; *bus != NULL; bus++) {
		if (!(*bus)->check_is_type(node))
			continue;

		node->bus_type = *bus;
		break;
	}
}
ERROR(bus_type, NULL, NULL, fixup_bus_type, NULL, NULL);


Rob
--
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: [PATCH 2/2] Warn on node name unit-addresses with '0x' or leading 0s

[David Gibson]

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

On Wed, Feb 24, 2016 at 09:01:30AM -0600, Rob Herring wrote:
> On Wed, Feb 24, 2016 at 11:44:56AM +1100, David Gibson wrote:
> > On Tue, Feb 23, 2016 at 08:35:46AM -0600, Rob Herring wrote:
> > > On Mon, Feb 22, 2016 at 11:47 PM, David Gibson
> > > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > > On Mon, Feb 22, 2016 at 10:51:46AM -0600, Rob Herring wrote:
> > > >> On Thu, Feb 18, 2016 at 11:07 PM, David Gibson
> > > >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > >> > On Thu, Feb 11, 2016 at 02:46:59PM -0600, Rob Herring wrote:
> > > >> >> Node name unit-addresses should never begin with 0x or leading 0s
> > > >> >> regardless of whether they have a bus specific address (i.e. one with
> > > >> >> commas) or not. Add warnings to check for these cases.
> > > >> >
> > > >> > Hmm.. I'm pretty sure that's true in practice, but it's not true in
> > > >> > theory.  A bus could define it's unit address format just about
> > > >> > however it wants, including with leading 0s.
> > > >>
> > > >> Only if it is not reviewed... This whole check is about what best
> > > >> practices are, not what is possible.
> > > >
> > > > Hmm.  dtc checks are really about checking for best practice at the
> > > > level of individual dts files, though, not bindings.
> > > 
> > > Checking simple-bus specifically would be checking a binding.
> > 
> > Sorry, I wasn't clear.  dtc checking the dts against a binding is
> > fine, but checking the sanity of the binding itself is beyond its
> > scope.
> > 
> > > >> > I think a better approach would be to add a test specific to
> > > >> > simple-bus devices (by looking at compatible on the parent) that fully
> > > >> > checks the unit address.
> > > >> >
> > > >> > From there we can start adding tests for other bus types.
> > > >>
> > > >> simple-bus is easy enough,
> > > >
> > > > So, start with that, then tackle the next problem.
> > > >
> > > >> but then next up would be I2C and SPI. We
> > > >> can't generically tell if a node is on I2C or SPI bus.
> > > >
> > > > Why not?  Or perhaps.. how generically do you need?  I think having a
> > > > big list of i2c / spi controllers would be acceptable here, if not
> > > > ideal.
> > > 
> > > So someone adds a new controller, puts crap in for unit addresses, and
> > > then no warnings until that compatible string is added to dtc. And I'm
> > > still left spending my time in reviews telling them to fix this
> > > trivial crap.
> > > 
> > > That's roughly 60 I2C controllers (families, so multiple compatible
> > > strings each) plus similar number of SPI controllers, OF-graph
> > > binding, and random other things where reg gets used.
> > 
> > Ah, I see.
> > 
> > Ok, I guess we do need to have an option for a "fallback" scheme for
> > unit addresses (i.e. hex) for bus types we don't specifically
> > recognize.  But I'd still like the logic to be:
> >       if (known bus type)
> >            check against format for this bus type
> >       else
> >            check against fallback format
> > 
> > Rather than putting the second test in with a hacked up set of
> > exclusions.
> 
> Okay, makes sense.
> 
> Do you think we still need simple-bus as an explicit type given the 
> check is the same as the default? Might be useful to have if we want to 
> add some checks that address translations work.

So they should be able to have common code to actually do the check /
formatting, but yes, I'd like an explicit check for simple-bus as
well.

> > To do this nicely, I think the best way will be to add a bus_type
> > field to the node structure in dtc, and have it populated (with an
> > option for "unknown") in an early check pass, that later unit address
> > tests can references as a prereq.
> > 
> > Pointer to a struct with unit address formatting functions, with NULL
> > for unknown is the obvious choice to me for bus_type.
> 
> So, something like this for the first stage:
> 
> static bool pci_bus_check_is_type(struct node *node)
> {
> 	struct property *prop;
> 	
> 	if (!node || !node->parent)
> 		return false;
> 
> 	prop = get_property(node->parent, "device_type");
> 	if (!prop)
> 		return false;
> 		
> 	if (strcmp(prop->val, "pci") == 0)
> 		return true;
> 		
> 	return false;
> }
> 
> static void pci_bus_check_unit_address()
> {
> 
> }
> 
> struct bus_type_fns {
> 	.check_is_type = pci_bus_check_is_type,
> 	.check_unit_address = pci_bus_check_unit_address,
> } pci_bus_fns;
> 
> struct bus_type_fns * {
> 	&pci_bus_fns,
> 	NULL
> } bus_types;
> 
> static void fixup_bus_type(struct check *c, struct node *root,
> 				  struct node *node)
> {
> 	struct bus_type_fns **bus;
> 	
> 	for (bus = bus_types; *bus != NULL; bus++) {
> 		if (!(*bus)->check_is_type(node))
> 			continue;
> 
> 		node->bus_type = *bus;
> 		break;
> 	}
> }
> ERROR(bus_type, NULL, NULL, fixup_bus_type, NULL, NULL);

Uh.. close-ish, but I think we can a bit better.  This approach means
the checks won't happen if someone forgets the device_type.  So, I
think it's preferable to determine bus types (where we can) for the
bus parent node, rather than the child nodes; then make the checks on
the child nodes based on the bus_type of the parent.  So maybe
something like

struct bus_type {
	.expected_addr_cells = 3,
	.expected_size_cells = 2,
	.is_type = is_pci_bridge,
	.check_unit_addr = pci_unit_addr,
} pci_bus_type;

struct bus_type {
	.expected_addr_cells = -1, /* For don't care */
	.expected_size_cells = -1,
	.is_type = is_simple_bridge,
	.check_unit_addr = default_unit_addr,
} simple_bus_type;

Checking the addr and size cells here means you can make the unit
address checker have the "reg" format checker as a prereq, so you
don't have to worry about badly constructed "reg" properties in the
unit address format function.

static void check_unit_address_format(struct check *c,
				      struct node *dt,
				      struct node *node)
{
	struct bus_type *bt;
	char expected
	
	if (!node->parent)
		return;
	bt = node->parent->bus_type;
	if (!bt)
		bt = default_bus_type;

	bt->check_unit_addr(c, dt, node);
}
			     


-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]