diff mbox series

[1/5] checks: add a string checks for label, bootargs and stdout-path

Message ID 20171117144515.10870-2-robh@kernel.org
State New
Headers show
Series Another batch of dtc checks | expand

Commit Message

Rob Herring (Arm) Nov. 17, 2017, 2:45 p.m. UTC 
Add more string property checks for label, bootargs, and stdout-path.

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

---
 checks.c                   | 4 ++++
 tests/bad-string-props.dts | 3 +++
 tests/run_tests.sh         | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

-- 
2.14.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 Nov. 20, 2017, 12:09 a.m. UTC | #1 
On Fri, Nov 17, 2017 at 08:45:11AM -0600, Rob Herring wrote:
> Add more string property checks for label, bootargs, and stdout-path.


Where does 'label' appear?  I'm not immediately recalling it as a
property with global meaning.

bootargs and stdout-path are from /chosen of course.  I have some
mixed feelings about whether it's reasonable to check it's a string
everywhere, rather than specifically just in /chosen.

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

> ---

>  checks.c                   | 4 ++++

>  tests/bad-string-props.dts | 3 +++

>  tests/run_tests.sh         | 2 +-

>  3 files changed, 8 insertions(+), 1 deletion(-)

> 

> diff --git a/checks.c b/checks.c

> index f5bf5f97a3ad..a4a9d37ca19b 100644

> --- a/checks.c

> +++ b/checks.c

> @@ -586,6 +586,9 @@ WARNING_IF_NOT_CELL(interrupt_cells_is_cell, "#interrupt-cells");

>  WARNING_IF_NOT_STRING(device_type_is_string, "device_type");

>  WARNING_IF_NOT_STRING(model_is_string, "model");

>  WARNING_IF_NOT_STRING(status_is_string, "status");

> +WARNING_IF_NOT_STRING(label_is_string, "label");

> +WARNING_IF_NOT_STRING(bootargs_is_string, "bootargs");

> +WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path");

>  

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

>  				  struct node *node)

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

>  

>  	&address_cells_is_cell, &size_cells_is_cell, &interrupt_cells_is_cell,

>  	&device_type_is_string, &model_is_string, &status_is_string,

> +	&label_is_string, &bootargs_is_string, &stdout_path_is_string,

>  

>  	&property_name_chars_strict,

>  	&node_name_chars_strict,

> diff --git a/tests/bad-string-props.dts b/tests/bad-string-props.dts

> index 396f82069cf7..9b5a7a1736ee 100644

> --- a/tests/bad-string-props.dts

> +++ b/tests/bad-string-props.dts

> @@ -4,4 +4,7 @@

>  	device_type = <0xdeadbeef>;

>  	model = <0xdeadbeef>;

>  	status = <0xdeadbeef>;

> +	bootargs = <0xdeadbeef>;

> +	stdout-path = <0xdeadbeef>;

> +	label = <0xdeadbeef>;

>  };

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

> index 850bc165e757..c610aaeb053e 100755

> --- a/tests/run_tests.sh

> +++ b/tests/run_tests.sh

