diff mbox series

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

Message ID 20230509143504.30382-4-fr0st61te@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Refactoring for GMA command | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Guessed tree name to be net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers warning 1 maintainers not CCed: conor+dt@kernel.org
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 14 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Ivan Mikhaylov May 9, 2023, 2:35 p.m. UTC
Add the mac-address-increment option for specify MAC address taken by
any other sources.

Signed-off-by: Paul Fertser <fercerpav@gmail.com>
Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 .../devicetree/bindings/net/ethernet-controller.yaml      | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Krzysztof Kozlowski May 10, 2023, 2:48 p.m. UTC | #1
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@gmail.com>
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> ---
>  .../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
Ivan Mikhaylov May 10, 2023, 11:31 p.m. UTC | #2
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@gmail.com>
> > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> > ---
> >  .../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@ibm.com/

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.

Thanks.
Krzysztof Kozlowski May 12, 2023, 6:22 a.m. UTC | #3
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@gmail.com>
>>> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
>>> ---
>>>  .../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@ibm.com/

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.

Best regards,
Krzysztof
Krzysztof Kozlowski May 12, 2023, 9:24 a.m. UTC | #4
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@gmail.com>
>>>>> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
>>>>> ---
>>>>>  .../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@ibm.com/
>>
>> 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
Ivan Mikhaylov May 12, 2023, 11:28 a.m. UTC | #5
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@gmail.com>
> > > > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> > > > ---
> > > >  .../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@ibm.com/
> 
> 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?

Thanks.
Ivan Mikhaylov May 16, 2023, 11:47 a.m. UTC | #6
On Fri, 2023-05-12 at 11:24 +0200, Krzysztof Kozlowski wrote:
> 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@gmail.com>
> > > > > > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> > > > > > ---
> > > > > >  .../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@ibm.com/
> > > 
> > > 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
> 

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
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


Thanks.
Krzysztof Kozlowski May 17, 2023, 8:36 a.m. UTC | #7
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?
>>
>> 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?

> 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.

I might be missing the context but there is no DTS example nor user of
this property, so how can I get such?

Best regards,
Krzysztof
Krzysztof Kozlowski May 17, 2023, 7:26 p.m. UTC | #8
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
Ivan Mikhaylov May 17, 2023, 9:38 p.m. UTC | #9
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?
> > > 
> > > 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.

> 
> > 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

"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?

> 
> 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/


Thanks.
Paul Fertser May 29, 2023, 8:59 p.m. UTC | #10
Hello Krzysztof,

Let me try to clarify a bit on the particular usecase and answer your
questions.

Let's consider a server motherboard manufactured and sold by a single
company. This motherboard includes I210 (Ethernet Controlleer) chip
along with the other necessary parts right there, soldered to the PCB,
non-replaceable. This I210 is connected to the host CPU with a PCIe
lane and acts as a regular network adapter. In addition to that this
chip is connected using NC-SI (management channel) to the BMC SoC
(also permanently soldered to the board).

There is a separate EEPROM connected directly to I210 which hosts its
firmware and many operational parameters, including the MAC
address. This EEPROM is not anyhow accessible by the BMC (the host can
read/write it using special protocol over PCIe). Intel expects the
board manufacturer to embed a MAC address from the manufacturer's
range in the EEPROM configuration. But in many cases it's desirable to
use a separate MAC address for the BMC (then I210 acts as if it has an
integrated switch), so the board manufacturer can, by its internal
policy, allocate two consecutive MAC addresses to each motherboard.

The only way BMC can learn the MAC address used by I210 is by a
special vendor-specific NC-SI command, and it can provide just a
single address, the one used by the host. NC-SI is using Ethernet
frames with a special type, so to execute this command the network
driver needs to be (at least partially) functional. I do not really
imagine nvmem getting support to read it this way.

On Wed, May 17, 2023 at 09:26:35PM +0200, Krzysztof Kozlowski wrote:
> I would like to remind this question.
> "why different boards with same device should have different offset/value?"

In the usecase we're aiming for the DT is describing a specific board
from manufacturer that guarantees the offset to be correct, as none of
the parts are replaceable and the MAC address is flashed into the
I210 EEPROM during manufacturing.

> 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?"

