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@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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:
signature.asc
Description: PGP signature
