[PATCH v2 0/3] Improve output type formatting

[PATCH v2 0/3] 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 (3):
  checks: Add markers on known properties
  dtc: Drop dts source restriction for yaml output
  treesource: Maintain phandle label/path on output

 checks.c     | 102 +++++++++++++++++++++++++++++++++++++++++----------
 dtc.c        |   2 -
 treesource.c |  23 ++++++++++--
 3 files changed, 101 insertions(+), 26 deletions(-)

-- 
2.27.0

[PATCH v2 1/3] 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>
---
v2:
 - Set marker.ref on phandle markers
 - Avoid adding markers if there's any conflicting type markers.
---
 checks.c | 102 ++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 82 insertions(+), 20 deletions(-)

diff --git a/checks.c b/checks.c
index e6c7c3eeacac..0f51b9111be1 100644
--- a/checks.c
+++ b/checks.c
@@ -58,6 +58,38 @@ struct check {
 #define CHECK(nm_, fn_, d_, ...) \
 	CHECK_ENTRY(nm_, fn_, d_, false, false, __VA_ARGS__)
 
+static struct marker *marker_add(struct marker **list, enum markertype type,
+				 unsigned int offset)
+{
+	struct marker *m = *list;
+
+	/* Check if we already have a different type or a type marker at the offset*/
+	for_each_marker_of_type(m, type) {
+		if ((m->type >= TYPE_UINT8) && (m->type != type))
+			return NULL;
+		if (m->type == type && m->offset == offset)
+			return NULL;
+	}
+
+	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;
+
+	return m;
+}
+
 static inline void  PRINTF(5, 6) check_msg(struct check *c, struct dt_info *dti,
 					   struct node *node,
 					   struct property *prop,
@@ -260,8 +292,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))
@@ -517,6 +553,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);
 
@@ -756,7 +793,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)
@@ -774,10 +811,16 @@ 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 (!is_multiple_of(prop->val.len, entrylen))
+	if (!is_multiple_of(prop->val.len, entrylen)) {
 		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)
+		if (!marker_add(&prop->val.markers, TYPE_UINT32, offset))
+			break;
 }
 WARNING(reg_format, check_reg_format, NULL, &addr_size_cells);
 
@@ -785,7 +828,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);
@@ -821,6 +864,10 @@ 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)
+		if (!marker_add(&prop->val.markers, TYPE_UINT32, offset))
+			break;
 }
 WARNING(ranges_format, check_ranges_format, "ranges", &addr_size_cells);
 WARNING(dma_ranges_format, check_ranges_format, "dma-ranges", &addr_size_cells);
@@ -1400,6 +1447,7 @@ static void check_property_phandle_args(struct check *c,
 	for (cell = 0; cell < prop->val.len / sizeof(cell_t); cell += cellsize + 1) {
 		struct node *provider_node;
 		struct property *cellprop;
+		struct marker *m;
 		int phandle;
 
 		phandle = propval_cell_n(prop, cell);
@@ -1416,19 +1464,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,
@@ -1454,7 +1489,13 @@ static void check_property_phandle_args(struct check *c,
 			FAIL_PROP(c, dti, node, prop,
 				  "property size (%d) too small for cell size %d",
 				  prop->val.len, cellsize);
+			break;
 		}
+
+		marker_add(&prop->val.markers, TYPE_UINT32, cell * sizeof(cell_t));
+		m = marker_add(&prop->val.markers, REF_PHANDLE, cell * sizeof(cell_t));
+		if (m)
+			m->ref = provider_node->fullpath;
 	}
 }
 
