[PATCH] DTC - validation by device_type

[PATCH] DTC - validation by device_type

[Paul Nasrat]

Jon,

It'd be nice to be able to validate by device_type, to ensure that we
don't reinvent the wheel (address vs. local-mac-address) and to give
embedded designers an added level of sanity checking.

Here is a proposed implementation with an implemented check for
"network".

Signed-off-by: Paul Nasrat <pnasrat@redhat.com>

diff --git a/livetree.c b/livetree.c
index ef54174..351773b 100644
--- a/livetree.c
+++ b/livetree.c
@@ -441,6 +441,52 @@ static int check_structure(struct node *
 				(propname), (node)->fullpath); \
 	} while (0)
 
+static int check_network(struct node *net)
+{
+
+	int ok = 1;
+	struct property *prop;
+
+	CHECK_HAVE_STREQ(net, "device_type", "network");
+	CHECK_HAVE(net, "reg");
+	CHECK_HAVE(net, "local-mac-address");
+	CHECK_HAVE(net, "mac-address");
+	CHECK_HAVE(net, "address-bits");
+
+	return ok;
+}
+
+static struct {
+	char *devtype;
+	int (*check_fn)(struct node *node);
+} devtype_checker_table[] = {
+	{"network", check_network},
+};
+
+static int check_devtypes(struct node *root)
+{
+
+	struct node *child;
+	struct property *prop;
+	int ok = 1;
+	int i;
+
+	for_each_child(root, child) {
+		/* check this node */
+		if ((prop = get_property((child), ("device_type"))))
+			for (i = 0; i < ARRAY_SIZE(devtype_checker_table); i++) {
+				if (streq(prop->val.val, devtype_checker_table[i].devtype))
+					if (! devtype_checker_table[i].check_fn(child)) {
+						ok = 0;
+						break;
+						}
+			}
+		ok = check_devtypes(child);
+	}
+
+	return ok;
+}
+
 static int check_root(struct node *root)
 {
 	struct property *prop;
@@ -716,6 +762,7 @@ int check_device_tree(struct node *dt)
 	ok = ok && check_cpus(dt);
 	ok = ok && check_memory(dt);
 	ok = ok && check_chosen(dt);
+	ok = ok && check_devtypes(dt);
 	if (! ok)
 		return 0;
 

Re: [PATCH] DTC - validation by device_type

[Hollis Blanchard]

On Mar 28, 2006, at 4:49 PM, Paul Nasrat wrote:
>
> It'd be nice to be able to validate by device_type, to ensure that we
> don't reinvent the wheel (address vs. local-mac-address) and to give
> embedded designers an added level of sanity checking.
>
> Here is a proposed implementation with an implemented check for
> "network".
>
> Signed-off-by: Paul Nasrat <pnasrat@redhat.com>
>
> diff --git a/livetree.c b/livetree.c
> index ef54174..351773b 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -441,6 +441,52 @@ static int check_structure(struct node *
>  				(propname), (node)->fullpath); \
>  	} while (0)
>
> +static int check_network(struct node *net)
> +{
> +
> +	int ok = 1;
> +	struct property *prop;
> +
> +	CHECK_HAVE_STREQ(net, "device_type", "network");

The only reason you got into this function was that device_type == 
"network"...

> +	CHECK_HAVE(net, "reg");
> +	CHECK_HAVE(net, "local-mac-address");
> +	CHECK_HAVE(net, "mac-address");
> +	CHECK_HAVE(net, "address-bits");
> +
> +	return ok;
> +}
> +
> +static struct {
> +	char *devtype;
> +	int (*check_fn)(struct node *node);
> +} devtype_checker_table[] = {
> +	{"network", check_network},
> +};
> +
> +static int check_devtypes(struct node *root)
> +{
> +
> +	struct node *child;
> +	struct property *prop;
> +	int ok = 1;
> +	int i;
> +
> +	for_each_child(root, child) {
> +		/* check this node */
> +		if ((prop = get_property((child), ("device_type"))))
> +			for (i = 0; i < ARRAY_SIZE(devtype_checker_table); i++) {
> +				if (streq(prop->val.val, devtype_checker_table[i].devtype))
> +					if (! devtype_checker_table[i].check_fn(child)) {
> +						ok = 0;
> +						break;
> +						}

Unless this is some weird style thing, that close brace seems 
misindented.

> +			}
> +		ok = check_devtypes(child);
> +	}
> +
> +	return ok;
> +}
> +
>  static int check_root(struct node *root)
>  {
>  	struct property *prop;
> @@ -716,6 +762,7 @@ int check_device_tree(struct node *dt)
>  	ok = ok && check_cpus(dt);
>  	ok = ok && check_memory(dt);
>  	ok = ok && check_chosen(dt);
> +	ok = ok && check_devtypes(dt);
>  	if (! ok)
>  		return 0;
>
>
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] DTC - validation by device_type

