[PATCH 0/3] Extend dtc with data type handling

[PATCH 0/3] Extend dtc with data type handling

[Tomasz Figa]

This series intends to extend dtc with appropriate infrastructure
to handle property data types. First, dtc is modified to preserve
type information when parsing DTS. Then type guessing is implemented
for flat and fs trees where type data is not available. After that,
DTS generation code is modified to use only type information when
printing property data.

Effect of this is the ability to implement checks for data types
of properties. Also, as a side effect, it's now possible to do
almost 1:1 dts <=> dts passes (not very useful, but shows that
things are working well).

Tomasz Figa (3):
  dtc: Keep type information from DTS
  dtc: Inject guessed type information in case of flat and fs trees
  dtc: Use type information when printing tree source

 data.c       |  62 +++++++++++++++++++++++-
 dtc-parser.y |  47 ++++++++++++++-----
 dtc.h        |  12 +++++
 flattree.c   |   2 +-
 fstree.c     |   6 ++-
 livetree.c   |  16 +++++--
 treesource.c | 150 +++++++++++++++++++++++++++++++++--------------------------
 7 files changed, 209 insertions(+), 86 deletions(-)

-- 
1.8.4.3

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

[PATCH 1/3] dtc: Keep type information from DTS

[Tomasz Figa]

Currently dtc loses information about data types when parsing DTS,
because it flattens all the parsed property data into a flat stream of
bytes. The only saved metadata is for references to other nodes inside
cell arrays. This makes it impossible to do any checks on data types on
livetree representation.

This patch makes dtc store type information inside data struct by using
marker infrastructure. A new type of marker is introduced that holds
type enum as its ref member. Such markers are then inserted wherever
data of given type starts, so information about type of each property
data section is preserved.

Signed-off-by: Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 data.c       |  3 ++-
 dtc-parser.y | 47 +++++++++++++++++++++++++++++++++++------------
 dtc.h        | 10 ++++++++++
 livetree.c   | 16 ++++++++++++----
 4 files changed, 59 insertions(+), 17 deletions(-)

diff --git a/data.c b/data.c
index 4c50b12..a33b475 100644
--- a/data.c
+++ b/data.c
@@ -27,7 +27,8 @@ void data_free(struct data d)
 	m = d.markers;
 	while (m) {
 		nm = m->next;
-		free(m->ref);
+		if (m->type != TYPE)
+			free(m->ref);
 		free(m);
 		m = nm;
 	}
diff --git a/dtc-parser.y b/dtc-parser.y
index 4864631..0ba6cc6 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -209,7 +209,9 @@ propdef:
 propdata:
 	  propdataprefix DT_STRING
 		{
-			$$ = data_merge($1, $2);
+			struct data d;
+			d = data_add_marker($1, TYPE, (char *)TYPE_STRING);
+			$$ = data_merge(d, $2);
 		}
 	| propdataprefix arrayprefix '>'
 		{
@@ -217,16 +219,21 @@ propdata:
 		}
 	| propdataprefix '[' bytestring ']'
 		{
-			$$ = data_merge($1, $3);
+			struct data d;
+			d = data_add_marker($1, TYPE, (char *)TYPE_BYTE);
+			$$ = data_merge(d, $3);
 		}
 	| propdataprefix DT_REF
 		{
-			$$ = data_add_marker($1, REF_PATH, $2);
+			struct data d;
+			d = data_add_marker($1, TYPE, (char *)TYPE_STRING);
+			$$ = data_add_marker(d, REF_PATH, $2);
 		}
 	| propdataprefix DT_INCBIN '(' DT_STRING ',' integer_prim ',' integer_prim ')'
 		{
 			FILE *f = srcfile_relative_open($4.val, NULL);
 			struct data d;
+			struct data orig;
 
 			if ($6 != 0)
 				if (fseek(f, $6, SEEK_SET) != 0)
@@ -236,18 +243,21 @@ propdata:
 						     strerror(errno));
 
 			d = data_copy_file(f, $8);
+			orig = data_add_marker($1, TYPE, (char *)TYPE_BYTE);
 
-			$$ = data_merge($1, d);
+			$$ = data_merge(orig, d);
 			fclose(f);
 		}
 	| propdataprefix DT_INCBIN '(' DT_STRING ')'
 		{
 			FILE *f = srcfile_relative_open($4.val, NULL);
 			struct data d = empty_data;
+			struct data orig;
 
+			orig = data_add_marker($1, TYPE, (char *)TYPE_BYTE);
 			d = data_copy_file(f, -1);
 
-			$$ = data_merge($1, d);
+			$$ = data_merge(orig, d);
 			fclose(f);
 		}
 	| propdata DT_LABEL
