Re: [PATCH v2] checks: Suppress warnings on overlay fragments

[Roger Quadros <rogerq@kernel.org>]

Hi David & Rob,

On 14/05/2023 09:42, david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org wrote:
> 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.
> 

FYI.
I'm facing a somewhat similar issue with this patch [1]
[1] https://lore.kernel.org/all/20230923080046.5373-2-rogerq@kernel.org/

arch/arm64/boot/dts/ti/k3-am642-evm-nand.dtso:65.8-140.3: Warning (avoid_default_addr_size): /fragment@3/__overlay__: Relying on default #address-cells value
arch/arm64/boot/dts/ti/k3-am642-evm-nand.dtso:65.8-140.3: Warning (avoid_default_addr_size): /fragment@3/__overlay__: Relying on default #size-cells value

To give some background:

The GPMC block has separate address spaces per chip select.

 From Documentation/devicetree/bindings/memory-controllers/ti,gpmc.yaml
   ranges:
     minItems: 1
     description: |
       Must be set up to reflect the memory layout with four
       integer values for each chip-select line in use,
       <cs-number> 0 <physical address of mapping> <size>

The ranges location in the device tree overlay is correct. The overlay is
obviously meaningless without the base tree. It depends on the #address-cells
and #size-cells defined in the base tree.

Your proposal to fix this is valid but is definitely not trivial to fix
especially for someone who is not familiar with dtc internals. :P

Is there something simpler we could do to resolve this issue for overlay nodes.
e.g. For overlay nodes we skip the "Relying on default address/size" check?

diff --git a/scripts/dtc/checks.c b/scripts/dtc/checks.c
index 9f31d2607182..a4e94c4b7b08 100644
--- a/scripts/dtc/checks.c
+++ b/scripts/dtc/checks.c
@@ -1197,6 +1197,9 @@ static void check_avoid_default_addr_size(struct check *c, struct dt_info *dti,
 	if (!node->parent)
 		return; /* Ignore root node */
 
+	if (streq(node->name, "__overlay__"))
+		return; /* Ignore overlay nodes */
+
 	reg = get_property(node, "reg");
 	ranges = get_property(node, "ranges");

-- 
cheers,
-roger