[Paul Nasrat]

On Tue, 2006-03-28 at 20:03 -0600, Hollis Blanchard wrote:
> On Mar 28, 2006, at 4:49 PM, Paul Nasrat wrote:
> >
> > It'd be nice to be able to validate by device_type, to ensure that we
> > don't reinvent the wheel (address vs. local-mac-address) and to give
> > embedded designers an added level of sanity checking.
> >
> > Here is a proposed implementation with an implemented check for
> > "network".

> >
> Unless this is some weird style thing, that close brace seems 
> misindented.

No you're not missing anything, both artefacts of how I got there.  Take
2.

As above

Signed-off-by: Paul Nasrat <pnasrat@redhat.com>

diff --git a/livetree.c b/livetree.c
index ef54174..bf8bb56 100644
--- a/livetree.c
+++ b/livetree.c
@@ -441,6 +441,51 @@ static int check_structure(struct node *
 				(propname), (node)->fullpath); \
 	} while (0)
 
+static int check_network(struct node *net)
+{
+
+	int ok = 1;
+	struct property *prop;
+
+	CHECK_HAVE(net, "reg");
+	CHECK_HAVE(net, "local-mac-address");
+	CHECK_HAVE(net, "mac-address");
+	CHECK_HAVE(net, "address-bits");
+
+	return ok;
+}
+
+static struct {
+	char *devtype;
+	int (*check_fn)(struct node *node);
+} devtype_checker_table[] = {
+	{"network", check_network},
+};
+
+static int check_devtypes(struct node *root)
+{
+
+	struct node *child;
+	struct property *prop;
+	int ok = 1;
+	int i;
+
+	for_each_child(root, child) {
+		/* check this node */
+		if ((prop = get_property((child), ("device_type"))))
+			for (i = 0; i < ARRAY_SIZE(devtype_checker_table); i++) {
+				if (streq(prop->val.val, devtype_checker_table[i].devtype))
+					if (! devtype_checker_table[i].check_fn(child)) {
+						ok = 0;
+						break;
+					}
+			}
+		ok = check_devtypes(child);
+	}
+
+	return ok;
+}
+
 static int check_root(struct node *root)
 {
 	struct property *prop;
@@ -716,6 +761,7 @@ int check_device_tree(struct node *dt)
 	ok = ok && check_cpus(dt);
 	ok = ok && check_memory(dt);
 	ok = ok && check_chosen(dt);
+	ok = ok && check_devtypes(dt);
 	if (! ok)
 		return 0;
 

Re: [PATCH] DTC - validation by device_type

[Jon Loeliger]

On Tue, 2006-03-28 at 20:33, Paul Nasrat wrote:

> Signed-off-by: Paul Nasrat <pnasrat@redhat.com>

Paul,

Sorry about the hang-time here.  Couple questions.


> +static int check_network(struct node *net)
> +{
> +
> +	int ok = 1;
> +	struct property *prop;
> +
> +	CHECK_HAVE(net, "reg");
> +	CHECK_HAVE(net, "local-mac-address");
> +	CHECK_HAVE(net, "mac-address");
> +	CHECK_HAVE(net, "address-bits");
> +
> +	return ok;
> +}

This says that all network nodes have to have all four
of those properties.  It's not clear to me that they all
need to have both local-mac-address and mac-address.
Specifically, 1275 seems to indicate to me that the
mac-address property, being introduced by the open
operation, might be dynamically set at run time, rather
than passed as a config parameter from, say, U-Boot to
the kernel.  Thus, it might not be necessary here.

Should this check be more like "has at least one of
local-mac-address and mac-address" instead?


> +static struct {
> +	char *devtype;
> +	int (*check_fn)(struct node *node);
> +} devtype_checker_table[] = {
> +	{"network", check_network},
> +};
> +
> +static int check_devtypes(struct node *root)
> +{
> +
> +	struct node *child;
> +	struct property *prop;
> +	int ok = 1;
> +	int i;
> +
> +	for_each_child(root, child) {
> +		/* check this node */
> +		if ((prop = get_property((child), ("device_type"))))
> +			for (i = 0; i < ARRAY_SIZE(devtype_checker_table); i++) {
> +				if (streq(prop->val.val, devtype_checker_table[i].devtype))
> +					if (! devtype_checker_table[i].check_fn(child)) {
> +						ok = 0;
> +						break;
> +					}
> +			}
> +		ok = check_devtypes(child);
> +	}
> +
> +	return ok;
> +}
> +
>  static int check_root(struct node *root)
>  {
>  	struct property *prop;
> @@ -716,6 +761,7 @@ int check_device_tree(struct node *dt)
>  	ok = ok && check_cpus(dt);
>  	ok = ok && check_memory(dt);
>  	ok = ok && check_chosen(dt);
> +	ok = ok && check_devtypes(dt);
>  	if (! ok)
>  		return 0;

I buy the rest of this.

Thanks,
jdl

Re: [PATCH] DTC - validation by device_type

[Paul Nasrat]

On Thu, 2006-04-20 at 10:35 -0500, Jon Loeliger wrote:
> On Tue, 2006-03-28 at 20:33, Paul Nasrat wrote:
> 
> > Signed-off-by: Paul Nasrat <pnasrat@redhat.com>
> 
> Paul,
> 
> Sorry about the hang-time here.  Couple questions.
> 
> 
> > +static int check_network(struct node *net)
> > +{
> > +
> > +	int ok = 1;
> > +	struct property *prop;
> > +
> > +	CHECK_HAVE(net, "reg");
> > +	CHECK_HAVE(net, "local-mac-address");
> > +	CHECK_HAVE(net, "mac-address");
> > +	CHECK_HAVE(net, "address-bits");
> > +
> > +	return ok;
> > +}
> 
> This says that all network nodes have to have all four
> of those properties.  It's not clear to me that they all
> need to have both local-mac-address and mac-address.


> Specifically, 1275 seems to indicate to me that the
> mac-address property, being introduced by the open
> operation, might be dynamically set at run time, rather
> than passed as a config parameter from, say, U-Boot to
> the kernel.  Thus, it might not be necessary here.


1275 defines shall to indicate a mandatory requirement.

p.165 (Annexe A) defines "network" device_type:

A standard package with this "device_type" property shall implement the
following methods:

open, ...

Requirements for open method:

Execute mac-address and create a "mac-address" property with that value.
...
A standard package with this "device_type" property shall implement the
following property if the associated device has a preassigned IEEE 802.3
MAC (network) address:

So for flattened device tree my reading would be something like:

local-mac-address - for the most part this would be compulsory - does
anyone have any cases where a device would not have a preassigned MAC
address?  Use of address in the booting-without-of and example should be
replaced by local-mac-address.

For a standard 1275 tree mac-address would be created on open, obviously
this isn't a hard requirement for compatibility so I think we could make
this optional.

Paul

Re: [PATCH] DTC - validation by device_type

[Segher Boessenkool]

>>> +	CHECK_HAVE(net, "reg");
>>> +	CHECK_HAVE(net, "local-mac-address");
>>> +	CHECK_HAVE(net, "mac-address");
>>> +	CHECK_HAVE(net, "address-bits");

1) It's hard to see how any (physical) network device would
not have a "reg" property.  It is not required though.

2) "local-mac-address" is not a required property.

3) "mac-address" is only required for nodes that have been
opened before.

4) "address-bits" is not required (taken as 48 if absent).

Please see Annex A (look at the property names, and perhaps
at the "network" device type).  _Always_ look at Annex A!
It's normative.


Segher

Re: [PATCH] DTC - validation by device_type

[Paul Nasrat]

On Mon, 2006-04-24 at 22:55 +0200, Segher Boessenkool wrote:

> 1) It's hard to see how any (physical) network device would
> not have a "reg" property.  It is not required though.
> 
> 2) "local-mac-address" is not a required property.
> 
> 3) "mac-address" is only required for nodes that have been
> opened before.
> 
> 4) "address-bits" is not required (taken as 48 if absent).
> 
> Please see Annex A (look at the property names, and perhaps
> at the "network" device type).  _Always_ look at Annex A!
> It's normative.

My bad.  Jon, just drop this from your queue then. 

Paul