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

[Ivan Mikhaylov]

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

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.