Re: [PATCH v2 1/3] checks: Add markers on known properties

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]



On Mon, Jun 21, 2021 at 12:22:45PM -0600, Rob Herring wrote:
> On Sat, Jun 19, 2021 at 3:22 AM David Gibson
> <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > 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 <robh@xxxxxxxxxx>
> > > ---
> > > 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".
> 
> marker_add_type_annotation to be consistent that marker_* functions
> work on markers?

Sure, that's fine too.  It's a local function, so naming consistency
isn't as important as for globals.

> > > +{
> > > +     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.
> 
> It's checking that we don't have markers of a different type within
> the property. While that is valid dts format, it's never the case for
> any known (by checks.c) properties.

That comment doesn't seem to quite match the thing above.

Besides, I don't think that's really true.  It would be entirely
sensible for 'reg' to have mixed u32 & u64 type to encode PCI
addresses.  Likewise it would make sense for 'ranges' to have mixed
u32 & u64 type if mapping between a 32-bit and 64-bit bus.

> It could be an assert() as it's
> one of those 'should never happen'.

An assert() should only be used if the problem must have been caused
by an error in the code.  Bad user input should never trigger an
assert().
> 
> We already have another case of (m->type >= TYPE_UINT8), so I can add
> an inline for that.

Thanks.

> 
> > 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.
> 
> Only the same type at a different offset is expected. More on that below.
> 
> >
> > > +                     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.
> 
> Multiple markers is the whole point. This is to match the existing
> behavior when we have:
> 
> reg = <baseA sizeA>, <baseB sizeB>;

Urgh, because you're using the type markers for that broken YAML
conversion.  Fine, ok.

> In this case, there's a type marker for each <> entry. That is then
> used by the output stages (dts and yaml). We already have a single
> marker as the format guessing based on property length being a
> multiple of 4 achieves that. Though, for that to work with the yaml
> output we'd have to do (dtb -> dts -> yaml).
> 
> Currently, the output formatting is dependent on the input format.
> This makes the output consistent in that brackets (via type markers)
> delineate #*-cells boundaries.
> 
> > >  }
> > >  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?
> 
> Well, it didn't really match up with adding markers. It was checking
> if markers were correct, but now we're adding the markers if not
> present. Plus the get_node_by_phandle() call after this is a better
> check for being a valid phandle. I can make this a separate commit.

Ok.

-- 
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

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Device Tree]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux