diff mbox series

[4/5] checks: check for #{size,address}-cells without child nodes

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

Commit Message

Rob Herring Nov. 17, 2017, 2:45 p.m. UTC 
nodes with a 'reg' property nor a ranges property.

An exception may be an overlay that adds nodes, but this case would need

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

---
 checks.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

-- 
2.14.1

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

Comments

David Gibson Nov. 20, 2017, 12:14 a.m. UTC | #1 
On Fri, Nov 17, 2017 at 08:45:14AM -0600, Rob Herring wrote:
> nodes with a 'reg' property nor a ranges property.

> 

> An exception may be an overlay that adds nodes, but this case would

> need


Sentence doesn't seem finished..

In any case, I'm not sure this is a good idea.  It's not uncommon to
have bus bridge nodes that ought to have a well defined #address and
#size cells, but just don't happen to have any plugged devices yet.
An overlay that adds nodes is one possibility, but a bus where the
children can be probed is another.

The check for 'ranges' will get some of those cases, but a bus bridge
which doesn't directly map the child address space into the parents
(e.g. indirect access) is perfectly legitimate still.

> 

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

> ---

>  checks.c | 26 ++++++++++++++++++++++++++

>  1 file changed, 26 insertions(+)

> 

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

> index 346b0256f9cb..a785b81bea07 100644

> --- a/checks.c

> +++ b/checks.c

> @@ -982,6 +982,31 @@ static void check_avoid_default_addr_size(struct check *c, struct dt_info *dti,

>  WARNING(avoid_default_addr_size, check_avoid_default_addr_size, NULL,

>  	&addr_size_cells);

>  

> +static void check_avoid_unecessary_addr_size(struct check *c, struct dt_info *dti,

> +					     struct node *node)

> +{

> +	struct property *prop;

> +	struct node *child;

> +	bool has_reg = false;

> +

> +	if (!node->parent || node->addr_cells < 0 || node->size_cells < 0)

> +		return;

> +

> +	if (get_property(node, "ranges") || !node->children)

> +		return;

> +

> +	for_each_child(node, child) {

> +		prop = get_property(child, "reg");

> +		if (prop)

> +			has_reg = true;

> +	}

> +

> +	if (!has_reg)

> +		FAIL(c, dti, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",

> +		     node->fullpath);

> +}

> +WARNING(avoid_unecessary_addr_size, check_avoid_unecessary_addr_size, NULL, &avoid_default_addr_size);

> +

>  static void check_obsolete_chosen_interrupt_controller(struct check *c,

>  						       struct dt_info *dti,

>  						       struct node *node)

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

>  	&simple_bus_reg,

>  

>  	&avoid_default_addr_size,

> +	&avoid_unecessary_addr_size,

>  	&obsolete_chosen_interrupt_controller,

>  

>  	&clocks_property,


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Rob Herring Nov. 20, 2017, 4:22 p.m. UTC | #2 
On Sun, Nov 19, 2017 at 6:14 PM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Fri, Nov 17, 2017 at 08:45:14AM -0600, Rob Herring wrote:

>> nodes with a 'reg' property nor a ranges property.

>>

>> An exception may be an overlay that adds nodes, but this case would

>> need

>

> Sentence doesn't seem finished..


It was there until I rebased. Since the line started with #, git dropped it...

"#{size,address}-cells in the overlay to properly compile already."

> In any case, I'm not sure this is a good idea.  It's not uncommon to

> have bus bridge nodes that ought to have a well defined #address and

> #size cells, but just don't happen to have any plugged devices yet.

> An overlay that adds nodes is one possibility, but a bus where the

> children can be probed is another.


Wouldn't an overlay without #{size,address}-cells have warnings from
avoid_default_addr_size?

As long as there are no child nodes, the check is not run. So we're
limited to false positives if we have a mixture of nodes with and
without unit addresses and only the nodes without unit addresses are
populated. I have seen this with PCI hosts with an interrupt
controller child node.

In general, I'm struggling with how to have tests that check for
things that we generally want to avoid, but we still allow exceptions.
Some of these may be things we want to avoid in new bindings, but
wouldn't fix for existing cases. Another example is things that
were/are valid for OF, but not FDT.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Gibson Nov. 22, 2017, 3:18 a.m. UTC | #3 
On Mon, Nov 20, 2017 at 10:22:53AM -0600, Rob Herring wrote:
> On Sun, Nov 19, 2017 at 6:14 PM, David Gibson

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

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

> >> nodes with a 'reg' property nor a ranges property.

> >>

> >> An exception may be an overlay that adds nodes, but this case would

> >> need

> >

> > Sentence doesn't seem finished..

> 

> It was there until I rebased. Since the line started with #, git dropped it...

> 

> "#{size,address}-cells in the overlay to properly compile already."


Ah, right.

> > In any case, I'm not sure this is a good idea.  It's not uncommon to

> > have bus bridge nodes that ought to have a well defined #address and

> > #size cells, but just don't happen to have any plugged devices yet.

> > An overlay that adds nodes is one possibility, but a bus where the

> > children can be probed is another.

> 

> Wouldn't an overlay without #{size,address}-cells have warnings from

> avoid_default_addr_size?


Well, yes.  The checks are pretty much designed for whole trees and
don't work well on overlays at the moment.  That's a rather more
substantial change to fix.

> As long as there are no child nodes, the check is not run.


Ah!  Sorry, I missed that.  Objection withdrawn in that case.

> So we're

> limited to false positives if we have a mixture of nodes with and

> without unit addresses and only the nodes without unit addresses are

> populated. I have seen this with PCI hosts with an interrupt

> controller child node.


Sounds like an incorrect representation if the intrrupt controller
doesn't have a PCI config space presence.

> In general, I'm struggling with how to have tests that check for

> things that we generally want to avoid, but we still allow exceptions.

> Some of these may be things we want to avoid in new bindings, but

> wouldn't fix for existing cases. Another example is things that

> were/are valid for OF, but not FDT.


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
diff mbox series

Patch

diff --git a/checks.c b/checks.c
index 346b0256f9cb..a785b81bea07 100644
--- a/checks.c
+++ b/checks.c
@@ -982,6 +982,31 @@  static void check_avoid_default_addr_size(struct check *c, struct dt_info *dti,
 WARNING(avoid_default_addr_size, check_avoid_default_addr_size, NULL,
 	&addr_size_cells);
 
+static void check_avoid_unecessary_addr_size(struct check *c, struct dt_info *dti,
+					     struct node *node)
+{
+	struct property *prop;
+	struct node *child;
+	bool has_reg = false;
+
+	if (!node->parent || node->addr_cells < 0 || node->size_cells < 0)
+		return;
+
+	if (get_property(node, "ranges") || !node->children)
+		return;
+
+	for_each_child(node, child) {
+		prop = get_property(child, "reg");
+		if (prop)
+			has_reg = true;
+	}
+
+	if (!has_reg)
+		FAIL(c, dti, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",
+		     node->fullpath);
+}
+WARNING(avoid_unecessary_addr_size, check_avoid_unecessary_addr_size, NULL, &avoid_default_addr_size);
+
 static void check_obsolete_chosen_interrupt_controller(struct check *c,
 						       struct dt_info *dti,
 						       struct node *node)
@@ -1306,6 +1331,7 @@  static struct check *check_table[] = {
 	&simple_bus_reg,
 
 	&avoid_default_addr_size,
+	&avoid_unecessary_addr_size,
 	&obsolete_chosen_interrupt_controller,
 
 	&clocks_property,