@@ -1596,7 +1637,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)
@@ -1614,6 +1655,8 @@ static void check_interrupts_property(struct check *c,
 
 		prop = get_property(parent, "interrupt-parent");
 		if (prop) {
+			struct marker *m;
+
 			phandle = propval_cell(prop);
 			if ((phandle == 0) || (phandle == -1)) {
 				/* Give up if this is an overlay with
@@ -1629,10 +1672,16 @@ static void check_interrupts_property(struct check *c,
 				FAIL_PROP(c, dti, parent, prop, "Bad phandle");
 				return;
 			}
-			if (!node_is_interrupt_provider(irq_node))
+			if (!node_is_interrupt_provider(irq_node)) {
 				FAIL(c, dti, irq_node,
 				     "Missing interrupt-controller or interrupt-map property");
+				return;
+			}
 
+			marker_add(&prop->val.markers, TYPE_UINT32, 0);
+			m = marker_add(&prop->val.markers, REF_PHANDLE, 0);
+			if (m)
+				m->ref = irq_node->fullpath;
 			break;
 		}
 
@@ -1655,7 +1704,12 @@ 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))
+		if (!marker_add(&irq_prop->val.markers, TYPE_UINT32, offset))
+			break;
 }
 WARNING(interrupts_property, check_interrupts_property, &phandle_references);
 
@@ -1763,6 +1817,7 @@ static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti,
 					struct node *endpoint)
 {
 	int phandle;
+	struct marker *m;
 	struct node *node;
 	struct property *prop;
 
@@ -1776,8 +1831,15 @@ static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti,
 		return NULL;
 
 	node = get_node_by_phandle(dti->dt, phandle);
-	if (!node)
+	if (!node) {
 		FAIL_PROP(c, dti, endpoint, prop, "graph phandle is not valid");
+		return NULL;
+	}
+
+	marker_add(&prop->val.markers, TYPE_UINT32, 0);
+	m = marker_add(&prop->val.markers, REF_PHANDLE, 0);
+	if (m)
+		m->ref = node->fullpath;
 
 	return node;
 }
-- 
2.27.0

[PATCH v2 2/3] 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

[PATCH v2 3/3] treesource: Maintain phandle label/path on output

[Rob Herring]

The dts output will just output phandle integer values, but often the
necessary markers are present with path or label references. Improve the
output and maintain phandle label or path references when present in dts
output.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
v2:
 - New patch
---
 treesource.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/treesource.c b/treesource.c
index 061ba8c9c5e8..36c5c1ec683e 100644
--- a/treesource.c
+++ b/treesource.c
@@ -229,6 +229,7 @@ static void write_propval(FILE *f, struct property *prop)
 		size_t chunk_len = (m->next ? m->next->offset : len) - m->offset;
 		size_t data_len = type_marker_length(m) ? : len - m->offset;
 		const char *p = &prop->val.val[m->offset];
+		struct marker *m_phandle;
 
 		if (has_data_type_information(m)) {
 			emit_type = m->type;
@@ -238,17 +239,31 @@ static void write_propval(FILE *f, struct property *prop)
 		else if (m->offset)
 			fputc(' ', f);
 
-		if (emit_type == TYPE_NONE) {
-			assert(chunk_len == 0);
+		if (emit_type == TYPE_NONE || chunk_len == 0)
 			continue;
-		}
 
 		switch(emit_type) {
 		case TYPE_UINT16:
 			write_propval_int(f, p, chunk_len, 2);
 			break;
 		case TYPE_UINT32:
-			write_propval_int(f, p, chunk_len, 4);
+			m_phandle = prop->val.markers;
+			for_each_marker_of_type(m_phandle, REF_PHANDLE)
+				if (m->offset == m_phandle->offset)
+					break;
+
+			if (m_phandle) {
+				if (m_phandle->ref[0] == '/')
+					fprintf(f, "&{%s}", m_phandle->ref);
+				else
+					fprintf(f, "&%s", m_phandle->ref);
+				if (chunk_len > 4) {
+					fputc(' ', f);
+					write_propval_int(f, p + 4, chunk_len - 4, 4);
+				}
+			} else {
+				write_propval_int(f, p, chunk_len, 4);
+			}
 			break;
 		case TYPE_UINT64:
 			write_propval_int(f, p, chunk_len, 8);
-- 
2.27.0

Re: [PATCH v2 1/3] checks: Add markers on known properties

[David Gibson]

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

On Tue, Jun 08, 2021 at 03:46:24PM -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>
> ---
> v2:
>  - Set marker.ref on phandle markers
>  - Avoid adding markers if there's any conflicting type markers.
> ---
>  checks.c | 102 ++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 82 insertions(+), 20 deletions(-)
> 
> diff --git a/checks.c b/checks.c
> index e6c7c3eeacac..0f51b9111be1 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -58,6 +58,38 @@ struct check {
>  #define CHECK(nm_, fn_, d_, ...) \
>  	CHECK_ENTRY(nm_, fn_, d_, false, false, __VA_ARGS__)
>  
> +static struct marker *marker_add(struct marker **list, enum markertype type,
> +				 unsigned int offset)

Now that this is only conditionally adding markers, it needs a
different name.  Maybe "add_type_annotation".

> +{
> +	struct marker *m = *list;

Since this is strictly about type annotations (and you don't have
parameters for the necessary ref parameter for other things), an
assert() that the given type is a TYPE_* wouldn't go astray.

> +
> +	/* Check if we already have a different type or a type marker at the offset*/
> +	for_each_marker_of_type(m, type) {
> +		if ((m->type >= TYPE_UINT8) && (m->type != type))

I'm assuming the >= TYPE_UINT8 is about checking that this is a type
marker rather than a ref marker.  Adding a helper function for that
would probably be a good idea.  Putting it inline in dtc.h would make
it less likely that we break it if we ever add new marker types in
future.

Checking for m->type != type doesn't seem useful to me.  If m->type ==
type then either it's at the same offset, in which case there's
nothing to do, or it's at a different offset in which case... well,
it's not totally clear what's going on, but it's probably safest to
leave it alone.

> +			return NULL;
> +		if (m->type == type && m->offset == offset)
> +			return NULL;
> +	}
> +
> +	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;
> +
> +	return m;
> +}
> +
>  static inline void  PRINTF(5, 6) check_msg(struct check *c, struct dt_info *dti,
>  					   struct node *node,
>  					   struct property *prop,
> @@ -260,8 +292,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))
> @@ -517,6 +553,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);
>  
> @@ -756,7 +793,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)
> @@ -774,10 +811,16 @@ 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 (!is_multiple_of(prop->val.len, entrylen))
> +	if (!is_multiple_of(prop->val.len, entrylen)) {
>  		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)
> +		if (!marker_add(&prop->val.markers, TYPE_UINT32, offset))
> +			break;

I don't see any point to adding multiple markers.  Each type marker
indicates the type until the next marker, so just adding one has the
same effect.

>  }
>  WARNING(reg_format, check_reg_format, NULL, &addr_size_cells);
>  
> @@ -785,7 +828,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);
> @@ -821,6 +864,10 @@ 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)
> +		if (!marker_add(&prop->val.markers, TYPE_UINT32, offset))
> +			break;
>  }
>  WARNING(ranges_format, check_ranges_format, "ranges", &addr_size_cells);
>  WARNING(dma_ranges_format, check_ranges_format, "dma-ranges", &addr_size_cells);
> @@ -1400,6 +1447,7 @@ static void check_property_phandle_args(struct check *c,
>  	for (cell = 0; cell < prop->val.len / sizeof(cell_t); cell += cellsize + 1) {
>  		struct node *provider_node;
>  		struct property *cellprop;
> +		struct marker *m;
>  		int phandle;
>  
>  		phandle = propval_cell_n(prop, cell);
> @@ -1416,19 +1464,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);
> -		}
> -

