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

[Roger Quadros <rogerq@kernel.org>]

On 22/01/2024 18:01, Rob Herring wrote:
> On Tue, Jan 16, 2024 at 02:54:23PM +0200, Roger Quadros wrote:
>> 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?
> 
> I think the overlay needs to define #address-cells/#size-cells because 
> otherwise the only way we can ever validate (not just with dtc, but any 
> tool) an overlay is by applying it. That of course means either you 
> have to change the overlay target to the parent node or allow something 
> like this to work:
> 
> #address-cells = <1>;
> #size-cells = <0>;
> 
> &test {
>   reg = <0x1234>;
> };
> 
> In this case, the sizes aren't really used, but serve as annotations for 
> the tools.

But one address/size-cells might not fit all cases in an overlay as the
nodes in the overlay may have parents with different address/size-cells.

-- 
cheers,
-roger