[PATCH 0/5] Improve output type formatting

[PATCH 0/5] Improve output type formatting

[Rob Herring]

This series improves maintaining type information in the output of dtc 
from sources without any type annotations such as dtb format. It also 
makes the output formatting less dependent on the input source 
bracketing. As there's already a bunch of type information in the 
checks, we simply need to have the checks add markers.

This is needed in part to be able to run DT schema validation on dtb 
files. I also plan to use the schema files to provide type information 
for all the properties not covered by the dtc checks. Why not do this 
for all the properties? It's possible, but it wouldn't be possible with 
just pure schema. The phandle+args patterns with variable cells would 
need to recreate the same parsing code.

Rob

Rob Herring (5):
  yamltree: Remove marker ordering dependency
  checks: Add check_is_cell() for all phandle+arg properties
  checks: Drop interrupt_cells_is_cell check
  checks: Add markers on known properties
  dtc: Drop dts source restriction for yaml output

 checks.c           | 96 ++++++++++++++++++++++++++++++++++++----------
 dtc.c              |  2 -
 tests/run_tests.sh |  4 +-
 yamltree.c         | 16 ++++----
 4 files changed, 86 insertions(+), 32 deletions(-)

-- 
2.27.0

[PATCH 1/5] yamltree: Remove marker ordering dependency

[Rob Herring]

The check for phandle markers is fragile because the phandle marker must
be after a type marker. The only guarantee for markers is they are in
offset order. The order at a specific offset is undefined.

Rework yaml_propval_int() to get the full marker list, so it can find a
phandle marker no matter the ordering.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 yamltree.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/yamltree.c b/yamltree.c
index e63d32fe142a..55908c829c98 100644
--- a/yamltree.c
+++ b/yamltree.c
@@ -29,11 +29,12 @@ char *yaml_error_name[] = {
 		    (emitter)->problem, __func__, __LINE__);		\
 })
 
