linux-kernel - Re: fwnode_for_each_child_node() and OF backend discrepancy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3ab8afab-b6b7-46aa-06d4-6740cee422d7@linaro.org>
Date:   Tue, 28 Jun 2022 15:51:00 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Michael Walle <michael@...le.cc>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Horatiu Vultur <horatiu.vultur@...rochip.com>
Subject: Re: fwnode_for_each_child_node() and OF backend discrepancy

On 28/06/2022 15:47, Michael Walle wrote:
> [adding Horatiu Vultur, because we now digress to the bug
> in the switch, rather than that odd OF behavior]
> 
> Am 2022-06-28 15:29, schrieb Andy Shevchenko:
>> On Tue, Jun 28, 2022 at 3:23 PM Michael Walle <michael@...le.cc> wrote:
>>>
>>>>> I was trying to fix the lan966x driver [1] which doesn't work if there
>>>>> are disabled nodes in between.
>>>>
>>>> Can you elaborate what's wrong now in the behaviour of the driver? In
>>>> the code it uses twice the _available variant.
>>>
>>> Imagine the following device tree snippet:
>>>   port0 {
>>>     reg = <0>;
>>>     status = "okay";
>>>   }
>>>   port1 {
>>>     reg = <1>;
>>>     status = "disabled";
>>>   }
>>>   port@2 {
>>>     reg = <2>;
>>>     status = "okay";
>>>   }
>>>
>>> The driver will set num_phys_ports to 2. When port@2 is probed, it
>>> will have the (correct!) physical port number 2. That will then
>>> trigger various EINVAL checks with "port_num >= num_phys_ports" or
>>> WARN()s.
>>
>> It means the above mentioned condition is wrong: it should be
>>
>> "port_idx >= num_phys_ports" (if the port_idx doesn't exists, that's
>> the bug in the first place)
> 
> I can't follow you here. Please note, that you need the actual
> physical port number. It's not a made up number, but corresponds
> to a physical port on that ethernet switch. So you can't just skip
> the disabled ones. port@2 must have port number 2.

The number "2" you get from the reg property, so where is exactly the
problem?

If you want to validate it against some maximum number of ports, based
on DTS, it makes no sense... One can supply a DTS with port number 10000
and 10000 nodes, or with specific property "maximum-port-number=10000".
It would make sense if you validate it against driver-hard-coded values
(which you later mentioned in your reply).


Best regards,
Krzysztof