Why are you removing this part of the check?

>  		provider_node = get_node_by_phandle(root, phandle);
>  		if (!provider_node) {
>  			FAIL_PROP(c, dti, node, prop,
> @@ -1454,7 +1489,13 @@ static void check_property_phandle_args(struct check *c,
>  			FAIL_PROP(c, dti, node, prop,
>  				  "property size (%d) too small for cell size %d",
>  				  prop->val.len, cellsize);
> +			break;
>  		}
> +
> +		marker_add(&prop->val.markers, TYPE_UINT32, cell * sizeof(cell_t));
> +		m = marker_add(&prop->val.markers, REF_PHANDLE, cell * sizeof(cell_t));
> +		if (m)
> +			m->ref = provider_node->fullpath;
>  	}
>  }
>  
> @@ -1596,7 +1637,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)
> @@ -1614,6 +1655,8 @@ static void check_interrupts_property(struct check *c,
>  
>  		prop = get_property(parent, "interrupt-parent");
>  		if (prop) {
> +			struct marker *m;
> +
>  			phandle = propval_cell(prop);
>  			if ((phandle == 0) || (phandle == -1)) {
>  				/* Give up if this is an overlay with
> @@ -1629,10 +1672,16 @@ static void check_interrupts_property(struct check *c,
>  				FAIL_PROP(c, dti, parent, prop, "Bad phandle");
>  				return;
>  			}
> -			if (!node_is_interrupt_provider(irq_node))
> +			if (!node_is_interrupt_provider(irq_node)) {
>  				FAIL(c, dti, irq_node,
>  				     "Missing interrupt-controller or interrupt-map property");
> +				return;
> +			}
>  
> +			marker_add(&prop->val.markers, TYPE_UINT32, 0);
> +			m = marker_add(&prop->val.markers, REF_PHANDLE, 0);
> +			if (m)
> +				m->ref = irq_node->fullpath;
>  			break;
>  		}
>  
> @@ -1655,7 +1704,12 @@ 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))
> +		if (!marker_add(&irq_prop->val.markers, TYPE_UINT32, offset))
> +			break;

Again, I don't see any point to adding multiple markers.

>  }
>  WARNING(interrupts_property, check_interrupts_property, &phandle_references);
>  
> @@ -1763,6 +1817,7 @@ static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti,
>  					struct node *endpoint)
>  {
>  	int phandle;
> +	struct marker *m;
>  	struct node *node;
>  	struct property *prop;
>  
> @@ -1776,8 +1831,15 @@ static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti,
>  		return NULL;
>  
>  	node = get_node_by_phandle(dti->dt, phandle);
> -	if (!node)
> +	if (!node) {
>  		FAIL_PROP(c, dti, endpoint, prop, "graph phandle is not valid");
> +		return NULL;
> +	}
> +
> +	marker_add(&prop->val.markers, TYPE_UINT32, 0);
> +	m = marker_add(&prop->val.markers, REF_PHANDLE, 0);
> +	if (m)
> +		m->ref = node->fullpath;
>  
>  	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 v2 1/3] checks: Add markers on known properties

[Rob Herring]

On Sat, Jun 19, 2021 at 3:22 AM David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Tue, Jun 08, 2021 at 03:46:24PM -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>
> > ---
> > v2:
> >  - Set marker.ref on phandle markers
> >  - Avoid adding markers if there's any conflicting type markers.
> > ---
> >  checks.c | 102 ++++++++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 82 insertions(+), 20 deletions(-)
> >
> > diff --git a/checks.c b/checks.c
> > index e6c7c3eeacac..0f51b9111be1 100644
> > --- a/checks.c
> > +++ b/checks.c
> > @@ -58,6 +58,38 @@ struct check {
> >  #define CHECK(nm_, fn_, d_, ...) \
> >       CHECK_ENTRY(nm_, fn_, d_, false, false, __VA_ARGS__)
> >
> > +static struct marker *marker_add(struct marker **list, enum markertype type,
> > +                              unsigned int offset)
>
> Now that this is only conditionally adding markers, it needs a
> different name.  Maybe "add_type_annotation".

marker_add_type_annotation to be consistent that marker_* functions
work on markers?

>
> > +{
> > +     struct marker *m = *list;
>
> Since this is strictly about type annotations (and you don't have
> parameters for the necessary ref parameter for other things), an
> assert() that the given type is a TYPE_* wouldn't go astray.
>
> > +
> > +     /* Check if we already have a different type or a type marker at the offset*/
> > +     for_each_marker_of_type(m, type) {
> > +             if ((m->type >= TYPE_UINT8) && (m->type != type))
>
> I'm assuming the >= TYPE_UINT8 is about checking that this is a type
> marker rather than a ref marker.  Adding a helper function for that
> would probably be a good idea.  Putting it inline in dtc.h would make
> it less likely that we break it if we ever add new marker types in
> future.

It's checking that we don't have markers of a different type within
the property. While that is valid dts format, it's never the case for
any known (by checks.c) properties. It could be an assert() as it's
one of those 'should never happen'.

We already have another case of (m->type >= TYPE_UINT8), so I can add
an inline for that.

> Checking for m->type != type doesn't seem useful to me.  If m->type ==
> type then either it's at the same offset, in which case there's
> nothing to do, or it's at a different offset in which case... well,
> it's not totally clear what's going on, but it's probably safest to
> leave it alone.

Only the same type at a different offset is expected. More on that below.

>
> > +                     return NULL;
> > +             if (m->type == type && m->offset == offset)
> > +                     return NULL;
> > +     }
> > +
> > +     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;
> > +
> > +     return m;
> > +}
> > +
> >  static inline void  PRINTF(5, 6) check_msg(struct check *c, struct dt_info *dti,
> >                                          struct node *node,
> >                                          struct property *prop,
> > @@ -260,8 +292,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))
> > @@ -517,6 +553,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);
> >
> > @@ -756,7 +793,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)
> > @@ -774,10 +811,16 @@ 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 (!is_multiple_of(prop->val.len, entrylen))
> > +     if (!is_multiple_of(prop->val.len, entrylen)) {
> >               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)
> > +             if (!marker_add(&prop->val.markers, TYPE_UINT32, offset))
> > +                     break;
>
> I don't see any point to adding multiple markers.  Each type marker
> indicates the type until the next marker, so just adding one has the
> same effect.

