From: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
To: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Devicetree Compiler
<devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH v2 1/3] checks: Add markers on known properties
Date: Mon, 26 Jul 2021 14:44:25 +1000 [thread overview]
Message-ID: <YP49qUsDIvFklb6r@yekko> (raw)
In-Reply-To: <CAL_JsqLYx7Zgd2v_CvTiF0yynB2HX=U+ROV-tkT7qqwxFPnjAQ-JsoAwUIsXosN+BqQ9rBEUg@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 --]
next prev parent reply other threads:[~2021-07-26 4:44 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-08 20:46 [PATCH v2 0/3] Improve output type formatting Rob Herring
[not found] ` <20210608204626.1545418-1-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
2021-06-08 20:46 ` [PATCH v2 1/3] checks: Add markers on known properties Rob Herring
[not found] ` <20210608204626.1545418-2-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
2021-06-19 9:22 ` David Gibson
2021-06-21 18:22 ` Rob Herring
[not found] ` <CAL_JsqLnbiz-TzH0C0vw57B-1J=N6jBSHeiyv5yKA+Z3+0fWPw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2021-07-12 2:37 ` David Gibson
2021-07-12 22:15 ` Rob Herring
[not found] ` <CAL_JsqLYx7Zgd2v_CvTiF0yynB2HX=U+ROV-tkT7qqwxFPnjAQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2021-07-26 4:44 ` David Gibson [this message]
2021-07-26 18:32 ` Rob Herring
2021-06-08 20:46 ` [PATCH v2 2/3] dtc: Drop dts source restriction for yaml output Rob Herring
2021-06-08 20:46 ` [PATCH v2 3/3] treesource: Maintain phandle label/path on output Rob Herring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YP49qUsDIvFklb6r@yekko \
--to=david-xt8fgy+axnrb3ne2bgzf6laj5h9x9tb+@public.gmane.org \
--cc=devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).