| Message ID | 20240902015051.11159-1-junjie.wan@inceptio.ai (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] dpaa2-switch: fix flooding domain among multiple vlans | expand |
On Mon, Sep 02, 2024 at 09:50:51AM +0800, Wan Junjie wrote: > Currently, dpaa2 switch only cares dst mac and egress interface > in FDB. And all ports with different vlans share the same FDB. > > This will make things messed up when one device connected to > dpaa2 switch via two interfaces. Ports get two different vlans > assigned. These two ports will race for a same dst mac entry > since multiple vlans share one FDB. > > FDB below may not show up at the same time. > 02:00:77:88:99:aa dev swp0 self > 02:00:77:88:99:aa dev swp1 self > But in fact, for rules on the bridge, they should be: > 02:00:77:88:99:aa dev swp0 vlan 10 master br0 > 02:00:77:88:99:aa dev swp1 vlan 20 master br0 > > This patch address this by borrowing unused form ports' FDB > when ports join bridge. And append offload flag to hardware > offloaded rules so we can tell them from those on bridges. > > Signed-off-by: Wan Junjie <junjie.wan@inceptio.ai> Hi Wan Junjie, Some minor feedback from my side. ... > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > index a293b08f36d4..217c68bb0faa 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > @@ -25,8 +25,17 @@ > > #define DEFAULT_VLAN_ID 1 > > -static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv) > +static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv, u16 vid) This, and several other lines in this patch, could be trivially line wrapped in order for them to be <= 80 columns wide, as is still preferred in Networking code. This and a number of other minor problems are flagged by: ./scripts/checkpatch.pl --strict --codespell --max-line-length=80 > { > + struct ethsw_core *ethsw = port_priv->ethsw_data; > + int i; > + > + if (port_priv->fdb->bridge_dev) { > + for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) > + if (ethsw->fdbs[i].vid == vid) > + return ethsw->fdbs[i].fdb_id; > + } > + /* Default vlan, use port's fdb id directly */ > return port_priv->fdb->fdb_id; > } > ... > @@ -191,10 +212,38 @@ static void *dpaa2_iova_to_virt(struct iommu_domain *domain, > static int dpaa2_switch_add_vlan(struct ethsw_port_priv *port_priv, u16 vid) > { > struct ethsw_core *ethsw = port_priv->ethsw_data; > + struct net_device *netdev = port_priv->netdev; > + struct dpsw_fdb_cfg fdb_cfg = {0}; > struct dpsw_vlan_cfg vcfg = {0}; > + struct dpaa2_switch_fdb *fdb; > + u16 fdb_id; > int err; > > - vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); > + /* If ports are under a bridge, then > + * every VLAN domain should use a different fdb. > + * If ports are standalone, and > + * vid is 1 this should reuse the allocated port fdb. > + */ > + if (port_priv->fdb->bridge_dev) { > + fdb = dpaa2_switch_fdb_get_unused(ethsw); > + if (!fdb) { > + /* if not available, create a new fdb */ > + err = dpsw_fdb_add(ethsw->mc_io, 0, ethsw->dpsw_handle, > + &fdb_id, &fdb_cfg); > + if (err) { > + netdev_err(netdev, "dpsw_fdb_add err %d\n", err); > + return err; > + } fdb is still NULL here. Based on my reading of dpaa2_switch_port_init() I think you need the following. Possibly also with an error check. fdb = dpaa2_switch_fdb_get_unused(ethsw); Flagged by Smatch. > + fdb->fdb_id = fdb_id; > + } > + fdb->vid = vid; > + fdb->in_use = true; > + fdb->bridge_dev = NULL; > + vcfg.fdb_id = fdb->fdb_id; > + } else { > + /* Standalone, port's private fdb shared */ > + vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); > + } > err = dpsw_vlan_add(ethsw->mc_io, 0, > ethsw->dpsw_handle, vid, &vcfg); > if (err) { ...
> On Mon, Sep 02, 2024 at 09:50:51AM +0800, Wan Junjie wrote: > > Currently, dpaa2 switch only cares dst mac and egress interface > > in FDB. And all ports with different vlans share the same FDB. > > > > This will make things messed up when one device connected to > > dpaa2 switch via two interfaces. Ports get two different vlans > > assigned. These two ports will race for a same dst mac entry > > since multiple vlans share one FDB. > > > > FDB below may not show up at the same time. > > 02:00:77:88:99:aa dev swp0 self > > 02:00:77:88:99:aa dev swp1 self > > But in fact, for rules on the bridge, they should be: > > 02:00:77:88:99:aa dev swp0 vlan 10 master br0 > > 02:00:77:88:99:aa dev swp1 vlan 20 master br0 > > > > This patch address this by borrowing unused form ports' FDB > > when ports join bridge. And append offload flag to hardware > > offloaded rules so we can tell them from those on bridges. > > > > Signed-off-by: Wan Junjie <junjie.wan@inceptio.ai> > > Hi Wan Junjie, > > Some minor feedback from my side. > > ... Hi Simon Horman, Thanks for your commnets. > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > > index a293b08f36d4..217c68bb0faa 100644 > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > > @@ -25,8 +25,17 @@ > > > > #define DEFAULT_VLAN_ID 1 > > > > -static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv) > > +static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv, u16 vid) > > This, and several other lines in this patch, could be trivially > line wrapped in order for them to be <= 80 columns wide, as is > still preferred in Networking code. > > This and a number of other minor problems are flagged by: > ./scripts/checkpatch.pl --strict --codespell --max-line-length=80 It is hard to keep all lines limited to the length of 80 since some function names are really long. I have tried to make the line length to 85 without breaking some if conditions into multiple lines. You will see in v3. > > { > > + struct ethsw_core *ethsw = port_priv->ethsw_data; > > + int i; > > + > > + if (port_priv->fdb->bridge_dev) { > > + for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) > > + if (ethsw->fdbs[i].vid == vid) > > + return ethsw->fdbs[i].fdb_id; > > + } > > + /* Default vlan, use port's fdb id directly */ > > return port_priv->fdb->fdb_id; > > } > > > > ... > > > @@ -191,10 +212,38 @@ static void *dpaa2_iova_to_virt(struct iommu_domain *domain, > > static int dpaa2_switch_add_vlan(struct ethsw_port_priv *port_priv, u16 vid) > > { > > struct ethsw_core *ethsw = port_priv->ethsw_data; > > + struct net_device *netdev = port_priv->netdev; > > + struct dpsw_fdb_cfg fdb_cfg = {0}; > > struct dpsw_vlan_cfg vcfg = {0}; > > + struct dpaa2_switch_fdb *fdb; > > + u16 fdb_id; > > int err; > > > > - vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); > > + /* If ports are under a bridge, then > > + * every VLAN domain should use a different fdb. > > + * If ports are standalone, and > > + * vid is 1 this should reuse the allocated port fdb. > > + */ > > + if (port_priv->fdb->bridge_dev) { > > + fdb = dpaa2_switch_fdb_get_unused(ethsw); > > + if (!fdb) { > > + /* if not available, create a new fdb */ > > + err = dpsw_fdb_add(ethsw->mc_io, 0, ethsw->dpsw_handle, > > + &fdb_id, &fdb_cfg); > > + if (err) { > > + netdev_err(netdev, "dpsw_fdb_add err %d\n", err); > > + return err; > > + } > > fdb is still NULL here. Based on my reading of dpaa2_switch_port_init() > I think you need the following. Possibly also with an error check. > > fdb = dpaa2_switch_fdb_get_unused(ethsw); > > Flagged by Smatch. Nice catch. The fdb is not assigned anyway. And num of fdbs is limited by HW. So here I can do a judge instead of creating a new one. if (port_priv->fdb->bridge_dev) { fdb = dpaa2_switch_fdb_get_unused(ethsw); if (!fdb) { netdev_err(netdev, "vlan nums reach max_fdbs limits\n"); return -ENOENT; } Thank you. > > + fdb->fdb_id = fdb_id; > > + } > > + fdb->vid = vid; > > + fdb->in_use = true; > > + fdb->bridge_dev = NULL; > > + vcfg.fdb_id = fdb->fdb_id; > > + } else { > > + /* Standalone, port's private fdb shared */ > > + vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); > > + } > > err = dpsw_vlan_add(ethsw->mc_io, 0, > > ethsw->dpsw_handle, vid, &vcfg); > > if (err) { > > ...
On Mon, Sep 02, 2024 at 09:50:51AM +0800, Wan Junjie wrote: > Currently, dpaa2 switch only cares dst mac and egress interface > in FDB. And all ports with different vlans share the same FDB. > > This will make things messed up when one device connected to > dpaa2 switch via two interfaces. Ports get two different vlans > assigned. These two ports will race for a same dst mac entry > since multiple vlans share one FDB. > > FDB below may not show up at the same time. > 02:00:77:88:99:aa dev swp0 self > 02:00:77:88:99:aa dev swp1 self > But in fact, for rules on the bridge, they should be: > 02:00:77:88:99:aa dev swp0 vlan 10 master br0 > 02:00:77:88:99:aa dev swp1 vlan 20 master br0 > > This patch address this by borrowing unused form ports' FDB > when ports join bridge. And append offload flag to hardware > offloaded rules so we can tell them from those on bridges. > Hi, Thanks a lot for looking into adding support for this. Could you please allow me one more day (I just got back from vacation) before you send a v3 so that I can have a look and maybe test your patch? Thanks, Ioana
On Mon, 2 Sep 2024 20:43:18 +0800 Wan Junjie wrote: > > > -static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv) > > > +static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv, u16 vid) > > > > This, and several other lines in this patch, could be trivially > > line wrapped in order for them to be <= 80 columns wide, as is > > still preferred in Networking code. > > > > This and a number of other minor problems are flagged by: > > ./scripts/checkpatch.pl --strict --codespell --max-line-length=80 > > It is hard to keep all lines limited to the length of 80 since some function > names are really long. I have tried to make the line length to 85 without > breaking some if conditions into multiple lines. You will see in v3. FWIW it's relatively common to break the line after return type: static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv, u16 vid)
On Mon, Sep 02, 2024 at 09:50:51AM +0800, Wan Junjie wrote: > Currently, dpaa2 switch only cares dst mac and egress interface > in FDB. And all ports with different vlans share the same FDB. > Indeed, the DPAA2 switch driver was written in such a way that learning is shared between VLANs. The concern at that time was the limited amount of resources which are allocated at DPSW creation time and which need to be managed by the driver in the best way possible. That being said, I would suggest actually changing the title of the commit to something like "dpaa2-switch: add support for per-VLAN learning" so that it's clear that the driver did not support this prior. > This will make things messed up when one device connected to > dpaa2 switch via two interfaces. Ports get two different vlans > assigned. These two ports will race for a same dst mac entry > since multiple vlans share one FDB. > > FDB below may not show up at the same time. > 02:00:77:88:99:aa dev swp0 self > 02:00:77:88:99:aa dev swp1 self > But in fact, for rules on the bridge, they should be: > 02:00:77:88:99:aa dev swp0 vlan 10 master br0 > 02:00:77:88:99:aa dev swp1 vlan 20 master br0 > > This patch address this by borrowing unused form ports' FDB > when ports join bridge. And append offload flag to hardware > offloaded rules so we can tell them from those on bridges. Borrowing from the unused FDBs is fine as long as no switch port wants to leave the bridge. In case all available FDBs are used for the per-VLAN FDBs, any port that wants to leave the bridge will silently fail to reserve a private FDB for itself and will just continue to use the bridge's one. This will break behavior which previously worked and it's not something that we want. The following commands demonstrate this unwanted behavior: $ ls-addsw --flooding-cfg=DPSW_FLOODING_PER_FDB --broadcast-cfg=DPSW_BROADCAST_PER_FDB dpni.2 dpni.3 dpmac.3 dpmac.4 Created ETHSW object dpsw.0 with the following 4 ports: eth2,eth3,eth4,eth5 $ ip link add br0 type bridge vlan_filtering 1 $ ip link set dev eth2 master br0 [ 496.653013] fsl_dpaa2_switch dpsw.0 eth2: Joining a bridge, got FDB #1 $ ip link set dev eth3 master br0 [ 544.891083] fsl_dpaa2_switch dpsw.0 eth3: Joining a bridge, got FDB #1 $ ip link set dev eth4 master br0 [ 547.807707] fsl_dpaa2_switch dpsw.0 eth4: Joining a bridge, got FDB #1 $ ip link set dev eth5 master br0 [ 556.491085] fsl_dpaa2_switch dpsw.0 eth5: Joining a bridge, got FDB #1 $ bridge vlan add vid 10 dev eth2 [ 667.742585] br0: Using FDB#2 for VLAN 10 $ bridge vlan add vid 15 dev eth2 [ 672.296365] br0: Using FDB#3 for VLAN 15 $ bridge vlan add vid 30 dev eth3 [ 679.156295] br0: Using FDB#4 for VLAN 30 $ bridge vlan add vid 35 dev eth3 [ 682.220775] fsl_dpaa2_switch dpsw.0 eth3: dpsw_fdb_add err -6 RTNETLINK answers: No such device or address # At this point, there are no more unused FDBs that could be # used for VLAN 35 so the last command fails. # We now try to instruct eth5 to leave the bridge. As we can see # below, the driver will continue to use the bridge's FDB for # PVID instead of finding and using a private FDB. $ ip link set dev eth5 nomaster [ 841.427875] fsl_dpaa2_switch dpsw.0 eth5: Leaving a bridge, continue to use FDB #1 since we assume we are the last port to become standalone The debug prints were added on top of your patch and the diff is below: diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c index 217c68bb0faa..8d922a70e154 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c @@ -81,12 +81,17 @@ static u16 dpaa2_switch_port_set_fdb(struct ethsw_port_priv *port_priv, if (!fdb) { port_priv->fdb->bridge_dev = NULL; + + netdev_err(port_priv->netdev, "Leaving a bridge, continue to use FDB #%d since we assume we are the last port to become standalone\n", + port_priv->fdb->fdb_id); return 0; } port_priv->fdb = fdb; port_priv->fdb->in_use = true; port_priv->fdb->bridge_dev = NULL; + + netdev_err(port_priv->netdev, "Leaving a bridge, got FDB #%d\n", port_priv->fdb->fdb_id); return 0; } @@ -127,6 +132,8 @@ static u16 dpaa2_switch_port_set_fdb(struct ethsw_port_priv *port_priv, /* Keep track of the new upper bridge device */ port_priv->fdb->bridge_dev = bridge_dev; + netdev_err(port_priv->netdev, "Joining a bridge, got FDB #%d\n", port_priv->fdb->fdb_id); + return 0; } @@ -240,6 +247,8 @@ static int dpaa2_switch_add_vlan(struct ethsw_port_priv *port_priv, u16 vid) fdb->in_use = true; fdb->bridge_dev = NULL; vcfg.fdb_id = fdb->fdb_id; + + netdev_err(port_priv->fdb->bridge_dev, "Using FDB#%d for VLAN %d\n", fdb->fdb_id, vid); } else { /* Standalone, port's private fdb shared */ vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); @@ -444,8 +453,10 @@ static int dpaa2_switch_dellink(struct ethsw_core *ethsw, u16 vid) /* mark fdb as unsued for this vlan */ for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) { fdb = ethsw->fdbs; - if (fdb[i].vid == vid) + if (fdb[i].vid == vid) { fdb[i].in_use = false; + dev_err(ethsw->dev, "VLAN %d was removed, FDB #%d no longer in use\n", vid, fdb[i].fdb_id); + } } for (i = 0; i < ethsw->sw_attr.num_ifs; i++) { @@ -475,6 +486,8 @@ static int dpaa2_switch_port_fdb_add_uc(struct ethsw_port_priv *port_priv, if (err) netdev_err(port_priv->netdev, "dpsw_fdb_add_unicast err %d\n", err); + + netdev_err(port_priv->netdev, "Added unicast address in FDB #%d\n", fdb_id); return err; } What I would suggest as a possible way to avoid the issue above is to set aside at probe time the FDBs which are required for the usecase in which all the ports are standalone. If there are more FDBs than the number of switch ports, then independent VLAN learning is possible, if not the driver should just go ahead and revert to shared VLAN learning. In case VLAN learning is shared, the driver could print a warning and let the user know why is that and what could be done. > > Signed-off-by: Wan Junjie <junjie.wan@inceptio.ai> > --- > v2: fix coding style issues > --- > .../ethernet/freescale/dpaa2/dpaa2-switch.c | 216 +++++++++++++----- > .../ethernet/freescale/dpaa2/dpaa2-switch.h | 3 +- > 2 files changed, 167 insertions(+), 52 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > index a293b08f36d4..217c68bb0faa 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > @@ -25,8 +25,17 @@ > > #define DEFAULT_VLAN_ID 1 > > -static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv) > +static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv, u16 vid) > { > + struct ethsw_core *ethsw = port_priv->ethsw_data; > + int i; > + > + if (port_priv->fdb->bridge_dev) { > + for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) > + if (ethsw->fdbs[i].vid == vid) > + return ethsw->fdbs[i].fdb_id; > + } > + /* Default vlan, use port's fdb id directly */ > return port_priv->fdb->fdb_id; > } > > @@ -34,7 +43,7 @@ static struct dpaa2_switch_fdb *dpaa2_switch_fdb_get_unused(struct ethsw_core *e > { > int i; > > - for (i = 0; i < ethsw->sw_attr.num_ifs; i++) > + for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) > if (!ethsw->fdbs[i].in_use) > return ðsw->fdbs[i]; > return NULL; > @@ -125,17 +134,29 @@ static void dpaa2_switch_fdb_get_flood_cfg(struct ethsw_core *ethsw, u16 fdb_id, > enum dpsw_flood_type type, > struct dpsw_egress_flood_cfg *cfg) > { > - int i = 0, j; > + u16 vid = 4096; > + int i, j; > > memset(cfg, 0, sizeof(*cfg)); > > + for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) { > + if (ethsw->fdbs[i].fdb_id == fdb_id) { > + vid = ethsw->fdbs[i].vid; > + break; > + } > + } > + > + i = 0; > /* Add all the DPAA2 switch ports found in the same bridging domain to > * the egress flooding domain > */ > for (j = 0; j < ethsw->sw_attr.num_ifs; j++) { > if (!ethsw->ports[j]) > continue; > - if (ethsw->ports[j]->fdb->fdb_id != fdb_id) > + > + if (vid == 4096 && ethsw->ports[j]->fdb->fdb_id != fdb_id) > + continue; > + if (vid < 4096 && !(ethsw->ports[j]->vlans[vid] & ETHSW_VLAN_MEMBER)) > continue; > > if (type == DPSW_BROADCAST && ethsw->ports[j]->bcast_flood) > @@ -191,10 +212,38 @@ static void *dpaa2_iova_to_virt(struct iommu_domain *domain, > static int dpaa2_switch_add_vlan(struct ethsw_port_priv *port_priv, u16 vid) > { > struct ethsw_core *ethsw = port_priv->ethsw_data; > + struct net_device *netdev = port_priv->netdev; > + struct dpsw_fdb_cfg fdb_cfg = {0}; > struct dpsw_vlan_cfg vcfg = {0}; > + struct dpaa2_switch_fdb *fdb; > + u16 fdb_id; > int err; > > - vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); > + /* If ports are under a bridge, then > + * every VLAN domain should use a different fdb. > + * If ports are standalone, and > + * vid is 1 this should reuse the allocated port fdb. > + */ > + if (port_priv->fdb->bridge_dev) { > + fdb = dpaa2_switch_fdb_get_unused(ethsw); > + if (!fdb) { > + /* if not available, create a new fdb */ > + err = dpsw_fdb_add(ethsw->mc_io, 0, ethsw->dpsw_handle, > + &fdb_id, &fdb_cfg); > + if (err) { > + netdev_err(netdev, "dpsw_fdb_add err %d\n", err); > + return err; > + } > + fdb->fdb_id = fdb_id; This leads to a NULL pointer dereference. Variable 'fdb' is NULL in this case and you are trying to assign its fdb_id field. What I would suggest is to actually create all the possible FDBs at probe time, initialize them as unused and then just grab them when needed. When there are no more unused FDBs, no more VLANs can be created and an error is returned directly. > + } > + fdb->vid = vid; > + fdb->in_use = true; > + fdb->bridge_dev = NULL; > + vcfg.fdb_id = fdb->fdb_id; > + } else { > + /* Standalone, port's private fdb shared */ > + vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); > + } > err = dpsw_vlan_add(ethsw->mc_io, 0, > ethsw->dpsw_handle, vid, &vcfg); > if (err) { > @@ -298,7 +347,7 @@ static int dpaa2_switch_port_add_vlan(struct ethsw_port_priv *port_priv, > */ > vcfg.num_ifs = 1; > vcfg.if_id[0] = port_priv->idx; > - vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); > + vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); > vcfg.options |= DPSW_VLAN_ADD_IF_OPT_FDB_ID; > err = dpsw_vlan_add_if(ethsw->mc_io, 0, ethsw->dpsw_handle, vid, &vcfg); > if (err) { > @@ -326,7 +375,7 @@ static int dpaa2_switch_port_add_vlan(struct ethsw_port_priv *port_priv, > return err; > } > > - return 0; > + return dpaa2_switch_fdb_set_egress_flood(ethsw, vcfg.fdb_id); > } > > static enum dpsw_stp_state br_stp_state_to_dpsw(u8 state) > @@ -379,6 +428,7 @@ static int dpaa2_switch_port_set_stp_state(struct ethsw_port_priv *port_priv, u8 > static int dpaa2_switch_dellink(struct ethsw_core *ethsw, u16 vid) > { > struct ethsw_port_priv *ppriv_local = NULL; > + struct dpaa2_switch_fdb *fdb = NULL; > int i, err; > > if (!ethsw->vlans[vid]) > @@ -391,6 +441,13 @@ static int dpaa2_switch_dellink(struct ethsw_core *ethsw, u16 vid) > } > ethsw->vlans[vid] = 0; > > + /* mark fdb as unsued for this vlan */ s/unsued/unused/ > + for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) { > + fdb = ethsw->fdbs; > + if (fdb[i].vid == vid) > + fdb[i].in_use = false; > + } > + > for (i = 0; i < ethsw->sw_attr.num_ifs; i++) { > ppriv_local = ethsw->ports[i]; > if (ppriv_local) > @@ -401,7 +458,7 @@ static int dpaa2_switch_dellink(struct ethsw_core *ethsw, u16 vid) > } > > static int dpaa2_switch_port_fdb_add_uc(struct ethsw_port_priv *port_priv, > - const unsigned char *addr) > + const unsigned char *addr, u16 vid) > { > struct dpsw_fdb_unicast_cfg entry = {0}; > u16 fdb_id; > @@ -411,7 +468,7 @@ static int dpaa2_switch_port_fdb_add_uc(struct ethsw_port_priv *port_priv, > entry.type = DPSW_FDB_ENTRY_STATIC; > ether_addr_copy(entry.mac_addr, addr); > > - fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); > + fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); > err = dpsw_fdb_add_unicast(port_priv->ethsw_data->mc_io, 0, > port_priv->ethsw_data->dpsw_handle, > fdb_id, &entry); > @@ -422,7 +479,7 @@ static int dpaa2_switch_port_fdb_add_uc(struct ethsw_port_priv *port_priv, > } > > static int dpaa2_switch_port_fdb_del_uc(struct ethsw_port_priv *port_priv, > - const unsigned char *addr) > + const unsigned char *addr, u16 vid) > { > struct dpsw_fdb_unicast_cfg entry = {0}; > u16 fdb_id; > @@ -432,10 +489,11 @@ static int dpaa2_switch_port_fdb_del_uc(struct ethsw_port_priv *port_priv, > entry.type = DPSW_FDB_ENTRY_STATIC; > ether_addr_copy(entry.mac_addr, addr); > > - fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); > + fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); > err = dpsw_fdb_remove_unicast(port_priv->ethsw_data->mc_io, 0, > port_priv->ethsw_data->dpsw_handle, > fdb_id, &entry); > + > /* Silently discard error for calling multiple times the del command */ > if (err && err != -ENXIO) > netdev_err(port_priv->netdev, > @@ -444,7 +502,7 @@ static int dpaa2_switch_port_fdb_del_uc(struct ethsw_port_priv *port_priv, > } > > static int dpaa2_switch_port_fdb_add_mc(struct ethsw_port_priv *port_priv, > - const unsigned char *addr) > + const unsigned char *addr, u16 vid) > { > struct dpsw_fdb_multicast_cfg entry = {0}; > u16 fdb_id; > @@ -455,7 +513,7 @@ static int dpaa2_switch_port_fdb_add_mc(struct ethsw_port_priv *port_priv, > entry.num_ifs = 1; > entry.if_id[0] = port_priv->idx; > > - fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); > + fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); > err = dpsw_fdb_add_multicast(port_priv->ethsw_data->mc_io, 0, > port_priv->ethsw_data->dpsw_handle, > fdb_id, &entry); > @@ -467,7 +525,7 @@ static int dpaa2_switch_port_fdb_add_mc(struct ethsw_port_priv *port_priv, > } > > static int dpaa2_switch_port_fdb_del_mc(struct ethsw_port_priv *port_priv, > - const unsigned char *addr) > + const unsigned char *addr, u16 vid) > { > struct dpsw_fdb_multicast_cfg entry = {0}; > u16 fdb_id; > @@ -478,7 +536,7 @@ static int dpaa2_switch_port_fdb_del_mc(struct ethsw_port_priv *port_priv, > entry.num_ifs = 1; > entry.if_id[0] = port_priv->idx; > > - fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); > + fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); > err = dpsw_fdb_remove_multicast(port_priv->ethsw_data->mc_io, 0, > port_priv->ethsw_data->dpsw_handle, > fdb_id, &entry); > @@ -778,11 +836,12 @@ struct ethsw_dump_ctx { > }; > > static int dpaa2_switch_fdb_dump_nl(struct fdb_dump_entry *entry, > - struct ethsw_dump_ctx *dump) > + struct ethsw_dump_ctx *dump, u16 vid) > { > int is_dynamic = entry->type & DPSW_FDB_ENTRY_DINAMIC; > u32 portid = NETLINK_CB(dump->cb->skb).portid; > u32 seq = dump->cb->nlh->nlmsg_seq; > + struct ethsw_port_priv *port_priv; > struct nlmsghdr *nlh; > struct ndmsg *ndm; > > @@ -798,7 +857,7 @@ static int dpaa2_switch_fdb_dump_nl(struct fdb_dump_entry *entry, > ndm->ndm_family = AF_BRIDGE; > ndm->ndm_pad1 = 0; > ndm->ndm_pad2 = 0; > - ndm->ndm_flags = NTF_SELF; > + ndm->ndm_flags = NTF_SELF | NTF_OFFLOADED; Maybe the changes around the FDB dump could be in a different patch? Ioana
Hi Ioana, Thank you for your comments. > > Currently, dpaa2 switch only cares dst mac and egress interface > > in FDB. And all ports with different vlans share the same FDB. > > > > Indeed, the DPAA2 switch driver was written in such a way that learning > is shared between VLANs. The concern at that time was the limited amount > of resources which are allocated at DPSW creation time and which need to > be managed by the driver in the best way possible. > > That being said, I would suggest actually changing the title of the > commit to something like "dpaa2-switch: add support for per-VLAN > learning" so that it's clear that the driver did not support this prior. LGTM. > > This will make things messed up when one device connected to > > dpaa2 switch via two interfaces. Ports get two different vlans > > assigned. These two ports will race for a same dst mac entry > > since multiple vlans share one FDB. > > > > FDB below may not show up at the same time. > > 02:00:77:88:99:aa dev swp0 self > > 02:00:77:88:99:aa dev swp1 self > > But in fact, for rules on the bridge, they should be: > > 02:00:77:88:99:aa dev swp0 vlan 10 master br0 > > 02:00:77:88:99:aa dev swp1 vlan 20 master br0 > > > > This patch address this by borrowing unused form ports' FDB > > when ports join bridge. And append offload flag to hardware > > offloaded rules so we can tell them from those on bridges. > > Borrowing from the unused FDBs is fine as long as no switch port wants > to leave the bridge. In case all available FDBs are used for the > per-VLAN FDBs, any port that wants to leave the bridge will silently > fail to reserve a private FDB for itself and will just continue to use > the bridge's one. > > This will break behavior which previously worked and it's not something > that we want. > > The following commands demonstrate this unwanted behavior: > > $ ls-addsw --flooding-cfg=DPSW_FLOODING_PER_FDB --broadcast-cfg=DPSW_BROADCAST_PER_FDB dpni.2 dpni.3 dpmac.3 dpmac.4 > Created ETHSW object dpsw.0 with the following 4 ports: eth2,eth3,eth4,eth5 > > $ ip link add br0 type bridge vlan_filtering 1 > $ ip link set dev eth2 master br0 > [ 496.653013] fsl_dpaa2_switch dpsw.0 eth2: Joining a bridge, got FDB #1 > $ ip link set dev eth3 master br0 > [ 544.891083] fsl_dpaa2_switch dpsw.0 eth3: Joining a bridge, got FDB #1 > $ ip link set dev eth4 master br0 > [ 547.807707] fsl_dpaa2_switch dpsw.0 eth4: Joining a bridge, got FDB #1 > $ ip link set dev eth5 master br0 > [ 556.491085] fsl_dpaa2_switch dpsw.0 eth5: Joining a bridge, got FDB #1 > > $ bridge vlan add vid 10 dev eth2 > [ 667.742585] br0: Using FDB#2 for VLAN 10 > $ bridge vlan add vid 15 dev eth2 > [ 672.296365] br0: Using FDB#3 for VLAN 15 > $ bridge vlan add vid 30 dev eth3 > [ 679.156295] br0: Using FDB#4 for VLAN 30 > $ bridge vlan add vid 35 dev eth3 > [ 682.220775] fsl_dpaa2_switch dpsw.0 eth3: dpsw_fdb_add err -6 > RTNETLINK answers: No such device or address > > # At this point, there are no more unused FDBs that could be > # used for VLAN 35 so the last command fails. > > # We now try to instruct eth5 to leave the bridge. As we can see > # below, the driver will continue to use the bridge's FDB for > # PVID instead of finding and using a private FDB. > $ ip link set dev eth5 nomaster > [ 841.427875] fsl_dpaa2_switch dpsw.0 eth5: Leaving a bridge, continue to use FDB #1 since we assume we are the last port to become standalone > I want to check one thing first with you. For dpsw, a 'standalone' interface is not usable, right? It is just in a state 'standalone' and needs more configurations before we use it for forwarding. If this is the case, can we just assign all standalone interfaces to a single FDB. Or even no FDB before they join a bridge. And if there are no VLANs assigned to any port, bridge should create a shared FDB for them. So num of FDB is associated with VLAN rather than interfaces. I'm not sure about this, so I'm trying to make it work in the previous way. But as you see in your test, it doesn't when you make them leave the bridge. If it is, I suggest we follow the new way above. In this way, the max_fdbs could only count how many VLANs we are going to have. Of source plus 1 extra default FDB for bridge. Now in this patch I did not take care of VLAN 0. It will create a new FDB. But later, it could reuse default FDB as well. > > The debug prints were added on top of your patch and the diff is below: > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2- switch.c > index 217c68bb0faa..8d922a70e154 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > @@ -81,12 +81,17 @@ static u16 dpaa2_switch_port_set_fdb(struct ethsw_port_priv *port_priv, > > if (!fdb) { > port_priv->fdb->bridge_dev = NULL; > + > + netdev_err(port_priv->netdev, "Leaving a bridge, continue to use FDB #%d since we assume we are the last port to become standalone\n", > + port_priv->fdb->fdb_id); > return 0; > } > > port_priv->fdb = fdb; > port_priv->fdb->in_use = true; > port_priv->fdb->bridge_dev = NULL; > + > + netdev_err(port_priv->netdev, "Leaving a bridge, got FDB #%d\n", port_priv->fdb->fdb_id); > return 0; > } > > @@ -127,6 +132,8 @@ static u16 dpaa2_switch_port_set_fdb(struct ethsw_port_priv *port_priv, > /* Keep track of the new upper bridge device */ > port_priv->fdb->bridge_dev = bridge_dev; > > + netdev_err(port_priv->netdev, "Joining a bridge, got FDB #%d\n", port_priv->fdb->fdb_id); > + > return 0; > } > > @@ -240,6 +247,8 @@ static int dpaa2_switch_add_vlan(struct ethsw_port_priv *port_priv, u16 vid) > fdb->in_use = true; > fdb->bridge_dev = NULL; > vcfg.fdb_id = fdb->fdb_id; > + > + netdev_err(port_priv->fdb->bridge_dev, "Using FDB#%d for VLAN %d\n", fdb->fdb_id, vid); > } else { > /* Standalone, port's private fdb shared */ > vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); > @@ -444,8 +453,10 @@ static int dpaa2_switch_dellink(struct ethsw_core *ethsw, u16 vid) > /* mark fdb as unsued for this vlan */ > for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) { > fdb = ethsw->fdbs; > - if (fdb[i].vid == vid) > + if (fdb[i].vid == vid) { > fdb[i].in_use = false; > + dev_err(ethsw->dev, "VLAN %d was removed, FDB #%d no longer in use\n", vid, fdb[i].fdb_id); > + } > } > > for (i = 0; i < ethsw->sw_attr.num_ifs; i++) { > @@ -475,6 +486,8 @@ static int dpaa2_switch_port_fdb_add_uc(struct ethsw_port_priv *port_priv, > if (err) > netdev_err(port_priv->netdev, > "dpsw_fdb_add_unicast err %d\n", err); > + > + netdev_err(port_priv->netdev, "Added unicast address in FDB #%d\n", fdb_id); > return err; > } > > > What I would suggest as a possible way to avoid the issue above is to > set aside at probe time the FDBs which are required for the usecase in > which all the ports are standalone. If there are more FDBs than the > number of switch ports, then independent VLAN learning is possible, if > not the driver should just go ahead and revert to shared VLAN learning. > In case VLAN learning is shared, the driver could print a warning and > let the user know why is that and what could be done. > Please comment on my question and suggest above. > > > > Signed-off-by: Wan Junjie <junjie.wan@inceptio.ai> > > --- > > v2: fix coding style issues > > --- > > .../ethernet/freescale/dpaa2/dpaa2-switch.c | 216 +++++++++++++----- > > .../ethernet/freescale/dpaa2/dpaa2-switch.h | 3 +- > > 2 files changed, 167 insertions(+), 52 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > > index a293b08f36d4..217c68bb0faa 100644 > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > > @@ -25,8 +25,17 @@ > > > > #define DEFAULT_VLAN_ID 1 > > > > -static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv) > > +static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv, u16 vid) > > { > > + struct ethsw_core *ethsw = port_priv->ethsw_data; > > + int i; > > + > > + if (port_priv->fdb->bridge_dev) { > > + for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) > > + if (ethsw->fdbs[i].vid == vid) > > + return ethsw->fdbs[i].fdb_id; > > + } > > + /* Default vlan, use port's fdb id directly */ > > return port_priv->fdb->fdb_id; > > } > > > > @@ -34,7 +43,7 @@ static struct dpaa2_switch_fdb *dpaa2_switch_fdb_get_unused(struct ethsw_core *e > > { > > int i; > > > > - for (i = 0; i < ethsw->sw_attr.num_ifs; i++) > > + for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) > > if (!ethsw->fdbs[i].in_use) > > return ðsw->fdbs[i]; > > return NULL; > > @@ -125,17 +134,29 @@ static void dpaa2_switch_fdb_get_flood_cfg(struct ethsw_core *ethsw, u16 fdb_id, > > enum dpsw_flood_type type, > > struct dpsw_egress_flood_cfg *cfg) > > { > > - int i = 0, j; > > + u16 vid = 4096; > > + int i, j; > > > > memset(cfg, 0, sizeof(*cfg)); > > > > + for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) { > > + if (ethsw->fdbs[i].fdb_id == fdb_id) { > > + vid = ethsw->fdbs[i].vid; > > + break; > > + } > > + } > > + > > + i = 0; > > /* Add all the DPAA2 switch ports found in the same bridging domain to > > * the egress flooding domain > > */ > > for (j = 0; j < ethsw->sw_attr.num_ifs; j++) { > > if (!ethsw->ports[j]) > > continue; > > - if (ethsw->ports[j]->fdb->fdb_id != fdb_id) > > + > > + if (vid == 4096 && ethsw->ports[j]->fdb->fdb_id != fdb_id) > > + continue; > > + if (vid < 4096 && !(ethsw->ports[j]->vlans[vid] & ETHSW_VLAN_MEMBER)) > > continue; > > > > if (type == DPSW_BROADCAST && ethsw->ports[j]->bcast_flood) > > @@ -191,10 +212,38 @@ static void *dpaa2_iova_to_virt(struct iommu_domain *domain, > > static int dpaa2_switch_add_vlan(struct ethsw_port_priv *port_priv, u16 vid) > > { > > struct ethsw_core *ethsw = port_priv->ethsw_data; > > + struct net_device *netdev = port_priv->netdev; > > + struct dpsw_fdb_cfg fdb_cfg = {0}; > > struct dpsw_vlan_cfg vcfg = {0}; > > + struct dpaa2_switch_fdb *fdb; > > + u16 fdb_id; > > int err; > > > > - vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); > > + /* If ports are under a bridge, then > > + * every VLAN domain should use a different fdb. > > + * If ports are standalone, and > > + * vid is 1 this should reuse the allocated port fdb. > > + */ > > + if (port_priv->fdb->bridge_dev) { > > + fdb = dpaa2_switch_fdb_get_unused(ethsw); > > + if (!fdb) { > > + /* if not available, create a new fdb */ > > + err = dpsw_fdb_add(ethsw->mc_io, 0, ethsw->dpsw_handle, > > + &fdb_id, &fdb_cfg); > > + if (err) { > > + netdev_err(netdev, "dpsw_fdb_add err %d\n", err); > > + return err; > > + } > > + fdb->fdb_id = fdb_id; > > This leads to a NULL pointer dereference. > Variable 'fdb' is NULL in this case and you are > trying to assign its fdb_id field. > > What I would suggest is to actually create all > the possible FDBs at probe time, initialize them > as unused and then just grab them when needed. > When there are no more unused FDBs, no more > VLANs can be created and an error is returned > directly. Exactly, Simon has noticed this in a previous email. And I have proposed the same as you suggest here. > > + } > > + fdb->vid = vid; > > + fdb->in_use = true; > > + fdb->bridge_dev = NULL; > > + vcfg.fdb_id = fdb->fdb_id; > > + } else { > > + /* Standalone, port's private fdb shared */ > > + vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); > > + } > > err = dpsw_vlan_add(ethsw->mc_io, 0, > > ethsw->dpsw_handle, vid, &vcfg); > > if (err) { > > @@ -298,7 +347,7 @@ static int dpaa2_switch_port_add_vlan(struct ethsw_port_priv *port_priv, > > */ > > vcfg.num_ifs = 1; > > vcfg.if_id[0] = port_priv->idx; > > - vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); > > + vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); > > vcfg.options |= DPSW_VLAN_ADD_IF_OPT_FDB_ID; > > err = dpsw_vlan_add_if(ethsw->mc_io, 0, ethsw->dpsw_handle, vid, &vcfg); > > if (err) { > > @@ -326,7 +375,7 @@ static int dpaa2_switch_port_add_vlan(struct ethsw_port_priv *port_priv, > > return err; > > } > > > > - return 0; > > + return dpaa2_switch_fdb_set_egress_flood(ethsw, vcfg.fdb_id); > > } > > > > static enum dpsw_stp_state br_stp_state_to_dpsw(u8 state) > > @@ -379,6 +428,7 @@ static int dpaa2_switch_port_set_stp_state(struct ethsw_port_priv *port_priv, u8 > > static int dpaa2_switch_dellink(struct ethsw_core *ethsw, u16 vid) > > { > > struct ethsw_port_priv *ppriv_local = NULL; > > + struct dpaa2_switch_fdb *fdb = NULL; > > int i, err; > > > > if (!ethsw->vlans[vid]) > > @@ -391,6 +441,13 @@ static int dpaa2_switch_dellink(struct ethsw_core *ethsw, u16 vid) > > } > > ethsw->vlans[vid] = 0; > > > > + /* mark fdb as unsued for this vlan */ > > s/unsued/unused/ Oops > > + for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) { > > + fdb = ethsw->fdbs; > > + if (fdb[i].vid == vid) > > + fdb[i].in_use = false; > > + } > > + > > for (i = 0; i < ethsw->sw_attr.num_ifs; i++) { > > ppriv_local = ethsw->ports[i]; > > if (ppriv_local) > > @@ -401,7 +458,7 @@ static int dpaa2_switch_dellink(struct ethsw_core *ethsw, u16 vid) > > } > > > > static int dpaa2_switch_port_fdb_add_uc(struct ethsw_port_priv *port_priv, > > - const unsigned char *addr) > > + const unsigned char *addr, u16 vid) > > { > > struct dpsw_fdb_unicast_cfg entry = {0}; > > u16 fdb_id; > > @@ -411,7 +468,7 @@ static int dpaa2_switch_port_fdb_add_uc(struct ethsw_port_priv *port_priv, > > entry.type = DPSW_FDB_ENTRY_STATIC; > > ether_addr_copy(entry.mac_addr, addr); > > > > - fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); > > + fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); > > err = dpsw_fdb_add_unicast(port_priv->ethsw_data->mc_io, 0, > > port_priv->ethsw_data->dpsw_handle, > > fdb_id, &entry); > > @@ -422,7 +479,7 @@ static int dpaa2_switch_port_fdb_add_uc(struct ethsw_port_priv *port_priv, > > } > > > > static int dpaa2_switch_port_fdb_del_uc(struct ethsw_port_priv *port_priv, > > - const unsigned char *addr) > > + const unsigned char *addr, u16 vid) > > { > > struct dpsw_fdb_unicast_cfg entry = {0}; > > u16 fdb_id; > > @@ -432,10 +489,11 @@ static int dpaa2_switch_port_fdb_del_uc(struct ethsw_port_priv *port_priv, > > entry.type = DPSW_FDB_ENTRY_STATIC; > > ether_addr_copy(entry.mac_addr, addr); > > > > - fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); > > + fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); > > err = dpsw_fdb_remove_unicast(port_priv->ethsw_data->mc_io, 0, > > port_priv->ethsw_data->dpsw_handle, > > fdb_id, &entry); > > + > > /* Silently discard error for calling multiple times the del command */ > > if (err && err != -ENXIO) > > netdev_err(port_priv->netdev, > > @@ -444,7 +502,7 @@ static int dpaa2_switch_port_fdb_del_uc(struct ethsw_port_priv *port_priv, > > } > > > > static int dpaa2_switch_port_fdb_add_mc(struct ethsw_port_priv *port_priv, > > - const unsigned char *addr) > > + const unsigned char *addr, u16 vid) > > { > > struct dpsw_fdb_multicast_cfg entry = {0}; > > u16 fdb_id; > > @@ -455,7 +513,7 @@ static int dpaa2_switch_port_fdb_add_mc(struct ethsw_port_priv *port_priv, > > entry.num_ifs = 1; > > entry.if_id[0] = port_priv->idx; > > > > - fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); > > + fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); > > err = dpsw_fdb_add_multicast(port_priv->ethsw_data->mc_io, 0, > > port_priv->ethsw_data->dpsw_handle, > > fdb_id, &entry); > > @@ -467,7 +525,7 @@ static int dpaa2_switch_port_fdb_add_mc(struct ethsw_port_priv *port_priv, > > } > > > > static int dpaa2_switch_port_fdb_del_mc(struct ethsw_port_priv *port_priv, > > - const unsigned char *addr) > > + const unsigned char *addr, u16 vid) > > { > > struct dpsw_fdb_multicast_cfg entry = {0}; > > u16 fdb_id; > > @@ -478,7 +536,7 @@ static int dpaa2_switch_port_fdb_del_mc(struct ethsw_port_priv *port_priv, > > entry.num_ifs = 1; > > entry.if_id[0] = port_priv->idx; > > > > - fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); > > + fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); > > err = dpsw_fdb_remove_multicast(port_priv->ethsw_data->mc_io, 0, > > port_priv->ethsw_data->dpsw_handle, > > fdb_id, &entry); > > @@ -778,11 +836,12 @@ struct ethsw_dump_ctx { > > }; > > > > static int dpaa2_switch_fdb_dump_nl(struct fdb_dump_entry *entry, > > - struct ethsw_dump_ctx *dump) > > + struct ethsw_dump_ctx *dump, u16 vid) > > { > > int is_dynamic = entry->type & DPSW_FDB_ENTRY_DINAMIC; > > u32 portid = NETLINK_CB(dump->cb->skb).portid; > > u32 seq = dump->cb->nlh->nlmsg_seq; > > + struct ethsw_port_priv *port_priv; > > struct nlmsghdr *nlh; > > struct ndmsg *ndm; > > > > @@ -798,7 +857,7 @@ static int dpaa2_switch_fdb_dump_nl(struct fdb_dump_entry *entry, > > ndm->ndm_family = AF_BRIDGE; > > ndm->ndm_pad1 = 0; > > ndm->ndm_pad2 = 0; > > - ndm->ndm_flags = NTF_SELF; > > + ndm->ndm_flags = NTF_SELF | NTF_OFFLOADED; > > Maybe the changes around the FDB dump could be in a different patch? > > Ioana Sure, Let me split it. But I'm going to have a two-week vacation from tomorrow. During this period, I can't work on the patch. You will have to wait a bit long before I return back to the office. Or if you like, please send a new patch for this as you wish. Wan Junjie
On Thu, Sep 05, 2024 at 02:15:53PM +0800, junjie.wan@inceptio.ai wrote: > Hi Ioana, > (...) > > I want to check one thing first with you. For dpsw, a 'standalone' interface is not usable, right? > It is just in a state 'standalone' and needs more configurations before we use it for forwarding. > If this is the case, can we just assign all standalone interfaces to a single FDB. Or even no FDB > before they join a bridge. And if there are no VLANs assigned to any port, bridge should > create a shared FDB for them. So num of FDB is associated with VLAN rather than interfaces. > When a DPSW interface in not under a bridge it behaves just as a regular network interface, no forwarding. This is the default behavior for any switch interface in Linux which is not under a bridge, it's not just the dpaa2-switch driver which does it. We cannot use the same FDB for all switch interfaces which are 'standalone' since this would basically mean that they can do forwarding between them, which is exactly what we do not want. > > > > Maybe the changes around the FDB dump could be in a different patch? > > > > Ioana > > Sure, Let me split it. But I'm going to have a two-week vacation from tomorrow. > During this period, I can't work on the patch. You will have to wait a bit long before I return > back to the office. Or if you like, please send a new patch for this as you wish. > Not a problem. Have a great time in your vacation. We can continue when you get back. Also, please feel free to contact me directly so that we can sync and maybe test any potential patches before actually submitting them. Regards, Ioana
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c index a293b08f36d4..217c68bb0faa 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c @@ -25,8 +25,17 @@ #define DEFAULT_VLAN_ID 1 -static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv) +static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv, u16 vid) { + struct ethsw_core *ethsw = port_priv->ethsw_data; + int i; + + if (port_priv->fdb->bridge_dev) { + for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) + if (ethsw->fdbs[i].vid == vid) + return ethsw->fdbs[i].fdb_id; + } + /* Default vlan, use port's fdb id directly */ return port_priv->fdb->fdb_id; } @@ -34,7 +43,7 @@ static struct dpaa2_switch_fdb *dpaa2_switch_fdb_get_unused(struct ethsw_core *e { int i; - for (i = 0; i < ethsw->sw_attr.num_ifs; i++) + for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) if (!ethsw->fdbs[i].in_use) return ðsw->fdbs[i]; return NULL; @@ -125,17 +134,29 @@ static void dpaa2_switch_fdb_get_flood_cfg(struct ethsw_core *ethsw, u16 fdb_id, enum dpsw_flood_type type, struct dpsw_egress_flood_cfg *cfg) { - int i = 0, j; + u16 vid = 4096; + int i, j; memset(cfg, 0, sizeof(*cfg)); + for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) { + if (ethsw->fdbs[i].fdb_id == fdb_id) { + vid = ethsw->fdbs[i].vid; + break; + } + } + + i = 0; /* Add all the DPAA2 switch ports found in the same bridging domain to * the egress flooding domain */ for (j = 0; j < ethsw->sw_attr.num_ifs; j++) { if (!ethsw->ports[j]) continue; - if (ethsw->ports[j]->fdb->fdb_id != fdb_id) + + if (vid == 4096 && ethsw->ports[j]->fdb->fdb_id != fdb_id) + continue; + if (vid < 4096 && !(ethsw->ports[j]->vlans[vid] & ETHSW_VLAN_MEMBER)) continue; if (type == DPSW_BROADCAST && ethsw->ports[j]->bcast_flood) @@ -191,10 +212,38 @@ static void *dpaa2_iova_to_virt(struct iommu_domain *domain, static int dpaa2_switch_add_vlan(struct ethsw_port_priv *port_priv, u16 vid) { struct ethsw_core *ethsw = port_priv->ethsw_data; + struct net_device *netdev = port_priv->netdev; + struct dpsw_fdb_cfg fdb_cfg = {0}; struct dpsw_vlan_cfg vcfg = {0}; + struct dpaa2_switch_fdb *fdb; + u16 fdb_id; int err; - vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); + /* If ports are under a bridge, then + * every VLAN domain should use a different fdb. + * If ports are standalone, and + * vid is 1 this should reuse the allocated port fdb. + */ + if (port_priv->fdb->bridge_dev) { + fdb = dpaa2_switch_fdb_get_unused(ethsw); + if (!fdb) { + /* if not available, create a new fdb */ + err = dpsw_fdb_add(ethsw->mc_io, 0, ethsw->dpsw_handle, + &fdb_id, &fdb_cfg); + if (err) { + netdev_err(netdev, "dpsw_fdb_add err %d\n", err); + return err; + } + fdb->fdb_id = fdb_id; + } + fdb->vid = vid; + fdb->in_use = true; + fdb->bridge_dev = NULL; + vcfg.fdb_id = fdb->fdb_id; + } else { + /* Standalone, port's private fdb shared */ + vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); + } err = dpsw_vlan_add(ethsw->mc_io, 0, ethsw->dpsw_handle, vid, &vcfg); if (err) { @@ -298,7 +347,7 @@ static int dpaa2_switch_port_add_vlan(struct ethsw_port_priv *port_priv, */ vcfg.num_ifs = 1; vcfg.if_id[0] = port_priv->idx; - vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); + vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); vcfg.options |= DPSW_VLAN_ADD_IF_OPT_FDB_ID; err = dpsw_vlan_add_if(ethsw->mc_io, 0, ethsw->dpsw_handle, vid, &vcfg); if (err) { @@ -326,7 +375,7 @@ static int dpaa2_switch_port_add_vlan(struct ethsw_port_priv *port_priv, return err; } - return 0; + return dpaa2_switch_fdb_set_egress_flood(ethsw, vcfg.fdb_id); } static enum dpsw_stp_state br_stp_state_to_dpsw(u8 state) @@ -379,6 +428,7 @@ static int dpaa2_switch_port_set_stp_state(struct ethsw_port_priv *port_priv, u8 static int dpaa2_switch_dellink(struct ethsw_core *ethsw, u16 vid) { struct ethsw_port_priv *ppriv_local = NULL; + struct dpaa2_switch_fdb *fdb = NULL; int i, err; if (!ethsw->vlans[vid]) @@ -391,6 +441,13 @@ static int dpaa2_switch_dellink(struct ethsw_core *ethsw, u16 vid) } ethsw->vlans[vid] = 0; + /* mark fdb as unsued for this vlan */ + for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) { + fdb = ethsw->fdbs; + if (fdb[i].vid == vid) + fdb[i].in_use = false; + } + for (i = 0; i < ethsw->sw_attr.num_ifs; i++) { ppriv_local = ethsw->ports[i]; if (ppriv_local) @@ -401,7 +458,7 @@ static int dpaa2_switch_dellink(struct ethsw_core *ethsw, u16 vid) } static int dpaa2_switch_port_fdb_add_uc(struct ethsw_port_priv *port_priv, - const unsigned char *addr) + const unsigned char *addr, u16 vid) { struct dpsw_fdb_unicast_cfg entry = {0}; u16 fdb_id; @@ -411,7 +468,7 @@ static int dpaa2_switch_port_fdb_add_uc(struct ethsw_port_priv *port_priv, entry.type = DPSW_FDB_ENTRY_STATIC; ether_addr_copy(entry.mac_addr, addr); - fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); + fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); err = dpsw_fdb_add_unicast(port_priv->ethsw_data->mc_io, 0, port_priv->ethsw_data->dpsw_handle, fdb_id, &entry); @@ -422,7 +479,7 @@ static int dpaa2_switch_port_fdb_add_uc(struct ethsw_port_priv *port_priv, } static int dpaa2_switch_port_fdb_del_uc(struct ethsw_port_priv *port_priv, - const unsigned char *addr) + const unsigned char *addr, u16 vid) { struct dpsw_fdb_unicast_cfg entry = {0}; u16 fdb_id; @@ -432,10 +489,11 @@ static int dpaa2_switch_port_fdb_del_uc(struct ethsw_port_priv *port_priv, entry.type = DPSW_FDB_ENTRY_STATIC; ether_addr_copy(entry.mac_addr, addr); - fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); + fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); err = dpsw_fdb_remove_unicast(port_priv->ethsw_data->mc_io, 0, port_priv->ethsw_data->dpsw_handle, fdb_id, &entry); + /* Silently discard error for calling multiple times the del command */ if (err && err != -ENXIO) netdev_err(port_priv->netdev, @@ -444,7 +502,7 @@ static int dpaa2_switch_port_fdb_del_uc(struct ethsw_port_priv *port_priv, } static int dpaa2_switch_port_fdb_add_mc(struct ethsw_port_priv *port_priv, - const unsigned char *addr) + const unsigned char *addr, u16 vid) { struct dpsw_fdb_multicast_cfg entry = {0}; u16 fdb_id; @@ -455,7 +513,7 @@ static int dpaa2_switch_port_fdb_add_mc(struct ethsw_port_priv *port_priv, entry.num_ifs = 1; entry.if_id[0] = port_priv->idx; - fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); + fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); err = dpsw_fdb_add_multicast(port_priv->ethsw_data->mc_io, 0, port_priv->ethsw_data->dpsw_handle, fdb_id, &entry); @@ -467,7 +525,7 @@ static int dpaa2_switch_port_fdb_add_mc(struct ethsw_port_priv *port_priv, } static int dpaa2_switch_port_fdb_del_mc(struct ethsw_port_priv *port_priv, - const unsigned char *addr) + const unsigned char *addr, u16 vid) { struct dpsw_fdb_multicast_cfg entry = {0}; u16 fdb_id; @@ -478,7 +536,7 @@ static int dpaa2_switch_port_fdb_del_mc(struct ethsw_port_priv *port_priv, entry.num_ifs = 1; entry.if_id[0] = port_priv->idx; - fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); + fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); err = dpsw_fdb_remove_multicast(port_priv->ethsw_data->mc_io, 0, port_priv->ethsw_data->dpsw_handle, fdb_id, &entry); @@ -778,11 +836,12 @@ struct ethsw_dump_ctx { }; static int dpaa2_switch_fdb_dump_nl(struct fdb_dump_entry *entry, - struct ethsw_dump_ctx *dump) + struct ethsw_dump_ctx *dump, u16 vid) { int is_dynamic = entry->type & DPSW_FDB_ENTRY_DINAMIC; u32 portid = NETLINK_CB(dump->cb->skb).portid; u32 seq = dump->cb->nlh->nlmsg_seq; + struct ethsw_port_priv *port_priv; struct nlmsghdr *nlh; struct ndmsg *ndm; @@ -798,7 +857,7 @@ static int dpaa2_switch_fdb_dump_nl(struct fdb_dump_entry *entry, ndm->ndm_family = AF_BRIDGE; ndm->ndm_pad1 = 0; ndm->ndm_pad2 = 0; - ndm->ndm_flags = NTF_SELF; + ndm->ndm_flags = NTF_SELF | NTF_OFFLOADED; ndm->ndm_type = 0; ndm->ndm_ifindex = dump->dev->ifindex; ndm->ndm_state = is_dynamic ? NUD_REACHABLE : NUD_NOARP; @@ -806,6 +865,15 @@ static int dpaa2_switch_fdb_dump_nl(struct fdb_dump_entry *entry, if (nla_put(dump->skb, NDA_LLADDR, ETH_ALEN, entry->mac_addr)) goto nla_put_failure; + port_priv = netdev_priv(dump->dev); + if (port_priv->fdb && port_priv->fdb->bridge_dev) { + if (nla_put_u32(dump->skb, NDA_MASTER, port_priv->fdb->bridge_dev->ifindex)) + goto nla_put_failure; + } + + if (vid && nla_put(dump->skb, NDA_VLAN, sizeof(u16), &vid)) + goto nla_put_failure; + nlmsg_end(dump->skb, nlh); skip: @@ -845,6 +913,7 @@ static int dpaa2_switch_fdb_iterate(struct ethsw_port_priv *port_priv, int err = 0, i; u8 *dma_mem; u16 fdb_id; + u16 vid; fdb_dump_size = ethsw->sw_attr.max_fdb_entries * sizeof(fdb_entry); dma_mem = kzalloc(fdb_dump_size, GFP_KERNEL); @@ -859,23 +928,25 @@ static int dpaa2_switch_fdb_iterate(struct ethsw_port_priv *port_priv, goto err_map; } - fdb_id = dpaa2_switch_port_get_fdb_id(port_priv); - err = dpsw_fdb_dump(ethsw->mc_io, 0, ethsw->dpsw_handle, fdb_id, - fdb_dump_iova, fdb_dump_size, &num_fdb_entries); - if (err) { - netdev_err(net_dev, "dpsw_fdb_dump() = %d\n", err); - goto err_dump; - } - - dma_unmap_single(dev, fdb_dump_iova, fdb_dump_size, DMA_FROM_DEVICE); - - fdb_entries = (struct fdb_dump_entry *)dma_mem; - for (i = 0; i < num_fdb_entries; i++) { - fdb_entry = fdb_entries[i]; - - err = cb(port_priv, &fdb_entry, data); - if (err) - goto end; + for (vid = 0; vid <= VLAN_VID_MASK; vid++) { + if (port_priv->vlans[vid] & ETHSW_VLAN_MEMBER) { + fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); + err = dpsw_fdb_dump(ethsw->mc_io, 0, ethsw->dpsw_handle, fdb_id, + fdb_dump_iova, fdb_dump_size, &num_fdb_entries); + if (err) { + netdev_err(net_dev, "dpsw_fdb_dump() = %d\n", err); + goto err_dump; + } + dma_unmap_single(dev, fdb_dump_iova, fdb_dump_size, DMA_FROM_DEVICE); + fdb_entries = (struct fdb_dump_entry *)dma_mem; + for (i = 0; i < num_fdb_entries; i++) { + fdb_entry = fdb_entries[i]; + + err = cb(port_priv, &fdb_entry, vid, data); + if (err) + goto end; + } + } } end: @@ -891,13 +962,13 @@ static int dpaa2_switch_fdb_iterate(struct ethsw_port_priv *port_priv, } static int dpaa2_switch_fdb_entry_dump(struct ethsw_port_priv *port_priv, - struct fdb_dump_entry *fdb_entry, + struct fdb_dump_entry *fdb_entry, u16 vid, void *data) { if (!dpaa2_switch_port_fdb_valid_entry(fdb_entry, port_priv)) return 0; - return dpaa2_switch_fdb_dump_nl(fdb_entry, data); + return dpaa2_switch_fdb_dump_nl(fdb_entry, data, vid); } static int dpaa2_switch_port_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb, @@ -920,7 +991,7 @@ static int dpaa2_switch_port_fdb_dump(struct sk_buff *skb, struct netlink_callba } static int dpaa2_switch_fdb_entry_fast_age(struct ethsw_port_priv *port_priv, - struct fdb_dump_entry *fdb_entry, + struct fdb_dump_entry *fdb_entry, u16 vid, void *data __always_unused) { if (!dpaa2_switch_port_fdb_valid_entry(fdb_entry, port_priv)) @@ -930,17 +1001,22 @@ static int dpaa2_switch_fdb_entry_fast_age(struct ethsw_port_priv *port_priv, return 0; if (fdb_entry->type & DPSW_FDB_ENTRY_TYPE_UNICAST) - dpaa2_switch_port_fdb_del_uc(port_priv, fdb_entry->mac_addr); + dpaa2_switch_port_fdb_del_uc(port_priv, fdb_entry->mac_addr, vid); else - dpaa2_switch_port_fdb_del_mc(port_priv, fdb_entry->mac_addr); + dpaa2_switch_port_fdb_del_mc(port_priv, fdb_entry->mac_addr, vid); return 0; } static void dpaa2_switch_port_fast_age(struct ethsw_port_priv *port_priv) { - dpaa2_switch_fdb_iterate(port_priv, - dpaa2_switch_fdb_entry_fast_age, NULL); + u16 vid; + + for (vid = 0; vid <= VLAN_VID_MASK; vid++) { + if (port_priv->vlans[vid] & ETHSW_VLAN_MEMBER) + dpaa2_switch_fdb_iterate(port_priv, + dpaa2_switch_fdb_entry_fast_age, NULL); + } } static int dpaa2_switch_port_vlan_add(struct net_device *netdev, __be16 proto, @@ -1670,10 +1746,25 @@ static int dpaa2_switch_port_attr_stp_state_set(struct net_device *netdev, return err; } +static int dpaa2_switch_port_flood_vlan(struct net_device *vdev, int vid, void *arg) +{ + struct ethsw_port_priv *port_priv = netdev_priv(arg); + struct ethsw_core *ethsw = port_priv->ethsw_data; + u16 fdb_id; + + if (!vdev) + return -ENODEV; + + fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); + return dpaa2_switch_fdb_set_egress_flood(ethsw, fdb_id); +} + static int dpaa2_switch_port_flood(struct ethsw_port_priv *port_priv, struct switchdev_brport_flags flags) { struct ethsw_core *ethsw = port_priv->ethsw_data; + struct net_device *netdev = port_priv->netdev; + int err; if (flags.mask & BR_BCAST_FLOOD) port_priv->bcast_flood = !!(flags.val & BR_BCAST_FLOOD); @@ -1681,6 +1772,13 @@ static int dpaa2_switch_port_flood(struct ethsw_port_priv *port_priv, if (flags.mask & BR_FLOOD) port_priv->ucast_flood = !!(flags.val & BR_FLOOD); + /* Recreate the egress flood domain of every vlan domain */ + err = vlan_for_each(netdev, dpaa2_switch_port_flood_vlan, netdev); + if (err) { + netdev_err(netdev, "Unable to restore vlan flood err (%d)\n", err); + return err; + } + return dpaa2_switch_fdb_set_egress_flood(ethsw, port_priv->fdb->fdb_id); } @@ -1838,14 +1936,14 @@ static int dpaa2_switch_port_mdb_add(struct net_device *netdev, if (dpaa2_switch_port_lookup_address(netdev, 0, mdb->addr)) return -EEXIST; - err = dpaa2_switch_port_fdb_add_mc(port_priv, mdb->addr); + err = dpaa2_switch_port_fdb_add_mc(port_priv, mdb->addr, mdb->vid); if (err) return err; err = dev_mc_add(netdev, mdb->addr); if (err) { netdev_err(netdev, "dev_mc_add err %d\n", err); - dpaa2_switch_port_fdb_del_mc(port_priv, mdb->addr); + dpaa2_switch_port_fdb_del_mc(port_priv, mdb->addr, mdb->vid); } return err; @@ -1879,6 +1977,7 @@ static int dpaa2_switch_port_del_vlan(struct ethsw_port_priv *port_priv, u16 vid struct net_device *netdev = port_priv->netdev; struct dpsw_vlan_if_cfg vcfg; int i, err; + u16 fdb_id; if (!port_priv->vlans[vid]) return -ENOENT; @@ -1917,6 +2016,12 @@ static int dpaa2_switch_port_del_vlan(struct ethsw_port_priv *port_priv, u16 vid } port_priv->vlans[vid] &= ~ETHSW_VLAN_MEMBER; + /* VLAN's member changes, recreate the egress flood domain */ + fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid); + err = dpaa2_switch_fdb_set_egress_flood(ethsw, fdb_id); + if (err) + return err; + /* Delete VLAN from switch if it is no longer configured on * any port */ @@ -1956,7 +2061,7 @@ static int dpaa2_switch_port_mdb_del(struct net_device *netdev, if (!dpaa2_switch_port_lookup_address(netdev, 0, mdb->addr)) return -ENOENT; - err = dpaa2_switch_port_fdb_del_mc(port_priv, mdb->addr); + err = dpaa2_switch_port_fdb_del_mc(port_priv, mdb->addr, mdb->vid); if (err) return err; @@ -2010,7 +2115,7 @@ static int dpaa2_switch_port_bridge_join(struct net_device *netdev, int err; /* Delete the previously manually installed VLAN 1 */ - err = dpaa2_switch_port_del_vlan(port_priv, 1); + err = dpaa2_switch_port_del_vlan(port_priv, DEFAULT_VLAN_ID); if (err) return err; @@ -2027,6 +2132,7 @@ static int dpaa2_switch_port_bridge_join(struct net_device *netdev, goto err_egress_flood; /* Recreate the egress flood domain of the FDB that we just left. */ + /* Not really need, since standalone fdb will be used by others */ err = dpaa2_switch_fdb_set_egress_flood(ethsw, old_fdb->fdb_id); if (err) goto err_egress_flood; @@ -2109,7 +2215,10 @@ static int dpaa2_switch_port_bridge_leave(struct net_device *netdev) if (err) return err; - /* Recreate the egress flood domain of the FDB that we just left */ + /* Recreate the egress flood domain of the FDB that we just left, + * only recreate for the default one, as for vlan domain, already created + * in the restore process. + */ err = dpaa2_switch_fdb_set_egress_flood(ethsw, old_fdb->fdb_id); if (err) return err; @@ -2278,10 +2387,10 @@ static void dpaa2_switch_event_work(struct work_struct *work) break; if (is_unicast_ether_addr(fdb_info->addr)) err = dpaa2_switch_port_fdb_add_uc(netdev_priv(dev), - fdb_info->addr); + fdb_info->addr, fdb_info->vid); else err = dpaa2_switch_port_fdb_add_mc(netdev_priv(dev), - fdb_info->addr); + fdb_info->addr, fdb_info->vid); if (err) break; fdb_info->offloaded = true; @@ -2292,9 +2401,11 @@ static void dpaa2_switch_event_work(struct work_struct *work) if (!fdb_info->added_by_user || fdb_info->is_local) break; if (is_unicast_ether_addr(fdb_info->addr)) - dpaa2_switch_port_fdb_del_uc(netdev_priv(dev), fdb_info->addr); + dpaa2_switch_port_fdb_del_uc(netdev_priv(dev), fdb_info->addr, + fdb_info->vid); else - dpaa2_switch_port_fdb_del_mc(netdev_priv(dev), fdb_info->addr); + dpaa2_switch_port_fdb_del_mc(netdev_priv(dev), fdb_info->addr, + fdb_info->vid); break; } @@ -3183,6 +3294,8 @@ static int dpaa2_switch_port_init(struct ethsw_port_priv *port_priv, u16 port) fdb->fdb_id = fdb_id; fdb->in_use = true; fdb->bridge_dev = NULL; + /* VLAN id 4096 mark as default standalone */ + fdb->vid = 4096; port_priv->fdb = fdb; /* We need to add VLAN 1 as the PVID on this port until it is under a @@ -3194,6 +3307,7 @@ static int dpaa2_switch_port_init(struct ethsw_port_priv *port_priv, u16 port) return err; /* Setup the egress flooding domains (broadcast, unknown unicast */ + /* not under bridge yet, using port_priv fdb */ err = dpaa2_switch_fdb_set_egress_flood(ethsw, port_priv->fdb->fdb_id); if (err) return err; @@ -3395,7 +3509,7 @@ static int dpaa2_switch_probe(struct fsl_mc_device *sw_dev) goto err_teardown; } - ethsw->fdbs = kcalloc(ethsw->sw_attr.num_ifs, sizeof(*ethsw->fdbs), + ethsw->fdbs = kcalloc(ethsw->sw_attr.max_fdbs, sizeof(*ethsw->fdbs), GFP_KERNEL); if (!ethsw->fdbs) { err = -ENOMEM; diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.h index 42b3ca73f55d..026ba2cf0e35 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.h +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.h @@ -102,6 +102,7 @@ struct dpaa2_switch_fq { struct dpaa2_switch_fdb { struct net_device *bridge_dev; u16 fdb_id; + u16 vid; bool in_use; }; @@ -249,7 +250,7 @@ int dpaa2_switch_port_vlans_del(struct net_device *netdev, const struct switchdev_obj_port_vlan *vlan); typedef int dpaa2_switch_fdb_cb_t(struct ethsw_port_priv *port_priv, - struct fdb_dump_entry *fdb_entry, + struct fdb_dump_entry *fdb_entry, u16 vid, void *data); /* TC offload */
Currently, dpaa2 switch only cares dst mac and egress interface in FDB. And all ports with different vlans share the same FDB. This will make things messed up when one device connected to dpaa2 switch via two interfaces. Ports get two different vlans assigned. These two ports will race for a same dst mac entry since multiple vlans share one FDB. FDB below may not show up at the same time. 02:00:77:88:99:aa dev swp0 self 02:00:77:88:99:aa dev swp1 self But in fact, for rules on the bridge, they should be: 02:00:77:88:99:aa dev swp0 vlan 10 master br0 02:00:77:88:99:aa dev swp1 vlan 20 master br0 This patch address this by borrowing unused form ports' FDB when ports join bridge. And append offload flag to hardware offloaded rules so we can tell them from those on bridges. Signed-off-by: Wan Junjie <junjie.wan@inceptio.ai> --- v2: fix coding style issues --- .../ethernet/freescale/dpaa2/dpaa2-switch.c | 216 +++++++++++++----- .../ethernet/freescale/dpaa2/dpaa2-switch.h | 3 +- 2 files changed, 167 insertions(+), 52 deletions(-)