Re: [PATCH v2 3/5] dt-bindings: net: add mac-address-increment option

[Krzysztof Kozlowski]

On 12/05/2023 13:28, Ivan Mikhaylov wrote:
> On Fri, 2023-05-12 at 08:22 +0200, Krzysztof Kozlowski wrote:
>> On 11/05/2023 01:31, Ivan Mikhaylov wrote:
>>> On Wed, 2023-05-10 at 16:48 +0200, Krzysztof Kozlowski wrote:
>>>> On 09/05/2023 16:35, Ivan Mikhaylov wrote:
>>>>> Add the mac-address-increment option for specify MAC address
>>>>> taken
>>>>> by
>>>>> any other sources.
>>>>>
>>>>> Signed-off-by: Paul Fertser <fercerpav@xxxxxxxxx>
>>>>> Signed-off-by: Ivan Mikhaylov <fr0st61te@xxxxxxxxx>
>>>>> ---
>>>>>  .../devicetree/bindings/net/ethernet-controller.yaml      | 8
>>>>> ++++++++
>>>>>  1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/net/ethernet-
>>>>> controller.yaml
>>>>> b/Documentation/devicetree/bindings/net/ethernet-
>>>>> controller.yaml
>>>>> index 00be387984ac..6900098c5105 100644
>>>>> --- a/Documentation/devicetree/bindings/net/ethernet-
>>>>> controller.yaml
>>>>> +++ b/Documentation/devicetree/bindings/net/ethernet-
>>>>> controller.yaml
>>>>> @@ -34,6 +34,14 @@ properties:
>>>>>      minItems: 6
>>>>>      maxItems: 6
>>>>>  
>>>>> +  mac-address-increment:
>>>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>>>> +    description:
>>>>> +      Specifies the MAC address increment to be added to the
>>>>> MAC
>>>>> address.
>>>>> +      Should be used in cases when there is a need to use MAC
>>>>> address
>>>>> +      different from one obtained by any other level, like u-
>>>>> boot
>>>>> or the
>>>>> +      NC-SI stack.
>>>>
>>>> We don't store MAC addresses in DT, but provide simple
>>>> placeholder
>>>> for
>>>> firmware or bootloader. Why shall we store static "increment"
>>>> part of
>>>> MAC address? Can't the firmware give you proper MAC address?
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> Krzysztof, maybe that's a point to make commit message with better
>>> explanation from my side. At current time there is at least two
>>> cases
>>> where I see it's possible to be used:
>>>
>>> 1. NC-SI
>>> 2. embedded
>>>
>>> At NC-SI level there is Get Mac Address command which provides to
>>> BMC
>>> mac address from the host which is same as host mac address, it
>>> happens
>>> at runtime and overrides old one.
>>>
>>> Also, this part was also to be discussed 2 years ago in this
>>> thread:
>>> https://lore.kernel.org/all/OF8E108F72.39D22E89-ON00258765.001E46EB-00258765.00251157@xxxxxxx/
>>
>> Which was not sent to Rob though...
>>
>>
>>>
>>> Where Milton provided this information:
>>>
>>> DTMF spec DSP0222 NC-SI (network controller sideband interface)
>>> is a method to provide a BMC (Baseboard management controller)
>>> shared
>>> access to an external ethernet port for comunication to the
>>> management
>>> network in the outside world.  The protocol describes ethernet
>>> packets 
>>> that control selective bridging implemented in a host network
>>> controller
>>> to share its phy.  Various NIC OEMs have added a query to find out
>>> the 
>>> address the host is using, and some vendors have added code to
>>> query
>>> host
>>> nic and set the BMC mac to a fixed offset (current hard coded +1
>>> from
>>> the host value).  If this is compiled in the kernel, the NIC OEM is
>>> recognised and the BMC doesn't miss the NIC response the address is
>>> set
>>> once each time the NCSI stack reinitializes.  This mechanism
>>> overrides
>>> any mac-address or local-mac-address or other assignment.
>>>
>>> DSP0222
>>> https://www.dmtf.org/documents/pmci/network-controller-sideband-interface-nc-si-specification-110
>>>
>>>
>>> In embedded case, sometimes you have different multiple ethernet
>>> interfaces which using one mac address which increments or
>>> decrements
>>> for particular interface, just for better explanation, there is
>>> patch
>>> with explanation which providing them such way of work:
>>> https://github.com/openwrt/openwrt/blob/master/target/linux/generic/pending-5.15/682-of_net-add-mac-address-increment-support.patch
>>>
>>> In their rep a lot of dts using such option.
>>
>> None of these explain why 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?

Anyway, commit msg also lacks any justification for this.

Best regards,
Krzysztof