dtc: checks for phandle with arg properties
v3 cover dtc: checks for phandle with arg properties
David,
Here's a new version of checks for phandle with arg style properties. This checks interrupt and gpio bindings and other common bindings which follow the same phandle plus args pattern.
This generates ~200 warnings building the ARM dts files in the kernel.
Rob
Rob Herring (3):
checks: add phandle with arg property checks
checks: add gpio binding properties check
checks: add interrupts property check
checks.c | 280 ++++++++++++++++++++++++++++++++++++++++++ dtc.h | 1 + livetree.c | 6 + tests/bad-gpio.dts | 13 ++ tests/bad-interrupt-cells.dts | 12 ++ tests/bad-phandle-cells.dts | 11 ++ tests/run_tests.sh | 4 + 7 files changed, 327 insertions(+) create mode 100644 tests/bad-gpio.dts create mode 100644 tests/bad-interrupt-cells.dts create mode 100644 tests/bad-phandle-cells.dts
Signature · 1 line
2.11.0
A message that is not in the archive (sent off-list or not yet received): 20170901185303.24770-1-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
PATCH v3 1/3 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 such as:
intc0: interrupt-controller@0 {
#interrupt-cells = <3>;
};
intc1: interrupt-controller@1 {
#interrupt-cells = <2>;
};
node {
interrupts-extended = <&intc0 1 2 3>, <&intc1 4 5>;
};Add checks for properties following this pattern.
---
v3:
- Add a check that the property size is a multiple of cell size
- Add a sanity check when markers are present that they match up with expected phandle positions
v2:
- Make each property a separate check
- Iterate over raw cells rather than markers
- Fix property length check for 2nd to Nth items
- Improve error messages. If cell sizes are wrong, the next iteration can get a bad (but valid) phandle.
- Add a test
checks.c | 126 ++++++++++++++++++++++++++++++++++++++++++++ dtc.h | 1 + livetree.c | 6 +++ tests/bad-phandle-cells.dts | 11 ++++ tests/run_tests.sh | 1 + 5 files changed, 145 insertions(+) create mode 100644 tests/bad-phandle-cells.dts
checks.c +126 -0
@@ -956,6 +956,115 @@ 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 node *root = dti->dt; +int cell, cellsize =0 ; + +if (prop->val.len %sizeof (cell_t)) { +FAIL (c, dti,"property '%s' size (%d) is invalid, expected multiple of %ld in node %s" , + prop->name, prop->val.len,sizeof (cell_t), node->fullpath); +return ; + } + +for (cell =0 ; cell < prop->val.len /sizeof (cell_t); cell += cellsize +1 ) { +struct node *provider_node; +struct property *cellprop; +int phandle; + + phandle =propval_cell_n (prop, cell); +/* +* Some bindings use a cell value 0 or -1 to skip over optional +* entries when each index position has a specific definition. +*/ +if (phandle ==0 || phandle ==-1 ) { + cellsize =0 ; +continue ; + } + +/* If we have markers, verify the current cell is a phandle */ +if (prop->val.markers) { +struct marker *m = prop->val.markers; +for_each_marker_of_type (m,REF_PHANDLE ) { +if (m->offset == (cell *sizeof (cell_t))) +break ; + } +if (!m) +FAIL (c, dti,"Property '%s', cell %d is not a phandle reference in %s" , + prop->name, cell, node->fullpath); + } + + provider_node =get_node_by_phandle (root, phandle); +if (!provider_node) { +FAIL (c, dti,"Could not get phandle node for %s:%s(cell %d)" , + node->fullpath, prop->name, cell); +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 property '%s' in node %s or bad phandle (referred from %s:%s[%d])" , + provider->cell_name, + provider_node->fullpath, + node->fullpath, prop->name, cell); +break ; + } + +if (prop->val.len < ((cell + 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 void check_provider_cells_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +struct provider *provider = c->data; +struct property *prop; + + prop =get_property (node, provider->prop_name); +if (!prop) +return ; + +check_property_phandle_args (c, dti, node, prop, provider); +} +#define WARNING_PROPERTY_PHANDLE_CELLS (nm, propname, cells_name, ...) \ + static struct provider nm##_provider = { (propname), (cells_name), __VA_ARGS__ }; \ + WARNING(nm##_property, check_provider_cells_property, &nm##_provider, &phandle_references); + +WARNING_PROPERTY_PHANDLE_CELLS (clocks,"clocks" ,"#clock-cells" ); +WARNING_PROPERTY_PHANDLE_CELLS (cooling_device,"cooling-device" ,"#cooling-cells" ); +WARNING_PROPERTY_PHANDLE_CELLS (dmas,"dmas" ,"#dma-cells" ); +WARNING_PROPERTY_PHANDLE_CELLS (hwlocks,"hwlocks" ,"#hwlock-cells" ); +WARNING_PROPERTY_PHANDLE_CELLS (interrupts_extended,"interrupts-extended" ,"#interrupt-cells" ); +WARNING_PROPERTY_PHANDLE_CELLS (io_channels,"io-channels" ,"#io-channel-cells" ); +WARNING_PROPERTY_PHANDLE_CELLS (iommus,"iommus" ,"#iommu-cells" ); +WARNING_PROPERTY_PHANDLE_CELLS (mboxes,"mboxes" ,"#mbox-cells" ); +WARNING_PROPERTY_PHANDLE_CELLS (msi_parent,"msi-parent" ,"#msi-cells" , true); +WARNING_PROPERTY_PHANDLE_CELLS (mux_controls,"mux-controls" ,"#mux-control-cells" ); +WARNING_PROPERTY_PHANDLE_CELLS (phys,"phys" ,"#phy-cells" ); +WARNING_PROPERTY_PHANDLE_CELLS (power_domains,"power-domains" ,"#power-domain-cells" ); +WARNING_PROPERTY_PHANDLE_CELLS (pwms,"pwms" ,"#pwm-cells" ); +WARNING_PROPERTY_PHANDLE_CELLS (resets,"resets" ,"#reset-cells" ); +WARNING_PROPERTY_PHANDLE_CELLS (sound_dais,"sound-dais" ,"#sound-dai-cells" ); +WARNING_PROPERTY_PHANDLE_CELLS (thermal_sensors,"thermal-sensors" ,"#thermal-sensor-cells" ); +static struct check *check_table[] = { &duplicate_node_names, &duplicate_property_names, &node_name_chars, &node_name_format, &property_name_chars,
@@ -987,6 +1096,23 @@ static struct check *check_table[] = { &avoid_default_addr_size, &obsolete_chosen_interrupt_controller, + &clocks_property, + &cooling_device_property, + &dmas_property, + &hwlocks_property, + &interrupts_extended_property, + &io_channels_property, + &iommus_property, + &mboxes_property, + &msi_parent_property, + &mux_controls_property, + &phys_property, + &power_domains_property, + &pwms_property, + &resets_property, + &sound_dais_property, + &thermal_sensors_property, + &always_fail, };
dtc.h +1 -0
@@ -216,6 +216,7 @@ void append_to_property(struct node *node,const char *get_unitname (struct node *node);struct property *get_property (struct node *node,const char *propname);cell_t propval_cell (struct property *prop); +cell_t propval_cell_n (struct property *prop,int n);struct property *get_property_by_label (struct node *tree,const char *label,struct node **node); struct marker *get_marker_label(struct node *tree, const char *label,
livetree.c +6 -0
@@ -396,6 +396,12 @@ cell_t propval_cell(struct property *prop)return fdt32_to_cpu (*((fdt32_t *)prop->val.val)); } +cell_t propval_cell_n (struct property *prop,int n) +{ +assert (prop->val.len /sizeof (cell_t) >= n); +return fdt32_to_cpu (*((fdt32_t *)prop->val.val + n)); +} +struct property *get_property_by_label (struct node *tree,const char *label,struct node **node) {
tests/bad-phandle-cells.dts new file +11 -0
@@ -0,0 +1,11 @@ +/dts-v1/; + +/ { + intc: interrupt-controller { + #interrupt-cells = <3>; + }; + + node { + interrupts-extended = <&intc>; + }; +};
tests/run_tests.sh +1 -0
@@ -550,6 +550,7 @@ dtc_tests () { check_tests unit-addr-without-reg.dts unit_address_vs_reg check_tests unit-addr-leading-0x.dts unit_address_format check_tests unit-addr-leading-0s.dts unit_address_format + check_tests bad-phandle-cells.dts interrupts_extended_property run_sh_test dtc-checkfails.sh node_name_chars -- -I dtb -O dtb bad_node_char.dtb run_sh_test dtc-checkfails.sh node_name_format -- -I dtb -O dtb bad_node_format.dtb run_sh_test dtc-checkfails.sh prop_name_chars -- -I dtb -O dtb bad_prop_char.dtb
Signature · 1 line
2.11.0
PATCH v3 2/3 checks: add gpio binding properties check
In reply to a message not in the archiveThe GPIO binding is different compared to other phandle plus args properties in that the property name has a variable, optional prefix. The format of the property name is [<name>-]gpio{s} where <name> can be any legal property string. Therefore, custom matching of property names is needed, but the common check_property_phandle_args() function can still be used.
It's possible that there are property names matching which are not GPIO binding specifiers. There's only been one case found in testing which is "[<vendor>,]nr-gpio{s}". This property has been blacklisted and the same should be done to any others we find. This check will prevent getting any more of these, too.
---
v3:
- Add David's R-by.
v2:
- New patch with GPIO checks split to separate patch
- Add check for deprecated [<name>-]gpio form
- Rework property name matching to include "gpio" and "gpios"
- Skip the "gpios" property in gpio hogs binding
- Improve the blacklisting comment
- Add a test
checks.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/bad-gpio.dts | 13 ++++++++++ tests/run_tests.sh | 2 ++ 3 files changed, 88 insertions(+) create mode 100644 tests/bad-gpio.dts
checks.c +73 -0
@@ -1065,6 +1065,76 @@ WARNING_PROPERTY_PHANDLE_CELLS(resets, "resets", "#reset-cells");WARNING_PROPERTY_PHANDLE_CELLS (sound_dais,"sound-dais" ,"#sound-dai-cells" );WARNING_PROPERTY_PHANDLE_CELLS (thermal_sensors,"thermal-sensors" ,"#thermal-sensor-cells" ); +static bool prop_is_gpio (struct property *prop) +{ +char *str; + +/* +* *-gpios and *-gpio can appear in property names, +* so skip over any false matches (only one known ATM) +*/ +if (strstr (prop->name,"nr-gpio" )) +return false; + + str =strrchr (prop->name,'-' ); +if (str) + str++; +else + str = prop->name; +if (!(streq (str,"gpios" ) ||streq (str,"gpio" ))) +return false; + +return true; +} + +static void check_gpios_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +struct property *prop; + +/* Skip GPIO hog nodes which have 'gpios' property */ +if (get_property (node,"gpio-hog" )) +return ; + +for_each_property (node, prop) { +struct provider provider; + +if (!prop_is_gpio (prop)) +continue ; + + provider.prop_name = prop->name; + provider.cell_name ="#gpio-cells" ; + provider.optional = false; +check_property_phandle_args (c, dti, node, prop, &provider); + } + +} +WARNING (gpios_property, check_gpios_property,NULL , &phandle_references); + +static void check_deprecated_gpio_property (struct check *c, +struct dt_info *dti, +struct node *node) +{ +struct property *prop; + +for_each_property (node, prop) { +char *str; + +if (!prop_is_gpio (prop)) +continue ; + + str =strstr (prop->name,"gpio" ); +if (!streq (str,"gpio" )) +continue ; + +FAIL (c, dti,"'[*-]gpio' is deprecated, use '[*-]gpios' instead for %s:%s" , + node->fullpath, prop->name); + } + +} +CHECK (deprecated_gpio_property, check_deprecated_gpio_property,NULL ); +static struct check *check_table[] = { &duplicate_node_names, &duplicate_property_names, &node_name_chars, &node_name_format, &property_name_chars,
@@ -1113,6 +1183,9 @@ static struct check *check_table[] = { &sound_dais_property, &thermal_sensors_property, + &deprecated_gpio_property, + &gpios_property, + &always_fail, };
tests/bad-gpio.dts new file +13 -0
@@ -0,0 +1,13 @@ +/dts-v1/; + +/ { + gpio: gpio-controller { + #gpio-cells = <3>; + }; + + node { + nr-gpios = <1>; + foo-gpios = <&gpio>; + bar-gpio = <&gpio 1 2 3>; + }; +};
tests/run_tests.sh +2 -0
@@ -551,6 +551,8 @@ dtc_tests () { check_tests unit-addr-leading-0x.dts unit_address_format check_tests unit-addr-leading-0s.dts unit_address_format check_tests bad-phandle-cells.dts interrupts_extended_property + check_tests bad-gpio.dts gpios_property + run_sh_test dtc-checkfails.sh deprecated_gpio_property -- -Wdeprecated_gpio_property -I dts -O dtb bad-gpio.dts run_sh_test dtc-checkfails.sh node_name_chars -- -I dtb -O dtb bad_node_char.dtb run_sh_test dtc-checkfails.sh node_name_format -- -I dtb -O dtb bad_node_format.dtb run_sh_test dtc-checkfails.sh prop_name_chars -- -I dtb -O dtb bad_prop_char.dtb
Signature · 1 line
2.11.0
PATCH v3 3/3 checks: add interrupts property check
In reply to a message not in the archiveAdd 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.
---
v3:
- Add a check that the property size is a multiple of cell size
- Add David's R-by.
v2:
- Add a test
- Check for interrupt-map when looking for interrupt parent.
- Check that explicit interrupt-parent node has an interrupt-controller or interrupt-map property.
checks.c | 81 +++++++++++++++++++++++++++++++++++++++++++ tests/bad-interrupt-cells.dts | 12 +++++++ tests/run_tests.sh | 1 + 3 files changed, 94 insertions(+) create mode 100644 tests/bad-interrupt-cells.dts
checks.c +81 -0
@@ -1135,6 +1135,86 @@ static void check_deprecated_gpio_property(struct check *c, }CHECK (deprecated_gpio_property, check_deprecated_gpio_property,NULL ); +static bool node_is_interrupt_provider (struct node *node) +{ +struct property *prop; + + prop =get_property (node,"interrupt-controller" ); +if (prop) +return true; + + prop =get_property (node,"interrupt-map" ); +if (prop) +return true; + +return false; +} +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 ; + +if (irq_prop->val.len %sizeof (cell_t)) +FAIL (c, dti,"property '%s' size (%d) is invalid, expected multiple of %ld in node %s" , + irq_prop->name, irq_prop->val.len,sizeof (cell_t), + node->fullpath); + +while (parent && !prop) { +if (parent != node &&node_is_interrupt_provider (parent)) { + 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 ; + } +if (!node_is_interrupt_provider (irq_node)) +FAIL (c, dti, +"Missing interrupt-controller or interrupt-map property in %s" , + irq_node->fullpath); + +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,
@@ -1185,6 +1265,7 @@ static struct check *check_table[] = { &deprecated_gpio_property, &gpios_property, + &interrupts_property, &always_fail, };
tests/bad-interrupt-cells.dts new file +12 -0
@@ -0,0 +1,12 @@ +/dts-v1/; + +/ { + interrupt-parent = <&intc>; + intc: interrupt-controller { + #interrupt-cells = <3>; + }; + + node { + interrupts = <1>; + }; +};
tests/run_tests.sh +1 -0
@@ -553,6 +553,7 @@ dtc_tests () { check_tests bad-phandle-cells.dts interrupts_extended_property check_tests bad-gpio.dts gpios_property run_sh_test dtc-checkfails.sh deprecated_gpio_property -- -Wdeprecated_gpio_property -I dts -O dtb bad-gpio.dts + check_tests bad-interrupt-cells.dts interrupts_property run_sh_test dtc-checkfails.sh node_name_chars -- -I dtb -O dtb bad_node_char.dtb run_sh_test dtc-checkfails.sh node_name_format -- -I dtb -O dtb bad_node_format.dtb run_sh_test dtc-checkfails.sh prop_name_chars -- -I dtb -O dtb bad_prop_char.dtb
Signature · 1 line
2.11.0
Quoted 7 lines · Rob Herring
David,
Here's a new version of checks for phandle with arg style properties. This checks interrupt and gpio bindings and other common bindings which follow the same phandle plus args pattern.
This generates ~200 warnings building the ARM dts files in the kernel.
Ping. Any further comments on this?
Rob
A message that is not in the archive (sent off-list or not yet received): CAL_JsqLuO24jUO3D_9ROT+tOS6iQEyPGgT8tts7C3bwXPWwskg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 10 lines · Rob Herring
Quoted 7 lines · Rob Herring
David,
Here's a new version of checks for phandle with arg style properties. This checks interrupt and gpio bindings and other common bindings which follow the same phandle plus args pattern.
This generates ~200 warnings building the ARM dts files in the kernel.
Ping. Any further comments on this?
Sorry, I know I've neglected this. I've been sick, and then flat out with my day job.
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 8 lines · Rob Herring
David,
Here's a new version of checks for phandle with arg style properties. This checks interrupt and gpio bindings and other common bindings which follow the same phandle plus args pattern.
This generates ~200 warnings building the ARM dts files in the kernel.
Sorry for the delay. Applied to master, thanks.
Quoted 20 lines
Rob
Rob Herring (3):
checks: add phandle with arg property checks
checks: add gpio binding properties check
checks: add interrupts property checkchecks.c | 280 ++++++++++++++++++++++++++++++++++++++++++ dtc.h | 1 + livetree.c | 6 + tests/bad-gpio.dts | 13 ++ tests/bad-interrupt-cells.dts | 12 ++ tests/bad-phandle-cells.dts | 11 ++ tests/run_tests.sh | 4 + 7 files changed, 327 insertions(+) create mode 100644 tests/bad-gpio.dts create mode 100644 tests/bad-interrupt-cells.dts create mode 100644 tests/bad-phandle-cells.dts
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