Re: [PATCH net-next v2 6/6] net: dsa: microchip: ksz9477: make switch MAC address configurable

[Florian Fainelli <f.fainelli@gmail.com>]

On 7/21/2023 5:19 PM, Vladimir Oltean wrote:
> 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.

Yes the MAC address of the DSA master should be used, that is what we 
would naturally want to target magic packets with.

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

Agreed, I don't think we should be accepting that 'local-mac-address' be 
specified, it does not serve any good and only makes everything more 
confusing because this is not the MAC address that is set into the 
port's net_device::dev_addr at all.

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

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=841f4f24053acad69240c6ab7427a1d24bc29491
-- 
Florian