[v2,3/4] checks: Warn on node name unit-addresses with '0x' or leading 0s

Message ID 20170210164717.1234-4-robh@kernel.org
State New
Headers show
Series
  • dtc unit-address and character set checks
Related show

Commit Message

Rob Herring Feb. 10, 2017, 4:47 p.m.
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@kernel.org>

---
v2:
- Split into separate check from unit_address_vs_reg

 checks.c                       | 21 +++++++++++++++++++++
 tests/run_tests.sh             |  2 ++
 tests/unit-addr-leading-0s.dts | 10 ++++++++++
 tests/unit-addr-leading-0x.dts | 10 ++++++++++
 4 files changed, 43 insertions(+)
 create mode 100644 tests/unit-addr-leading-0s.dts
 create mode 100644 tests/unit-addr-leading-0x.dts

-- 
2.10.1

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

Comments

David Gibson Feb. 13, 2017, 4:52 a.m. | #1
On Fri, Feb 10, 2017 at 10:47:16AM -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.

> 

> Signed-off-by: Rob Herring <robh@kernel.org>

> ---

> v2:

> - Split into separate check from unit_address_vs_reg


I'm still not thrilled with applying this test unconditionally -
especially when 4/4 introduces pretty much exactly the infrastructure
to do this better.  If you add a unit name formatter function to the
bus_type structure you really can then extend unit_address_vs_reg to
verify them against each other, which will cover this as well more
subtle mismatches.

Obviously it would only do it for known bus types - but adding a
"simple-bus" type would cover a lot of the cases, and a few for i2c
and spi would cover most of the rest.

> 

>  checks.c                       | 21 +++++++++++++++++++++

>  tests/run_tests.sh             |  2 ++

>  tests/unit-addr-leading-0s.dts | 10 ++++++++++

>  tests/unit-addr-leading-0x.dts | 10 ++++++++++

>  4 files changed, 43 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 67237ffe594e..16d17d20caec 100644

> --- a/checks.c

> +++ b/checks.c

> @@ -296,6 +296,26 @@ static void check_unit_address_vs_reg(struct check *c, struct dt_info *dti,

>  }

>  WARNING(unit_address_vs_reg, check_unit_address_vs_reg, NULL);

>  

> +static void check_unit_address_format(struct check *c, struct dt_info *dti,

> +				      struct node *node)

> +{

> +	const char *unitname = get_unitname(node);

> +

> +	if (!unitname[0])

> +		return;

> +

> +	if (!strncmp(unitname, "0x", 2)) {

> +		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);

> +}

> +WARNING(unit_address_format, check_unit_address_format, NULL, &node_name_format);

> +

>  static void check_property_name_chars(struct check *c, struct dt_info *dti,

>  				      struct node *node)

