From: "david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org" <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
To: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: "Qun-wei Lin (林群崴)"
<Qun-wei.Lin-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
"devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
"Casper Li (李中榮)"
<casper.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
"Kuan-Ying Lee (李冠穎)"
<Kuan-Ying.Lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
"Chinwen Chang (張錦文)"
<chinwen.chang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
"Ivan Tseng (曾志軒)"
<ivan.tseng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Subject: Re: [PATCH v2] checks: Suppress warnings on overlay fragments
Date: Sun, 14 May 2023 16:42:14 +1000 [thread overview]
Message-ID: <ZGCCxgVHQ3wJoXFt@yekko> (raw)
In-Reply-To: <CAL_Jsq+L50RZ-s55tncFMHA1AwL5An13n2OVPzknnpu=uOvzhQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 5647 bytes --]
On Tue, Apr 18, 2023 at 12:33:26PM -0500, Rob Herring wrote:
> On Thu, Apr 13, 2023 at 7:44 AM Qun-wei Lin (林群崴)
> <Qun-wei.Lin-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> >
> > On Thu, 2023-03-09 at 09:12 -0600, Rob Herring wrote:
> > > On Wed, Mar 8, 2023 at 3:17 AM Qun-Wei Lin <qun-wei.lin@mediatek.com>
> > > wrote:
> > > >
> > > > The overlay fragment is a special case where some properties are
> > > > not
> > > > present in the overlay source file, but in the base file.
> > > >
> > > > example:
> > > > +-----------------------------+--------------------+
> > > > > base.dts | overlay.dts |
> > > >
> > > > +-----------------------------+--------------------+
> > > > > /dts-v1/; | /dts-v1/; |
> > > > > | /plugin/; |
> > > > > /{ | |
> > > > > parent: test { | &parent { |
> > > > > #address-cells = <1>; | child@0 { |
> > > > > #size-cells = <0>; | reg = <0x0>; |
> > > > > }; | }; |
> > > > > }; | }; |
> > > >
> > > > +-----------------------------+--------------------+
> > > >
> > > > It will cause the following false alarms when compiling the overlay
> > > > dts.
> > > >
> > > > 1. /fragment@0/__overlay__: Character '_' not recommended in node
> > > > name
> > > > 2. /fragment@0/__overlay__: Relying on default #address-cells value
> > > > 3. /fragment@0/__overlay__: Relying on default #size-cells value
> > > > 4. /fragment@0/__overlay__:reg: property has invalid length (4
> > > > bytes)
> > > > (#address-cells == 2, #size-cells == 1)
> > > >
> > > > This workaround will fix them by skip checking for node named
> > > > __overlay__.
> > > >
> > > > Signed-off-by: Qun-Wei Lin <qun-wei.lin-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > > > ---
> > > > V1 -> V2:
> > > > - Add is_overlay_node() helper
> > > > - Skip anything starting with "__" in
> > > > check_node_name_chars_strict()
> > > >
> > > > checks.c | 18 ++++++++++++++++++
> > > > 1 file changed, 18 insertions(+)
> > >
> > > Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > >
> > > Though I do wonder if as a matter of policy on overlay structure, if
> > > we should require an overlay to have the parent node with
> > > #address-cells/#size-cells. In the end that would be duplicated data,
> > > but without it there's no way to parse and validate reg/ranges in an
> > > unapplied overlay. That's just one example issue in being able to
> > > validate overlays.
> > >
> > > Rob
> >
> > Hi Rob,
> >
> > Thank you for your review.
> >
> > I think I've found another problem:
> > +-------------------------+----------------+
> > | base.dts | overlay.dts |
> > +-------------------------+----------------+
> > | /dts-v1/; | /dts-v1/; |
> > | /{ | /plugin/; |
> > | #address-cells = <1>; | |
> > | #size-cells = <0>; | &test { |
> > | test: example@0 { | reg = <0x1>; |
> > | reg = <0x0>; | }; |
> > | }; | |
> > | }; | |
> > +-------------------------+----------------+
> >
> > The following error message is printed when compiling:
> > Warning (reg_format): /fragment@0/__overlay__:reg: property has invalid
> > length (4 bytes) (#address-cells == 2, #size-cells == 1)
> > Warning (unit_address_vs_reg): /fragment@0/__overlay__: node has a reg
> > or ranges property, but no unit name
> > Warning (avoid_default_addr_size): /fragment@0/__overlay__: Relying on
> > default #address-cells value
> > Warning (avoid_default_addr_size): /fragment@0/__overlay__: Relying on
> > default #size-cells value
> >
> > We can't get the #address-cells/#size-cells of the parent of the
> > example node in the overlay structure.
> > Do you think we should change it to is_overlay_node(node) instead of
> > is_overlay_node(node->parent)?
> > Or we just need to skip checking for node names starting with "__" in
> > check_node_name_chars_strict()?
>
> I think this is the tip of the iceberg in terms of being able to
> validate overlays. Turning off address checks just kicks the problem
> to schema validation. Perhaps the overlay should be structured to
> include the parent bus node with #address-cells and #size-cells.
Right. So, I think to handle this properly we need to change the
structure of dtc:
* Rather than applying source level overlays as we parse them, we
should parse them each separately into a structure, then
internally apply them
* Checks would then need to be divided into those (A) that can be
checked on any individual overlay fragment, and those (B) that can
only be checked on a complete tree
* We'd run the (A) checks before merging the overlays in dtc, and
the (B) checks only after doing so.
* For runtime overlays (/plugin/) we'd skip the (B) checks entirely,
which would accomplish what you're aiming for here.
I had plans to make a restructure like this quite a while ago, but
I've never had the time to go ahead with it. If you want to give it a
go, that would be great.
--
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:[~2023-05-14 6:42 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-08 9:15 [PATCH v2] checks: Suppress warnings on overlay fragments Qun-Wei Lin
[not found] ` <20230308091539.11178-1-qun-wei.lin-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
2023-03-09 15:12 ` Rob Herring
[not found] ` <CAL_JsqKTM=gaQGZhrBCRkBusYYMci0mJGAFf9RTvCx00G2OJzg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2023-04-13 12:43 ` Qun-wei Lin (林群崴)
[not found] ` <347151917fb777e66a54e983efbbe0303f63e01a.camel-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
2023-04-18 17:33 ` Rob Herring
[not found] ` <CAL_Jsq+L50RZ-s55tncFMHA1AwL5An13n2OVPzknnpu=uOvzhQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2023-05-14 6:42 ` david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+ [this message]
2024-01-16 12:54 ` Roger Quadros
2024-01-22 16:01 ` Rob Herring
2024-01-23 14:49 ` Roger Quadros
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=ZGCCxgVHQ3wJoXFt@yekko \
--to=david-xt8fgy+axnrb3ne2bgzf6laj5h9x9tb+@public.gmane.org \
--cc=Kuan-Ying.Lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org \
--cc=Qun-wei.Lin-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org \
--cc=casper.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org \
--cc=chinwen.chang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org \
--cc=devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=ivan.tseng-NuS5LvNUpcJWk0Htik3J/w@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).