Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <408ee74c-e6ed-d654-af04-58bd7d1e087b@linaro.org>
Date:   Wed, 17 May 2023 21:26:35 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v2 3/5] dt-bindings: net: add mac-address-increment option
Content-Language: en-US
To:     Ivan Mikhaylov <fr0st61te@gmail.com>,
        Samuel Mendoza-Jonas <sam@mendozajonas.com>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc:     netdev@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, openbmc@lists.ozlabs.org,
        Paul Fertser <fercerpav@gmail.com>
References: <20230509143504.30382-1-fr0st61te@gmail.com>
 <20230509143504.30382-4-fr0st61te@gmail.com>
 <6b5be71e-141e-c02a-8cba-a528264b26c2@linaro.org>
 <fc3dae42f2dfdf046664d964bae560ff6bb32f69.camel@gmail.com>
 <8de01e81-43dc-71af-f56f-4fba957b0b0b@linaro.org>
 <be85bef7e144ebe08f422bf53bb81b59a130cb29.camel@gmail.com>
 <5b826dc7-2d02-d4ed-3b6a-63737abe732b@linaro.org>
 <e6247cb39cc16a9328d9432e0595745b67c0aed5.camel@gmail.com>
 <38ae4ceb-da21-d73e-9625-1918b4ab4e16@linaro.org>
 <5d7421b6a419a9645f97e6240b1dfbf47ffcab4e.camel@gmail.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <5d7421b6a419a9645f97e6240b1dfbf47ffcab4e.camel@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 17/05/2023 23:38, Ivan Mikhaylov wrote:
> On Wed, 2023-05-17 at 10:36 +0200, Krzysztof Kozlowski wrote:
>> On 16/05/2023 13:47, Ivan Mikhaylov wrote:
>> hy this is property of the hardware. I
>>>>>> understand
>>>>>> that this is something you want Linux to do, but DT is not
>>>>>> for
>>>>>> that
>>>>>> purpose. Do not encode system policies into DT and what above
>>>>>> commit
>>>>>> says is a policy.
>>>>>>
>>>>>
>>>>> Krzysztof, okay then to which DT subsystem it should belong? To
>>>>> ftgmac100 after conversion?
>>>>
>>>> To my understanding, decision to add some numbers to MAC address
>>>> does
>>>> not look like DT property at all. Otherwise please help me to
>>>> understand
>>>> - why different boards with same device should have different
>>>> offset/value?

I would like to remind this question.
"why different boards with same device should have different offset/value?"

It was literally ignored and you started explaining network cards and
BMC. I don't understand why, but it does not help your case.

Let me extend this question with one more:
"Why for all your boards of one type, so using the same DTS, would you
use one value of incrementing MAC address?"

>>>>
>>>> Anyway, commit msg also lacks any justification for this.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> Krzysztof, essentially some PCIe network cards have like an
>>> additional
>>> *MII interface which connects directly to a BMC (separate SoC for
>>> managing a motherboard) and by sending special ethernet type frames
>>> over that connection (called NC-SI) the BMC can obtain MAC, get
>>> link
>>> parameters etc. So it's natural for a vendor to allocate two MACs
>>> per
>>> such a board with PCIe card intergrated, with one MAC "flashed
>>> into"
>>> the network card, under the assumption that the BMC should
>>
>> Who makes the assumption that next MAC should differ by 1 or 2?
> 
> Krzysztof, in this above case BMC does, BMC should care about changing
> it and doing it with current codebase without any options just by some
> hardcoded numbers which is wrong.

But you hard-code the number, just in BMC DTS. How does it differ from
BMC hard-coding it differently?

You encode policy - or software decisions - into Devicetree.

> 
>>
>>> automatically use the next MAC. So it's the property of the
>>> hardware as
>>> the vendor designs it, not a matter of usage policy.
>>>
>>> Also at the nvmem binding tree is "nvmem-cell-cells" which is
>>> literally
>>> the same as what was proposed but on different level.
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/nvmem?id=7e2805c203a6c8dc85c1cfda205161ed39ae82d5
>>
>> How is this similar? This points the location of mac address on some
>> NV
>> storage. You add fixed value which should be added to the Ethernet.
> 
> It's not the points the location, this particular option provides this
> increment for mac addresses to make use of them with multiple
> interfaces. Just part of above commit:
> "It's used as a base for calculating addresses for multiple interfaces.
> It's done by adding proper values. Actual offsets are picked by
> manufacturers and vary across devices."
> 
> It is same as we talked before about mac-address-increment in openwrt
> project, if you want examples, you can look into their github. And same
> as we trying to achieve here.
> 
> https://github.com/openwrt/openwrt/blob/master/target/linux/generic/pending-5.15/682-of_net-add-mac-address-increment-support.patch

Awesome... so if project added wrong property to bindings, e.g. SW
property, you find it as an argument for anyone else.

No, that's not how it works.

> 
> "Lots of embedded devices use the mac-address of other interface
> extracted from nvmem cells and increments it by one or two. Add two
> bindings to integrate this and directly use the right mac-address for
> the interface. Some example are some routers that use the gmac
> mac-address stored in the art partition and increments it by one for
> the
> wifi. mac-address-increment-byte bindings is used to tell what byte of
> the mac-address has to be increased (if not defined the last byte is
> increased) and mac-address-increment tells how much the byte decided
> early has to be increased."
> 
> Don't you see similarity with nvmem commit?

Explanation is similar, but you are using wrong argument to justify the
property. The MAC address is stored in some NVMEM cell. There is such
NVMEM cell. That's the hardware property, thus it is justified in DT.

Now how MAC address will be modified - by 1, 2, 3, 252 - is not related
to that commit, because it is a software decision.

Again, we are back to the previous question to which you answered "BMC
will do it". I understand this is property for the BMC DTS, thus:
Why for all your boards of one type, so using one DTS, would you use one
value of incrementing MAC address?
Why devices with same board cannot use different values? One board "1"
and second "2" for MAC increments? I am sure that one customer could
have it different.

The choice how much you increment some MAC address is not a hardware
property. It does not even look like a firmware property. If playing
with this property was done by firmware, like we do for all MAC address
fields, then I would expect here some references to it. Which you did
not provide, I believe.


> 
>>
>> I might be missing the context but there is no DTS example nor user
>> of
>> this property, so how can I get such?
>>
> 
> I don't see it either in linux kernel DTS tree but it in DTS doc.
> 
> Also, just a little bit history about older propositions
> https://lore.kernel.org/all/?q=mac-address-increment
> https://lore.kernel.org/all/20200919214941.8038-5-ansuelsmth@gmail.com/

I don't see any user there, except the same rejected proposal:

https://lore.kernel.org/all/CAL_JsqKhyeh2=pJcpBKkh+s3FM__DY+VoYSYJLRUErrujTLn9A@mail.gmail.com/

If you want to convince us, please illustrate it in a real world
upstreamed DTS (or explain why it cannot). Otherwise I don't see
justification as it is not a hardware property.

This is a NAK from me.

Feel free to ping Rob in some later time, as he might have different
opinion.

Best regards,
Krzysztof