@@ -274,22 +284,35 @@ propdataprefix:
 arrayprefix:
 	DT_BITS DT_LITERAL '<'
 		{
-			$$.data = empty_data;
 			$$.bits = eval_literal($2, 0, 7);
 
-			if (($$.bits !=  8) &&
-			    ($$.bits != 16) &&
-			    ($$.bits != 32) &&
-			    ($$.bits != 64))
-			{
+			switch ($$.bits) {
+			case 8:
+				$$.data = data_add_marker(empty_data,
+						TYPE, (char *)TYPE_ARRAY_INT8);
+				break;
+			case 16:
+				$$.data = data_add_marker(empty_data,
+						TYPE, (char *)TYPE_ARRAY_INT16);
+				break;
+			case 64:
+				$$.data = data_add_marker(empty_data,
+						TYPE, (char *)TYPE_ARRAY_INT64);
+				break;
+			default:
 				print_error("Only 8, 16, 32 and 64-bit elements"
 					    " are currently supported");
 				$$.bits = 32;
+				/* Fall through */
+			case 32:
+				$$.data = data_add_marker(empty_data,
+						TYPE, (char *)TYPE_ARRAY_INT32);
 			}
 		}
 	| '<'
 		{
-			$$.data = empty_data;
+			$$.data = data_add_marker(empty_data,
+						TYPE, (char *)TYPE_ARRAY_INT32);
 			$$.bits = 32;
 		}
 	| arrayprefix integer_prim
diff --git a/dtc.h b/dtc.h
index 20e4d56..e800600 100644
--- a/dtc.h
+++ b/dtc.h
@@ -72,6 +72,16 @@ enum markertype {
 	REF_PHANDLE,
 	REF_PATH,
 	LABEL,
+	TYPE,
+};
+
+enum datatype {
+	TYPE_STRING,
+	TYPE_BYTE,
+	TYPE_ARRAY_INT8,
+	TYPE_ARRAY_INT16,
+	TYPE_ARRAY_INT32,
+	TYPE_ARRAY_INT64,
 };
 
 struct  marker {
diff --git a/livetree.c b/livetree.c
index b61465f..9a1536d 100644
--- a/livetree.c
+++ b/livetree.c
@@ -530,16 +530,24 @@ cell_t get_node_phandle(struct node *root, struct node *node)
 	node->phandle = phandle;
 
 	if (!get_property(node, "linux,phandle")
-	    && (phandle_format & PHANDLE_LEGACY))
+	    && (phandle_format & PHANDLE_LEGACY)) {
+		struct data d;
+
+		d = data_add_marker(empty_data, TYPE, (char *)TYPE_ARRAY_INT32);
 		add_property(node,
 			     build_property("linux,phandle",
-					    data_append_cell(empty_data, phandle)));
+					    data_append_cell(d, phandle)));
+	}
 
 	if (!get_property(node, "phandle")
-	    && (phandle_format & PHANDLE_EPAPR))
+	    && (phandle_format & PHANDLE_EPAPR)) {
+		struct data d;
+
+		d = data_add_marker(empty_data, TYPE, (char *)TYPE_ARRAY_INT32);
 		add_property(node,
 			     build_property("phandle",
-					    data_append_cell(empty_data, phandle)));
+					    data_append_cell(d, phandle)));
+	}
 
 	/* If the node *does* have a phandle property, we must
 	 * be dealing with a self-referencing phandle, which will be
-- 
1.8.4.3

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

[PATCH 2/3] dtc: Inject guessed type information in case of flat and fs trees

[Tomasz Figa]

Since flat and fs trees do not provide any kind of type information,
just raw data, types must be guessed based on some data analysis, as it
is currently done when generating DTS back from livetree representation.

This patch moves data type guessing from livetree printing code to
generic helper function that can be called when parsing flat and fs
trees. In effect, printing can be modified to use type markers alone
instead of guessing in further patch.

Signed-off-by: Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 data.c     | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h      |  2 ++
 flattree.c |  2 +-
 fstree.c   |  6 ++++--
 4 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/data.c b/data.c
index a33b475..25888ae 100644
--- a/data.c
+++ b/data.c
@@ -268,3 +268,62 @@ bool data_is_one_string(struct data d)
 
 	return true;
 }
+
+static bool isstring(char c)
+{
+	return (isprint(c)
+		|| (c == '\0')
+		|| strchr("\a\b\t\n\v\f\r", c));
+}
+
+struct data data_detect_type(struct data d)
+{
+	int len = d.len;
+	const char *p = d.val;
+	int nnotstring = 0, nnul = 0;
+	struct marker *m;
+	int i;
+
+	if (len == 0)
+		return d;
+
+	assert(d.markers == NULL);
+
+	for (i = 0; i < len; i++) {
+		if (!isstring(p[i]))
+			nnotstring++;
+		if (p[i] == '\0')
+			nnul++;
+	}
+
+	if ((p[len-1] == '\0') && (nnotstring == 0) && (nnul < (len-nnul))) {
+		int offset = 0;
+		/* String(s) */
+		do {
+			m = xmalloc(sizeof(*m));
+			m->offset = offset;
+			m->type = TYPE;
+			m->next = NULL;
+			m->ref = (char *)TYPE_STRING;
+			d = data_append_markers(d, m);
+			offset += strlen(p + offset) + 1;
+		} while (offset < len && p[offset] != '\0');
+		return d;
+	} else if (((len % sizeof(cell_t)) == 0)) {
+		/* Cells */
+		m = xmalloc(sizeof(*m));
+		m->offset = 0;
+		m->type = TYPE;
+		m->next = NULL;
+		m->ref = (char *)TYPE_ARRAY_INT32;
+		return data_append_markers(d, m);
+	} else {
+		/* Bytes */
+		m = xmalloc(sizeof(*m));
+		m->offset = 0;
+		m->type = TYPE;
+		m->next = NULL;
+		m->ref = (char *)TYPE_BYTE;
+		return data_append_markers(d, m);
+	}
+}
diff --git a/dtc.h b/dtc.h
index e800600..6dd4797 100644
--- a/dtc.h
+++ b/dtc.h
@@ -128,6 +128,8 @@ struct data data_append_align(struct data d, int align);
 
 struct data data_add_marker(struct data d, enum markertype type, char *ref);
 
