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;
Date:   Sat, 22 Jul 2023 03:19:10 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Tristram.Ha@microchip.com
Cc:     o.rempel@pengutronix.de, kernel@pengutronix.de,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        UNGLinuxDriver@microchip.com, Woojung.Huh@microchip.com,
        andrew@lunn.ch, linux@armlinux.org.uk, devicetree@vger.kernel.org,
        pabeni@redhat.com, kuba@kernel.org, davem@davemloft.net,
        Arun.Ramadoss@microchip.com, edumazet@google.com,
        f.fainelli@gmail.com, conor+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, robh+dt@kernel.org
Subject: Re: [PATCH net-next v2 6/6] net: dsa: microchip: ksz9477: make
 switch MAC address configurable
Message-ID: <20230722001910.fst7vmvi4ktob4lt@skbuf>
References: <20230721135501.1464455-1-o.rempel@pengutronix.de>
 <20230721135501.1464455-7-o.rempel@pengutronix.de>
 <BYAPR11MB3558A296C1D1830F15AC6BEFEC3FA@BYAPR11MB3558.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BYAPR11MB3558A296C1D1830F15AC6BEFEC3FA@BYAPR11MB3558.namprd11.prod.outlook.com>
Precedence: bulk

On Fri, Jul 21, 2023 at 10:09:57PM +0000, Tristram.Ha@microchip.com wrote:
> > The switch MAC address is used for sending pause frames and for Wake on Magic
> > Packet. So, make use of local-mac-address property in the switch node
> > root and configure it in the HW.
> >
> 
> > @@ -1211,6 +1211,14 @@ int ksz9477_setup(struct dsa_switch *ds)
> >         if (dev->wakeup_source)
> >                 ksz_write8(dev, REG_SW_PME_CTRL, PME_ENABLE);
> > 
> > +       if (is_valid_ether_addr(dev->mac_addr)) {
> > +               int i;
> > +
> > +               for (i = 0; i < ETH_ALEN; i++)
> > +                       ksz_write8(dev, REG_SW_MAC_ADDR_0 + i,
> > +                                  dev->mac_addr[i]);
> > +       }
> > +
> 
> > +
> > +               mac = of_get_property(dev->dev->of_node, "local-mac-address",
> > +                                     NULL);
> > +               if (mac)
> > +                       memcpy(dev->mac_addr, mac, ETH_ALEN);
>  
> So the Magic Packet uses the same MAC address for all KSZ9477 switches
> if local-mac-address is not defined.  Is that practical in real operating
> environment?

"same address" which is 00:00:00:00:00:00 AFAIU.

Do you mean to ask "what do we do for systems without this device tree
property, don't we want to support Magic Packet for them too?". If so,
I agree, it is a valid concern (although we need to modify it anyway,
to add "wakeup-source", apparently).

Additionally, "local-mac-address" would be overloaded with a secondary
meaning compared to what ethernet-controller.yaml says:

      Specifies the MAC address that was assigned to the network device.

...which this is not.

Since the switch hardware doesn't have a register per user port that
could be kept in sync with the MAC address of the Linux interface, it
means that the address for WOL and for flow control will always require
a special way for the user to find out about it.

I'm thinking, would it be simpler to just program the hardware with the
MAC address of the DSA master? Every DSA master has one; we can track
changes to it; no special device tree properties are needed; it also
kinda makes sense as a MAC SA for flow control as well; DSA user ports
also inherit it if they don't have a local-mac-address of their own.
If there needs to be an override for that auto-selected MAC address, a
device tree property can be added later, in the "microchip," namespace.

But "how does the user find out what MAC address to use for WOL" still
remains a concern, when it's stuffed in the device tree. Is there going
to be a BIOS customized for the KSZ9477 which always fixes up the
local-mac-address of the switch node with what's set there?

> The DSA driver used to have an API to set the MAC address to the switch,
> but it was removed because nobody used it.
> 

Which API is that? "git log -GREG_SW_MAC_ADDR_0 drivers/net/dsa/" came
up with nothing.