| Message ID | 20190222125815.12866-1-vkoul@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: dsa: read mac address from DT for slave device | expand |
On Fri, Feb 22, 2019 at 06:28:15PM +0530, 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. > > Signed-off-by: Xiaofei Shen <xiaofeis@codeaurora.org> > Signed-off-by: Vinod Koul <vkoul@kernel.org> Hi Xiaofei, Vinod It would be good to document this in the binding. Andrew
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; >
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. 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; >>
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 > > 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; >>>
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; >>>>
On 2019-02-26 15:45, xiaofeis@codeaurora.org wrote: > 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). > Customer doesn't care about the MAC controller's MAC address, just leave it as the driver randomly generated. They just want to assign the MAC address on wan and lan DSA logical interface. Many customer doesn't use DSA, for example, they use eth0.1/eth0.2 for lan/wan with one MAC controller. They configure switch wan port in vlan2 group, and lan port in vlan1 group, they usually assign mac address on the logical interface(eth0.1ð0.2), i think this is similar with DSA slave interfaces. >>> >>> 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; >>>>>
On 2/26/2019 6:04 PM, xiaofeis@codeaurora.org wrote: > On 2019-02-26 15:45, xiaofeis@codeaurora.org wrote: >> 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). >> > Customer doesn't care about the MAC controller's MAC address, just leave > it as the driver > randomly generated. They just want to assign the MAC address on wan and > lan DSA logical > interface. > > Many customer doesn't use DSA, for example, they use eth0.1/eth0.2 for > lan/wan with one MAC controller. > They configure switch wan port in vlan2 group, and lan port in vlan1 > group, they usually assign mac address > on the logical interface(eth0.1ð0.2), i think this is similar with > DSA slave interfaces. Yes it is a similar use case, and in both cases there is no really a functional need for a separate MAC address for lan/eth0.1 or wan/eth0.2 since the switch should be configured to perform IVL (Individual VLAN Learning) and would determine the egress port just fine based on the MAC DA. Because it is an established practice does not mean we should not challenge it :). My issue with your change is that because DSA is meant to be a flexible framework we do not know the type/nature of DSA master network device that is going to be used. That DSA master network device may or may not have it own MAC DA filtering capability. Having to filter its own MAC address is fine and a well established behavior, having to filter for more than one unicast address starts to be questionable and eats up filter space that could be better used for filtering MC addresses instead. Another possible concern is a station trying to spoof the MAC address, some switches may support protecting only one UC/management MAC address, so having more than one could create security attack surfaces. To give you an example, I work with 3 generations of DSA master network controllers (bcmgenet and bcmsysport drivers). - GENET supports 17 perfect filters, but we must include its own MAC address, the broadcast address and that leaves only 15 filters for MC - SYSTEMPORT is always attached to a switch but supports filtering the MAC DA based on its own MAC and then it is in promiscuous mode So with your scheme, we would leave only 13 filters for MC on GENET and we would putting the interface in promiscuous mode for SYSTEMPORT. Until we have a better switch-side filtering framework (and this is being worked on right now), I would prefer that we defer accepting those type of features. Andrew and Vivien might feel differently about that though.
On 2019-02-27 10:04, xiaofeis@codeaurora.org wrote: > On 2019-02-26 15:45, xiaofeis@codeaurora.org wrote: >> 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). >> > Customer doesn't care about the MAC controller's MAC address, just > leave it as the driver > randomly generated. They just want to assign the MAC address on wan > and lan DSA logical > interface. > > Many customer doesn't use DSA, for example, they use eth0.1/eth0.2 for > lan/wan with one MAC controller. > They configure switch wan port in vlan2 group, and lan port in vlan1 > group, they usually assign mac address > on the logical interface(eth0.1ð0.2), i think this is similar with > DSA slave interfaces. > Unlike vlan logical interface, dsa port already has it's dts node, so it's easy and natural to add the mac address property for DSA driver use. On DSA configuration, we won't assign ip on mater eth0. The mac DA from wan port will be the wan0's mac address, the mac DA from lan port will be the lan0's mac address, they are separate. I think the this is in general on some customer's deploy. >>>> >>>> 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; >>>>>>
On 2019-02-27 11:13, Florian Fainelli wrote: > On 2/26/2019 6:04 PM, xiaofeis@codeaurora.org wrote: >> On 2019-02-26 15:45, xiaofeis@codeaurora.org wrote: >>> 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). >>> >> Customer doesn't care about the MAC controller's MAC address, just >> leave >> it as the driver >> randomly generated. They just want to assign the MAC address on wan >> and >> lan DSA logical >> interface. >> >> Many customer doesn't use DSA, for example, they use eth0.1/eth0.2 for >> lan/wan with one MAC controller. >> They configure switch wan port in vlan2 group, and lan port in vlan1 >> group, they usually assign mac address >> on the logical interface(eth0.1ð0.2), i think this is similar with >> DSA slave interfaces. > > Yes it is a similar use case, and in both cases there is no really a > functional need for a separate MAC address for lan/eth0.1 or wan/eth0.2 > since the switch should be configured to perform IVL (Individual VLAN > Learning) and would determine the egress port just fine based on the > MAC > DA. Because it is an established practice does not mean we should not > challenge it :). > > My issue with your change is that because DSA is meant to be a flexible > framework we do not know the type/nature of DSA master network device > that is going to be used. That DSA master network device may or may not > have it own MAC DA filtering capability. Having to filter its own MAC > address is fine and a well established behavior, having to filter for > more than one unicast address starts to be questionable and eats up > filter space that could be better used for filtering MC addresses > instead. Another possible concern is a station trying to spoof the MAC > address, some switches may support protecting only one UC/management > MAC > address, so having more than one could create security attack surfaces. > > To give you an example, I work with 3 generations of DSA master network > controllers (bcmgenet and bcmsysport drivers). > > - GENET supports 17 perfect filters, but we must include its own MAC > address, the broadcast address and that leaves only 15 filters for MC > > - SYSTEMPORT is always attached to a switch but supports filtering the > MAC DA based on its own MAC and then it is in promiscuous mode > > So with your scheme, we would leave only 13 filters for MC on GENET and > we would putting the interface in promiscuous mode for SYSTEMPORT. > > Until we have a better switch-side filtering framework (and this is > being worked on right now), I would prefer that we defer accepting > those > type of features. Andrew and Vivien might feel differently about that > though. This patch is just add one more option, if there is valid mac address populated in the DTS, then use it or else still inherti from master. I don't think it will break the DSA flexible framework. I think this change make DSA more flexible on MAC address setting. Many cusomter use some of our QCA chips, some direclty use DSA, some use internal similar mechanism(one netdevice for each switch port with swtichdev), we didn't see any limiation when they populiate the mac address for each port in DTS with only one HW mac controller. So my opinion is this patch is want to add a option which is already used in many products, this change does not break anything, developer/customer can chose use or not.
On 2/27/2019 6:23 PM, xiaofeis@codeaurora.org wrote: > On 2019-02-27 11:13, Florian Fainelli wrote: >> On 2/26/2019 6:04 PM, xiaofeis@codeaurora.org wrote: >>> On 2019-02-26 15:45, xiaofeis@codeaurora.org wrote: >>>> 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). >>>> >>> Customer doesn't care about the MAC controller's MAC address, just leave >>> it as the driver >>> randomly generated. They just want to assign the MAC address on wan and >>> lan DSA logical >>> interface. >>> >>> Many customer doesn't use DSA, for example, they use eth0.1/eth0.2 for >>> lan/wan with one MAC controller. >>> They configure switch wan port in vlan2 group, and lan port in vlan1 >>> group, they usually assign mac address >>> on the logical interface(eth0.1ð0.2), i think this is similar with >>> DSA slave interfaces. >> >> Yes it is a similar use case, and in both cases there is no really a >> functional need for a separate MAC address for lan/eth0.1 or wan/eth0.2 >> since the switch should be configured to perform IVL (Individual VLAN >> Learning) and would determine the egress port just fine based on the MAC >> DA. Because it is an established practice does not mean we should not >> challenge it :). >> >> My issue with your change is that because DSA is meant to be a flexible >> framework we do not know the type/nature of DSA master network device >> that is going to be used. That DSA master network device may or may not >> have it own MAC DA filtering capability. Having to filter its own MAC >> address is fine and a well established behavior, having to filter for >> more than one unicast address starts to be questionable and eats up >> filter space that could be better used for filtering MC addresses >> instead. Another possible concern is a station trying to spoof the MAC >> address, some switches may support protecting only one UC/management MAC >> address, so having more than one could create security attack surfaces. >> >> To give you an example, I work with 3 generations of DSA master network >> controllers (bcmgenet and bcmsysport drivers). >> >> - GENET supports 17 perfect filters, but we must include its own MAC >> address, the broadcast address and that leaves only 15 filters for MC >> >> - SYSTEMPORT is always attached to a switch but supports filtering the >> MAC DA based on its own MAC and then it is in promiscuous mode >> >> So with your scheme, we would leave only 13 filters for MC on GENET and >> we would putting the interface in promiscuous mode for SYSTEMPORT. >> >> Until we have a better switch-side filtering framework (and this is >> being worked on right now), I would prefer that we defer accepting those >> type of features. Andrew and Vivien might feel differently about that >> though. > This patch is just add one more option, if there is valid mac address > populated > in the DTS, then use it or else still inherti from master. I don't think > it will > break the DSA flexible framework. I think this change make DSA more > flexible on > MAC address setting. > Many cusomter use some of our QCA chips, some direclty use DSA, some use > internal similar > mechanism(one netdevice for each switch port with swtichdev), we didn't > see any limiation > when they populiate the mac address for each port in DTS with only one > HW mac controller. > So my opinion is this patch is want to add a option which is already > used in many > products, this change does not break anything, developer/customer can > chose use or not. As I wrote, I am not totally opposed to it, I would prefer we had a better infrastructure for UC/MC filtering in place before landing this change but that is not there yet, and won't happen over night. So please address Andrew's feedback to provide an update to the DSA binding document, repost and I will certainly Ack it this time. Please be considerate of people giving you feedback and do not try to circle back to your use case and just explaining in a different way than "works for me, accept it", because that's not going to work really well in the long run. Looking forward for more contributions on the qca8k driver, thanks!
On 2019-02-22 22:26, Andrew Lunn wrote: > On Fri, Feb 22, 2019 at 06:28:15PM +0530, 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. >> >> Signed-off-by: Xiaofei Shen <xiaofeis@codeaurora.org> >> Signed-off-by: Vinod Koul <vkoul@kernel.org> > > Hi Xiaofei, Vinod > > It would be good to document this in the binding. > > Andrew From 4c8597161975dbef5fbe814aeee93599b4c57d37 Mon Sep 17 00:00:00 2001 From: xiaofeis <xiaofeis@codeaurora.org> Date: Fri, 1 Mar 2019 13:54:52 +0800 Subject: [PATCH] net: dsa: Add support for port mac address Allow port network device's mac address to be retrieved from the device tree. Signed-off-by: xiaofeis <xiaofeis@codeaurora.org> --- Documentation/devicetree/bindings/net/dsa/dsa.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.txt b/Documentation/devicetree/bindings/net/dsa/dsa.txt index cfe8f64..1816f82 100644 --- a/Documentation/devicetree/bindings/net/dsa/dsa.txt +++ b/Documentation/devicetree/bindings/net/dsa/dsa.txt @@ -71,6 +71,10 @@ properties, described in binding documents: Documentation/devicetree/bindings/net/fixed-link.txt for details. +- local-mac-address : See + Documentation/devicetree/bindings/net/ethernet.txt + for details. + Example The following example shows three switches on three MDIO busses, @@ -99,6 +103,7 @@ linked into one DSA cluster. port@1 { reg = <1>; label = "lan1"; + local-mac-address = [000000000001]; }; port@2 {
On 2019-02-28 11:54, Florian Fainelli wrote: > On 2/27/2019 6:23 PM, xiaofeis@codeaurora.org wrote: >> On 2019-02-27 11:13, Florian Fainelli wrote: >>> On 2/26/2019 6:04 PM, xiaofeis@codeaurora.org wrote: >>>> On 2019-02-26 15:45, xiaofeis@codeaurora.org wrote: >>>>> 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). >>>>> >>>> Customer doesn't care about the MAC controller's MAC address, just >>>> leave >>>> it as the driver >>>> randomly generated. They just want to assign the MAC address on wan >>>> and >>>> lan DSA logical >>>> interface. >>>> >>>> Many customer doesn't use DSA, for example, they use eth0.1/eth0.2 >>>> for >>>> lan/wan with one MAC controller. >>>> They configure switch wan port in vlan2 group, and lan port in vlan1 >>>> group, they usually assign mac address >>>> on the logical interface(eth0.1ð0.2), i think this is similar >>>> with >>>> DSA slave interfaces. >>> >>> Yes it is a similar use case, and in both cases there is no really a >>> functional need for a separate MAC address for lan/eth0.1 or >>> wan/eth0.2 >>> since the switch should be configured to perform IVL (Individual VLAN >>> Learning) and would determine the egress port just fine based on the >>> MAC >>> DA. Because it is an established practice does not mean we should not >>> challenge it :). >>> >>> My issue with your change is that because DSA is meant to be a >>> flexible >>> framework we do not know the type/nature of DSA master network device >>> that is going to be used. That DSA master network device may or may >>> not >>> have it own MAC DA filtering capability. Having to filter its own MAC >>> address is fine and a well established behavior, having to filter for >>> more than one unicast address starts to be questionable and eats up >>> filter space that could be better used for filtering MC addresses >>> instead. Another possible concern is a station trying to spoof the >>> MAC >>> address, some switches may support protecting only one UC/management >>> MAC >>> address, so having more than one could create security attack >>> surfaces. >>> >>> To give you an example, I work with 3 generations of DSA master >>> network >>> controllers (bcmgenet and bcmsysport drivers). >>> >>> - GENET supports 17 perfect filters, but we must include its own MAC >>> address, the broadcast address and that leaves only 15 filters for MC >>> >>> - SYSTEMPORT is always attached to a switch but supports filtering >>> the >>> MAC DA based on its own MAC and then it is in promiscuous mode >>> >>> So with your scheme, we would leave only 13 filters for MC on GENET >>> and >>> we would putting the interface in promiscuous mode for SYSTEMPORT. >>> >>> Until we have a better switch-side filtering framework (and this is >>> being worked on right now), I would prefer that we defer accepting >>> those >>> type of features. Andrew and Vivien might feel differently about that >>> though. >> This patch is just add one more option, if there is valid mac address >> populated >> in the DTS, then use it or else still inherti from master. I don't >> think >> it will >> break the DSA flexible framework. I think this change make DSA more >> flexible on >> MAC address setting. >> Many cusomter use some of our QCA chips, some direclty use DSA, some >> use >> internal similar >> mechanism(one netdevice for each switch port with swtichdev), we >> didn't >> see any limiation >> when they populiate the mac address for each port in DTS with only one >> HW mac controller. >> So my opinion is this patch is want to add a option which is already >> used in many >> products, this change does not break anything, developer/customer can >> chose use or not. > > As I wrote, I am not totally opposed to it, I would prefer we had a > better infrastructure for UC/MC filtering in place before landing this > change but that is not there yet, and won't happen over night. So > please > address Andrew's feedback to provide an update to the DSA binding > document, repost and I will certainly Ack it this time. > > Please be considerate of people giving you feedback and do not try to > circle back to your use case and just explaining in a different way > than > "works for me, accept it", because that's not going to work really well > in the long run. > > Looking forward for more contributions on the qca8k driver, thanks! Thanks Florian, I have reply Andres's mail to provide DSA binding document change.
On 04-03-19, 11:46, xiaofeis@codeaurora.org wrote: > On 2019-02-22 22:26, Andrew Lunn wrote: > > On Fri, Feb 22, 2019 at 06:28:15PM +0530, 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. > > > > > > Signed-off-by: Xiaofei Shen <xiaofeis@codeaurora.org> > > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > > > > Hi Xiaofei, Vinod > > > > It would be good to document this in the binding. > > > > Andrew > > From 4c8597161975dbef5fbe814aeee93599b4c57d37 Mon Sep 17 00:00:00 2001 > From: xiaofeis <xiaofeis@codeaurora.org> > Date: Fri, 1 Mar 2019 13:54:52 +0800 > Subject: [PATCH] net: dsa: Add support for port mac address > > Allow port network device's mac address to be retrieved from the > device tree. > > Signed-off-by: xiaofeis <xiaofeis@codeaurora.org> > --- > Documentation/devicetree/bindings/net/dsa/dsa.txt | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.txt > b/Documentation/devicetree/bindings/net/dsa/dsa.txt > index cfe8f64..1816f82 100644 > --- a/Documentation/devicetree/bindings/net/dsa/dsa.txt > +++ b/Documentation/devicetree/bindings/net/dsa/dsa.txt > @@ -71,6 +71,10 @@ properties, described in binding documents: > Documentation/devicetree/bindings/net/fixed-link.txt > for details. > > +- local-mac-address : See > + Documentation/devicetree/bindings/net/ethernet.txt > + for details. > + Can you send this as independent patch to Dave and cc netdev and others on this thread. This looks good to me so feel free to add Reviewed-by: Vinod Koul <vkoul@kernel.org> > Example > > The following example shows three switches on three MDIO busses, > @@ -99,6 +103,7 @@ linked into one DSA cluster. > port@1 { > reg = <1>; > label = "lan1"; > + local-mac-address = [000000000001]; > }; > > port@2 {
> The following example shows three switches on three MDIO busses, > @@ -99,6 +103,7 @@ linked into one DSA cluster. > port@1 { > reg = <1>; > label = "lan1"; > + local-mac-address = [000000000001]; > }; bindings/soc/fsl/cpm_qe/network.txt: local-mac-address = [ 00 00 00 00 00 00 ]; bindings/net/ibm,emac.txt: local-mac-address = [00 04 AC E3 1B 1E]; bindings/net/ibm,emac.txt: local-mac-address = [000000000000]; /* Filled in by U-Boot */ bindings/net/fsl-fec.txt: local-mac-address = [00 04 9F 01 1B B9]; bindings/net/fsl-fec.txt: local-mac-address = [00 04 9F 01 1B B9]; bindings/net/socionext,uniphier-ave4.txt: local-mac-address = [00 00 00 00 00 00]; bindings/net/opencores-ethoc.txt: local-mac-address = [00 50 c2 13 6f 00]; bindings/net/marvell-orion-net.txt: local-mac-address = [00 00 00 00 00 00]; bindings/net/davicom-dm9000.txt: local-mac-address = [00 00 de ad be ef]; bindings/net/altera_tse.txt: local-mac-address = [ 00 00 00 00 00 00 ]; bindings/net/altera_tse.txt: local-mac-address = [ 00 00 00 00 00 00 ]; bindings/net/macb.txt: local-mac-address = [3a 0e 03 04 05 06]; bindings/net/brcm,systemport.txt: local-mac-address = [ 00 11 22 33 44 55 ]; bindings/net/fsl-tsec-phy.txt: local-mac-address = [ 00 E0 0C 00 73 00 ]; bindings/net/hisilicon-hns-nic.txt: local-mac-address = [a2 14 e4 4b 56 76]; bindings/net/keystone-netcp.txt: local-mac-address = [02 18 31 7e 3e 6f]; Does the compiler really turn 000000000001 into 6 bytes? Please use the more preferred option. Andrew
On 2019-02-28 11:54, Florian Fainelli wrote: > On 2/27/2019 6:23 PM, xiaofeis@codeaurora.org wrote: >> On 2019-02-27 11:13, Florian Fainelli wrote: >>> On 2/26/2019 6:04 PM, xiaofeis@codeaurora.org wrote: >>>> On 2019-02-26 15:45, xiaofeis@codeaurora.org wrote: >>>>> 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). >>>>> >>>> Customer doesn't care about the MAC controller's MAC address, just >>>> leave >>>> it as the driver >>>> randomly generated. They just want to assign the MAC address on wan >>>> and >>>> lan DSA logical >>>> interface. >>>> >>>> Many customer doesn't use DSA, for example, they use eth0.1/eth0.2 >>>> for >>>> lan/wan with one MAC controller. >>>> They configure switch wan port in vlan2 group, and lan port in vlan1 >>>> group, they usually assign mac address >>>> on the logical interface(eth0.1ð0.2), i think this is similar >>>> with >>>> DSA slave interfaces. >>> >>> Yes it is a similar use case, and in both cases there is no really a >>> functional need for a separate MAC address for lan/eth0.1 or >>> wan/eth0.2 >>> since the switch should be configured to perform IVL (Individual VLAN >>> Learning) and would determine the egress port just fine based on the >>> MAC >>> DA. Because it is an established practice does not mean we should not >>> challenge it :). >>> >>> My issue with your change is that because DSA is meant to be a >>> flexible >>> framework we do not know the type/nature of DSA master network device >>> that is going to be used. That DSA master network device may or may >>> not >>> have it own MAC DA filtering capability. Having to filter its own MAC >>> address is fine and a well established behavior, having to filter for >>> more than one unicast address starts to be questionable and eats up >>> filter space that could be better used for filtering MC addresses >>> instead. Another possible concern is a station trying to spoof the >>> MAC >>> address, some switches may support protecting only one UC/management >>> MAC >>> address, so having more than one could create security attack >>> surfaces. >>> >>> To give you an example, I work with 3 generations of DSA master >>> network >>> controllers (bcmgenet and bcmsysport drivers). >>> >>> - GENET supports 17 perfect filters, but we must include its own MAC >>> address, the broadcast address and that leaves only 15 filters for MC >>> >>> - SYSTEMPORT is always attached to a switch but supports filtering >>> the >>> MAC DA based on its own MAC and then it is in promiscuous mode >>> >>> So with your scheme, we would leave only 13 filters for MC on GENET >>> and >>> we would putting the interface in promiscuous mode for SYSTEMPORT. >>> >>> Until we have a better switch-side filtering framework (and this is >>> being worked on right now), I would prefer that we defer accepting >>> those >>> type of features. Andrew and Vivien might feel differently about that >>> though. >> This patch is just add one more option, if there is valid mac address >> populated >> in the DTS, then use it or else still inherti from master. I don't >> think >> it will >> break the DSA flexible framework. I think this change make DSA more >> flexible on >> MAC address setting. >> Many cusomter use some of our QCA chips, some direclty use DSA, some >> use >> internal similar >> mechanism(one netdevice for each switch port with swtichdev), we >> didn't >> see any limiation >> when they populiate the mac address for each port in DTS with only one >> HW mac controller. >> So my opinion is this patch is want to add a option which is already >> used in many >> products, this change does not break anything, developer/customer can >> chose use or not. > > As I wrote, I am not totally opposed to it, I would prefer we had a > better infrastructure for UC/MC filtering in place before landing this > change but that is not there yet, and won't happen over night. So > please > address Andrew's feedback to provide an update to the DSA binding > document, repost and I will certainly Ack it this time. > > Please be considerate of people giving you feedback and do not try to > circle back to your use case and just explaining in a different way > than > "works for me, accept it", because that's not going to work really well > in the long run. > > Looking forward for more contributions on the qca8k driver, thanks! The change for DSA binding document has been applied by David in another mail, do you want Vinod or me repost this patch. Thanks.
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;