+struct data data_detect_type(struct data d);
+
 bool data_is_one_string(struct data d);
 
 /* DT constraints */
diff --git a/flattree.c b/flattree.c
index bd99fa2..93cdacb 100644
--- a/flattree.c
+++ b/flattree.c
@@ -690,7 +690,7 @@ static struct property *flat_read_property(struct inbuf *dtbuf,
 	if ((flags & FTF_VARALIGN) && (proplen >= 8))
 		flat_realign(dtbuf, 8);
 
-	val = flat_read_data(dtbuf, proplen);
+	val = data_detect_type(flat_read_data(dtbuf, proplen));
 
 	return build_property(name, val);
 }
diff --git a/fstree.c b/fstree.c
index f377453..bb3fd50 100644
--- a/fstree.c
+++ b/fstree.c
@@ -58,9 +58,11 @@ static struct node *read_fstree(const char *dirname)
 					"WARNING: Cannot open %s: %s\n",
 					tmpnam, strerror(errno));
 			} else {
+				struct data d = data_copy_file(pfile,
+								st.st_size);
+
 				prop = build_property(xstrdup(de->d_name),
-						      data_copy_file(pfile,
-								     st.st_size));
+							data_detect_type(d));
 				add_property(tree, prop);
 				fclose(pfile);
 			}
-- 
1.8.4.3

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

[PATCH 3/3] dtc: Use type information when printing tree source

[Tomasz Figa]

Since we already have type information in livetree representation, there
is no need to guess types when printing DTS. This patch modifies
printing code to use type information.

Signed-off-by: Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 treesource.c | 150 +++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 84 insertions(+), 66 deletions(-)

diff --git a/treesource.c b/treesource.c
index ffebb77..1c440a0 100644
--- a/treesource.c
+++ b/treesource.c
@@ -54,29 +54,44 @@ static void write_prefix(FILE *f, int level)
 		fputc('\t', f);
 }
 
-static bool isstring(char c)
+static size_t prop_len(struct data *val, struct marker *marker)
 {
-	return (isprint(c)
-		|| (c == '\0')
-		|| strchr("\a\b\t\n\v\f\r", c));
+	struct marker *m = marker->next;
+
+	for_each_marker_of_type(m, TYPE)
+		return m->offset - marker->offset;
+
+	return val->len - marker->offset;
 }
 