Here we assume that for all the boards supported by a particular DT
the board manufacturer guarantees the MAC address offset by internal
production policy, by allocating the addresses from the manufacturer's
pool.

> 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.

But MAC address of an Ethernet equipment is an inherent part of the
hardware. It's just that we can't store it in an nvmem-addressable
cell in this case, unfortunately.

> 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.

You assume that the customers might be allocating their own MAC
addresses for the network interface of a motherboard, that might be
true if the customer gets such a board from an ODM. But such a
customer not willing to follow the MAC address offsets policy is not
much different from a customer who e.g. modifies flash partitions or
storage format making the nvmem references invalid, and so requiring a
separate DT.

> 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.

Can you please tell how you would imagine a responsible vendor tackle
the usecase I outlined? Guess it's not by a startup script that would
be getting a MAC address from an interface, applying the offset, and
then change it on the same interface?

Thank you for the review and discussion.
Krzysztof Kozlowski June 4, 2023, 10:23 a.m. UTC | #11
On 29/05/2023 22:59, Paul Fertser wrote:
> Hello Krzysztof,
> 
> Let me try to clarify a bit on the particular usecase and answer your
> questions.
> 
> Let's consider a server motherboard manufactured and sold by a single
> company. This motherboard includes I210 (Ethernet Controlleer) chip
> along with the other necessary parts right there, soldered to the PCB,
> non-replaceable. This I210 is connected to the host CPU with a PCIe
> lane and acts as a regular network adapter. In addition to that this
> chip is connected using NC-SI (management channel) to the BMC SoC
> (also permanently soldered to the board).
> 
> There is a separate EEPROM connected directly to I210 which hosts its
> firmware and many operational parameters, including the MAC
> address. This EEPROM is not anyhow accessible by the BMC (the host can
> read/write it using special protocol over PCIe). Intel expects the
> board manufacturer to embed a MAC address from the manufacturer's
> range in the EEPROM configuration. But in many cases it's desirable to
> use a separate MAC address for the BMC (then I210 acts as if it has an
> integrated switch), so the board manufacturer can, by its internal
> policy, allocate two consecutive MAC addresses to each motherboard.
> 
> The only way BMC can learn the MAC address used by I210 is by a
> special vendor-specific NC-SI command, and it can provide just a
> single address, the one used by the host. NC-SI is using Ethernet
> frames with a special type, so to execute this command the network
> driver needs to be (at least partially) functional. I do not really
> imagine nvmem getting support to read it this way.
> 
> On Wed, May 17, 2023 at 09:26:35PM +0200, Krzysztof Kozlowski wrote:
>> I would like to remind this question.
>> "why different boards with same device should have different offset/value?"
> 
> In the usecase we're aiming for the DT is describing a specific board
> from manufacturer that guarantees the offset to be correct, as none of
> the parts are replaceable and the MAC address is flashed into the
> I210 EEPROM during manufacturing.
> 
>> 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?"
> 
> Here we assume that for all the boards supported by a particular DT
> the board manufacturer guarantees the MAC address offset by internal
> production policy, by allocating the addresses from the manufacturer's
> pool.

OK, embed such information in the commit or property description.

> 
>> 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.
> 
> But MAC address of an Ethernet equipment is an inherent part of the
> hardware. It's just that we can't store it in an nvmem-addressable
> cell in this case, unfortunately.
> 
>> 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.
> 
> You assume that the customers might be allocating their own MAC
> addresses for the network interface of a motherboard, that might be
> true if the customer gets such a board from an ODM. But such a
> customer not willing to follow the MAC address offsets policy is not
> much different from a customer who e.g. modifies flash partitions or
> storage format making the nvmem references invalid, and so requiring a
> separate DT.
> 
>> 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.
> 
> Can you please tell how you would imagine a responsible vendor tackle
> the usecase I outlined?

I would imagine him to upstream the DTS. I asked yo illustrate it. There
is still no DTS user for it so I have doubts it is used as intended.

> Guess it's not by a startup script that would
> be getting a MAC address from an interface, applying the offset, and
> then change it on the same interface?
> 
> Thank you for the review and discussion.
> 

Best regards,
Krzysztof
diff mbox series

Patch

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.
+
   max-frame-size:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: