On Tue, Jun 08, 2021 at 03:46:24PM -0500, Rob Herring wrote: > For properties we already have checks for, we know the type and how to > parse them. Use this to add type and phandle markers so we have them when > the source did not (e.g. dtb format). > > Signed-off-by: Rob Herring > --- > v2: > - Set marker.ref on phandle markers > - Avoid adding markers if there's any conflicting type markers. > --- > checks.c | 102 ++++++++++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 82 insertions(+), 20 deletions(-) > > diff --git a/checks.c b/checks.c > index e6c7c3eeacac..0f51b9111be1 100644 > --- a/checks.c > +++ b/checks.c > @@ -58,6 +58,38 @@ struct check { > #define CHECK(nm_, fn_, d_, ...) \ > CHECK_ENTRY(nm_, fn_, d_, false, false, __VA_ARGS__) > > +static struct marker *marker_add(struct marker **list, enum markertype type, > + unsigned int offset) Now that this is only conditionally adding markers, it needs a different name. Maybe "add_type_annotation". > +{ > + struct marker *m = *list; Since this is strictly about type annotations (and you don't have parameters for the necessary ref parameter for other things), an assert() that the given type is a TYPE_* wouldn't go astray. > + > + /* Check if we already have a different type or a type marker at the offset*/ > + for_each_marker_of_type(m, type) { > + if ((m->type >= TYPE_UINT8) && (m->type != type)) I'm assuming the >= TYPE_UINT8 is about checking that this is a type marker rather than a ref marker. Adding a helper function for that would probably be a good idea. Putting it inline in dtc.h would make it less likely that we break it if we ever add new marker types in future. Checking for m->type != type doesn't seem useful to me. If m->type == type then either it's at the same offset, in which case there's nothing to do, or it's at a different offset in which case... well, it's not totally clear what's going on, but it's probably safest to leave it alone. > + return NULL; > + if (m->type == type && m->offset == offset) > + return NULL; > + } > + > + m = xmalloc(sizeof(*m)); > + m->type = type; > + m->offset = offset; > + m->next = NULL; > + m->ref = NULL; > + > + /* Find the insertion point, markers are in order by offset */ > + while (*list && ((*list)->offset < m->offset)) > + list = &((*list)->next); > + > + if (*list) { > + m->next = (*list)->next; > + (*list)->next = m; > + } else > + *list = m; > + > + return m; > +} > + > static inline void PRINTF(5, 6) check_msg(struct check *c, struct dt_info *dti, > struct node *node, > struct property *prop, > @@ -260,8 +292,12 @@ static void check_is_cell(struct check *c, struct dt_info *dti, > if (!prop) > return; /* Not present, assumed ok */ > > - if (prop->val.len != sizeof(cell_t)) > + if (prop->val.len != sizeof(cell_t)) { > FAIL_PROP(c, dti, node, prop, "property is not a single cell"); > + return; > + } > + > + marker_add(&prop->val.markers, TYPE_UINT32, 0); > } > #define WARNING_IF_NOT_CELL(nm, propname) \ > WARNING(nm, check_is_cell, (propname)) > @@ -517,6 +553,7 @@ static cell_t check_phandle_prop(struct check *c, struct dt_info *dti, > * we treat it as having no phandle data for now. */ > return 0; > } > + marker_add(&prop->val.markers, TYPE_UINT32, 0); > > phandle = propval_cell(prop); > > @@ -756,7 +793,7 @@ static void check_reg_format(struct check *c, struct dt_info *dti, > struct node *node) > { > struct property *prop; > - int addr_cells, size_cells, entrylen; > + int addr_cells, size_cells, entrylen, offset; > > prop = get_property(node, "reg"); > if (!prop) > @@ -774,10 +811,16 @@ static void check_reg_format(struct check *c, struct dt_info *dti, > size_cells = node_size_cells(node->parent); > entrylen = (addr_cells + size_cells) * sizeof(cell_t); > > - if (!is_multiple_of(prop->val.len, entrylen)) > + if (!is_multiple_of(prop->val.len, entrylen)) { > FAIL_PROP(c, dti, node, prop, "property has invalid length (%d bytes) " > "(#address-cells == %d, #size-cells == %d)", > prop->val.len, addr_cells, size_cells); > + return; > + } > + > + for (offset = 0; offset < prop->val.len; offset += entrylen) > + if (!marker_add(&prop->val.markers, TYPE_UINT32, offset)) > + break; I don't see any point to adding multiple markers. Each type marker indicates the type until the next marker, so just adding one has the same effect. > } > WARNING(reg_format, check_reg_format, NULL, &addr_size_cells); > > @@ -785,7 +828,7 @@ static void check_ranges_format(struct check *c, struct dt_info *dti, > struct node *node) > { > struct property *prop; > - int c_addr_cells, p_addr_cells, c_size_cells, p_size_cells, entrylen; > + int c_addr_cells, p_addr_cells, c_size_cells, p_size_cells, entrylen, offset; > const char *ranges = c->data; > > prop = get_property(node, ranges); > @@ -821,6 +864,10 @@ static void check_ranges_format(struct check *c, struct dt_info *dti, > "#size-cells == %d)", ranges, prop->val.len, > p_addr_cells, c_addr_cells, c_size_cells); > } > + > + for (offset = 0; offset < prop->val.len; offset += entrylen) > + if (!marker_add(&prop->val.markers, TYPE_UINT32, offset)) > + break; > } > WARNING(ranges_format, check_ranges_format, "ranges", &addr_size_cells); > WARNING(dma_ranges_format, check_ranges_format, "dma-ranges", &addr_size_cells); > @@ -1400,6 +1447,7 @@ static void check_property_phandle_args(struct check *c, > for (cell = 0; cell < prop->val.len / sizeof(cell_t); cell += cellsize + 1) { > struct node *provider_node; > struct property *cellprop; > + struct marker *m; > int phandle; > > phandle = propval_cell_n(prop, cell); > @@ -1416,19 +1464,6 @@ static void check_property_phandle_args(struct check *c, > 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_PROP(c, dti, node, prop, > - "cell %d is not a phandle reference", > - cell); > - } > - Why are you removing this part of the check? > provider_node = get_node_by_phandle(root, phandle); > if (!provider_node) { > FAIL_PROP(c, dti, node, prop, > @@ -1454,7 +1489,13 @@ static void check_property_phandle_args(struct check *c, > FAIL_PROP(c, dti, node, prop, > "property size (%d) too small for cell size %d", > prop->val.len, cellsize); > + break; > } > + > + marker_add(&prop->val.markers, TYPE_UINT32, cell * sizeof(cell_t)); > + m = marker_add(&prop->val.markers, REF_PHANDLE, cell * sizeof(cell_t)); > + if (m) > + m->ref = provider_node->fullpath; > } > } > > @@ -1596,7 +1637,7 @@ static void check_interrupts_property(struct check *c, > struct node *root = dti->dt; > struct node *irq_node = NULL, *parent = node; > struct property *irq_prop, *prop = NULL; > - int irq_cells, phandle; > + int irq_cells, phandle, offset; > > irq_prop = get_property(node, "interrupts"); > if (!irq_prop) > @@ -1614,6 +1655,8 @@ static void check_interrupts_property(struct check *c, > > prop = get_property(parent, "interrupt-parent"); > if (prop) { > + struct marker *m; > + > phandle = propval_cell(prop); > if ((phandle == 0) || (phandle == -1)) { > /* Give up if this is an overlay with > @@ -1629,10 +1672,16 @@ static void check_interrupts_property(struct check *c, > FAIL_PROP(c, dti, parent, prop, "Bad phandle"); > return; > } > - if (!node_is_interrupt_provider(irq_node)) > + if (!node_is_interrupt_provider(irq_node)) { > FAIL(c, dti, irq_node, > "Missing interrupt-controller or interrupt-map property"); > + return; > + } > > + marker_add(&prop->val.markers, TYPE_UINT32, 0); > + m = marker_add(&prop->val.markers, REF_PHANDLE, 0); > + if (m) > + m->ref = irq_node->fullpath; > break; > } > > @@ -1655,7 +1704,12 @@ static void check_interrupts_property(struct check *c, > FAIL_PROP(c, dti, node, prop, > "size is (%d), expected multiple of %d", > irq_prop->val.len, (int)(irq_cells * sizeof(cell_t))); > + return; > } > + > + for (offset = 0; offset < irq_prop->val.len; offset += irq_cells * sizeof(cell_t)) > + if (!marker_add(&irq_prop->val.markers, TYPE_UINT32, offset)) > + break; Again, I don't see any point to adding multiple markers. > } > WARNING(interrupts_property, check_interrupts_property, &phandle_references); > > @@ -1763,6 +1817,7 @@ static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti, > struct node *endpoint) > { > int phandle; > + struct marker *m; > struct node *node; > struct property *prop; > > @@ -1776,8 +1831,15 @@ static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti, > return NULL; > > node = get_node_by_phandle(dti->dt, phandle); > - if (!node) > + if (!node) { > FAIL_PROP(c, dti, endpoint, prop, "graph phandle is not valid"); > + return NULL; > + } > + > + marker_add(&prop->val.markers, TYPE_UINT32, 0); > + m = marker_add(&prop->val.markers, REF_PHANDLE, 0); > + if (m) > + m->ref = node->fullpath; > > return node; > } -- 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