-static void write_propval_string(FILE *f, struct data val)
+static void write_propval_string(FILE *f, struct data val,
+				 struct marker *marker)
 {
-	const char *str = val.val;
+	const char *str = &val.val[marker->offset];
+	size_t offset = marker->offset;
+	size_t len = prop_len(&val, marker);
 	int i;
-	struct marker *m = val.markers;
+	struct marker *m = marker->next;
 
-	assert(str[val.len-1] == '\0');
+	assert(str[len-1] == '\0');
 
-	while (m && (m->offset == 0)) {
+	while (m && (m->offset == offset)) {
 		if (m->type == LABEL)
 			fprintf(f, "%s: ", m->ref);
+		else if (m->type == REF_PATH) {
+			if (offset + len == val.len) {
+				fprintf(f, "&%s", m->ref);
+				goto labels;
+			} else {
+				fprintf(f, "&%s, ", m->ref);
+				return;
+			}
+		}
 		m = m->next;
 	}
 	fprintf(f, "\"");
 
-	for (i = 0; i < (val.len-1); i++) {
+	for (i = 0; i < (len-1); i++) {
 		char c = str[i];
 
 		switch (c) {
@@ -107,17 +122,6 @@ static void write_propval_string(FILE *f, struct data val)
 		case '\"':
 			fprintf(f, "\\\"");
 			break;
-		case '\0':
-			fprintf(f, "\", ");
-			while (m && (m->offset < i)) {
-				if (m->type == LABEL) {
-					assert(m->offset == (i+1));
-					fprintf(f, "%s: ", m->ref);
-				}
-				m = m->next;
-			}
-			fprintf(f, "\"");
-			break;
 		default:
 			if (isprint(c))
 				fprintf(f, "%c", c);
@@ -125,20 +129,26 @@ static void write_propval_string(FILE *f, struct data val)
 				fprintf(f, "\\x%02hhx", c);
 		}
 	}
-	fprintf(f, "\"");
 
+	if (offset + len != val.len) {
+		fprintf(f, "\", ");
+		return;
+	}
+
+	fprintf(f, "\"");
+labels:
 	/* Wrap up any labels at the end of the value */
-	for_each_marker_of_type(m, LABEL) {
-		assert (m->offset == val.len);
+	for_each_marker_of_type(m, LABEL)
 		fprintf(f, " %s:", m->ref);
-	}
 }
 
-static void write_propval_cells(FILE *f, struct data val)
+static void write_propval_cells(FILE *f, struct data val, struct marker *marker)
 {
-	void *propend = val.val + val.len;
-	cell_t *cp = (cell_t *)val.val;
-	struct marker *m = val.markers;
+	size_t offset = marker->offset;
+	size_t len = prop_len(&val, marker);
+	void *propend = val.val + offset + len;
+	cell_t *cp = (cell_t *)(val.val + offset);
+	struct marker *m = marker->next;
 
 	fprintf(f, "<");
 	for (;;) {
@@ -146,6 +156,13 @@ static void write_propval_cells(FILE *f, struct data val)
 			if (m->type == LABEL) {
 				assert(m->offset == ((char *)cp - val.val));
 				fprintf(f, "%s: ", m->ref);
+			} else if (m->type == REF_PHANDLE) {
+				assert(m->offset == ((char *)cp - val.val));
+				fprintf(f, "&%s", m->ref);
+				++cp;
+				if ((void *)cp >= propend)
+					goto done;
+				fprintf(f, " ");
 			}
 			m = m->next;
 		}
@@ -156,19 +173,26 @@ static void write_propval_cells(FILE *f, struct data val)
 		fprintf(f, " ");
 	}
 
+done:
+	if (offset + len != val.len) {
+		fprintf(f, ">, ");
+		return;
+	}
+
 	/* Wrap up any labels at the end of the value */
-	for_each_marker_of_type(m, LABEL) {
-		assert (m->offset == val.len);
+	for_each_marker_of_type(m, LABEL)
 		fprintf(f, " %s:", m->ref);
-	}
+
 	fprintf(f, ">");
 }
 
-static void write_propval_bytes(FILE *f, struct data val)
+static void write_propval_bytes(FILE *f, struct data val, struct marker *marker)
 {
-	void *propend = val.val + val.len;
-	const char *bp = val.val;
-	struct marker *m = val.markers;
+	size_t offset = marker->offset;
+	size_t len = prop_len(&val, marker);
+	void *propend = val.val + offset + len;
+	const char *bp = val.val + marker->offset;
+	struct marker *m = marker->next;
 
 	fprintf(f, "[");
 	for (;;) {
@@ -184,52 +208,46 @@ static void write_propval_bytes(FILE *f, struct data val)
 		fprintf(f, " ");
 	}
 
+	if (offset + len != val.len) {
+		fprintf(f, "], ");
+		return;
+	}
+
 	/* Wrap up any labels at the end of the value */
-	for_each_marker_of_type(m, LABEL) {
-		assert (m->offset == val.len);
+	for_each_marker_of_type(m, LABEL)
 		fprintf(f, " %s:", m->ref);
-	}
+
 	fprintf(f, "]");
 }
 
 static void write_propval(FILE *f, struct property *prop)
 {
-	int len = prop->val.len;
-	const char *p = prop->val.val;
 	struct marker *m = prop->val.markers;
-	int nnotstring = 0, nnul = 0;
-	int nnotstringlbl = 0, nnotcelllbl = 0;
-	int i;
+	int len = prop->val.len;
 
 	if (len == 0) {
 		fprintf(f, ";\n");
 		return;
 	}
 
-	for (i = 0; i < len; i++) {
-		if (! isstring(p[i]))
-			nnotstring++;
-		if (p[i] == '\0')
-			nnul++;
-	}
-
-	for_each_marker_of_type(m, LABEL) {
-		if ((m->offset > 0) && (prop->val.val[m->offset - 1] != '\0'))
-			nnotstringlbl++;
-		if ((m->offset % sizeof(cell_t)) != 0)
-			nnotcelllbl++;
-	}
-
 	fprintf(f, " = ");
-	if ((p[len-1] == '\0') && (nnotstring == 0) && (nnul < (len-nnul))
-	    && (nnotstringlbl == 0)) {
-		write_propval_string(f, prop->val);
-	} else if (((len % sizeof(cell_t)) == 0) && (nnotcelllbl == 0)) {
-		write_propval_cells(f, prop->val);
-	} else {
-		write_propval_bytes(f, prop->val);
-	}
 
+	for_each_marker_of_type(m, TYPE) {
+		switch ((intptr_t)m->ref) {
+		case TYPE_STRING:
+			write_propval_string(f, prop->val, m);
+			break;
+		case TYPE_ARRAY_INT32:
+		case TYPE_ARRAY_INT64:
+			write_propval_cells(f, prop->val, m);
+			break;
+		case TYPE_BYTE:
+		case TYPE_ARRAY_INT8:
+		case TYPE_ARRAY_INT16:
+		default:
+			write_propval_bytes(f, prop->val, m);
+		}
+	}
 	fprintf(f, ";\n");
 }
 
-- 
1.8.4.3

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

Re: [PATCH 1/3] dtc: Keep type information from DTS

[Stephen Warren]

On 12/13/2013 09:49 AM, Tomasz Figa wrote:
> Currently dtc loses information about data types when parsing DTS,
> because it flattens all the parsed property data into a flat stream of
> bytes. The only saved metadata is for references to other nodes inside
> cell arrays. This makes it impossible to do any checks on data types on
> livetree representation.
> 
> This patch makes dtc store type information inside data struct by using
> marker infrastructure. A new type of marker is introduced that holds
> type enum as its ref member. Such markers are then inserted wherever
> data of given type starts, so information about type of each property
> data section is preserved.

> diff --git a/dtc-parser.y b/dtc-parser.y

>  	| propdataprefix DT_REF
>  		{
> -			$$ = data_add_marker($1, REF_PATH, $2);
> +			struct data d;
> +			d = data_add_marker($1, TYPE, (char *)TYPE_STRING);
> +			$$ = data_add_marker(d, REF_PATH, $2);
>  		}

I guess here, the lexer does give us a string that's the target of the
reference, so this is correct. However, I wonder if semantically we
shouldn't call this a TYPE_REFERENCE, so we can distinguish between
references and regular strings?

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

> @@ -530,16 +530,24 @@ cell_t get_node_phandle(struct node *root, struct node *node)
>  	node->phandle = phandle;
>  
>  	if (!get_property(node, "linux,phandle")
> -	    && (phandle_format & PHANDLE_LEGACY))
> +	    && (phandle_format & PHANDLE_LEGACY)) {
> +		struct data d;
> +
> +		d = data_add_marker(empty_data, TYPE, (char *)TYPE_ARRAY_INT32);

Similarly here, can we encode as e.g. TYPE_PHANDLE or something like
that, so we keep the semantic information?

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

Re: [PATCH 0/3] Extend dtc with data type handling

[David Gibson]

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

On Fri, Dec 13, 2013 at 05:49:09PM +0100, Tomasz Figa wrote:
> This series intends to extend dtc with appropriate infrastructure
> to handle property data types. First, dtc is modified to preserve
> type information when parsing DTS. Then type guessing is implemented
> for flat and fs trees where type data is not available. After that,
> DTS generation code is modified to use only type information when
> printing property data.

Hrm.  I think this is completely backwards to the right approach.

For dt checking what we want is a schema.  That can specify datatypes
amongst other constraints, but in the end you can check the actual
bytes of the DT against the constraints imposed by the schema,
regardless of how those bytes are presented to the checker.

By adding type information to the in-flight tree you're checking not
the actual content of the DT, but how you've chosen to express that
information in DTS format.

More concretely, we should be able to schema-check trees in binary
format, not just source.  With these patches that will fail if -I dtb
makes the wrong type guesses.

> Effect of this is the ability to implement checks for data types
> of properties. Also, as a side effect, it's now possible to do
> almost 1:1 dts <=> dts passes (not very useful, but shows that
> things are working well).
> 
> Tomasz Figa (3):
>   dtc: Keep type information from DTS
>   dtc: Inject guessed type information in case of flat and fs trees
>   dtc: Use type information when printing tree source
> 
>  data.c       |  62 +++++++++++++++++++++++-
>  dtc-parser.y |  47 ++++++++++++++-----
>  dtc.h        |  12 +++++
>  flattree.c   |   2 +-
>  fstree.c     |   6 ++-
>  livetree.c   |  16 +++++--
>  treesource.c | 150 +++++++++++++++++++++++++++++++++--------------------------
>  7 files changed, 209 insertions(+), 86 deletions(-)
> 

-- 
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: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] Extend dtc with data type handling