> @@ -546,7 +546,7 @@ dtc_tests () {

>      check_tests bad-name-property.dts name_properties

>  

>      check_tests bad-ncells.dts address_cells_is_cell size_cells_is_cell interrupt_cells_is_cell

> -    check_tests bad-string-props.dts device_type_is_string model_is_string status_is_string

> +    check_tests bad-string-props.dts device_type_is_string model_is_string status_is_string bootargs_is_string stdout_path_is_string label_is_string

>      check_tests bad-reg-ranges.dts reg_format ranges_format

>      check_tests bad-empty-ranges.dts ranges_format

>      check_tests reg-ranges-root.dts reg_format ranges_format


-- 
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 (Arm) Nov. 20, 2017, 5:26 p.m. UTC | #2 
On Sun, Nov 19, 2017 at 6:09 PM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Fri, Nov 17, 2017 at 08:45:11AM -0600, Rob Herring wrote:

>> Add more string property checks for label, bootargs, and stdout-path.

>

> Where does 'label' appear?  I'm not immediately recalling it as a

> property with global meaning.


It's documented in DT spec/ePAPR. It can be anywhere. It's the
property for the human readable identifiers such as the label on
ethernet ports, serial ports, LEDs, etc.

> bootargs and stdout-path are from /chosen of course.  I have some

> mixed feelings about whether it's reasonable to check it's a string

> everywhere, rather than specifically just in /chosen.


We don't really want to see the same property names with different
meanings. I think checking the location is a separate check. You've
generally suggested splitting up tests rather than have all in one
tests. Not sure if we should check known properties only appear in
chosen or that chosen only has known properties (assuming we can
enumerate that list. Maybe we allow things like "linux,*").

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 Nov. 22, 2017, 3:12 a.m. UTC | #3 
On Mon, Nov 20, 2017 at 11:26:49AM -0600, Rob Herring wrote:
> On Sun, Nov 19, 2017 at 6:09 PM, David Gibson

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

> > On Fri, Nov 17, 2017 at 08:45:11AM -0600, Rob Herring wrote:

> >> Add more string property checks for label, bootargs, and stdout-path.

> >

> > Where does 'label' appear?  I'm not immediately recalling it as a

> > property with global meaning.

> 

> It's documented in DT spec/ePAPR. It can be anywhere. It's the

> property for the human readable identifiers such as the label on

> ethernet ports, serial ports, LEDs, etc.


Ah, ok.  I'd completely forgotten about that one.  Ok, that's a fine
check then.

> > bootargs and stdout-path are from /chosen of course.  I have some

> > mixed feelings about whether it's reasonable to check it's a string

> > everywhere, rather than specifically just in /chosen.

> 

> We don't really want to see the same property names with different

> meanings.


Hm.  As a rule of thumb, yes, but I don't know that it's super
important for properties that aren't very common.  Even then /chosen
is really special.

> I think checking the location is a separate check. You've

> generally suggested splitting up tests rather than have all in one

> tests. Not sure if we should check known properties only appear in

> chosen or that chosen only has known properties (assuming we can

> enumerate that list. Maybe we allow things like "linux,*").


I think for now a better approach would be to add one or more checks
specifically to validate the /chosen 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
diff mbox series

Patch

diff --git a/checks.c b/checks.c
index f5bf5f97a3ad..a4a9d37ca19b 100644
--- a/checks.c
+++ b/checks.c
@@ -586,6 +586,9 @@  WARNING_IF_NOT_CELL(interrupt_cells_is_cell, "#interrupt-cells");
 WARNING_IF_NOT_STRING(device_type_is_string, "device_type");
 WARNING_IF_NOT_STRING(model_is_string, "model");
 WARNING_IF_NOT_STRING(status_is_string, "status");
+WARNING_IF_NOT_STRING(label_is_string, "label");
+WARNING_IF_NOT_STRING(bootargs_is_string, "bootargs");
+WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path");
 
 static void fixup_addr_size_cells(struct check *c, struct dt_info *dti,
 				  struct node *node)
@@ -1236,6 +1239,7 @@  static struct check *check_table[] = {
 
 	&address_cells_is_cell, &size_cells_is_cell, &interrupt_cells_is_cell,
 	&device_type_is_string, &model_is_string, &status_is_string,
+	&label_is_string, &bootargs_is_string, &stdout_path_is_string,
 
 	&property_name_chars_strict,
 	&node_name_chars_strict,
diff --git a/tests/bad-string-props.dts b/tests/bad-string-props.dts
index 396f82069cf7..9b5a7a1736ee 100644
--- a/tests/bad-string-props.dts
+++ b/tests/bad-string-props.dts
@@ -4,4 +4,7 @@ 
 	device_type = <0xdeadbeef>;
 	model = <0xdeadbeef>;
 	status = <0xdeadbeef>;
+	bootargs = <0xdeadbeef>;
+	stdout-path = <0xdeadbeef>;
+	label = <0xdeadbeef>;
 };
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 850bc165e757..c610aaeb053e 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -546,7 +546,7 @@  dtc_tests () {
     check_tests bad-name-property.dts name_properties
 
     check_tests bad-ncells.dts address_cells_is_cell size_cells_is_cell interrupt_cells_is_cell
-    check_tests bad-string-props.dts device_type_is_string model_is_string status_is_string
+    check_tests bad-string-props.dts device_type_is_string model_is_string status_is_string bootargs_is_string stdout_path_is_string label_is_string
     check_tests bad-reg-ranges.dts reg_format ranges_format
     check_tests bad-empty-ranges.dts ranges_format
     check_tests reg-ranges-root.dts reg_format ranges_format