checks: add phandle with arg property checks
PATCH 1/2 checks: add phandle with arg property checks
Many common bindings follow the same pattern of client properties containing a phandle and N arg cells where N is defined in the provider with a '#<specifier>-cells' property. Add a checks for properties following this pattern.
--- checks.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+)
checks.c +117 -0
@@ -956,6 +956,120 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,WARNING (obsolete_chosen_interrupt_controller, check_obsolete_chosen_interrupt_controller,NULL ); +struct provider { +const char *prop_name; +const char *cell_name; +bool optional; +}; + +static void check_property_phandle_args (struct check *c, +struct dt_info *dti, +struct node *node, +struct property *prop, +const struct provider *provider) +{ +struct marker *m = prop->val.markers; + +if (!m) { +FAIL (c, dti,"Missing phandles in %s:%s" , + node->fullpath, prop->name); +return ; + } +for_each_marker_of_type (m,REF_PHANDLE ) { +int cellsize; +struct node *root = dti->dt; +struct node *provider_node; +struct property *cellprop; + + provider_node =get_node_by_ref (root, m->ref); +if (!provider_node) { +FAIL (c, dti,"Could not get provider for %s:%s" , + node->fullpath, prop->name); +break ; + } + + cellprop =get_property (provider_node, provider->cell_name); +if (cellprop) { + cellsize =propval_cell (cellprop); + }else if (provider->optional) { + cellsize =0 ; + }else { +FAIL (c, dti,"Missing %s in provider %s for %s" , + provider->cell_name, + provider_node->fullpath, + node->fullpath); +break ; + } + +if (prop->val.len < ((cellsize +1 ) *sizeof (cell_t))) { +FAIL (c, dti,"%s property size (%d) too small for cell size %d in %s" , + prop->name, prop->val.len, cellsize, node->fullpath); + } + } +} + +static const struct provider providers[] = { + {"clocks" ,"#clock-cells" }, + {"phys" ,"#phy-cells" }, + {"interrupts-extended" ,"#interrupt-cells" }, + {"mboxes" ,"#mbox-cells" }, + {"pwms" ,"#pwm-cells" }, + {"dmas" ,"#dma-cells" }, + {"resets" ,"#reset-cells" }, + {"hwlocks" ,"#hwlock-cells" }, + {"power-domains" ,"#power-domain-cells" }, + {"io-channels" ,"#io-channel-cells" }, + {"iommus" ,"#iommu-cells" }, + {"mux-controls" ,"#mux-control-cells" }, + {"cooling-device" ,"#cooling-cells" }, + {"thermal-sensors" ,"#thermal-sensor-cells" }, + {"sound-dais" ,"#sound-dai-cells" }, + {"msi-parent" ,"#msi-cells" , true }, + {NULL }, +}; + +static void check_provider_cells_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +int i; + +for (i =0 ; providers[i].prop_name; i++) { +struct property *prop =get_property (node, providers[i].prop_name); +if (!prop) +continue ; +check_property_phandle_args (c, dti, node, prop, &providers[i]); + } +} +WARNING (provider_cells_property, check_provider_cells_property,NULL ); + +static void check_gpio_cells_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +struct property *prop; + +for_each_property (node, prop) { +char *str; +struct provider provider; + +/* Skip over false matches */ +if (strstr (prop->name,"nr-gpio" )) +continue ; + + str =strrchr (prop->name,'-' ); +if (!str || !(streq (str,"-gpio" ) ||streq (str,"-gpios" ))) +continue ; + + provider.prop_name = prop->name; + provider.cell_name ="#gpio-cells" ; + provider.optional = false; +check_property_phandle_args (c, dti, node, prop, &provider); + } + +} +WARNING (gpio_cells_property, check_gpio_cells_property,NULL ); +static struct check *check_table[] = { &duplicate_node_names, &duplicate_property_names, &node_name_chars, &node_name_format, &property_name_chars,
@@ -987,6 +1101,9 @@ static struct check *check_table[] = { &avoid_default_addr_size, &obsolete_chosen_interrupt_controller, + &provider_cells_property, + &gpio_cells_property, + &always_fail, };
Signature · 1 line
2.11.0
A message that is not in the archive (sent off-list or not yet received): 20170814214807.338-1-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
PATCH 2/2 checks: add interrupts property check
Add a check for nodes with interrupts property that they have a valid parent, the parent has #interrupt-cells property, and the size is a valid multiple of #interrupt-cells.
This may not handle every possible case and doesn't deal with translation thru interrupt-map properties, but should be enough for modern dts files.
--- checks.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
checks.c +58 -0
@@ -1070,6 +1070,63 @@ static void check_gpio_cells_property(struct check *c, }WARNING (gpio_cells_property, check_gpio_cells_property,NULL ); +static void check_interrupts_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +struct node *root = dti->dt; +struct node *irq_node =NULL , *parent = node; +struct property *irq_prop, *prop =NULL ; +int irq_cells, phandle; + + irq_prop =get_property (node,"interrupts" ); +if (!irq_prop) +return ; + +while (parent && !prop) { +if (parent != node) { + prop =get_property (parent,"interrupt-controller" ); +if (prop) { + irq_node = parent; +break ; + } + } + + prop =get_property (parent,"interrupt-parent" ); +if (prop) { + phandle =propval_cell (prop); + irq_node =get_node_by_phandle (root, phandle); +if (!irq_node) { +FAIL (c, dti,"Bad interrupt-parent phandle for %s" , + node->fullpath); +return ; + } +break ; + } + + parent = parent->parent; + } + +if (!irq_node) { +FAIL (c, dti,"Missing interrupt-parent for %s" , node->fullpath); +return ; + } + + prop =get_property (irq_node,"#interrupt-cells" ); +if (!prop) { +FAIL (c, dti,"Missing #interrupt-cells in interrupt-parent %s" , + irq_node->fullpath); +return ; + } + + irq_cells =propval_cell (prop); +if (irq_prop->val.len % (irq_cells *sizeof (cell_t))) { +FAIL (c, dti,"interrupts size is (%d), expected multiple of %d in %s" , + irq_prop->val.len, (int )(irq_cells *sizeof (cell_t)), node->fullpath); + } +} +WARNING (interrupts_property, check_interrupts_property, &phandle_references); +static struct check *check_table[] = { &duplicate_node_names, &duplicate_property_names, &node_name_chars, &node_name_format, &property_name_chars,
@@ -1103,6 +1160,7 @@ static struct check *check_table[] = { &provider_cells_property, &gpio_cells_property, + &interrupts_property, &always_fail, };
Signature · 1 line
2.11.0
A message that is not in the archive (sent off-list or not yet received): 20170814214807.338-2-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Re: [PATCH 2/2] checks: add interrupts property check
Quoted 31 lines · Rob Herring
Add a check for nodes with interrupts property that they have a valid parent, the parent has #interrupt-cells property, and the size is a valid multiple of #interrupt-cells.
This may not handle every possible case and doesn't deal with translation thru interrupt-map properties, but should be enough for modern dts files.
Signed-off-by Rob Herring <[email protected]>--- checks.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)checks.c +10
-0@@ -1070,6 +1070,63 @@ static void check_gpio_cells_property(struct check *c, }WARNING (gpio_cells_property, check_gpio_cells_property,NULL ); +static void check_interrupts_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +struct node *root = dti->dt; +struct node *irq_node =NULL , *parent = node; +struct property *irq_prop, *prop =NULL ; +int irq_cells, phandle; + + irq_prop =get_property (node,"interrupts" );
checks.c static void check_gpio_cells_property(struct check *c, · line 1092
Quoted 10 lines
checks.c +10
-0+if (!irq_prop) +return ; + +while (parent && !prop) { +if (parent != node) { + prop =get_property (parent,"interrupt-controller" ); +if (prop) { + irq_node = parent; +break ; + }
This also needs a similar interrupt-map check here which I somehow dropped in rebasing.
Quoted 51 lines
checks.c +38
-1+ } + + prop =get_property (parent,"interrupt-parent" ); +if (prop) { + phandle =propval_cell (prop); + irq_node =get_node_by_phandle (root, phandle); +if (!irq_node) { +FAIL (c, dti,"Bad interrupt-parent phandle for %s" , + node->fullpath); +return ; + } +break ; + } + + parent = parent->parent; + } + +if (!irq_node) { +FAIL (c, dti,"Missing interrupt-parent for %s" , node->fullpath); +return ; + } + + prop =get_property (irq_node,"#interrupt-cells" ); +if (!prop) { +FAIL (c, dti,"Missing #interrupt-cells in interrupt-parent %s" , + irq_node->fullpath); +return ; + } + + irq_cells =propval_cell (prop); +if (irq_prop->val.len % (irq_cells *sizeof (cell_t))) { +FAIL (c, dti,"interrupts size is (%d), expected multiple of %d in %s" , + irq_prop->val.len, (int )(irq_cells *sizeof (cell_t)), node->fullpath); + } +} +WARNING (interrupts_property, check_interrupts_property, &phandle_references); +static struct check *check_table[] = { &duplicate_node_names, &duplicate_property_names, &node_name_chars, &node_name_format, &property_name_chars,@@ -1103,6 +1160,7 @@ static struct check *check_table[] = { &provider_cells_property, &gpio_cells_property, + &interrupts_property, &always_fail, }; --2.11.0
Re: [PATCH 2/2] checks: add interrupts property check
In reply to a message not in the archiveQuoted 26 lines · Rob Herring
Add a check for nodes with interrupts property that they have a valid parent, the parent has #interrupt-cells property, and the size is a valid multiple of #interrupt-cells.
This may not handle every possible case and doesn't deal with translation thru interrupt-map properties, but should be enough for modern dts files.
Signed-off-by Rob Herring <[email protected]>--- checks.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)checks.c +5
-0@@ -1070,6 +1070,63 @@ static void check_gpio_cells_property(struct check *c, }WARNING (gpio_cells_property, check_gpio_cells_property,NULL ); +static void check_interrupts_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +struct node *root = dti->dt;
checks.c static void check_gpio_cells_property(struct check *c, · line 1087
Quoted 10 lines
checks.c +10
-0+struct node *irq_node =NULL , *parent = node; +struct property *irq_prop, *prop =NULL ; +int irq_cells, phandle; + + irq_prop =get_property (node,"interrupts" ); +if (!irq_prop) +return ; + +while (parent && !prop) { +if (parent != node) {
So, it's kind of academic, but is it actually disallowed for an interrupt-controller node to itself have interrupts which are implicityly routed to itself?
Quoted 8 lines
checks.c +8
-0+ prop =get_property (parent,"interrupt-controller" ); +if (prop) { + irq_node = parent; +break ; + } + } + + prop =get_property (parent,"interrupt-parent" );
checks.c static void check_gpio_cells_property(struct check *c, · line 1105
Quoted 10 lines
checks.c +10
-0+if (prop) { + phandle =propval_cell (prop); + irq_node =get_node_by_phandle (root, phandle); +if (!irq_node) { +FAIL (c, dti,"Bad interrupt-parent phandle for %s" , + node->fullpath); +return ; + } +break ; + }
As noted you also need a check for interrupt-map.
Quoted 7 lines
checks.c +7
-0+ + parent = parent->parent; + } + +if (!irq_node) { +FAIL (c, dti,"Missing interrupt-parent for %s" , node->fullpath); +return ;
checks.c static void check_gpio_cells_property(struct check *c, · line 1122
Quoted 10 lines
checks.c +10
-0+ } + + prop =get_property (irq_node,"#interrupt-cells" ); +if (!prop) { +FAIL (c, dti,"Missing #interrupt-cells in interrupt-parent %s" , + irq_node->fullpath); +return ; + } + + irq_cells =propval_cell (prop);
So this is unsafe if #interrupt-cells is misformatted in the interrupt controller. There's already a test for that (using WARNING_IF_NOT_CELL), so you just need to make this check dependent on that one.
Quoted 11 lines
checks.c +7
-0+if (irq_prop->val.len % (irq_cells *sizeof (cell_t))) { +FAIL (c, dti,"interrupts size is (%d), expected multiple of %d in %s" , + irq_prop->val.len, (int )(irq_cells *sizeof (cell_t)), node->fullpath); + } +} +WARNING (interrupts_property, check_interrupts_property, &phandle_references); +static struct check *check_table[] = { &duplicate_node_names, &duplicate_property_names, &node_name_chars, &node_name_format, &property_name_chars,
checks.c static struct check *check_table[] = { · line 1166
Quoted 7 lines
checks.c +1
-0@@ -1103,6 +1160,7 @@ static struct check *check_table[] = { &provider_cells_property, &gpio_cells_property, + &interrupts_property, &always_fail, };
With both these patches testcases to make sure the checks actually trip on a bad example would be good.
Signature · 4 lines
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
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 2/2] checks: add interrupts property check
Quoted 33 lines · David Gibson
Quoted 32 lines · Rob Herring
Add a check for nodes with interrupts property that they have a valid parent, the parent has #interrupt-cells property, and the size is a valid multiple of #interrupt-cells.
This may not handle every possible case and doesn't deal with translation thru interrupt-map properties, but should be enough for modern dts files.
Signed-off-by Rob Herring <[email protected]>--- checks.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)checks.c +11
-0@@ -1070,6 +1070,63 @@ static void check_gpio_cells_property(struct check *c, }WARNING (gpio_cells_property, check_gpio_cells_property,NULL ); +static void check_interrupts_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +struct node *root = dti->dt; +struct node *irq_node =NULL , *parent = node; +struct property *irq_prop, *prop =NULL ; +int irq_cells, phandle; + + irq_prop =get_property (node,"interrupts" ); +if (!irq_prop)
Quoted 8 lines
checks.c static void check_gpio_cells_property(struct check *c, · line 1087
Quoted 4 lines
checks.c +4
-0+return ; + +while (parent && !prop) { +if (parent != node) {So, it's kind of academic, but is it actually disallowed for an interrupt-controller node to itself have interrupts which are implicityly routed to itself?
I think so, yes. Because otherwise how do you describe a chained interrupt controller? "interrupts" in the chained controller's node are the parent's interrupts. I guess you have to rely on the compatible implying any interrupts connected to itself. It could work with interrupts-extended though.
[...]
Quoted 11 lines
checks.c static struct check *check_table[] = { · line 1166
Quoted 8 lines
checks.c +1
-0@@ -1103,6 +1160,7 @@ static struct check *check_table[] = { &provider_cells_property, &gpio_cells_property, + &interrupts_property, &always_fail, };With both these patches testcases to make sure the checks actually trip on a bad example would be good.
Yeah, I wanted to get some initial feedback first.
Rob
A message that is not in the archive (sent off-list or not yet received): CAL_JsqJuUu-6FpLqQZZyVdCueV18CHRZ4qQUvacSrn2XAfpiSQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Re: [PATCH 2/2] checks: add interrupts property check
Quoted 44 lines · Rob Herring
Quoted 33 lines · David Gibson
Quoted 32 lines · Rob Herring
Add a check for nodes with interrupts property that they have a valid parent, the parent has #interrupt-cells property, and the size is a valid multiple of #interrupt-cells.
This may not handle every possible case and doesn't deal with translation thru interrupt-map properties, but should be enough for modern dts files.
Signed-off-by Rob Herring <[email protected]>--- checks.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)checks.c +11
-0@@ -1070,6 +1070,63 @@ static void check_gpio_cells_property(struct check *c, }WARNING (gpio_cells_property, check_gpio_cells_property,NULL ); +static void check_interrupts_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +struct node *root = dti->dt; +struct node *irq_node =NULL , *parent = node; +struct property *irq_prop, *prop =NULL ; +int irq_cells, phandle; + + irq_prop =get_property (node,"interrupts" ); +if (!irq_prop)Quoted 8 lines
checks.c static void check_gpio_cells_property(struct check *c, · line 1087
Quoted 4 lines
checks.c +4
-0+return ; + +while (parent && !prop) { +if (parent != node) {So, it's kind of academic, but is it actually disallowed for an interrupt-controller node to itself have interrupts which are implicityly routed to itself?
Quoted 5 lines
I think so, yes. Because otherwise how do you describe a chained interrupt controller? "interrupts" in the chained controller's node are the parent's interrupts. I guess you have to rely on the compatible implying any interrupts connected to itself. It could work with interrupts-extended though.
Ah.. yes. My guess would be that you could do it by using an explicit interrupt parent - which would mean that interrupt-parent would need to take precedence over an interrupt controller in the same node. I guess we should probably check the old interrupt binding document.
Note that a possible, though strange case is for a node to have interrupt-parent pointing at itself, with itself being an interrupt nexus redirecting the interrupts elsewhere. I've used this in the past to handle a device with interrupts going to multiple interrupt controllers before interrupts-extended was invented.
Quoted 9 lines
[...]
Quoted 6 lines
Quoted 6 lines
checks.c +1
-0@@ -1103,6 +1160,7 @@ static struct check *check_table[] = { &provider_cells_property, &gpio_cells_property, + &interrupts_property,
Quoted 7 lines
Quoted 5 lines
checks.c static struct check *check_table[] = { · line 1166
Quoted 2 lines
checks.c +0
-0&always_fail, };With both these patches testcases to make sure the checks actually trip on a bad example would be good.
Yeah, I wanted to get some initial feedback first.
Fair enough.
Signature · 4 lines
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
On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:
Yay! Someone actually implementing checks.
Quoted 4 lines
Many common bindings follow the same pattern of client properties containing a phandle and N arg cells where N is defined in the provider with a '#<specifier>-cells' property. Add a checks for properties following this pattern.
I think this description would be easier to follow if you led with an example.
Quoted 2 lines
Signed-off-by Rob Herring <[email protected]>
Looks pretty good, though I have some suggestions.
Quoted 22 lines
--- checks.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+)checks.c +10
-0@@ -956,6 +956,120 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,WARNING (obsolete_chosen_interrupt_controller, check_obsolete_chosen_interrupt_controller,NULL ); +struct provider { +const char *prop_name; +const char *cell_name; +bool optional; +}; + +static void check_property_phandle_args(struct check *c, + struct dt_info *dti, + struct node *node, + struct property *prop,
checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 978
Quoted 10 lines
checks.c +10
-0+ const struct provider *provider) +{ +struct marker *m = prop->val.markers; + +if (!m) { +FAIL (c, dti,"Missing phandles in %s:%s" , + node->fullpath, prop->name); +return ; + } +for_each_marker_of_type (m,REF_PHANDLE ) {
So going through the markers I think is not the best approach. That'll work if the source contains a reference here, which it usually will, but there are some circumstances where it could contain a "raw" phandle value (the most obvious being when you're decompiling an existing dtb).
But I don't think you really need to. Instead you should be able to read the actual value, look it up with get_node_by_phandle(). You can make this check dependent on fixup_phandle_references to make sure it's executed after the references are resolved.
That should also let you more strictly verify the property in the client node, including checking for misaligned entries of extraneous bits.
checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 989
Quoted 11 lines
checks.c +11
-0+int cellsize; +struct node *root = dti->dt; +struct node *provider_node; +struct property *cellprop; + + provider_node =get_node_by_ref (root, m->ref); +if (!provider_node) { +FAIL (c, dti,"Could not get provider for %s:%s" , + node->fullpath, prop->name); +break ; + }
AFAIK the "provider" terminology isn't standardized somewhere. It's a reasonable term internally, but if can rephrase to it in the displayed messages that would be a bonus.
Quoted 29 lines
checks.c +29
-0+ cellprop =get_property (provider_node, provider->cell_name); +if (cellprop) { + cellsize =propval_cell (cellprop); + }else if (provider->optional) { + cellsize =0 ; + }else { +FAIL (c, dti,"Missing %s in provider %s for %s" , + provider->cell_name, + provider_node->fullpath, + node->fullpath); +break ; + } + +if (prop->val.len < ((cellsize +1 ) *sizeof (cell_t))) { +FAIL (c, dti,"%s property size (%d) too small for cell size %d in %s" , + prop->name, prop->val.len, cellsize, node->fullpath); + } + } +} + +static const struct provider providers[] = { + {"clocks" ,"#clock-cells" }, + {"phys" ,"#phy-cells" }, + {"interrupts-extended" ,"#interrupt-cells" }, + {"mboxes" ,"#mbox-cells" }, + {"pwms" ,"#pwm-cells" }, + {"dmas" ,"#dma-cells" }, + {"resets" ,"#reset-cells" }, + {"hwlocks" ,"#hwlock-cells" },
checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 1029
Quoted 10 lines
checks.c +10
-0+ { "power-domains", "#power-domain-cells" }, + { "io-channels", "#io-channel-cells" }, + { "iommus", "#iommu-cells" }, + { "mux-controls", "#mux-control-cells" }, + { "cooling-device", "#cooling-cells" }, + { "thermal-sensors", "#thermal-sensor-cells" }, + { "sound-dais", "#sound-dai-cells" }, + { "msi-parent", "#msi-cells", true }, + { NULL }, +};
How hard would it be to use macros to make each of these providers different check structures. I think the output would be a bit more useful if errors in different types of these things was reported as a different check failure rather than one big generic one.
It does mean listing everything in the check_table which is a pain. I would really like to change things so that a single macro can both declare the check and add it to the master list, but I haven't thought of a portable way to do that so far.
If you do need the providers array, ARRAY_SIZE() is preferred over a terminator NULL.
Quoted 31 lines
checks.c +31
-0+static void check_provider_cells_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +int i; + +for (i =0 ; providers[i].prop_name; i++) { +struct property *prop =get_property (node, providers[i].prop_name); +if (!prop) +continue ; +check_property_phandle_args (c, dti, node, prop, &providers[i]); + } +} +WARNING (provider_cells_property, check_provider_cells_property,NULL ); + +static void check_gpio_cells_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +struct property *prop; + +for_each_property (node, prop) { +char *str; +struct provider provider; + +/* Skip over false matches */ +if (strstr (prop->name,"nr-gpio" )) +continue ; + + str =strrchr (prop->name,'-' ); +if (!str || !(streq (str,"-gpio" ) ||streq (str,"-gpios" )))
checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 1071
Quoted 10 lines
checks.c +10
-0+continue ; + + provider.prop_name = prop->name; + provider.cell_name ="#gpio-cells" ; + provider.optional = false; +check_property_phandle_args (c, dti, node, prop, &provider); + } + +} +WARNING (gpio_cells_property, check_gpio_cells_property,NULL );
Since the gpio stuff is a bit tweaker, it would be good to go into a separate patch. Then the commit message can explain what the logic here is for (I'm having a little trouble following it).
Quoted 13 lines
checks.c +3
-0static struct check *check_table[] = { &duplicate_node_names, &duplicate_property_names, &node_name_chars, &node_name_format, &property_name_chars,@@ -987,6 +1101,9 @@ static struct check *check_table[] = { &avoid_default_addr_size, &obsolete_chosen_interrupt_controller, + &provider_cells_property, + &gpio_cells_property, + &always_fail, };
Signature · 4 lines
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
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 3 lines · David Gibson
On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:
Yay! Someone actually implementing checks.
Not like it is my first. I'll celebrate when someone else does.
Quoted 51 lines
Quoted 4 lines
Many common bindings follow the same pattern of client properties containing a phandle and N arg cells where N is defined in the provider with a '#<specifier>-cells' property. Add a checks for properties following this pattern.
I think this description would be easier to follow if you led with an example.
Quoted 2 lines
Signed-off-by Rob Herring <[email protected]>Looks pretty good, though I have some suggestions.
Quoted 22 lines
--- checks.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+)checks.c +10
-0@@ -956,6 +956,120 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,WARNING (obsolete_chosen_interrupt_controller, check_obsolete_chosen_interrupt_controller,NULL ); +struct provider { +const char *prop_name; +const char *cell_name; +bool optional; +}; + +static void check_property_phandle_args(struct check *c, + struct dt_info *dti, + struct node *node, + struct property *prop,checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 978
Quoted 10 lines
checks.c +10
-0+ const struct provider *provider) +{ +struct marker *m = prop->val.markers; + +if (!m) { +FAIL (c, dti,"Missing phandles in %s:%s" , + node->fullpath, prop->name); +return ; + } +for_each_marker_of_type (m,REF_PHANDLE ) {So going through the markers I think is not the best approach. That'll work if the source contains a reference here, which it usually will, but there are some circumstances where it could contain a "raw" phandle value (the most obvious being when you're decompiling an existing dtb).
Quoted 4 lines
But I don't think you really need to. Instead you should be able to read the actual value, look it up with
get_node_by_phandle(). You can make this check dependent onfixup_phandle_referencesto make sure it's executed after the references are resolved.
That's how I implemented it initially...
Quoted 15 lines
That should also let you more strictly verify the property in the client node, including checking for misaligned entries of extraneous bits.
checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 989
Quoted 11 lines
checks.c +11
-0+int cellsize; +struct node *root = dti->dt; +struct node *provider_node; +struct property *cellprop; + + provider_node =get_node_by_ref (root, m->ref); +if (!provider_node) { +FAIL (c, dti,"Could not get provider for %s:%s" , + node->fullpath, prop->name); +break ; + }
Quoted 3 lines
AFAIK the "provider" terminology isn't standardized somewhere. It's a reasonable term internally, but if can rephrase to it in the displayed messages that would be a bonus.
The kernel uses that at least. I guess maybe "phandle node" will work.
Quoted 36 lines
Quoted 36 lines
checks.c +36
-0+ cellprop =get_property (provider_node, provider->cell_name); +if (cellprop) { + cellsize =propval_cell (cellprop); + }else if (provider->optional) { + cellsize =0 ; + }else { +FAIL (c, dti,"Missing %s in provider %s for %s" , + provider->cell_name, + provider_node->fullpath, + node->fullpath); +break ; + } + +if (prop->val.len < ((cellsize +1 ) *sizeof (cell_t))) { +FAIL (c, dti,"%s property size (%d) too small for cell size %d in %s" , + prop->name, prop->val.len, cellsize, node->fullpath); + } + } +} + +static const struct provider providers[] = { + {"clocks" ,"#clock-cells" }, + {"phys" ,"#phy-cells" }, + {"interrupts-extended" ,"#interrupt-cells" }, + {"mboxes" ,"#mbox-cells" }, + {"pwms" ,"#pwm-cells" }, + {"dmas" ,"#dma-cells" }, + {"resets" ,"#reset-cells" }, + {"hwlocks" ,"#hwlock-cells" }, + {"power-domains" ,"#power-domain-cells" }, + {"io-channels" ,"#io-channel-cells" }, + {"iommus" ,"#iommu-cells" }, + {"mux-controls" ,"#mux-control-cells" }, + {"cooling-device" ,"#cooling-cells" }, + {"thermal-sensors" ,"#thermal-sensor-cells" }, + {"sound-dais" ,"#sound-dai-cells" },
Quoted 8 lines
checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 1029
Quoted 3 lines
checks.c +3
-0+ { "msi-parent", "#msi-cells", true }, + { NULL }, +};How hard would it be to use macros to make each of these providers different check structures. I think the output would be a bit more useful if errors in different types of these things was reported as a different check failure rather than one big generic one.
Should be doable.
Quoted 4 lines
It does mean listing everything in the
check_tablewhich is a pain. I would really like to change things so that a single macro can both declare the check and add it to the master list, but I haven't thought of a portable way to do that so far.
This is done in the kernel frequently using linker sections. Each check entry would get put into a specific section, then you just iterate through the entries. Would that work? I could imagine that linker magic may not be all the portable.
Quoted 2 lines
If you do need the providers array,
ARRAY_SIZE()is preferred over a terminator NULL.
Okay.
Quoted 37 lines
Quoted 37 lines
checks.c +37
-0+static void check_provider_cells_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +int i; + +for (i =0 ; providers[i].prop_name; i++) { +struct property *prop =get_property (node, providers[i].prop_name); +if (!prop) +continue ; +check_property_phandle_args (c, dti, node, prop, &providers[i]); + } +} +WARNING (provider_cells_property, check_provider_cells_property,NULL ); + +static void check_gpio_cells_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +struct property *prop; + +for_each_property (node, prop) { +char *str; +struct provider provider; + +/* Skip over false matches */ +if (strstr (prop->name,"nr-gpio" )) +continue ; + + str =strrchr (prop->name,'-' ); +if (!str || !(streq (str,"-gpio" ) ||streq (str,"-gpios" ))) +continue ; + + provider.prop_name = prop->name; + provider.cell_name ="#gpio-cells" ; + provider.optional = false; +check_property_phandle_args (c, dti, node, prop, &provider);
Quoted 8 lines
checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 1071
Quoted 4 lines
checks.c +4
-0+ } + +} +WARNING (gpio_cells_property, check_gpio_cells_property,NULL );Since the gpio stuff is a bit tweaker, it would be good to go into a separate patch. Then the commit message can explain what the logic here is for (I'm having a little trouble following it).
Okay.
Rob
A message that is not in the archive (sent off-list or not yet received): CAL_Jsq+yA2W=SD9zSkWxw-R16uNELeqPqJK3fVMvi0_jhEbooQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 60 lines · Rob Herring
Quoted 3 lines · David Gibson
On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:
Yay! Someone actually implementing checks.
Not like it is my first. I'll celebrate when someone else does.
Quoted 51 lines
Quoted 4 lines
Many common bindings follow the same pattern of client properties containing a phandle and N arg cells where N is defined in the provider with a '#<specifier>-cells' property. Add a checks for properties following this pattern.
I think this description would be easier to follow if you led with an example.
Quoted 2 lines
Signed-off-by Rob Herring <[email protected]>Looks pretty good, though I have some suggestions.
Quoted 22 lines
--- checks.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+)checks.c +10
-0@@ -956,6 +956,120 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,WARNING (obsolete_chosen_interrupt_controller, check_obsolete_chosen_interrupt_controller,NULL ); +struct provider { +const char *prop_name; +const char *cell_name; +bool optional; +}; + +static void check_property_phandle_args(struct check *c, + struct dt_info *dti, + struct node *node, + struct property *prop,checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 978
Quoted 10 lines
checks.c +10
-0+ const struct provider *provider) +{ +struct marker *m = prop->val.markers; + +if (!m) { +FAIL (c, dti,"Missing phandles in %s:%s" , + node->fullpath, prop->name); +return ; + } +for_each_marker_of_type (m,REF_PHANDLE ) {So going through the markers I think is not the best approach. That'll work if the source contains a reference here, which it usually will, but there are some circumstances where it could contain a "raw" phandle value (the most obvious being when you're decompiling an existing dtb).
Quoted 6 lines
Quoted 4 lines
But I don't think you really need to. Instead you should be able to read the actual value, look it up with
get_node_by_phandle(). You can make this check dependent onfixup_phandle_referencesto make sure it's executed after the references are resolved.That's how I implemented it initially...
Ok.. why did you change?
Quoted 15 lines
Quoted 15 lines
That should also let you more strictly verify the property in the client node, including checking for misaligned entries of extraneous bits.
Quoted 11 lines
checks.c +11
-0+int cellsize; +struct node *root = dti->dt; +struct node *provider_node; +struct property *cellprop; + + provider_node =get_node_by_ref (root, m->ref); +if (!provider_node) { +FAIL (c, dti,"Could not get provider for %s:%s" , + node->fullpath, prop->name); +break ; + }
Quoted 5 lines
Quoted 3 lines
AFAIK the "provider" terminology isn't standardized somewhere. It's a reasonable term internally, but if can rephrase to it in the displayed messages that would be a bonus.
The kernel uses that at least. I guess maybe "phandle node" will work.
Hm, if the kernel uses the term, that's probably enough, actually.
Quoted 45 lines
Quoted 36 lines
Quoted 36 lines
checks.c +36
-0+ cellprop =get_property (provider_node, provider->cell_name); +if (cellprop) { + cellsize =propval_cell (cellprop); + }else if (provider->optional) { + cellsize =0 ; + }else { +FAIL (c, dti,"Missing %s in provider %s for %s" , + provider->cell_name, + provider_node->fullpath, + node->fullpath); +break ; + } + +if (prop->val.len < ((cellsize +1 ) *sizeof (cell_t))) { +FAIL (c, dti,"%s property size (%d) too small for cell size %d in %s" , + prop->name, prop->val.len, cellsize, node->fullpath); + } + } +} + +static const struct provider providers[] = { + {"clocks" ,"#clock-cells" }, + {"phys" ,"#phy-cells" }, + {"interrupts-extended" ,"#interrupt-cells" }, + {"mboxes" ,"#mbox-cells" }, + {"pwms" ,"#pwm-cells" }, + {"dmas" ,"#dma-cells" }, + {"resets" ,"#reset-cells" }, + {"hwlocks" ,"#hwlock-cells" }, + {"power-domains" ,"#power-domain-cells" }, + {"io-channels" ,"#io-channel-cells" }, + {"iommus" ,"#iommu-cells" }, + {"mux-controls" ,"#mux-control-cells" }, + {"cooling-device" ,"#cooling-cells" }, + {"thermal-sensors" ,"#thermal-sensor-cells" }, + {"sound-dais" ,"#sound-dai-cells" },Quoted 8 lines
checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 1029
Quoted 3 lines
checks.c +3
-0+ { "msi-parent", "#msi-cells", true }, + { NULL }, +};How hard would it be to use macros to make each of these providers different check structures. I think the output would be a bit more useful if errors in different types of these things was reported as a different check failure rather than one big generic one.
Quoted 1 line
Should be doable.
Ok. Not essential, but I think it would improve the output.
Quoted 9 lines
Quoted 4 lines
It does mean listing everything in the
check_tablewhich is a pain. I would really like to change things so that a single macro can both declare the check and add it to the master list, but I haven't thought of a portable way to do that so far.This is done in the kernel frequently using linker sections. Each check entry would get put into a specific section, then you just iterate through the entries. Would that work? I could imagine that linker magic may not be all the portable.
Right. Linker sections are the usual way to do this, but that requires lower level knowledge of how the toolchain works than I really want to put into dtc. At heart dtc is really a very straightforward standard C program, so I don't want to introduce dependencies on a specific compiler.
Quoted 55 lines
Quoted 3 lines
If you do need the providers array,
ARRAY_SIZE()is preferred over a terminator NULL.Okay.
Quoted 37 lines
Quoted 37 lines
checks.c +37
-0+static void check_provider_cells_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +int i; + +for (i =0 ; providers[i].prop_name; i++) { +struct property *prop =get_property (node, providers[i].prop_name); +if (!prop) +continue ; +check_property_phandle_args (c, dti, node, prop, &providers[i]); + } +} +WARNING (provider_cells_property, check_provider_cells_property,NULL ); + +static void check_gpio_cells_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +struct property *prop; + +for_each_property (node, prop) { +char *str; +struct provider provider; + +/* Skip over false matches */ +if (strstr (prop->name,"nr-gpio" )) +continue ; + + str =strrchr (prop->name,'-' ); +if (!str || !(streq (str,"-gpio" ) ||streq (str,"-gpios" ))) +continue ; + + provider.prop_name = prop->name; + provider.cell_name ="#gpio-cells" ; + provider.optional = false; +check_property_phandle_args (c, dti, node, prop, &provider);Quoted 8 lines
checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 1071
Quoted 4 lines
checks.c +4
-0+ } + +} +WARNING (gpio_cells_property, check_gpio_cells_property,NULL );Since the gpio stuff is a bit tweaker, it would be good to go into a separate patch. Then the commit message can explain what the logic here is for (I'm having a little trouble following it).
Okay.
Rob
Signature · 4 lines
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
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 61 lines · David Gibson
Quoted 60 lines · Rob Herring
Quoted 3 lines · David Gibson
On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:
Yay! Someone actually implementing checks.
Not like it is my first. I'll celebrate when someone else does.
Quoted 51 lines
Quoted 4 lines
Many common bindings follow the same pattern of client properties containing a phandle and N arg cells where N is defined in the provider with a '#<specifier>-cells' property. Add a checks for properties following this pattern.
I think this description would be easier to follow if you led with an example.
Quoted 2 lines
Signed-off-by Rob Herring <[email protected]>Looks pretty good, though I have some suggestions.
Quoted 22 lines
--- checks.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+)checks.c +10
-0@@ -956,6 +956,120 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,WARNING (obsolete_chosen_interrupt_controller, check_obsolete_chosen_interrupt_controller,NULL ); +struct provider { +const char *prop_name; +const char *cell_name; +bool optional; +}; + +static void check_property_phandle_args(struct check *c, + struct dt_info *dti, + struct node *node, + struct property *prop,checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 978
Quoted 10 lines
checks.c +10
-0+ const struct provider *provider) +{ +struct marker *m = prop->val.markers; + +if (!m) { +FAIL (c, dti,"Missing phandles in %s:%s" , + node->fullpath, prop->name); +return ; + } +for_each_marker_of_type (m,REF_PHANDLE ) {So going through the markers I think is not the best approach. That'll work if the source contains a reference here, which it usually will, but there are some circumstances where it could contain a "raw" phandle value (the most obvious being when you're decompiling an existing dtb).
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
But I don't think you really need to. Instead you should be able to read the actual value, look it up with
get_node_by_phandle(). You can make this check dependent onfixup_phandle_referencesto make sure it's executed after the references are resolved.That's how I implemented it initially...
Ok.. why did you change?
Just because the code is a bit more compact using markers. But decompiling a dtb is a good reason I didn't think of.
Rob
A message that is not in the archive (sent off-list or not yet received): CAL_JsqKynMDP2LBjPj2KSgJiVfmNM4e1=uHYUO6GcUjXt7oJbQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 68 lines · Rob Herring
Quoted 66 lines · David Gibson
Quoted 65 lines · Rob Herring
Quoted 3 lines · David Gibson
On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:
Yay! Someone actually implementing checks.
Not like it is my first. I'll celebrate when someone else does.
Quoted 55 lines
Quoted 4 lines
Many common bindings follow the same pattern of client properties containing a phandle and N arg cells where N is defined in the provider with a '#<specifier>-cells' property. Add a checks for properties following this pattern.
I think this description would be easier to follow if you led with an example.
Quoted 2 lines
Signed-off-by Rob Herring <[email protected]>Looks pretty good, though I have some suggestions.
Quoted 22 lines
--- checks.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+)checks.c +10
-0@@ -956,6 +956,120 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,WARNING (obsolete_chosen_interrupt_controller, check_obsolete_chosen_interrupt_controller,NULL ); +struct provider { +const char *prop_name; +const char *cell_name; +bool optional; +}; + +static void check_property_phandle_args(struct check *c, + struct dt_info *dti, + struct node *node, + struct property *prop,checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 978
Quoted 10 lines
checks.c +10
-0+ const struct provider *provider) +{ +struct marker *m = prop->val.markers; + +if (!m) { +FAIL (c, dti,"Missing phandles in %s:%s" , + node->fullpath, prop->name); +return ; + } +for_each_marker_of_type (m,REF_PHANDLE ) {So going through the markers I think is not the best approach. That'll work if the source contains a reference here, which it usually will, but there are some circumstances where it could contain a "raw" phandle value (the most obvious being when you're decompiling an existing dtb).
But I don't think you really need to. Instead you should be able to read the actual value, look it up with
get_node_by_phandle(). You can make this check dependent onfixup_phandle_referencesto make sure it's executed after the references are resolved.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
That's how I implemented it initially...
Ok.. why did you change?
Just because the code is a bit more compact using markers. But decompiling a dtb is a good reason I didn't think of.
Ok. Well, if you can revert to the first way, that should be good.
Signature · 4 lines
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
Quoted 5 lines · David Gibson
Quoted 4 lines · Rob Herring
Quoted 2 lines · David Gibson
On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:
[...]
Quoted 10 lines
Quoted 9 lines
Quoted 4 lines
It does mean listing everything in the
check_tablewhich is a pain. I would really like to change things so that a single macro can both declare the check and add it to the master list, but I haven't thought of a portable way to do that so far.This is done in the kernel frequently using linker sections. Each check entry would get put into a specific section, then you just iterate through the entries. Would that work? I could imagine that linker magic may not be all the portable.
Quoted 5 lines
Right. Linker sections are the usual way to do this, but that requires lower level knowledge of how the toolchain works than I really want to put into dtc. At heart dtc is really a very straightforward standard C program, so I don't want to introduce dependencies on a specific compiler.
To follow up on this, what about using dlopen/dlsym and having libraries with checks? That should be fairly portable. dtc could load checks libs from some known or env path. Each checks library would provide some known entry point we can call for it to register its checks.
Rob
A message that is not in the archive (sent off-list or not yet received): CAL_JsqLEwYN9eto2PRiWG7aEzBOeNeADP0miOS_jgs5cifmvOA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 26 lines · Rob Herring
Quoted 5 lines · David Gibson
Quoted 4 lines · Rob Herring
Quoted 2 lines · David Gibson
On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:
[...]
Quoted 10 lines
Quoted 9 lines
Quoted 4 lines
It does mean listing everything in the
check_tablewhich is a pain. I would really like to change things so that a single macro can both declare the check and add it to the master list, but I haven't thought of a portable way to do that so far.This is done in the kernel frequently using linker sections. Each check entry would get put into a specific section, then you just iterate through the entries. Would that work? I could imagine that linker magic may not be all the portable.
Quoted 5 lines
Right. Linker sections are the usual way to do this, but that requires lower level knowledge of how the toolchain works than I really want to put into dtc. At heart dtc is really a very straightforward standard C program, so I don't want to introduce dependencies on a specific compiler.
Quoted 5 lines
To follow up on this, what about using dlopen/dlsym and having libraries with checks? That should be fairly portable. dtc could load checks libs from some known or env path. Each checks library would provide some known entry point we can call for it to register its checks.
I don't immediately see how this addresses the problem. Either we'd have to have every check in a separate library, which would be pretty painful, or each library would need a list of checks, and we'd have the same issue generating it as we do already.
Signature · 4 lines
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