[Tomasz Figa]

On Monday 23 of December 2013 23:08:14 David Gibson wrote:
> On Fri, Dec 13, 2013 at 05:49:09PM +0100, Tomasz Figa wrote:
> > This series intends to extend dtc with appropriate infrastructure
> > to handle property data types. First, dtc is modified to preserve
> > type information when parsing DTS. Then type guessing is implemented
> > for flat and fs trees where type data is not available. After that,
> > DTS generation code is modified to use only type information when
> > printing property data.
> 
> Hrm.  I think this is completely backwards to the right approach.
> 
> For dt checking what we want is a schema.  That can specify datatypes
> amongst other constraints, but in the end you can check the actual
> bytes of the DT against the constraints imposed by the schema,
> regardless of how those bytes are presented to the checker.

Those constraints are usually types, so you need a way to extract them
from source DTS.

> 
> By adding type information to the in-flight tree you're checking not
> the actual content of the DT, but how you've chosen to express that
> information in DTS format.
> 
> More concretely, we should be able to schema-check trees in binary
> format, not just source.  With these patches that will fail if -I dtb
> makes the wrong type guesses.

I'm not quite sure what do you want to check in binary format, where
properties are just strings of bytes. All you can check is whether there
is enough bytes or possibly some value constraints on particular parts
of it, but not types.