-static void yaml_propval_int(yaml_emitter_t *emitter, struct marker *markers, char *data, unsigned int len, int width)
+static void yaml_propval_int(yaml_emitter_t *emitter, struct marker *markers,
+	char *data, unsigned int seq_offset, unsigned int len, int width)
 {
 	yaml_event_t event;
 	void *tag;
-	unsigned int off, start_offset = markers->offset;
+	unsigned int off;
 
 	switch(width) {
 		case 1: tag = "!u8"; break;
@@ -66,7 +67,7 @@ static void yaml_propval_int(yaml_emitter_t *emitter, struct marker *markers, ch
 			m = markers;
 			is_phandle = false;
 			for_each_marker_of_type(m, REF_PHANDLE) {
-				if (m->offset == (start_offset + off)) {
+				if (m->offset == (seq_offset + off)) {
 					is_phandle = true;
 					break;
 				}
@@ -114,6 +115,7 @@ static void yaml_propval(yaml_emitter_t *emitter, struct property *prop)
 	yaml_event_t event;
 	unsigned int len = prop->val.len;
 	struct marker *m = prop->val.markers;
+	struct marker *markers = prop->val.markers;
 
 	/* Emit the property name */
 	yaml_scalar_event_initialize(&event, NULL,
@@ -151,19 +153,19 @@ static void yaml_propval(yaml_emitter_t *emitter, struct property *prop)
 
 		switch(m->type) {
 		case TYPE_UINT16:
-			yaml_propval_int(emitter, m, data, chunk_len, 2);
+			yaml_propval_int(emitter, markers, data, m->offset, chunk_len, 2);
 			break;
 		case TYPE_UINT32:
-			yaml_propval_int(emitter, m, data, chunk_len, 4);
+			yaml_propval_int(emitter, markers, data, m->offset, chunk_len, 4);
 			break;
 		case TYPE_UINT64:
-			yaml_propval_int(emitter, m, data, chunk_len, 8);
+			yaml_propval_int(emitter, markers, data, m->offset, chunk_len, 8);
 			break;
 		case TYPE_STRING:
 			yaml_propval_string(emitter, data, chunk_len);
 			break;
 		default:
-			yaml_propval_int(emitter, m, data, chunk_len, 1);
+			yaml_propval_int(emitter, markers, data, m->offset, chunk_len, 1);
 			break;
 		}
 	}
-- 
2.27.0

Re: [PATCH 1/5] yamltree: Remove marker ordering dependency

[David Gibson]

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

On Tue, May 25, 2021 at 08:03:31PM -0500, Rob Herring wrote:
> The check for phandle markers is fragile because the phandle marker must
> be after a type marker. The only guarantee for markers is they are in
> offset order. The order at a specific offset is undefined.
> 
> Rework yaml_propval_int() to get the full marker list, so it can find a
> phandle marker no matter the ordering.
> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Frankly, I think the difference in data models is enough that yaml
output will *always* be fragile, but this helps somewhat so I've
applied it.

> ---
>  yamltree.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/yamltree.c b/yamltree.c
> index e63d32fe142a..55908c829c98 100644
> --- a/yamltree.c
> +++ b/yamltree.c
> @@ -29,11 +29,12 @@ char *yaml_error_name[] = {
>  		    (emitter)->problem, __func__, __LINE__);		\
>  })
>  
> -static void yaml_propval_int(yaml_emitter_t *emitter, struct marker *markers, char *data, unsigned int len, int width)
> +static void yaml_propval_int(yaml_emitter_t *emitter, struct marker *markers,
> +	char *data, unsigned int seq_offset, unsigned int len, int width)
>  {
>  	yaml_event_t event;
>  	void *tag;
> -	unsigned int off, start_offset = markers->offset;
> +	unsigned int off;
>  
>  	switch(width) {
>  		case 1: tag = "!u8"; break;
> @@ -66,7 +67,7 @@ static void yaml_propval_int(yaml_emitter_t *emitter, struct marker *markers, ch
>  			m = markers;
>  			is_phandle = false;
>  			for_each_marker_of_type(m, REF_PHANDLE) {
> -				if (m->offset == (start_offset + off)) {
> +				if (m->offset == (seq_offset + off)) {
>  					is_phandle = true;
>  					break;
>  				}
> @@ -114,6 +115,7 @@ static void yaml_propval(yaml_emitter_t *emitter, struct property *prop)
>  	yaml_event_t event;
>  	unsigned int len = prop->val.len;
>  	struct marker *m = prop->val.markers;
> +	struct marker *markers = prop->val.markers;
>  
>  	/* Emit the property name */
>  	yaml_scalar_event_initialize(&event, NULL,
> @@ -151,19 +153,19 @@ static void yaml_propval(yaml_emitter_t *emitter, struct property *prop)
>  
>  		switch(m->type) {
>  		case TYPE_UINT16:
> -			yaml_propval_int(emitter, m, data, chunk_len, 2);
> +			yaml_propval_int(emitter, markers, data, m->offset, chunk_len, 2);
>  			break;
>  		case TYPE_UINT32:
> -			yaml_propval_int(emitter, m, data, chunk_len, 4);
> +			yaml_propval_int(emitter, markers, data, m->offset, chunk_len, 4);
>  			break;
>  		case TYPE_UINT64:
> -			yaml_propval_int(emitter, m, data, chunk_len, 8);
> +			yaml_propval_int(emitter, markers, data, m->offset, chunk_len, 8);
>  			break;
>  		case TYPE_STRING:
>  			yaml_propval_string(emitter, data, chunk_len);
>  			break;
>  		default:
> -			yaml_propval_int(emitter, m, data, chunk_len, 1);
> +			yaml_propval_int(emitter, markers, data, m->offset, chunk_len, 1);
>  			break;
>  		}
>  	}

-- 
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: 833 bytes --]

[PATCH 2/5] checks: Add check_is_cell() for all phandle+arg properties

[Rob Herring]

There's already a check for '#.*-cells' properties, so let's enable it for
all the ones we already know about.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/checks.c b/checks.c
index eb91c8ea22fa..6a01a2468c42 100644
--- a/checks.c
+++ b/checks.c
@@ -1466,7 +1466,8 @@ static void check_provider_cells_property(struct check *c,
 }
 #define WARNING_PROPERTY_PHANDLE_CELLS(nm, propname, cells_name, ...) \
 	static struct provider nm##_provider = { (propname), (cells_name), __VA_ARGS__ }; \
-	WARNING(nm##_property, check_provider_cells_property, &nm##_provider, &phandle_references);
+	WARNING_IF_NOT_CELL(nm##_is_cell, cells_name); \
+	WARNING(nm##_property, check_provider_cells_property, &nm##_provider, &nm##_is_cell, &phandle_references);
 
 WARNING_PROPERTY_PHANDLE_CELLS(clocks, "clocks", "#clock-cells");
 WARNING_PROPERTY_PHANDLE_CELLS(cooling_device, "cooling-device", "#cooling-cells");
@@ -1843,21 +1844,37 @@ static struct check *check_table[] = {
 	&chosen_node_is_root, &chosen_node_bootargs, &chosen_node_stdout_path,
 
 	&clocks_property,
+	&clocks_is_cell,
 	&cooling_device_property,
+	&cooling_device_is_cell,
 	&dmas_property,
+	&dmas_is_cell,
 	&hwlocks_property,
+	&hwlocks_is_cell,
 	&interrupts_extended_property,
+	&interrupts_extended_is_cell,
 	&io_channels_property,
+	&io_channels_is_cell,
 	&iommus_property,
+	&iommus_is_cell,
 	&mboxes_property,
+	&mboxes_is_cell,
 	&msi_parent_property,
+	&msi_parent_is_cell,
 	&mux_controls_property,
+	&mux_controls_is_cell,
 	&phys_property,
+	&phys_is_cell,
 	&power_domains_property,
+	&power_domains_is_cell,
 	&pwms_property,
+	&pwms_is_cell,
 	&resets_property,
+	&resets_is_cell,
 	&sound_dai_property,
+	&sound_dai_is_cell,
 	&thermal_sensors_property,
+	&thermal_sensors_is_cell,
 
 	&deprecated_gpio_property,
 	&gpios_property,
-- 
2.27.0

Re: [PATCH 2/5] checks: Add check_is_cell() for all phandle+arg properties

[David Gibson]

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

On Tue, May 25, 2021 at 08:03:32PM -0500, Rob Herring wrote:
> There's already a check for '#.*-cells' properties, so let's enable it for
> all the ones we already know about.
> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Good idea, applied.

> ---
>  checks.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/checks.c b/checks.c
> index eb91c8ea22fa..6a01a2468c42 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -1466,7 +1466,8 @@ static void check_provider_cells_property(struct check *c,
>  }
>  #define WARNING_PROPERTY_PHANDLE_CELLS(nm, propname, cells_name, ...) \
>  	static struct provider nm##_provider = { (propname), (cells_name), __VA_ARGS__ }; \
> -	WARNING(nm##_property, check_provider_cells_property, &nm##_provider, &phandle_references);
> +	WARNING_IF_NOT_CELL(nm##_is_cell, cells_name); \
> +	WARNING(nm##_property, check_provider_cells_property, &nm##_provider, &nm##_is_cell, &phandle_references);
>  
>  WARNING_PROPERTY_PHANDLE_CELLS(clocks, "clocks", "#clock-cells");
>  WARNING_PROPERTY_PHANDLE_CELLS(cooling_device, "cooling-device", "#cooling-cells");
> @@ -1843,21 +1844,37 @@ static struct check *check_table[] = {
>  	&chosen_node_is_root, &chosen_node_bootargs, &chosen_node_stdout_path,
>  
>  	&clocks_property,
> +	&clocks_is_cell,
>  	&cooling_device_property,
> +	&cooling_device_is_cell,
>  	&dmas_property,
> +	&dmas_is_cell,
>  	&hwlocks_property,
> +	&hwlocks_is_cell,
>  	&interrupts_extended_property,
> +	&interrupts_extended_is_cell,
>  	&io_channels_property,
> +	&io_channels_is_cell,
>  	&iommus_property,
> +	&iommus_is_cell,
>  	&mboxes_property,
> +	&mboxes_is_cell,
>  	&msi_parent_property,
> +	&msi_parent_is_cell,
>  	&mux_controls_property,
> +	&mux_controls_is_cell,
>  	&phys_property,
> +	&phys_is_cell,
>  	&power_domains_property,
> +	&power_domains_is_cell,
>  	&pwms_property,
> +	&pwms_is_cell,
>  	&resets_property,
> +	&resets_is_cell,
>  	&sound_dai_property,
> +	&sound_dai_is_cell,
>  	&thermal_sensors_property,
> +	&thermal_sensors_is_cell,

Incidentally, if anyone can see a decent, portable way of avoiding the
ugly redundancy of listing all the tests in this array, I'd love to
hear it.

>  	&deprecated_gpio_property,
>  	&gpios_property,

-- 
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: 833 bytes --]

Re: [PATCH 2/5] checks: Add check_is_cell() for all phandle+arg properties

[Rob Herring]

On Mon, Jun 7, 2021 at 9:25 PM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Tue, May 25, 2021 at 08:03:32PM -0500, Rob Herring wrote:
> > There's already a check for '#.*-cells' properties, so let's enable it for
> > all the ones we already know about.
> >
> > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
> Good idea, applied.
>
> > ---
> >  checks.c | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/checks.c b/checks.c
> > index eb91c8ea22fa..6a01a2468c42 100644
> > --- a/checks.c
> > +++ b/checks.c
> > @@ -1466,7 +1466,8 @@ static void check_provider_cells_property(struct check *c,
> >  }
> >  #define WARNING_PROPERTY_PHANDLE_CELLS(nm, propname, cells_name, ...) \
> >       static struct provider nm##_provider = { (propname), (cells_name), __VA_ARGS__ }; \
> > -     WARNING(nm##_property, check_provider_cells_property, &nm##_provider, &phandle_references);
> > +     WARNING_IF_NOT_CELL(nm##_is_cell, cells_name); \
> > +     WARNING(nm##_property, check_provider_cells_property, &nm##_provider, &nm##_is_cell, &phandle_references);
> >
> >  WARNING_PROPERTY_PHANDLE_CELLS(clocks, "clocks", "#clock-cells");
> >  WARNING_PROPERTY_PHANDLE_CELLS(cooling_device, "cooling-device", "#cooling-cells");
> > @@ -1843,21 +1844,37 @@ static struct check *check_table[] = {
> >       &chosen_node_is_root, &chosen_node_bootargs, &chosen_node_stdout_path,
> >
> >       &clocks_property,
> > +     &clocks_is_cell,
> >       &cooling_device_property,
> > +     &cooling_device_is_cell,
> >       &dmas_property,
> > +     &dmas_is_cell,
> >       &hwlocks_property,
> > +     &hwlocks_is_cell,
> >       &interrupts_extended_property,
> > +     &interrupts_extended_is_cell,
> >       &io_channels_property,
> > +     &io_channels_is_cell,
> >       &iommus_property,
> > +     &iommus_is_cell,
> >       &mboxes_property,
> > +     &mboxes_is_cell,
> >       &msi_parent_property,
> > +     &msi_parent_is_cell,
> >       &mux_controls_property,
> > +     &mux_controls_is_cell,
> >       &phys_property,
> > +     &phys_is_cell,
> >       &power_domains_property,
> > +     &power_domains_is_cell,
> >       &pwms_property,
> > +     &pwms_is_cell,
> >       &resets_property,
> > +     &resets_is_cell,
> >       &sound_dai_property,
> > +     &sound_dai_is_cell,
> >       &thermal_sensors_property,
> > +     &thermal_sensors_is_cell,
>
> Incidentally, if anyone can see a decent, portable way of avoiding the
> ugly redundancy of listing all the tests in this array, I'd love to
> hear it.

If we have a standard prefix to match on, we could generate the list:

nm -P checks.o | sed -n -e 's/^\(check_[a-zA-Z0-9_]*\).*/\&\1,/p'

(Obviously, 'check_' is not the right prefix currently.)

Most of the work here is in the build system and I don't really care
to implement this twice. :( What happened to the Makefile wrapping
meson?

Rob

Re: [PATCH 2/5] checks: Add check_is_cell() for all phandle+arg properties

[David Gibson]

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

On Tue, Jun 08, 2021 at 09:45:42AM -0500, Rob Herring wrote:
> On Mon, Jun 7, 2021 at 9:25 PM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Tue, May 25, 2021 at 08:03:32PM -0500, Rob Herring wrote:
> > > There's already a check for '#.*-cells' properties, so let's enable it for
> > > all the ones we already know about.
> > >
> > > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >
> > Good idea, applied.
> >
> > > ---
> > >  checks.c | 19 ++++++++++++++++++-
> > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/checks.c b/checks.c
> > > index eb91c8ea22fa..6a01a2468c42 100644
> > > --- a/checks.c
> > > +++ b/checks.c
> > > @@ -1466,7 +1466,8 @@ static void check_provider_cells_property(struct check *c,
> > >  }
> > >  #define WARNING_PROPERTY_PHANDLE_CELLS(nm, propname, cells_name, ...) \
> > >       static struct provider nm##_provider = { (propname), (cells_name), __VA_ARGS__ }; \
> > > -     WARNING(nm##_property, check_provider_cells_property, &nm##_provider, &phandle_references);
> > > +     WARNING_IF_NOT_CELL(nm##_is_cell, cells_name); \
> > > +     WARNING(nm##_property, check_provider_cells_property, &nm##_provider, &nm##_is_cell, &phandle_references);
> > >
> > >  WARNING_PROPERTY_PHANDLE_CELLS(clocks, "clocks", "#clock-cells");
> > >  WARNING_PROPERTY_PHANDLE_CELLS(cooling_device, "cooling-device", "#cooling-cells");
> > > @@ -1843,21 +1844,37 @@ static struct check *check_table[] = {
> > >       &chosen_node_is_root, &chosen_node_bootargs, &chosen_node_stdout_path,
> > >
> > >       &clocks_property,
> > > +     &clocks_is_cell,
> > >       &cooling_device_property,
> > > +     &cooling_device_is_cell,
> > >       &dmas_property,
> > > +     &dmas_is_cell,
> > >       &hwlocks_property,
> > > +     &hwlocks_is_cell,
> > >       &interrupts_extended_property,
> > > +     &interrupts_extended_is_cell,
> > >       &io_channels_property,
> > > +     &io_channels_is_cell,
> > >       &iommus_property,
> > > +     &iommus_is_cell,
> > >       &mboxes_property,
> > > +     &mboxes_is_cell,
> > >       &msi_parent_property,
> > > +     &msi_parent_is_cell,
> > >       &mux_controls_property,
> > > +     &mux_controls_is_cell,
> > >       &phys_property,
> > > +     &phys_is_cell,
> > >       &power_domains_property,
> > > +     &power_domains_is_cell,
> > >       &pwms_property,
> > > +     &pwms_is_cell,
> > >       &resets_property,
> > > +     &resets_is_cell,
> > >       &sound_dai_property,
> > > +     &sound_dai_is_cell,
> > >       &thermal_sensors_property,
> > > +     &thermal_sensors_is_cell,
> >
> > Incidentally, if anyone can see a decent, portable way of avoiding the
> > ugly redundancy of listing all the tests in this array, I'd love to
> > hear it.
> 
> If we have a standard prefix to match on, we could generate the list:
> 
> nm -P checks.o | sed -n -e 's/^\(check_[a-zA-Z0-9_]*\).*/\&\1,/p'
> 
> (Obviously, 'check_' is not the right prefix currently.)
> 
> Most of the work here is in the build system and I don't really care
> to implement this twice. :( What happened to the Makefile wrapping
> meson?

There are a handful of issues with the meson stuff that we're (rather
slowly) ironing out before I'm ready to replace the existing makefiles
entirely.

-- 
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: 833 bytes --]

[PATCH 3/5] checks: Drop interrupt_cells_is_cell check

[Rob Herring]

With the prior commit, this check is now redundant.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c           | 3 +--
 tests/run_tests.sh | 4 ++--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/checks.c b/checks.c
index 6a01a2468c42..7ff044df6837 100644
--- a/checks.c
+++ b/checks.c
@@ -672,7 +672,6 @@ ERROR(omit_unused_nodes, fixup_omit_unused_nodes, NULL, &phandle_references, &pa
  */
 WARNING_IF_NOT_CELL(address_cells_is_cell, "#address-cells");
 WARNING_IF_NOT_CELL(size_cells_is_cell, "#size-cells");
-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");
@@ -1809,7 +1808,7 @@ static struct check *check_table[] = {
 	&phandle_references, &path_references,
 	&omit_unused_nodes,
 
-	&address_cells_is_cell, &size_cells_is_cell, &interrupt_cells_is_cell,
+	&address_cells_is_cell, &size_cells_is_cell,
 	&device_type_is_string, &model_is_string, &status_is_string,
 	&label_is_string,
 
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 0e8ecdb40c2c..0e270feb3e47 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -691,7 +691,7 @@ dtc_tests () {
     run_sh_test "$SRCDIR/dtc-fatal.sh" -I dts -O dtb "$SRCDIR/nonexist-node-ref2.dts"
     check_tests "$SRCDIR/bad-name-property.dts" name_properties
 
-    check_tests "$SRCDIR/bad-ncells.dts" address_cells_is_cell size_cells_is_cell interrupt_cells_is_cell
+    check_tests "$SRCDIR/bad-ncells.dts" address_cells_is_cell size_cells_is_cell interrupts_extended_is_cell
     check_tests "$SRCDIR/bad-string-props.dts" device_type_is_string model_is_string status_is_string label_is_string compatible_is_string_list names_is_string_list
     check_tests "$SRCDIR/bad-chosen.dts" chosen_node_is_root
     check_tests "$SRCDIR/bad-chosen.dts" chosen_node_bootargs
@@ -740,7 +740,7 @@ dtc_tests () {
 
 
     # Check warning options
-    run_sh_test "$SRCDIR/dtc-checkfails.sh" address_cells_is_cell interrupt_cells_is_cell -n size_cells_is_cell -- -Wno_size_cells_is_cell -I dts -O dtb "$SRCDIR/bad-ncells.dts"
+    run_sh_test "$SRCDIR/dtc-checkfails.sh" address_cells_is_cell interrupts_extended_is_cell -n size_cells_is_cell -- -Wno_size_cells_is_cell -I dts -O dtb "$SRCDIR/bad-ncells.dts"
     run_sh_test "$SRCDIR/dtc-fails.sh" -n test-warn-output.test.dtb -I dts -O dtb "$SRCDIR/bad-ncells.dts"
     run_sh_test "$SRCDIR/dtc-fails.sh" test-error-output.test.dtb -I dts -O dtb bad-ncells.dts -Esize_cells_is_cell
     run_sh_test "$SRCDIR/dtc-checkfails.sh" always_fail -- -Walways_fail -I dts -O dtb "$SRCDIR/test_tree1.dts"
-- 
2.27.0

Re: [PATCH 3/5] checks: Drop interrupt_cells_is_cell check

[David Gibson]

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

On Tue, May 25, 2021 at 08:03:33PM -0500, Rob Herring wrote:
> With the prior commit, this check is now redundant.

Applied, thanks.

> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  checks.c           | 3 +--
>  tests/run_tests.sh | 4 ++--
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/checks.c b/checks.c
> index 6a01a2468c42..7ff044df6837 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -672,7 +672,6 @@ ERROR(omit_unused_nodes, fixup_omit_unused_nodes, NULL, &phandle_references, &pa
>   */
>  WARNING_IF_NOT_CELL(address_cells_is_cell, "#address-cells");
>  WARNING_IF_NOT_CELL(size_cells_is_cell, "#size-cells");
> -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");
> @@ -1809,7 +1808,7 @@ static struct check *check_table[] = {
>  	&phandle_references, &path_references,
>  	&omit_unused_nodes,
>  
> -	&address_cells_is_cell, &size_cells_is_cell, &interrupt_cells_is_cell,
> +	&address_cells_is_cell, &size_cells_is_cell,
>  	&device_type_is_string, &model_is_string, &status_is_string,
>  	&label_is_string,
>  
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 0e8ecdb40c2c..0e270feb3e47 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -691,7 +691,7 @@ dtc_tests () {
>      run_sh_test "$SRCDIR/dtc-fatal.sh" -I dts -O dtb "$SRCDIR/nonexist-node-ref2.dts"
>      check_tests "$SRCDIR/bad-name-property.dts" name_properties
>  
> -    check_tests "$SRCDIR/bad-ncells.dts" address_cells_is_cell size_cells_is_cell interrupt_cells_is_cell
> +    check_tests "$SRCDIR/bad-ncells.dts" address_cells_is_cell size_cells_is_cell interrupts_extended_is_cell
>      check_tests "$SRCDIR/bad-string-props.dts" device_type_is_string model_is_string status_is_string label_is_string compatible_is_string_list names_is_string_list
>      check_tests "$SRCDIR/bad-chosen.dts" chosen_node_is_root
>      check_tests "$SRCDIR/bad-chosen.dts" chosen_node_bootargs
> @@ -740,7 +740,7 @@ dtc_tests () {
>  
>  
>      # Check warning options
> -    run_sh_test "$SRCDIR/dtc-checkfails.sh" address_cells_is_cell interrupt_cells_is_cell -n size_cells_is_cell -- -Wno_size_cells_is_cell -I dts -O dtb "$SRCDIR/bad-ncells.dts"
> +    run_sh_test "$SRCDIR/dtc-checkfails.sh" address_cells_is_cell interrupts_extended_is_cell -n size_cells_is_cell -- -Wno_size_cells_is_cell -I dts -O dtb "$SRCDIR/bad-ncells.dts"
>      run_sh_test "$SRCDIR/dtc-fails.sh" -n test-warn-output.test.dtb -I dts -O dtb "$SRCDIR/bad-ncells.dts"
>      run_sh_test "$SRCDIR/dtc-fails.sh" test-error-output.test.dtb -I dts -O dtb bad-ncells.dts -Esize_cells_is_cell
>      run_sh_test "$SRCDIR/dtc-checkfails.sh" always_fail -- -Walways_fail -I dts -O dtb "$SRCDIR/test_tree1.dts"

-- 
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: 833 bytes --]

[PATCH 4/5] checks: Add markers on known properties

[Rob Herring]

For properties we already have checks for, we know the type and how to
parse them. Use this to add type and phandle markers so we have them when
the source did not (e.g. dtb format).

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c | 74 ++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 18 deletions(-)

diff --git a/checks.c b/checks.c
index 7ff044df6837..69ad3eec50b5 100644
--- a/checks.c
+++ b/checks.c
@@ -58,6 +58,32 @@ struct check {
 #define CHECK(nm_, fn_, d_, ...) \
 	CHECK_ENTRY(nm_, fn_, d_, false, false, __VA_ARGS__)
 
+static void marker_add(struct marker **list, enum markertype type, unsigned int offset)
+{
+	struct marker *m = *list;
+
+	/* Check if we already have the same marker */
+	for_each_marker_of_type(m, type)
+		if (m->type == type && m->offset == offset)
+			return;
+
+	m = xmalloc(sizeof(*m));
+	m->type = type;
+	m->offset = offset;
+	m->next = NULL;
+	m->ref = NULL;
+
+	/* Find the insertion point, markers are in order by offset */
+	while (*list && ((*list)->offset < m->offset))
+		list = &((*list)->next);
+
+	if (*list) {
+		m->next = (*list)->next;
+		(*list)->next = m;
+	} else
+		*list = m;
+}
+
 static inline void  PRINTF(5, 6) check_msg(struct check *c, struct dt_info *dti,
 					   struct node *node,
 					   struct property *prop,
@@ -252,8 +278,12 @@ static void check_is_cell(struct check *c, struct dt_info *dti,
 	if (!prop)
 		return; /* Not present, assumed ok */
 
-	if (prop->val.len != sizeof(cell_t))
+	if (prop->val.len != sizeof(cell_t)) {
 		FAIL_PROP(c, dti, node, prop, "property is not a single cell");
+		return;
+	}
+
+	marker_add(&prop->val.markers, TYPE_UINT32, 0);
 }
 #define WARNING_IF_NOT_CELL(nm, propname) \
 	WARNING(nm, check_is_cell, (propname))
@@ -509,6 +539,7 @@ static cell_t check_phandle_prop(struct check *c, struct dt_info *dti,
 		 * we treat it as having no phandle data for now. */
 		return 0;
 	}
+	marker_add(&prop->val.markers, TYPE_UINT32, 0);
 
 	phandle = propval_cell(prop);
 
@@ -748,7 +779,7 @@ static void check_reg_format(struct check *c, struct dt_info *dti,
 			     struct node *node)
 {
 	struct property *prop;
-	int addr_cells, size_cells, entrylen;
+	int addr_cells, size_cells, entrylen, offset;
 
 	prop = get_property(node, "reg");
 	if (!prop)
@@ -766,10 +797,15 @@ static void check_reg_format(struct check *c, struct dt_info *dti,
 	size_cells = node_size_cells(node->parent);
 	entrylen = (addr_cells + size_cells) * sizeof(cell_t);
 
-	if (!entrylen || (prop->val.len % entrylen) != 0)
+	if (!entrylen || (prop->val.len % entrylen) != 0) {
 		FAIL_PROP(c, dti, node, prop, "property has invalid length (%d bytes) "
 			  "(#address-cells == %d, #size-cells == %d)",
 			  prop->val.len, addr_cells, size_cells);
+		return;
+	}
+
+	for (offset = 0; offset < prop->val.len; offset += entrylen)
+		marker_add(&prop->val.markers, TYPE_UINT32, offset);
 }
 WARNING(reg_format, check_reg_format, NULL, &addr_size_cells);
 
@@ -777,7 +813,7 @@ static void check_ranges_format(struct check *c, struct dt_info *dti,
 				struct node *node)
 {
 	struct property *prop;
-	int c_addr_cells, p_addr_cells, c_size_cells, p_size_cells, entrylen;
+	int c_addr_cells, p_addr_cells, c_size_cells, p_size_cells, entrylen, offset;
 	const char *ranges = c->data;
 
 	prop = get_property(node, ranges);
@@ -813,6 +849,9 @@ static void check_ranges_format(struct check *c, struct dt_info *dti,
 			  "#size-cells == %d)", ranges, prop->val.len,
 			  p_addr_cells, c_addr_cells, c_size_cells);
 	}
+
+	for (offset = 0; offset < prop->val.len; offset += entrylen)
+		marker_add(&prop->val.markers, TYPE_UINT32, offset);
 }
 WARNING(ranges_format, check_ranges_format, "ranges", &addr_size_cells);
 WARNING(dma_ranges_format, check_ranges_format, "dma-ranges", &addr_size_cells);
@@ -1408,19 +1447,6 @@ static void check_property_phandle_args(struct check *c,
 			continue;
 		}
 
-		/* If we have markers, verify the current cell is a phandle */
-		if (prop->val.markers) {
-			struct marker *m = prop->val.markers;
-			for_each_marker_of_type(m, REF_PHANDLE) {
-				if (m->offset == (cell * sizeof(cell_t)))
-					break;
-			}
-			if (!m)
-				FAIL_PROP(c, dti, node, prop,
-					  "cell %d is not a phandle reference",
-					  cell);
-		}
-
 		provider_node = get_node_by_phandle(root, phandle);
 		if (!provider_node) {
 			FAIL_PROP(c, dti, node, prop,
@@ -1447,6 +1473,9 @@ static void check_property_phandle_args(struct check *c,
 				  "property size (%d) too small for cell size %d",
 				  prop->val.len, cellsize);
 		}
+
+		marker_add(&prop->val.markers, REF_PHANDLE, cell * sizeof(cell_t));
+		marker_add(&prop->val.markers, TYPE_UINT32, cell * sizeof(cell_t));
 	}
 }
 
@@ -1588,7 +1617,7 @@ static void check_interrupts_property(struct check *c,
 	struct node *root = dti->dt;
 	struct node *irq_node = NULL, *parent = node;
 	struct property *irq_prop, *prop = NULL;
-	int irq_cells, phandle;
+	int irq_cells, phandle, offset;
 
 	irq_prop = get_property(node, "interrupts");
 	if (!irq_prop)
@@ -1625,6 +1654,8 @@ static void check_interrupts_property(struct check *c,
 				FAIL(c, dti, irq_node,
 				     "Missing interrupt-controller or interrupt-map property");
 
+			marker_add(&prop->val.markers, TYPE_UINT32, 0);
+			marker_add(&prop->val.markers, REF_PHANDLE, 0);
 			break;
 		}
 
@@ -1647,7 +1678,11 @@ static void check_interrupts_property(struct check *c,
 		FAIL_PROP(c, dti, node, prop,
 			  "size is (%d), expected multiple of %d",
 			  irq_prop->val.len, (int)(irq_cells * sizeof(cell_t)));
+		return;
 	}
+
+	for (offset = 0; offset < irq_prop->val.len; offset += irq_cells * sizeof(cell_t))
+		marker_add(&irq_prop->val.markers, TYPE_UINT32, offset);
 }
 WARNING(interrupts_property, check_interrupts_property, &phandle_references);
 
@@ -1771,6 +1806,9 @@ static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti,
 	if (!node)
 		FAIL_PROP(c, dti, endpoint, prop, "graph phandle is not valid");
 
+	marker_add(&prop->val.markers, TYPE_UINT32, 0);
+	marker_add(&prop->val.markers, REF_PHANDLE, 0);
+
 	return node;
 }
 
-- 
2.27.0

Re: [PATCH 4/5] checks: Add markers on known properties

[David Gibson]

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

On Tue, May 25, 2021 at 08:03:34PM -0500, Rob Herring wrote:
> For properties we already have checks for, we know the type and how to
> parse them. Use this to add type and phandle markers so we have them when
> the source did not (e.g. dtb format).
> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  checks.c | 74 ++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 56 insertions(+), 18 deletions(-)
> 
> diff --git a/checks.c b/checks.c
> index 7ff044df6837..69ad3eec50b5 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -58,6 +58,32 @@ struct check {
>  #define CHECK(nm_, fn_, d_, ...) \
>  	CHECK_ENTRY(nm_, fn_, d_, false, false, __VA_ARGS__)
>  
> +static void marker_add(struct marker **list, enum markertype type, unsigned int offset)
> +{
> +	struct marker *m = *list;
> +
> +	/* Check if we already have the same marker */
> +	for_each_marker_of_type(m, type)
> +		if (m->type == type && m->offset == offset)
> +			return;
> +
> +	m = xmalloc(sizeof(*m));
> +	m->type = type;
> +	m->offset = offset;
> +	m->next = NULL;
> +	m->ref = NULL;
> +
> +	/* Find the insertion point, markers are in order by offset */
> +	while (*list && ((*list)->offset < m->offset))
> +		list = &((*list)->next);
> +
> +	if (*list) {
> +		m->next = (*list)->next;
> +		(*list)->next = m;
> +	} else
> +		*list = m;
> +}
> +
>  static inline void  PRINTF(5, 6) check_msg(struct check *c, struct dt_info *dti,
>  					   struct node *node,
>  					   struct property *prop,
> @@ -252,8 +278,12 @@ static void check_is_cell(struct check *c, struct dt_info *dti,
>  	if (!prop)
>  		return; /* Not present, assumed ok */
>  
> -	if (prop->val.len != sizeof(cell_t))
> +	if (prop->val.len != sizeof(cell_t)) {
>  		FAIL_PROP(c, dti, node, prop, "property is not a single cell");
> +		return;
> +	}
> +
> +	marker_add(&prop->val.markers, TYPE_UINT32, 0);
>  }
>  #define WARNING_IF_NOT_CELL(nm, propname) \
>  	WARNING(nm, check_is_cell, (propname))
> @@ -509,6 +539,7 @@ static cell_t check_phandle_prop(struct check *c, struct dt_info *dti,
>  		 * we treat it as having no phandle data for now. */
>  		return 0;
>  	}
> +	marker_add(&prop->val.markers, TYPE_UINT32, 0);
>  
>  	phandle = propval_cell(prop);
>  
> @@ -748,7 +779,7 @@ static void check_reg_format(struct check *c, struct dt_info *dti,
>  			     struct node *node)
>  {
>  	struct property *prop;
> -	int addr_cells, size_cells, entrylen;
> +	int addr_cells, size_cells, entrylen, offset;
>  
>  	prop = get_property(node, "reg");
>  	if (!prop)
> @@ -766,10 +797,15 @@ static void check_reg_format(struct check *c, struct dt_info *dti,
>  	size_cells = node_size_cells(node->parent);
>  	entrylen = (addr_cells + size_cells) * sizeof(cell_t);
>  
> -	if (!entrylen || (prop->val.len % entrylen) != 0)
> +	if (!entrylen || (prop->val.len % entrylen) != 0) {
>  		FAIL_PROP(c, dti, node, prop, "property has invalid length (%d bytes) "
>  			  "(#address-cells == %d, #size-cells == %d)",
>  			  prop->val.len, addr_cells, size_cells);
> +		return;
> +	}
> +
> +	for (offset = 0; offset < prop->val.len; offset += entrylen)
> +		marker_add(&prop->val.markers, TYPE_UINT32, offset);

This doesn't seem quite right.  A 'reg' property could definitely be
u64s rather than u32s (amongst other possibilities, but u64 is the
most likely).  The user can even indicate that using /bits/ 64, but
this will overrule that.

>  }
>  WARNING(reg_format, check_reg_format, NULL, &addr_size_cells);
>  
> @@ -777,7 +813,7 @@ static void check_ranges_format(struct check *c, struct dt_info *dti,
>  				struct node *node)
>  {
>  	struct property *prop;
> -	int c_addr_cells, p_addr_cells, c_size_cells, p_size_cells, entrylen;
> +	int c_addr_cells, p_addr_cells, c_size_cells, p_size_cells, entrylen, offset;
>  	const char *ranges = c->data;
>  
>  	prop = get_property(node, ranges);
> @@ -813,6 +849,9 @@ static void check_ranges_format(struct check *c, struct dt_info *dti,
>  			  "#size-cells == %d)", ranges, prop->val.len,
>  			  p_addr_cells, c_addr_cells, c_size_cells);
>  	}
> +
> +	for (offset = 0; offset < prop->val.len; offset += entrylen)
> +		marker_add(&prop->val.markers, TYPE_UINT32, offset);

Same thing for ranges.

>  }
>  WARNING(ranges_format, check_ranges_format, "ranges", &addr_size_cells);
>  WARNING(dma_ranges_format, check_ranges_format, "dma-ranges", &addr_size_cells);
> @@ -1408,19 +1447,6 @@ static void check_property_phandle_args(struct check *c,
>  			continue;
>  		}
>  
> -		/* If we have markers, verify the current cell is a phandle */
> -		if (prop->val.markers) {
> -			struct marker *m = prop->val.markers;
> -			for_each_marker_of_type(m, REF_PHANDLE) {
> -				if (m->offset == (cell * sizeof(cell_t)))
> -					break;
> -			}
> -			if (!m)
> -				FAIL_PROP(c, dti, node, prop,
> -					  "cell %d is not a phandle reference",
> -					  cell);
> -		}
> -
>  		provider_node = get_node_by_phandle(root, phandle);
>  		if (!provider_node) {
>  			FAIL_PROP(c, dti, node, prop,
> @@ -1447,6 +1473,9 @@ static void check_property_phandle_args(struct check *c,
>  				  "property size (%d) too small for cell size %d",
>  				  prop->val.len, cellsize);
>  		}
> +
> +		marker_add(&prop->val.markers, REF_PHANDLE, cell * sizeof(cell_t));

This is definitely broken.  It's safe enough to add TYPE_ markers, but
REF_PHANDLE requires a label or path to be valid, which you don't add,
and have no way of deducing.  I'm kind of surprised you didn't cause a
crash in the later code that fixes up references with this.

> +		marker_add(&prop->val.markers, TYPE_UINT32, cell * sizeof(cell_t));
>  	}
>  }
>  
> @@ -1588,7 +1617,7 @@ static void check_interrupts_property(struct check *c,
>  	struct node *root = dti->dt;
>  	struct node *irq_node = NULL, *parent = node;
>  	struct property *irq_prop, *prop = NULL;
> -	int irq_cells, phandle;
> +	int irq_cells, phandle, offset;
>  
>  	irq_prop = get_property(node, "interrupts");
>  	if (!irq_prop)
> @@ -1625,6 +1654,8 @@ static void check_interrupts_property(struct check *c,
>  				FAIL(c, dti, irq_node,
>  				     "Missing interrupt-controller or interrupt-map property");
>  
> +			marker_add(&prop->val.markers, TYPE_UINT32, 0);
> +			marker_add(&prop->val.markers, REF_PHANDLE, 0);

Ditto.

>  			break;
>  		}
>  
> @@ -1647,7 +1678,11 @@ static void check_interrupts_property(struct check *c,
>  		FAIL_PROP(c, dti, node, prop,
>  			  "size is (%d), expected multiple of %d",
>  			  irq_prop->val.len, (int)(irq_cells * sizeof(cell_t)));
> +		return;
>  	}
> +
> +	for (offset = 0; offset < irq_prop->val.len; offset += irq_cells * sizeof(cell_t))
> +		marker_add(&irq_prop->val.markers, TYPE_UINT32, offset);

Same problem as for reg and ranges.

>  }
>  WARNING(interrupts_property, check_interrupts_property, &phandle_references);
>  
> @@ -1771,6 +1806,9 @@ static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti,
>  	if (!node)
>  		FAIL_PROP(c, dti, endpoint, prop, "graph phandle is not valid");
>  
> +	marker_add(&prop->val.markers, TYPE_UINT32, 0);
> +	marker_add(&prop->val.markers, REF_PHANDLE, 0);
> +

And same problem with REF_PHANDLE.

>  	return 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: 833 bytes --]

Re: [PATCH 4/5] checks: Add markers on known properties

[Rob Herring]

On Mon, Jun 7, 2021 at 9:25 PM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Tue, May 25, 2021 at 08:03:34PM -0500, Rob Herring wrote:
> > For properties we already have checks for, we know the type and how to
> > parse them. Use this to add type and phandle markers so we have them when
> > the source did not (e.g. dtb format).
> >
> > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> >  checks.c | 74 ++++++++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 56 insertions(+), 18 deletions(-)


> > @@ -766,10 +797,15 @@ static void check_reg_format(struct check *c, struct dt_info *dti,
> >       size_cells = node_size_cells(node->parent);
> >       entrylen = (addr_cells + size_cells) * sizeof(cell_t);
> >
> > -     if (!entrylen || (prop->val.len % entrylen) != 0)
> > +     if (!entrylen || (prop->val.len % entrylen) != 0) {
> >               FAIL_PROP(c, dti, node, prop, "property has invalid length (%d bytes) "
> >                         "(#address-cells == %d, #size-cells == %d)",
> >                         prop->val.len, addr_cells, size_cells);
> > +             return;
> > +     }
> > +
> > +     for (offset = 0; offset < prop->val.len; offset += entrylen)
> > +             marker_add(&prop->val.markers, TYPE_UINT32, offset);
>
> This doesn't seem quite right.  A 'reg' property could definitely be
> u64s rather than u32s (amongst other possibilities, but u64 is the
> most likely).  The user can even indicate that using /bits/ 64, but
> this will overrule that.

Ignoring malformed sizes, it can only be u32 unless the input is .dts
and using /bits/ or []. I'll make marker_add fail if there's any type
marker rather than just a matching marker.


> >  }
> >  WARNING(ranges_format, check_ranges_format, "ranges", &addr_size_cells);
> >  WARNING(dma_ranges_format, check_ranges_format, "dma-ranges", &addr_size_cells);
> > @@ -1408,19 +1447,6 @@ static void check_property_phandle_args(struct check *c,
> >                       continue;
> >               }
> >
> > -             /* If we have markers, verify the current cell is a phandle */
> > -             if (prop->val.markers) {
> > -                     struct marker *m = prop->val.markers;
> > -                     for_each_marker_of_type(m, REF_PHANDLE) {
> > -                             if (m->offset == (cell * sizeof(cell_t)))
> > -                                     break;
> > -                     }
> > -                     if (!m)
> > -                             FAIL_PROP(c, dti, node, prop,
> > -                                       "cell %d is not a phandle reference",
> > -                                       cell);
> > -             }
> > -
> >               provider_node = get_node_by_phandle(root, phandle);
> >               if (!provider_node) {
> >                       FAIL_PROP(c, dti, node, prop,
> > @@ -1447,6 +1473,9 @@ static void check_property_phandle_args(struct check *c,
> >                                 "property size (%d) too small for cell size %d",
> >                                 prop->val.len, cellsize);
> >               }
> > +
> > +             marker_add(&prop->val.markers, REF_PHANDLE, cell * sizeof(cell_t));
>
> This is definitely broken.  It's safe enough to add TYPE_ markers, but
> REF_PHANDLE requires a label or path to be valid, which you don't add,
> and have no way of deducing.  I'm kind of surprised you didn't cause a
> crash in the later code that fixes up references with this.

Didn't see any failures, so maybe the fixup runs first? I'll look at
adding the path or perhaps we can loosen this requirement?

Rob

Re: [PATCH 4/5] checks: Add markers on known properties

[David Gibson]

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

On Tue, Jun 08, 2021 at 07:49:20AM -0500, Rob Herring wrote:
> On Mon, Jun 7, 2021 at 9:25 PM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Tue, May 25, 2021 at 08:03:34PM -0500, Rob Herring wrote:
> > > For properties we already have checks for, we know the type and how to
> > > parse them. Use this to add type and phandle markers so we have them when
> > > the source did not (e.g. dtb format).
> > >
> > > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > ---
> > >  checks.c | 74 ++++++++++++++++++++++++++++++++++++++++++--------------
> > >  1 file changed, 56 insertions(+), 18 deletions(-)
> 
> 
> > > @@ -766,10 +797,15 @@ static void check_reg_format(struct check *c, struct dt_info *dti,
> > >       size_cells = node_size_cells(node->parent);
> > >       entrylen = (addr_cells + size_cells) * sizeof(cell_t);
> > >
> > > -     if (!entrylen || (prop->val.len % entrylen) != 0)
> > > +     if (!entrylen || (prop->val.len % entrylen) != 0) {
> > >               FAIL_PROP(c, dti, node, prop, "property has invalid length (%d bytes) "
> > >                         "(#address-cells == %d, #size-cells == %d)",
> > >                         prop->val.len, addr_cells, size_cells);
> > > +             return;
> > > +     }
> > > +
> > > +     for (offset = 0; offset < prop->val.len; offset += entrylen)
> > > +             marker_add(&prop->val.markers, TYPE_UINT32, offset);
> >
> > This doesn't seem quite right.  A 'reg' property could definitely be
> > u64s rather than u32s (amongst other possibilities, but u64 is the
> > most likely).  The user can even indicate that using /bits/ 64, but
> > this will overrule that.
> 
> Ignoring malformed sizes, it can only be u32 unless the input is .dts
> and using /bits/ or [].

Yes.. and using /bits/ is exactly the case I'm talking about.

> I'll make marker_add fail if there's any type
> marker rather than just a matching marker.
> 
> 
> > >  }
> > >  WARNING(ranges_format, check_ranges_format, "ranges", &addr_size_cells);
> > >  WARNING(dma_ranges_format, check_ranges_format, "dma-ranges", &addr_size_cells);
> > > @@ -1408,19 +1447,6 @@ static void check_property_phandle_args(struct check *c,
> > >                       continue;
> > >               }
> > >
> > > -             /* If we have markers, verify the current cell is a phandle */
> > > -             if (prop->val.markers) {
> > > -                     struct marker *m = prop->val.markers;
> > > -                     for_each_marker_of_type(m, REF_PHANDLE) {
> > > -                             if (m->offset == (cell * sizeof(cell_t)))
> > > -                                     break;
> > > -                     }
> > > -                     if (!m)
> > > -                             FAIL_PROP(c, dti, node, prop,
> > > -                                       "cell %d is not a phandle reference",
> > > -                                       cell);
> > > -             }
> > > -
> > >               provider_node = get_node_by_phandle(root, phandle);
> > >               if (!provider_node) {
> > >                       FAIL_PROP(c, dti, node, prop,
> > > @@ -1447,6 +1473,9 @@ static void check_property_phandle_args(struct check *c,
> > >                                 "property size (%d) too small for cell size %d",
> > >                                 prop->val.len, cellsize);
> > >               }
> > > +
> > > +             marker_add(&prop->val.markers, REF_PHANDLE, cell * sizeof(cell_t));
> >
> > This is definitely broken.  It's safe enough to add TYPE_ markers, but
> > REF_PHANDLE requires a label or path to be valid, which you don't add,
> > and have no way of deducing.  I'm kind of surprised you didn't cause a
> > crash in the later code that fixes up references with this.
> 
> Didn't see any failures, so maybe the fixup runs first? I'll look at
> adding the path or perhaps we can loosen this requirement?

Neither of those seems like the right approach.  REF_PHANDLE *means* a
place to replace a phandle, we're just re-using that to indicate that
we have a phandle here.  If you want to know it's a phandle without
also having a reference to fill in there, then we should add a
TYPE_PHANDLE marker.

-- 
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: 833 bytes --]

Re: [PATCH 4/5] checks: Add markers on known properties

[Rob Herring]

On Tue, Jun 15, 2021 at 12:26 AM David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Tue, Jun 08, 2021 at 07:49:20AM -0500, Rob Herring wrote:
> > On Mon, Jun 7, 2021 at 9:25 PM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > >
> > > On Tue, May 25, 2021 at 08:03:34PM -0500, Rob Herring wrote:
> > > > For properties we already have checks for, we know the type and how to
> > > > parse them. Use this to add type and phandle markers so we have them when
> > > > the source did not (e.g. dtb format).
> > > >
> > > > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > > ---
> > > >  checks.c | 74 ++++++++++++++++++++++++++++++++++++++++++--------------
> > > >  1 file changed, 56 insertions(+), 18 deletions(-)
> >
> >
> > > > @@ -766,10 +797,15 @@ static void check_reg_format(struct check *c, struct dt_info *dti,
> > > >       size_cells = node_size_cells(node->parent);
> > > >       entrylen = (addr_cells + size_cells) * sizeof(cell_t);
> > > >
> > > > -     if (!entrylen || (prop->val.len % entrylen) != 0)
> > > > +     if (!entrylen || (prop->val.len % entrylen) != 0) {
> > > >               FAIL_PROP(c, dti, node, prop, "property has invalid length (%d bytes) "
> > > >                         "(#address-cells == %d, #size-cells == %d)",
> > > >                         prop->val.len, addr_cells, size_cells);
> > > > +             return;
> > > > +     }
> > > > +
> > > > +     for (offset = 0; offset < prop->val.len; offset += entrylen)
> > > > +             marker_add(&prop->val.markers, TYPE_UINT32, offset);
> > >
> > > This doesn't seem quite right.  A 'reg' property could definitely be
> > > u64s rather than u32s (amongst other possibilities, but u64 is the
> > > most likely).  The user can even indicate that using /bits/ 64, but
> > > this will overrule that.
> >
> > Ignoring malformed sizes, it can only be u32 unless the input is .dts
> > and using /bits/ or [].
>
> Yes.. and using /bits/ is exactly the case I'm talking about.
>
> > I'll make marker_add fail if there's any type
> > marker rather than just a matching marker.
> >
> >
> > > >  }
> > > >  WARNING(ranges_format, check_ranges_format, "ranges", &addr_size_cells);
> > > >  WARNING(dma_ranges_format, check_ranges_format, "dma-ranges", &addr_size_cells);
> > > > @@ -1408,19 +1447,6 @@ static void check_property_phandle_args(struct check *c,
> > > >                       continue;
> > > >               }
> > > >
> > > > -             /* If we have markers, verify the current cell is a phandle */
> > > > -             if (prop->val.markers) {
> > > > -                     struct marker *m = prop->val.markers;
> > > > -                     for_each_marker_of_type(m, REF_PHANDLE) {
> > > > -                             if (m->offset == (cell * sizeof(cell_t)))
> > > > -                                     break;
> > > > -                     }
> > > > -                     if (!m)
> > > > -                             FAIL_PROP(c, dti, node, prop,
> > > > -                                       "cell %d is not a phandle reference",
> > > > -                                       cell);
> > > > -             }
> > > > -
> > > >               provider_node = get_node_by_phandle(root, phandle);
> > > >               if (!provider_node) {
> > > >                       FAIL_PROP(c, dti, node, prop,
> > > > @@ -1447,6 +1473,9 @@ static void check_property_phandle_args(struct check *c,
> > > >                                 "property size (%d) too small for cell size %d",
> > > >                                 prop->val.len, cellsize);
> > > >               }
> > > > +
> > > > +             marker_add(&prop->val.markers, REF_PHANDLE, cell * sizeof(cell_t));
> > >
> > > This is definitely broken.  It's safe enough to add TYPE_ markers, but
> > > REF_PHANDLE requires a label or path to be valid, which you don't add,
> > > and have no way of deducing.  I'm kind of surprised you didn't cause a
> > > crash in the later code that fixes up references with this.
> >
> > Didn't see any failures, so maybe the fixup runs first? I'll look at
> > adding the path or perhaps we can loosen this requirement?
>
> Neither of those seems like the right approach.  REF_PHANDLE *means* a
> place to replace a phandle,

What does it mean after we've replaced a phandle? We should be
removing the marker when that is done if it only means that. But
adding more lifetime to markers seems fragile as currently they are
add only.

> we're just re-using that to indicate that
> we have a phandle here.  If you want to know it's a phandle without
> also having a reference to fill in there, then we should add a
> TYPE_PHANDLE marker.

Well, look at v2 as I went the adding the reference route. That also
allows the dts output to have references.

I don't think TYPE_PHANDLE is right as we already have a type with
TYPE_UINT32. The phandle is an extra annotation. So we'd have to treat
it special from TYPE_UINT* such that has_data_type_information() is
false for TYPE_PHANDLE. Doable, but that blurs the current
distinction.

Rob

[PATCH 5/5] dtc: Drop dts source restriction for yaml output

[Rob Herring]

YAML output was restricted to dts input as there are some dependencies
on source annotations which get lost with other input formats. With the
addition of markers by the checks, YAML output from dtb format becomes
more useful.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 dtc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/dtc.c b/dtc.c
index 3962d3f4b178..e3decb2f4229 100644
--- a/dtc.c
+++ b/dtc.c
@@ -353,8 +353,6 @@ int main(int argc, char *argv[])
 		dt_to_source(outf, dti);
 #ifndef NO_YAML
 	} else if (streq(outform, "yaml")) {
-		if (!streq(inform, "dts"))
-			die("YAML output format requires dts input format\n");
 		dt_to_yaml(outf, dti);
 #endif
 	} else if (streq(outform, "dtb")) {
-- 
2.27.0