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

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

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

On Mon, Jul 12, 2021 at 04:15:43PM -0600, Rob Herring wrote:
> On Sun, Jul 11, 2021 at 8:39 PM David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > 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-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> 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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > > > ---
> > > > > 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.
> 
> The comment applies to both 'if' conditions. I can make it 2 comments.

Sorry, I wasn't clear.  I didn't mean the code comment I meant your
remark in the thread "It's checking that...".  It didn't seem apropos
to my comment it's in reply to.

> > 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.
> 
> Yes, we could have all sorts of encoding. We could mix strings, u8,
> u16, u32, and u64 too, but we don't because that would be insane given
> next to zero type information is maintained.

Who's "we"?  My point is that you're interacting with user supplied
type information, and we can't control what the user supplies.

> But this isn't really about what encoding we could have for 'reg' as
> if we have any type information already, then we do nothing.

But.. that's *not* what this logic does.  It's quite explicitly more
complicated than "if there's any other type information, then do
nothing".

> This is
> only what is the encoding when there is no other information to go on
> besides the property name. IMO, that should be bracketing each entry
> which is all we're doing here. Maybe you disagree on bracketing, but
> as a user shouldn't I be able to get the output I want?

Yes... that's my point...

> Debating it is
> like debating tabs vs. spaces (BTW, dtc already decided I get tabs).
> So maybe all this would be better in a linter, but we don't have one
> and if I wrote one it would start with forking dtc. We could make this
> all a command line option I suppose.
> 
> > > 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().
> 
> Maybe I misunderstood what you suggested an assert() for.

I didn't, you were the one suggesting an assert().

> > > > > @@ -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.
> 
> Uhh well, type markers were added in the first place to support YAML,

No, they weren't.  They were added first so that dts -I dts -O dts
would preserve input formatting more closely.  Shortly afterwards they
were used for the YAML stuff.  They're taking what was originally just
a user choice of input formatting and giving it semantic importance,
which I have always thought was a super fragile approach.  But, I've
merged it anyway, since people seem to really want it and I haven't
had time to work on something else myself.

> but they are used on the dts output as well which has changed it a lot
> from what was output before. The YAML and dts code is just about the
> same in terms of how the type information is used. If anything is
> broken, it's the dts output guessing. Multiples of 4 bytes are always
> UINT32 data?

dts output was never supposed to be reliably type accurate - just like
any decompiler.  It was just trying to take a simple guess for
convenience in debugging.  It *does* guarantee that the -O dts output
will produce byte for byte identical dtb output if you feed it back
into dtc.

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