IMHO what we primarily need is source validation. Of course using schemas,
but that's already decided.

Best regards,
Tomasz

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

Re: [PATCH 1/3] dtc: Keep type information from DTS

[Tomasz Figa]

On Monday 16 of December 2013 11:08:33 Stephen Warren wrote:
> On 12/13/2013 09:49 AM, Tomasz Figa wrote:
> > Currently dtc loses information about data types when parsing DTS,
> > because it flattens all the parsed property data into a flat stream of
> > bytes. The only saved metadata is for references to other nodes inside
> > cell arrays. This makes it impossible to do any checks on data types on
> > livetree representation.
> > 
> > This patch makes dtc store type information inside data struct by using
> > marker infrastructure. A new type of marker is introduced that holds
> > type enum as its ref member. Such markers are then inserted wherever
> > data of given type starts, so information about type of each property
> > data section is preserved.
> 
> > diff --git a/dtc-parser.y b/dtc-parser.y
> 
> >  	| propdataprefix DT_REF
> >  		{
> > -			$$ = data_add_marker($1, REF_PATH, $2);
> > +			struct data d;
> > +			d = data_add_marker($1, TYPE, (char *)TYPE_STRING);
> > +			$$ = data_add_marker(d, REF_PATH, $2);
> >  		}
> 
> I guess here, the lexer does give us a string that's the target of the
> reference, so this is correct. However, I wonder if semantically we
> shouldn't call this a TYPE_REFERENCE, so we can distinguish between
> references and regular strings?