Multiple markers is the whole point. This is to match the existing
behavior when we have:

reg = <baseA sizeA>, <baseB sizeB>;

In this case, there's a type marker for each <> entry. That is then
used by the output stages (dts and yaml). We already have a single
marker as the format guessing based on property length being a
multiple of 4 achieves that. Though, for that to work with the yaml
output we'd have to do (dtb -> dts -> yaml).

Currently, the output formatting is dependent on the input format.
This makes the output consistent in that brackets (via type markers)
delineate #*-cells boundaries.

> >  }
> >  WARNING(reg_format, check_reg_format, NULL, &addr_size_cells);
> >
> > @@ -785,7 +828,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);
> > @@ -821,6 +864,10 @@ 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)
> > +             if (!marker_add(&prop->val.markers, TYPE_UINT32, offset))
> > +                     break;
> >  }
> >  WARNING(ranges_format, check_ranges_format, "ranges", &addr_size_cells);
> >  WARNING(dma_ranges_format, check_ranges_format, "dma-ranges", &addr_size_cells);
> > @@ -1400,6 +1447,7 @@ static void check_property_phandle_args(struct check *c,
> >       for (cell = 0; cell < prop->val.len / sizeof(cell_t); cell += cellsize + 1) {
> >               struct node *provider_node;
> >               struct property *cellprop;
> > +             struct marker *m;
> >               int phandle;
> >
> >               phandle = propval_cell_n(prop, cell);
> > @@ -1416,19 +1464,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);
> > -             }
> > -
>
> Why are you removing this part of the check?

Well, it didn't really match up with adding markers. It was checking
if markers were correct, but now we're adding the markers if not
present. Plus the get_node_by_phandle() call after this is a better
check for being a valid phandle. I can make this a separate commit.

Rob

Re: [PATCH v2 1/3] checks: Add markers on known properties

[David Gibson]

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

On Mon, Jun 21, 2021 at 12:22:45PM -0600, Rob Herring wrote:
> On Sat, Jun 19, 2021 at 3:22 AM David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Tue, Jun 08, 2021 at 03:46:24PM -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>
> > > ---
> > > v2:
> > >  - Set marker.ref on phandle markers
> > >  - Avoid adding markers if there's any conflicting type markers.
> > > ---
> > >  checks.c | 102 ++++++++++++++++++++++++++++++++++++++++++++-----------
> > >  1 file changed, 82 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/checks.c b/checks.c
> > > index e6c7c3eeacac..0f51b9111be1 100644
> > > --- a/checks.c
> > > +++ b/checks.c
> > > @@ -58,6 +58,38 @@ struct check {
> > >  #define CHECK(nm_, fn_, d_, ...) \
> > >       CHECK_ENTRY(nm_, fn_, d_, false, false, __VA_ARGS__)
> > >
> > > +static struct marker *marker_add(struct marker **list, enum markertype type,
> > > +                              unsigned int offset)
> >
> > Now that this is only conditionally adding markers, it needs a
> > different name.  Maybe "add_type_annotation".
> 
> marker_add_type_annotation to be consistent that marker_* functions
> work on markers?

Sure, that's fine too.  It's a local function, so naming consistency
isn't as important as for globals.

> > > +{
> > > +     struct marker *m = *list;
> >
> > Since this is strictly about type annotations (and you don't have
> > parameters for the necessary ref parameter for other things), an
> > assert() that the given type is a TYPE_* wouldn't go astray.
> >
> > > +
> > > +     /* Check if we already have a different type or a type marker at the offset*/
> > > +     for_each_marker_of_type(m, type) {
> > > +             if ((m->type >= TYPE_UINT8) && (m->type != type))
> >
> > I'm assuming the >= TYPE_UINT8 is about checking that this is a type
> > marker rather than a ref marker.  Adding a helper function for that
> > would probably be a good idea.  Putting it inline in dtc.h would make
> > it less likely that we break it if we ever add new marker types in
> > future.
> 
> It's checking that we don't have markers of a different type within
> the property. While that is valid dts format, it's never the case for
> any known (by checks.c) properties.

That comment doesn't seem to quite match the thing above.

Besides, I don't think that's really true.  It would be entirely
sensible for 'reg' to have mixed u32 & u64 type to encode PCI
addresses.  Likewise it would make sense for 'ranges' to have mixed
u32 & u64 type if mapping between a 32-bit and 64-bit bus.

> It could be an assert() as it's
> one of those 'should never happen'.

An assert() should only be used if the problem must have been caused
by an error in the code.  Bad user input should never trigger an
assert().
> 
> We already have another case of (m->type >= TYPE_UINT8), so I can add
> an inline for that.

Thanks.

> 
> > Checking for m->type != type doesn't seem useful to me.  If m->type ==
> > type then either it's at the same offset, in which case there's
> > nothing to do, or it's at a different offset in which case... well,
> > it's not totally clear what's going on, but it's probably safest to
> > leave it alone.
> 
> Only the same type at a different offset is expected. More on that below.
> 
> >
> > > +                     return NULL;
> > > +             if (m->type == type && m->offset == offset)
> > > +                     return NULL;
> > > +     }
> > > +
> > > +     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;
> > > +
> > > +     return m;
> > > +}
> > > +
> > >  static inline void  PRINTF(5, 6) check_msg(struct check *c, struct dt_info *dti,
> > >                                          struct node *node,
> > >                                          struct property *prop,
> > > @@ -260,8 +292,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))
> > > @@ -517,6 +553,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);
> > >
> > > @@ -756,7 +793,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)
> > > @@ -774,10 +811,16 @@ 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 (!is_multiple_of(prop->val.len, entrylen))
> > > +     if (!is_multiple_of(prop->val.len, entrylen)) {
> > >               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)
> > > +             if (!marker_add(&prop->val.markers, TYPE_UINT32, offset))
> > > +                     break;
> >
> > I don't see any point to adding multiple markers.  Each type marker
> > indicates the type until the next marker, so just adding one has the
> > same effect.
> 
> Multiple markers is the whole point. This is to match the existing
> behavior when we have:
> 
> reg = <baseA sizeA>, <baseB sizeB>;

