Re: [PATCH] net: dsa: read mac address from DT for slave device

[xiaofeis@codeaurora.org]

On 2019-02-26 01:27, Florian Fainelli wrote:
> On 2/25/19 5:28 AM, xiaofeis@codeaurora.org wrote:
>> Hi Florian
>> 
>> We have two slave DSA interfaces, wan0 and lan0, one is for wan port,
>> and the other is for lan port. Customer has it's mac address pool, 
>> they
>> want
>> to assign the mac address from the pool on wan0, lan0, and other
>> interfaces like
>> wifi, bt. Coreboot/uboot will populate it to the DTS node, so the 
>> driver
>> can
>> get it from it's node. For DSA slave interface, it already has it's 
>> own
>> DTS node, it's
>> easy to just add one porperty "local-mac-address" there for the usage 
>> in
>> DSA driver.
>> 
>> If not use DSA framework, normally we will use eth0.x and eth0.y for 
>> wan
>> and lan.
>> On this case, customer usually also assign the MAC address on these
>> logical interface
>> from it's pool.
> 
> OK, but this is not necessary per my previous explanation: the CPU <=>
> WAN pipe is a separate broadcast domain (otherwise it is a security 
> hole
> since you exposing LAN machines to the outside world), and so there is
> no need for a separate MAC address. It might be convenient to have one,
> especially for the provider, if they run a management software (e.g.:
> TR69), but it is not required per-se.
> 
> Let me ask a secondary question here, how many Ethernet MACs connect to
> the switch in this configuration? Is there one that is supposed to be
> assigned all LAN traffic and one that is supposed to be assigned all 
> WAN
> traffic? If so, then what you are doing makes even less
> 

Only one MAC connected to switch cpu port, both lan0 and wan0 are on the 
top of
same interface(eth0).

>> 
>> On 2019-02-22 23:43, Florian Fainelli wrote:
>>> On 2/22/19 4:58 AM, Vinod Koul wrote:
>>>> From: Xiaofei Shen <xiaofeis@codeaurora.org>
>>>> 
>>>> Before creating a slave netdevice, get the mac address from DTS and
>>>> apply in case it is valid.
>>> 
>>> Can you explain your use case in details?
>>> 
>>> Assigning a MAC address to a network device that represents a switch
>>> port does not quite make sense in general. The switch port is really
>>> representing one end of a pipe, so one side you have stations and on 
>>> the
>>> other side, you have the CPU/management Ethernet MAC controller's MAC
>>> address which constitutes a station as well. The DSA slave network
>>> devices are just software constructs meant to steer traffic towards
>>> specific ports of the switch, but they are all from the perpsective 
>>> of
>>> traffic reaching the CPU Port in the first place, therefore traffic 
>>> that
>>> is generally a known unicast Ethernet frame with the CPU's MAC 
>>> address
>>> as MAC DA (and of course all types of unknown MC, management traffic
>>> etc.)
>>> 
>>> By default, DSA switch need to come up in a configuration where all
>>> ports (except CPU/management) must be strictly separate from every 
>>> other
>>> port such that we can achieve what a standalone Ethernet NIC would 
>>> do.
>>> This works because all ports are isolated from one another, so there 
>>> is
>>> no cross talk and so having the same MAC address (the one from the 
>>> CPU)
>>> on the DSA slave network devices just works, each port is a separate
>>> broadcast domain.
>>> 
>>> Once you start bridging one or ore ports, the bridge root port will 
>>> have
>>> a MAC address, most likely the one the CPU/management Ethernet MAC, 
>>> but
>>> similarly, this is not an issue and that's exactly how a software 
>>> bridge
>>> would work as well.
>>> 
>>>> 
>>>> Signed-off-by: Xiaofei Shen <xiaofeis@codeaurora.org>
>>>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
>>>> ---
>>>>  include/net/dsa.h | 1 +
>>>>  net/dsa/dsa2.c    | 1 +
>>>>  net/dsa/slave.c   | 5 ++++-
>>>>  3 files changed, 6 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>>>> index b3eefe8e18fd..aa24ce756679 100644
>>>> --- a/include/net/dsa.h
>>>> +++ b/include/net/dsa.h
>>>> @@ -198,6 +198,7 @@ struct dsa_port {
>>>>      unsigned int        index;
>>>>      const char        *name;
>>>>      const struct dsa_port    *cpu_dp;
>>>> +    const char        *mac;
>>>>      struct device_node    *dn;
>>>>      unsigned int        ageing_time;
>>>>      u8            stp_state;
>>>> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
>>>> index a1917025e155..afb7d9fa42f6 100644
>>>> --- a/net/dsa/dsa2.c
>>>> +++ b/net/dsa/dsa2.c
>>>> @@ -261,6 +261,7 @@ static int dsa_port_setup(struct dsa_port *dp)
>>>>      int err = 0;
>>>> 
>>>>      memset(&dp->devlink_port, 0, sizeof(dp->devlink_port));
>>>> +    dp->mac = of_get_mac_address(dp->dn);
>>>> 
>>>>      if (dp->type != DSA_PORT_TYPE_UNUSED)
>>>>          err = devlink_port_register(ds->devlink, &dp->devlink_port,
>>>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>>>> index a3fcc1d01615..8e64c4e947c6 100644
>>>> --- a/net/dsa/slave.c
>>>> +++ b/net/dsa/slave.c
>>>> @@ -1308,7 +1308,10 @@ int dsa_slave_create(struct dsa_port *port)
>>>>      slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
>>>>      slave_dev->hw_features |= NETIF_F_HW_TC;
>>>>      slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
>>>> -    eth_hw_addr_inherit(slave_dev, master);
>>>> +    if (port->mac && is_valid_ether_addr(port->mac))
>>>> +        ether_addr_copy(slave_dev->dev_addr, port->mac);
>>>> +    else
>>>> +        eth_hw_addr_inherit(slave_dev, master);
>>>>      slave_dev->priv_flags |= IFF_NO_QUEUE;
>>>>      slave_dev->netdev_ops = &dsa_slave_netdev_ops;
>>>>      slave_dev->switchdev_ops = &dsa_slave_switchdev_ops;
>>>>