From mboxrd@z Thu Jan 1 00:00:00 1970 From: xiaofeis@codeaurora.org Subject: Re: [PATCH] net: dsa: read mac address from DT for slave device Date: Tue, 26 Feb 2019 15:45:32 +0800 Message-ID: <967ec7b53270f1e29bb25e087c1a67a4@codeaurora.org> References: <20190222125815.12866-1-vkoul@kernel.org> <9baf097f-37ea-2bef-92dd-25a7040a16ed@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: Sender: netdev-owner@vger.kernel.org To: Florian Fainelli Cc: Vinod Koul , "David S. Miller" , linux-arm-msm@vger.kernel.org, Bjorn Andersson , Andrew Lunn , Vivien Didelot , Niklas Cassel , netdev@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.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 >>>> >>>> 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 >>>> Signed-off-by: Vinod Koul >>>> --- >>>>  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; >>>>