Urgh, because you're using the type markers for that broken YAML
conversion.  Fine, ok.

> In this case, there's a type marker for each <> entry. That is then
> used by the output stages (dts and yaml). We already have a single
> marker as the format guessing based on property length being a
> multiple of 4 achieves that. Though, for that to work with the yaml
> output we'd have to do (dtb -> dts -> yaml).
> 
> Currently, the output formatting is dependent on the input format.
> This makes the output consistent in that brackets (via type markers)
> delineate #*-cells boundaries.
> 
> > >  }
> > >  WARNING(reg_format, check_reg_format, NULL, &addr_size_cells);
> > >
> > > @@ -785,7 +828,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);
> > > @@ -821,6 +864,10 @@ 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)
> > > +             if (!marker_add(&prop->val.markers, TYPE_UINT32, offset))
> > > +                     break;
> > >  }
> > >  WARNING(ranges_format, check_ranges_format, "ranges", &addr_size_cells);
> > >  WARNING(dma_ranges_format, check_ranges_format, "dma-ranges", &addr_size_cells);
> > > @@ -1400,6 +1447,7 @@ static void check_property_phandle_args(struct check *c,
> > >       for (cell = 0; cell < prop->val.len / sizeof(cell_t); cell += cellsize + 1) {
> > >               struct node *provider_node;
> > >               struct property *cellprop;
> > > +             struct marker *m;
> > >               int phandle;
> > >
> > >               phandle = propval_cell_n(prop, cell);
> > > @@ -1416,19 +1464,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);
> > > -             }
> > > -
> >
> > Why are you removing this part of the check?
> 
> Well, it didn't really match up with adding markers. It was checking
> if markers were correct, but now we're adding the markers if not
> present. Plus the get_node_by_phandle() call after this is a better
> check for being a valid phandle. I can make this a separate commit.

Ok.

-- 
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 v2 1/3] checks: Add markers on known properties

[Rob Herring]

On Sun, Jul 11, 2021 at 8:39 PM David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Mon, Jun 21, 2021 at 12:22:45PM -0600, Rob Herring wrote:
> > On Sat, Jun 19, 2021 at 3:22 AM David Gibson
> > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > >
> > > On Tue, Jun 08, 2021 at 03:46:24PM -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>
> > > > ---
> > > > v2:
> > > >  - Set marker.ref on phandle markers
> > > >  - Avoid adding markers if there's any conflicting type markers.
> > > > ---
> > > >  checks.c | 102 ++++++++++++++++++++++++++++++++++++++++++++-----------
> > > >  1 file changed, 82 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/checks.c b/checks.c
> > > > index e6c7c3eeacac..0f51b9111be1 100644
> > > > --- a/checks.c
> > > > +++ b/checks.c
> > > > @@ -58,6 +58,38 @@ struct check {
> > > >  #define CHECK(nm_, fn_, d_, ...) \
> > > >       CHECK_ENTRY(nm_, fn_, d_, false, false, __VA_ARGS__)
> > > >
> > > > +static struct marker *marker_add(struct marker **list, enum markertype type,
> > > > +                              unsigned int offset)
> > >
> > > Now that this is only conditionally adding markers, it needs a
> > > different name.  Maybe "add_type_annotation".
> >
> > marker_add_type_annotation to be consistent that marker_* functions
> > work on markers?
>
> Sure, that's fine too.  It's a local function, so naming consistency
> isn't as important as for globals.
>
> > > > +{
> > > > +     struct marker *m = *list;
> > >
> > > Since this is strictly about type annotations (and you don't have
> > > parameters for the necessary ref parameter for other things), an
> > > assert() that the given type is a TYPE_* wouldn't go astray.
> > >
> > > > +
> > > > +     /* Check if we already have a different type or a type marker at the offset*/
> > > > +     for_each_marker_of_type(m, type) {
> > > > +             if ((m->type >= TYPE_UINT8) && (m->type != type))
> > >
> > > I'm assuming the >= TYPE_UINT8 is about checking that this is a type
> > > marker rather than a ref marker.  Adding a helper function for that
> > > would probably be a good idea.  Putting it inline in dtc.h would make
> > > it less likely that we break it if we ever add new marker types in
> > > future.
> >
> > It's checking that we don't have markers of a different type within
> > the property. While that is valid dts format, it's never the case for
> > any known (by checks.c) properties.
>
> That comment doesn't seem to quite match the thing above.

The comment applies to both 'if' conditions. I can make it 2 comments.

> Besides, I don't think that's really true.  It would be entirely
> sensible for 'reg' to have mixed u32 & u64 type to encode PCI
> addresses.  Likewise it would make sense for 'ranges' to have mixed
> u32 & u64 type if mapping between a 32-bit and 64-bit bus.

Yes, we could have all sorts of encoding. We could mix strings, u8,
u16, u32, and u64 too, but we don't because that would be insane given
next to zero type information is maintained.

But this isn't really about what encoding we could have for 'reg' as
if we have any type information already, then we do nothing. This is
only what is the encoding when there is no other information to go on
besides the property name. IMO, that should be bracketing each entry
which is all we're doing here. Maybe you disagree on bracketing, but
as a user shouldn't I be able to get the output I want? Debating it is
like debating tabs vs. spaces (BTW, dtc already decided I get tabs).
So maybe all this would be better in a linter, but we don't have one
and if I wrote one it would start with forking dtc. We could make this
all a command line option I suppose.

> > It could be an assert() as it's
> > one of those 'should never happen'.
>
> An assert() should only be used if the problem must have been caused
> by an error in the code.  Bad user input should never trigger an
> assert().

Maybe I misunderstood what you suggested an assert() for.


> > > > @@ -774,10 +811,16 @@ 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 (!is_multiple_of(prop->val.len, entrylen))
> > > > +     if (!is_multiple_of(prop->val.len, entrylen)) {
> > > >               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)
> > > > +             if (!marker_add(&prop->val.markers, TYPE_UINT32, offset))
> > > > +                     break;
> > >
> > > I don't see any point to adding multiple markers.  Each type marker
> > > indicates the type until the next marker, so just adding one has the
> > > same effect.
> >
> > Multiple markers is the whole point. This is to match the existing
> > behavior when we have:
> >
> > reg = <baseA sizeA>, <baseB sizeB>;
>
> Urgh, because you're using the type markers for that broken YAML
> conversion.  Fine, ok.

Uhh well, type markers were added in the first place to support YAML,
but they are used on the dts output as well which has changed it a lot
from what was output before. The YAML and dts code is just about the
same in terms of how the type information is used. If anything is
broken, it's the dts output guessing. Multiples of 4 bytes are always
UINT32 data?

Rob

Re: [PATCH v2 1/3] checks: Add markers on known properties

[David Gibson]

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

On Mon, Jul 12, 2021 at 04:15:43PM -0600, Rob Herring wrote:
> On Sun, Jul 11, 2021 at 8:39 PM David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Mon, Jun 21, 2021 at 12:22:45PM -0600, Rob Herring wrote:
> > > On Sat, Jun 19, 2021 at 3:22 AM David Gibson
> > > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > >
> > > > On Tue, Jun 08, 2021 at 03:46:24PM -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>
> > > > > ---
> > > > > v2:
> > > > >  - Set marker.ref on phandle markers
> > > > >  - Avoid adding markers if there's any conflicting type markers.
> > > > > ---
> > > > >  checks.c | 102 ++++++++++++++++++++++++++++++++++++++++++++-----------
> > > > >  1 file changed, 82 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/checks.c b/checks.c
> > > > > index e6c7c3eeacac..0f51b9111be1 100644
> > > > > --- a/checks.c
> > > > > +++ b/checks.c
> > > > > @@ -58,6 +58,38 @@ struct check {
> > > > >  #define CHECK(nm_, fn_, d_, ...) \
> > > > >       CHECK_ENTRY(nm_, fn_, d_, false, false, __VA_ARGS__)
> > > > >
> > > > > +static struct marker *marker_add(struct marker **list, enum markertype type,
> > > > > +                              unsigned int offset)
> > > >
> > > > Now that this is only conditionally adding markers, it needs a
> > > > different name.  Maybe "add_type_annotation".
> > >
> > > marker_add_type_annotation to be consistent that marker_* functions
> > > work on markers?
> >
> > Sure, that's fine too.  It's a local function, so naming consistency
> > isn't as important as for globals.
> >
> > > > > +{
> > > > > +     struct marker *m = *list;
> > > >
> > > > Since this is strictly about type annotations (and you don't have
> > > > parameters for the necessary ref parameter for other things), an
> > > > assert() that the given type is a TYPE_* wouldn't go astray.
> > > >
> > > > > +
> > > > > +     /* Check if we already have a different type or a type marker at the offset*/
> > > > > +     for_each_marker_of_type(m, type) {
> > > > > +             if ((m->type >= TYPE_UINT8) && (m->type != type))
> > > >
> > > > I'm assuming the >= TYPE_UINT8 is about checking that this is a type
> > > > marker rather than a ref marker.  Adding a helper function for that
> > > > would probably be a good idea.  Putting it inline in dtc.h would make
> > > > it less likely that we break it if we ever add new marker types in
> > > > future.
> > >
> > > It's checking that we don't have markers of a different type within
> > > the property. While that is valid dts format, it's never the case for
> > > any known (by checks.c) properties.
> >
> > That comment doesn't seem to quite match the thing above.
> 
> The comment applies to both 'if' conditions. I can make it 2 comments.

Sorry, I wasn't clear.  I didn't mean the code comment I meant your
remark in the thread "It's checking that...".  It didn't seem apropos
to my comment it's in reply to.

> > Besides, I don't think that's really true.  It would be entirely
> > sensible for 'reg' to have mixed u32 & u64 type to encode PCI
> > addresses.  Likewise it would make sense for 'ranges' to have mixed
> > u32 & u64 type if mapping between a 32-bit and 64-bit bus.
> 
> Yes, we could have all sorts of encoding. We could mix strings, u8,
> u16, u32, and u64 too, but we don't because that would be insane given
> next to zero type information is maintained.

Who's "we"?  My point is that you're interacting with user supplied
type information, and we can't control what the user supplies.

> But this isn't really about what encoding we could have for 'reg' as
> if we have any type information already, then we do nothing.

But.. that's *not* what this logic does.  It's quite explicitly more
complicated than "if there's any other type information, then do
nothing".

> This is
> only what is the encoding when there is no other information to go on
> besides the property name. IMO, that should be bracketing each entry
> which is all we're doing here. Maybe you disagree on bracketing, but
> as a user shouldn't I be able to get the output I want?

Yes... that's my point...

> Debating it is
> like debating tabs vs. spaces (BTW, dtc already decided I get tabs).
> So maybe all this would be better in a linter, but we don't have one
> and if I wrote one it would start with forking dtc. We could make this
> all a command line option I suppose.
> 
> > > It could be an assert() as it's
> > > one of those 'should never happen'.
> >
> > An assert() should only be used if the problem must have been caused
> > by an error in the code.  Bad user input should never trigger an
> > assert().
> 
> Maybe I misunderstood what you suggested an assert() for.

I didn't, you were the one suggesting an assert().

> > > > > @@ -774,10 +811,16 @@ 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 (!is_multiple_of(prop->val.len, entrylen))
> > > > > +     if (!is_multiple_of(prop->val.len, entrylen)) {
> > > > >               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)
> > > > > +             if (!marker_add(&prop->val.markers, TYPE_UINT32, offset))
> > > > > +                     break;
> > > >
> > > > I don't see any point to adding multiple markers.  Each type marker
> > > > indicates the type until the next marker, so just adding one has the
> > > > same effect.
> > >
> > > Multiple markers is the whole point. This is to match the existing
> > > behavior when we have:
> > >
> > > reg = <baseA sizeA>, <baseB sizeB>;
> >
> > Urgh, because you're using the type markers for that broken YAML
> > conversion.  Fine, ok.
> 
> Uhh well, type markers were added in the first place to support YAML,

No, they weren't.  They were added first so that dts -I dts -O dts
would preserve input formatting more closely.  Shortly afterwards they
were used for the YAML stuff.  They're taking what was originally just
a user choice of input formatting and giving it semantic importance,
which I have always thought was a super fragile approach.  But, I've
merged it anyway, since people seem to really want it and I haven't
had time to work on something else myself.

> but they are used on the dts output as well which has changed it a lot
> from what was output before. The YAML and dts code is just about the
> same in terms of how the type information is used. If anything is
> broken, it's the dts output guessing. Multiples of 4 bytes are always
> UINT32 data?

dts output was never supposed to be reliably type accurate - just like
any decompiler.  It was just trying to take a simple guess for
convenience in debugging.  It *does* guarantee that the -O dts output
will produce byte for byte identical dtb output if you feed it back
into dtc.

-- 
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 v2 1/3] checks: Add markers on known properties

[Rob Herring]

On Sun, Jul 25, 2021 at 10:44 PM David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Mon, Jul 12, 2021 at 04:15:43PM -0600, Rob Herring wrote:
> > On Sun, Jul 11, 2021 at 8:39 PM David Gibson
> > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > >
> > > On Mon, Jun 21, 2021 at 12:22:45PM -0600, Rob Herring wrote:
> > > > On Sat, Jun 19, 2021 at 3:22 AM David Gibson
> > > > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > > >
> > > > > On Tue, Jun 08, 2021 at 03:46:24PM -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>
> > > > > > ---
> > > > > > v2:
> > > > > >  - Set marker.ref on phandle markers
> > > > > >  - Avoid adding markers if there's any conflicting type markers.
> > > > > > ---
> > > > > >  checks.c | 102 ++++++++++++++++++++++++++++++++++++++++++++-----------
> > > > > >  1 file changed, 82 insertions(+), 20 deletions(-)
> > > > > >
> > > > > > diff --git a/checks.c b/checks.c
> > > > > > index e6c7c3eeacac..0f51b9111be1 100644
> > > > > > --- a/checks.c
> > > > > > +++ b/checks.c
> > > > > > @@ -58,6 +58,38 @@ struct check {
> > > > > >  #define CHECK(nm_, fn_, d_, ...) \
> > > > > >       CHECK_ENTRY(nm_, fn_, d_, false, false, __VA_ARGS__)
> > > > > >
> > > > > > +static struct marker *marker_add(struct marker **list, enum markertype type,
> > > > > > +                              unsigned int offset)
> > > > >
> > > > > Now that this is only conditionally adding markers, it needs a
> > > > > different name.  Maybe "add_type_annotation".
> > > >
> > > > marker_add_type_annotation to be consistent that marker_* functions
> > > > work on markers?
> > >
> > > Sure, that's fine too.  It's a local function, so naming consistency
> > > isn't as important as for globals.
> > >
> > > > > > +{
> > > > > > +     struct marker *m = *list;
> > > > >
> > > > > Since this is strictly about type annotations (and you don't have
> > > > > parameters for the necessary ref parameter for other things), an
> > > > > assert() that the given type is a TYPE_* wouldn't go astray.
> > > > >
> > > > > > +
> > > > > > +     /* Check if we already have a different type or a type marker at the offset*/
> > > > > > +     for_each_marker_of_type(m, type) {
> > > > > > +             if ((m->type >= TYPE_UINT8) && (m->type != type))
> > > > >
> > > > > I'm assuming the >= TYPE_UINT8 is about checking that this is a type
> > > > > marker rather than a ref marker.  Adding a helper function for that
> > > > > would probably be a good idea.  Putting it inline in dtc.h would make
> > > > > it less likely that we break it if we ever add new marker types in
> > > > > future.
> > > >
> > > > It's checking that we don't have markers of a different type within
> > > > the property. While that is valid dts format, it's never the case for
> > > > any known (by checks.c) properties.
> > >
> > > That comment doesn't seem to quite match the thing above.
> >
> > The comment applies to both 'if' conditions. I can make it 2 comments.
>
> Sorry, I wasn't clear.  I didn't mean the code comment I meant your
> remark in the thread "It's checking that...".  It didn't seem apropos
> to my comment it's in reply to.
>
> > > Besides, I don't think that's really true.  It would be entirely
> > > sensible for 'reg' to have mixed u32 & u64 type to encode PCI
> > > addresses.  Likewise it would make sense for 'ranges' to have mixed
> > > u32 & u64 type if mapping between a 32-bit and 64-bit bus.
> >
> > Yes, we could have all sorts of encoding. We could mix strings, u8,
> > u16, u32, and u64 too, but we don't because that would be insane given
> > next to zero type information is maintained.
>
> Who's "we"?  My point is that you're interacting with user supplied
> type information, and we can't control what the user supplies.

"We" is the modern users of FDT following the DT spec and where
bindings are reviewed. At least within that (sizable) set of users, we
do control what the user supplies (with schema checks). I get that DTC
needs to not care about the input beyond what's legal for the language
like any compiler. However, that line has already been blurred in dtc.
Pretty much all of checks.c goes beyond just what's allowed for the
language. For example, who's to say 'interrupts' can't be a string for
some user?

> > But this isn't really about what encoding we could have for 'reg' as
> > if we have any type information already, then we do nothing.
>
> But.. that's *not* what this logic does.  It's quite explicitly more
> complicated than "if there's any other type information, then do
> nothing".

Yes and no. It depends on the caller of marker_add() when we only have
a type marker at the beginning. In the cases of reg, ranges, #*-cells
and interrupts, it will do nothing. For phandle+arg properties, it
will still bracket each entry if there's only a TYPE marker at offset
0. We should be consistent there at least.

I went with let's get consistent output regardless of the input which
seemed like a good thing to me. If you don't think that's an
improvement, I can drop that though as dts -> dtb -> dts/yaml can
accomplish the same thing.

> > This is
> > only what is the encoding when there is no other information to go on
> > besides the property name. IMO, that should be bracketing each entry
> > which is all we're doing here. Maybe you disagree on bracketing, but
> > as a user shouldn't I be able to get the output I want?
>
> Yes... that's my point...

That you disagree on bracketing or that I should be able to get the
output I want?

I know you don't like any requirements on input formatting, but surely
you have some opinion on preferred formatting.

> > Debating it is
> > like debating tabs vs. spaces (BTW, dtc already decided I get tabs).
> > So maybe all this would be better in a linter, but we don't have one
> > and if I wrote one it would start with forking dtc. We could make this
> > all a command line option I suppose.
> >
> > > > It could be an assert() as it's
> > > > one of those 'should never happen'.
> > >
> > > An assert() should only be used if the problem must have been caused
> > > by an error in the code.  Bad user input should never trigger an
> > > assert().
> >
> > Maybe I misunderstood what you suggested an assert() for.
>
> I didn't, you were the one suggesting an assert().

I mixed up the context where you did suggest one...

> > > > > > @@ -774,10 +811,16 @@ 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 (!is_multiple_of(prop->val.len, entrylen))
> > > > > > +     if (!is_multiple_of(prop->val.len, entrylen)) {
> > > > > >               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)
> > > > > > +             if (!marker_add(&prop->val.markers, TYPE_UINT32, offset))
> > > > > > +                     break;
> > > > >
> > > > > I don't see any point to adding multiple markers.  Each type marker
> > > > > indicates the type until the next marker, so just adding one has the
> > > > > same effect.
> > > >
> > > > Multiple markers is the whole point. This is to match the existing
> > > > behavior when we have:
> > > >
> > > > reg = <baseA sizeA>, <baseB sizeB>;
> > >
> > > Urgh, because you're using the type markers for that broken YAML
> > > conversion.  Fine, ok.
> >
> > Uhh well, type markers were added in the first place to support YAML,
>
> No, they weren't.  They were added first so that dts -I dts -O dts
> would preserve input formatting more closely.

I think I remember why Grant wrote this and I upstreamed it. All of
this only happened because of schema validation work. Like many
things, the easy part went in first...

>  Shortly afterwards they
> were used for the YAML stuff.  They're taking what was originally just
> a user choice of input formatting and giving it semantic importance,
> which I have always thought was a super fragile approach.  But, I've
> merged it anyway, since people seem to really want it and I haven't
> had time to work on something else myself.

Yes, you've made that perfectly clear. This series is about being less
dependent on user's input formatting by being more consistent on the
output formatting.

The issue here boils down to the #*-cells pattern being a PIA to deal
with. It's a pretty peculiar construct that anything utilizing DT has
to know how to handle. I'm just trying to not implement it yet again
in the schema tools. If we redesigned everything, we'd most certainly
embed type and size information directly in properties. I'm not
holding my breath on any of that happening (there are discussions
happening though), so we're pretty much stuck with what we have other
than making the bracketing significant.

> > but they are used on the dts output as well which has changed it a lot
> > from what was output before. The YAML and dts code is just about the
> > same in terms of how the type information is used. If anything is
> > broken, it's the dts output guessing. Multiples of 4 bytes are always
> > UINT32 data?
>
> dts output was never supposed to be reliably type accurate - just like
> any decompiler.  It was just trying to take a simple guess for
> convenience in debugging.  It *does* guarantee that the -O dts output
> will produce byte for byte identical dtb output if you feed it back
> into dtc.

Why guess when we already have code that knows the type of properties
and we can accurately output the correct type?

No one is going to complain about better debug output.

Rob