>  {

> @@ -753,6 +773,7 @@ static struct check *check_table[] = {

>  	&addr_size_cells, &reg_format, &ranges_format,

>  

>  	&unit_address_vs_reg,

> +	&unit_address_format,

>  

>  	&avoid_default_addr_size,

>  	&obsolete_chosen_interrupt_controller,

> diff --git a/tests/run_tests.sh b/tests/run_tests.sh

> index ed489dbdd269..0f5c3db79b80 100755

> --- a/tests/run_tests.sh

> +++ b/tests/run_tests.sh

> @@ -540,6 +540,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_format

> +    check_tests unit-addr-leading-0s.dts unit_address_format

>      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 000000000000..7c8e2cebbc84

> --- /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 000000000000..7ed7254e8dc2

> --- /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
Rob Herring Feb. 13, 2017, 8:11 p.m. | #2
On Sun, Feb 12, 2017 at 10:52 PM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Fri, Feb 10, 2017 at 10:47:16AM -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.

>>

>> Signed-off-by: Rob Herring <robh@kernel.org>

>> ---

>> v2:

>> - Split into separate check from unit_address_vs_reg

>

> I'm still not thrilled with applying this test unconditionally -

> especially when 4/4 introduces pretty much exactly the infrastructure

> to do this better.  If you add a unit name formatter function to the

> bus_type structure you really can then extend unit_address_vs_reg to

> verify them against each other, which will cover this as well more

> subtle mismatches.

>

> Obviously it would only do it for known bus types - but adding a

> "simple-bus" type would cover a lot of the cases, and a few for i2c

> and spi would cover most of the rest.


Sigh. Around in circles we go. As we discussed before, this is simple,
but common problem check. It is not a complete check of the entire
unit-address. It only checks the first number (in case of ',') and
doesn't validate the number itself. While in theory we can conceive of
a unit-address that would be a problem for this check, we have not
identified one. It's not like there's lots of unknown cases here. We
know the buses that are out there for the most part. We can always
blacklist any problematic cases or turn off the check by default if
the need arises.

As far as bus types, I could easily support simple-bus, but there's no
way to identify i2c and spi (and others like mdio) buses reliably.
Maybe go off node names if we got those standardized, but I can't
check standard node names for the same reason.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Gibson Feb. 14, 2017, 2:07 a.m. | #3
On Mon, Feb 13, 2017 at 02:11:57PM -0600, Rob Herring wrote:
> On Sun, Feb 12, 2017 at 10:52 PM, David Gibson

> <david@gibson.dropbear.id.au> wrote:

> > On Fri, Feb 10, 2017 at 10:47:16AM -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.

> >>

> >> Signed-off-by: Rob Herring <robh@kernel.org>

> >> ---

> >> v2:

> >> - Split into separate check from unit_address_vs_reg

> >

> > I'm still not thrilled with applying this test unconditionally -

> > especially when 4/4 introduces pretty much exactly the infrastructure

> > to do this better.  If you add a unit name formatter function to the

> > bus_type structure you really can then extend unit_address_vs_reg to

> > verify them against each other, which will cover this as well more

> > subtle mismatches.

> >

> > Obviously it would only do it for known bus types - but adding a

> > "simple-bus" type would cover a lot of the cases, and a few for i2c

> > and spi would cover most of the rest.

> 

> Sigh. Around in circles we go. As we discussed before, this is simple,

> but common problem check. It is not a complete check of the entire

> unit-address.


Right.. and I'm saying why do that, when you're so close to having a
complete check of the entire unit address.

> It only checks the first number (in case of ',') and

> doesn't validate the number itself. While in theory we can conceive of

> a unit-address that would be a problem for this check, we have not

> identified one. It's not like there's lots of unknown cases here. We

> know the buses that are out there for the most part. We can always

> blacklist any problematic cases or turn off the check by default if

> the need arises.


> As far as bus types, I could easily support simple-bus, but there's no

> way to identify i2c and spi (and others like mdio) buses reliably.

> Maybe go off node names if we got those standardized, but I can't

> check standard node names for the same reason.


Ah, ok, that makes things a bit more problematic.

Alright, I can live with this test if you turn it off for anything
that has a specific bus type identified.  i.e. specific bus types
should always have unit address checks which supersede this quick and
dirty 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

Patch hide | download patch | download mbox

diff --git a/checks.c b/checks.c
index 67237ffe594e..16d17d20caec 100644
--- a/checks.c
+++ b/checks.c
@@ -296,6 +296,26 @@  static void check_unit_address_vs_reg(struct check *c, struct dt_info *dti,
 }
 WARNING(unit_address_vs_reg, check_unit_address_vs_reg, NULL);
 
+static void check_unit_address_format(struct check *c, struct dt_info *dti,
+				      struct node *node)
+{
+	const char *unitname = get_unitname(node);
+
+	if (!unitname[0])
+		return;
+
+	if (!strncmp(unitname, "0x", 2)) {
+		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);
+}
+WARNING(unit_address_format, check_unit_address_format, NULL, &node_name_format);
+
 static void check_property_name_chars(struct check *c, struct dt_info *dti,
 				      struct node *node)
 {
@@ -753,6 +773,7 @@  static struct check *check_table[] = {
 	&addr_size_cells, &reg_format, &ranges_format,
 
 	&unit_address_vs_reg,
+	&unit_address_format,
 
 	&avoid_default_addr_size,
 	&obsolete_chosen_interrupt_controller,
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index ed489dbdd269..0f5c3db79b80 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -540,6 +540,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_format
+    check_tests unit-addr-leading-0s.dts unit_address_format
     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 000000000000..7c8e2cebbc84
--- /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 000000000000..7ed7254e8dc2
--- /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>;
+	};
+};