Hmm, a REF_PATH tag is already being added by this code, so you don't lose
this information. Still, maybe it would be better to unify REF_* and TYPE
tags and add TYPE_PATH and TYPE_PHANDLE types...

> 
> > diff --git a/livetree.c b/livetree.c
> 
> > @@ -530,16 +530,24 @@ cell_t get_node_phandle(struct node *root, struct node *node)
> >  	node->phandle = phandle;
> >  
> >  	if (!get_property(node, "linux,phandle")
> > -	    && (phandle_format & PHANDLE_LEGACY))
> > +	    && (phandle_format & PHANDLE_LEGACY)) {
> > +		struct data d;
> > +
> > +		d = data_add_marker(empty_data, TYPE, (char *)TYPE_ARRAY_INT32);
> 
> Similarly here, can we encode as e.g. TYPE_PHANDLE or something like
> that, so we keep the semantic information?

"linux,phandle" property isn't really a phandle (as in reference by
phandle), but an integer assigning phandle number for given node, so
I'm not sure if TYPE_PHANDLE would be correct here.

Best regards,
Tomasz

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

Re: [PATCH 0/3] Extend dtc with data type handling

[David Gibson]

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

On Mon, Dec 23, 2013 at 08:00:54PM +0100, Tomasz Figa wrote:
> On Monday 23 of December 2013 23:08:14 David Gibson wrote:
> > On Fri, Dec 13, 2013 at 05:49:09PM +0100, Tomasz Figa wrote:
> > > This series intends to extend dtc with appropriate infrastructure
> > > to handle property data types. First, dtc is modified to preserve
> > > type information when parsing DTS. Then type guessing is implemented
> > > for flat and fs trees where type data is not available. After that,
> > > DTS generation code is modified to use only type information when
> > > printing property data.
> > 
> > Hrm.  I think this is completely backwards to the right approach.
> > 
> > For dt checking what we want is a schema.  That can specify datatypes
> > amongst other constraints, but in the end you can check the actual
> > bytes of the DT against the constraints imposed by the schema,
> > regardless of how those bytes are presented to the checker.
> 
> Those constraints are usually types, so you need a way to extract them
> from source DTS.

You really don't.

Consumers of the dt intepret it without type information, because they
know what's supposed to be there.  Likewise if we know the schema, we
can check whether the dt data fits it without having to have it
annotated with type information from another source.

> > By adding type information to the in-flight tree you're checking not
> > the actual content of the DT, but how you've chosen to express that
> > information in DTS format.
> > 
> > More concretely, we should be able to schema-check trees in binary
> > format, not just source.  With these patches that will fail if -I dtb
> > makes the wrong type guesses.
> 
> I'm not quite sure what do you want to check in binary format, where
> properties are just strings of bytes. All you can check is whether there
> is enough bytes or possibly some value constraints on particular parts
> of it, but not types.

You can check that if a consumer interprets the dt bytes according to
the schema, it will get sane data - and that's exactly what matters.

If a schema specifies a string followed by a 32-bit integer, that
implies constraints on the bytes, and that's what matters to a dt
consumer.  So we might expect:
	prop = "abc", <0xabcdef00>;
But:
	prop = <0x61626300>, "\xab\xcd\xef";

will still be interpreted correctly by the consumer, even though the
"type" information is wrong.

However:
	prop = "a\0b", <0xabcdef00>;

will NOT be parsed correctly by the consumer, even though the "type"
information is correct.

> IMHO what we primarily need is source validation. Of course using schemas,
> but that's already decided.

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

Re: [PATCH 1/3] dtc: Keep type information from DTS

[Grant Likely]

