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