Re: [PATCH 4/5] checks: Add markers on known properties

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4526 bytes --]

On Tue, Jun 08, 2021 at 07:49:20AM -0500, Rob Herring wrote:
> On Mon, Jun 7, 2021 at 9:25 PM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Tue, May 25, 2021 at 08:03:34PM -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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > ---
> > >  checks.c | 74 ++++++++++++++++++++++++++++++++++++++++++--------------
> > >  1 file changed, 56 insertions(+), 18 deletions(-)
> 
> 
> > > @@ -766,10 +797,15 @@ 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 (!entrylen || (prop->val.len % entrylen) != 0)
> > > +     if (!entrylen || (prop->val.len % entrylen) != 0) {
> > >               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)
> > > +             marker_add(&prop->val.markers, TYPE_UINT32, offset);
> >
> > This doesn't seem quite right.  A 'reg' property could definitely be
> > u64s rather than u32s (amongst other possibilities, but u64 is the
> > most likely).  The user can even indicate that using /bits/ 64, but
> > this will overrule that.
> 
> Ignoring malformed sizes, it can only be u32 unless the input is .dts
> and using /bits/ or [].

Yes.. and using /bits/ is exactly the case I'm talking about.

> I'll make marker_add fail if there's any type
> marker rather than just a matching marker.
> 
> 
> > >  }
> > >  WARNING(ranges_format, check_ranges_format, "ranges", &addr_size_cells);
> > >  WARNING(dma_ranges_format, check_ranges_format, "dma-ranges", &addr_size_cells);
> > > @@ -1408,19 +1447,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);
> > > -             }
> > > -
> > >               provider_node = get_node_by_phandle(root, phandle);
> > >               if (!provider_node) {
> > >                       FAIL_PROP(c, dti, node, prop,
> > > @@ -1447,6 +1473,9 @@ static void check_property_phandle_args(struct check *c,
> > >                                 "property size (%d) too small for cell size %d",
> > >                                 prop->val.len, cellsize);
> > >               }
> > > +
> > > +             marker_add(&prop->val.markers, REF_PHANDLE, cell * sizeof(cell_t));
> >
> > This is definitely broken.  It's safe enough to add TYPE_ markers, but
> > REF_PHANDLE requires a label or path to be valid, which you don't add,
> > and have no way of deducing.  I'm kind of surprised you didn't cause a
> > crash in the later code that fixes up references with this.
> 
> Didn't see any failures, so maybe the fixup runs first? I'll look at
> adding the path or perhaps we can loosen this requirement?

Neither of those seems like the right approach.  REF_PHANDLE *means* a
place to replace a phandle, we're just re-using that to indicate that
we have a phandle here.  If you want to know it's a phandle without
also having a reference to fill in there, then we should add a
TYPE_PHANDLE marker.

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]