On Mon, 16 Dec 2013 11:08:33 -0700, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 12/13/2013 09:49 AM, Tomasz Figa wrote:
> > Currently dtc loses information about data types when parsing DTS,
> > because it flattens all the parsed property data into a flat stream of
> > bytes. The only saved metadata is for references to other nodes inside
> > cell arrays. This makes it impossible to do any checks on data types on
> > livetree representation.
> > 
> > This patch makes dtc store type information inside data struct by using
> > marker infrastructure. A new type of marker is introduced that holds
> > type enum as its ref member. Such markers are then inserted wherever
> > data of given type starts, so information about type of each property
> > data section is preserved.
> 
> > diff --git a/dtc-parser.y b/dtc-parser.y
> 
> >  	| propdataprefix DT_REF
> >  		{
> > -			$$ = data_add_marker($1, REF_PATH, $2);
> > +			struct data d;
> > +			d = data_add_marker($1, TYPE, (char *)TYPE_STRING);
> > +			$$ = data_add_marker(d, REF_PATH, $2);
> >  		}
> 
> I guess here, the lexer does give us a string that's the target of the
> reference, so this is correct. However, I wonder if semantically we
> shouldn't call this a TYPE_REFERENCE, so we can distinguish between
> references and regular strings?
> 
> > diff --git a/livetree.c b/livetree.c
> 
> > @@ -530,16 +530,24 @@ cell_t get_node_phandle(struct node *root, struct node *node)
> >  	node->phandle = phandle;
> >  
> >  	if (!get_property(node, "linux,phandle")
> > -	    && (phandle_format & PHANDLE_LEGACY))
> > +	    && (phandle_format & PHANDLE_LEGACY)) {
> > +		struct data d;
> > +
> > +		d = data_add_marker(empty_data, TYPE, (char *)TYPE_ARRAY_INT32);
> 
> Similarly here, can we encode as e.g. TYPE_PHANDLE or something like
> that, so we keep the semantic information?

I think both those are reasonable and useful extensions also. On the
whole I think the patch is the right thing to do. Feel free to add my
acked by:

Acked-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

> 

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

Re: [PATCH 1/3] dtc: Keep type information from DTS

[David Gibson]

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

On Tue, Jan 21, 2014 at 12:06:06PM +0000, Grant Likely wrote:
> On Mon, 16 Dec 2013 11:08:33 -0700, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> > On 12/13/2013 09:49 AM, Tomasz Figa wrote:
> > > Currently dtc loses information about data types when parsing DTS,
> > > because it flattens all the parsed property data into a flat stream of
> > > bytes. The only saved metadata is for references to other nodes inside
> > > cell arrays. This makes it impossible to do any checks on data types on
> > > livetree representation.
> > > 
> > > This patch makes dtc store type information inside data struct by using
> > > marker infrastructure. A new type of marker is introduced that holds
> > > type enum as its ref member. Such markers are then inserted wherever
> > > data of given type starts, so information about type of each property
> > > data section is preserved.
> > 
> > > diff --git a/dtc-parser.y b/dtc-parser.y
> > 
> > >  	| propdataprefix DT_REF
> > >  		{
> > > -			$$ = data_add_marker($1, REF_PATH, $2);
> > > +			struct data d;
> > > +			d = data_add_marker($1, TYPE, (char *)TYPE_STRING);
> > > +			$$ = data_add_marker(d, REF_PATH, $2);
> > >  		}
> > 
> > I guess here, the lexer does give us a string that's the target of the
> > reference, so this is correct. However, I wonder if semantically we
> > shouldn't call this a TYPE_REFERENCE, so we can distinguish between
> > references and regular strings?
> > 
> > > diff --git a/livetree.c b/livetree.c
> > 
> > > @@ -530,16 +530,24 @@ cell_t get_node_phandle(struct node *root, struct node *node)
> > >  	node->phandle = phandle;
> > >  
> > >  	if (!get_property(node, "linux,phandle")
> > > -	    && (phandle_format & PHANDLE_LEGACY))
> > > +	    && (phandle_format & PHANDLE_LEGACY)) {
> > > +		struct data d;
> > > +
> > > +		d = data_add_marker(empty_data, TYPE, (char *)TYPE_ARRAY_INT32);
> > 
> > Similarly here, can we encode as e.g. TYPE_PHANDLE or something like
> > that, so we keep the semantic information?
> 
> I think both those are reasonable and useful extensions also. On the
> whole I think the patch is the right thing to do. Feel free to add my
> acked by:
> 
> Acked-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Still a NACK from me, I think this is fundamentally the wrong
approach.

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