| Message ID | 20201204175416.1445937-1-vladimir.oltean@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net] net: mscc: ocelot: fix dropping of unknown IPv4 multicast on Seville | expand |
On 04/12/2020 19:54:16+0200, Vladimir Oltean wrote: > The current assumption is that the felix DSA driver has flooding knobs > per traffic class, while ocelot switchdev has a single flooding knob. > This was correct for felix VSC9959 and ocelot VSC7514, but with the > introduction of seville VSC9953, we see a switch driven by felix.c which > has a single flooding knob. > > So it is clear that we must do what should have been done from the > beginning, which is not to overwrite the configuration done by ocelot.c > in felix, but instead to teach the common ocelot library about the > differences in our switches, and set up the flooding PGIDs centrally. > > The effect that the bogus iteration through FELIX_NUM_TC has upon > seville is quite dramatic. ANA_FLOODING is located at 0x00b548, and > ANA_FLOODING_IPMC is located at 0x00b54c. So the bogus iteration will > actually overwrite ANA_FLOODING_IPMC when attempting to write > ANA_FLOODING[1]. There is no ANA_FLOODING[1] in sevile, just ANA_FLOODING. > > And when ANA_FLOODING_IPMC is overwritten with a bogus value, the effect > is that ANA_FLOODING_IPMC gets the value of 0x0003CF7D: > MC6_DATA = 61, > MC6_CTRL = 61, > MC4_DATA = 60, > MC4_CTRL = 0. > Because MC4_CTRL is zero, this means that IPv4 multicast control packets > are not flooded, but dropped. An invalid configuration, and this is how > the issue was actually spotted. > > Reported-by: Eldar Gasanov <eldargasanov2@gmail.com> > Reported-by: Maxim Kochetkov <fido_max@inbox.ru> > Tested-by: Eldar Gasanov <eldargasanov2@gmail.com> > Fixes: 84705fc16552 ("net: dsa: felix: introduce support for Seville VSC9953 switch") > Fixes: 3c7b51bd39b2 ("net: dsa: felix: allow flooding for all traffic classes") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > drivers/net/dsa/ocelot/felix.c | 7 ------- > drivers/net/dsa/ocelot/felix_vsc9959.c | 1 + > drivers/net/dsa/ocelot/seville_vsc9953.c | 1 + > drivers/net/ethernet/mscc/ocelot.c | 9 +++++---- > drivers/net/ethernet/mscc/ocelot_vsc7514.c | 1 + > include/soc/mscc/ocelot.h | 3 +++ > 6 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c > index ada75fa15861..7dc230677b78 100644 > --- a/drivers/net/dsa/ocelot/felix.c > +++ b/drivers/net/dsa/ocelot/felix.c > @@ -588,7 +588,6 @@ static int felix_setup(struct dsa_switch *ds) > struct ocelot *ocelot = ds->priv; > struct felix *felix = ocelot_to_felix(ocelot); > int port, err; > - int tc; > > err = felix_init_structs(felix, ds->num_ports); > if (err) > @@ -627,12 +626,6 @@ static int felix_setup(struct dsa_switch *ds) > ocelot_write_rix(ocelot, > ANA_PGID_PGID_PGID(GENMASK(ocelot->num_phys_ports, 0)), > ANA_PGID_PGID, PGID_UC); > - /* Setup the per-traffic class flooding PGIDs */ > - for (tc = 0; tc < FELIX_NUM_TC; tc++) > - ocelot_write_rix(ocelot, ANA_FLOODING_FLD_MULTICAST(PGID_MC) | > - ANA_FLOODING_FLD_BROADCAST(PGID_MC) | > - ANA_FLOODING_FLD_UNICAST(PGID_UC), > - ANA_FLOODING, tc); > > ds->mtu_enforcement_ingress = true; > ds->configure_vlan_while_not_filtering = true; > diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c > index 3e925b8d5306..2e5bbdca5ea4 100644 > --- a/drivers/net/dsa/ocelot/felix_vsc9959.c > +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c > @@ -1429,6 +1429,7 @@ static int felix_pci_probe(struct pci_dev *pdev, > pci_set_drvdata(pdev, felix); > ocelot = &felix->ocelot; > ocelot->dev = &pdev->dev; > + ocelot->num_flooding_pgids = FELIX_NUM_TC; > felix->info = &felix_info_vsc9959; > felix->switch_base = pci_resource_start(pdev, > felix->info->switch_pci_bar); > diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c > index 1d420c4a2f0f..ebbaf6817ec8 100644 > --- a/drivers/net/dsa/ocelot/seville_vsc9953.c > +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c > @@ -1210,6 +1210,7 @@ static int seville_probe(struct platform_device *pdev) > > ocelot = &felix->ocelot; > ocelot->dev = &pdev->dev; > + ocelot->num_flooding_pgids = 1; > felix->info = &seville_info_vsc9953; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c > index 2632fe2d2448..abea8dd2b0cb 100644 > --- a/drivers/net/ethernet/mscc/ocelot.c > +++ b/drivers/net/ethernet/mscc/ocelot.c > @@ -1551,10 +1551,11 @@ int ocelot_init(struct ocelot *ocelot) > SYS_FRM_AGING_MAX_AGE(307692), SYS_FRM_AGING); > > /* Setup flooding PGIDs */ > - ocelot_write_rix(ocelot, ANA_FLOODING_FLD_MULTICAST(PGID_MC) | > - ANA_FLOODING_FLD_BROADCAST(PGID_MC) | > - ANA_FLOODING_FLD_UNICAST(PGID_UC), > - ANA_FLOODING, 0); > + for (i = 0; i < ocelot->num_flooding_pgids; i++) > + ocelot_write_rix(ocelot, ANA_FLOODING_FLD_MULTICAST(PGID_MC) | > + ANA_FLOODING_FLD_BROADCAST(PGID_MC) | > + ANA_FLOODING_FLD_UNICAST(PGID_UC), > + ANA_FLOODING, i); > ocelot_write(ocelot, ANA_FLOODING_IPMC_FLD_MC6_DATA(PGID_MCIPV6) | > ANA_FLOODING_IPMC_FLD_MC6_CTRL(PGID_MC) | > ANA_FLOODING_IPMC_FLD_MC4_DATA(PGID_MCIPV4) | > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c > index dc00772950e5..1e7729421a82 100644 > --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c > +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c > @@ -1254,6 +1254,7 @@ static int mscc_ocelot_probe(struct platform_device *pdev) > } > > ocelot->num_phys_ports = of_get_child_count(ports); > + ocelot->num_flooding_pgids = 1; > > ocelot->vcap = vsc7514_vcap_props; > ocelot->inj_prefix = OCELOT_TAG_PREFIX_NONE; > diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h > index ea1de185f2e4..731116611390 100644 > --- a/include/soc/mscc/ocelot.h > +++ b/include/soc/mscc/ocelot.h > @@ -621,6 +621,9 @@ struct ocelot { > /* Keep track of the vlan port masks */ > u32 vlan_mask[VLAN_N_VID]; > > + /* Switches like VSC9959 have flooding per traffic class */ > + int num_flooding_pgids; > + > /* In tables like ANA:PORT and the ANA:PGID:PGID mask, > * the CPU is located after the physical ports (at the > * num_phys_ports index). > -- > 2.25.1 >
On Fri, 4 Dec 2020 19:13:29 +0100 Alexandre Belloni wrote: > On 04/12/2020 19:54:16+0200, Vladimir Oltean wrote: > > The current assumption is that the felix DSA driver has flooding knobs > > per traffic class, while ocelot switchdev has a single flooding knob. > > This was correct for felix VSC9959 and ocelot VSC7514, but with the > > introduction of seville VSC9953, we see a switch driven by felix.c which > > has a single flooding knob. > > > > So it is clear that we must do what should have been done from the > > beginning, which is not to overwrite the configuration done by ocelot.c > > in felix, but instead to teach the common ocelot library about the > > differences in our switches, and set up the flooding PGIDs centrally. > > > > The effect that the bogus iteration through FELIX_NUM_TC has upon > > seville is quite dramatic. ANA_FLOODING is located at 0x00b548, and > > ANA_FLOODING_IPMC is located at 0x00b54c. So the bogus iteration will > > actually overwrite ANA_FLOODING_IPMC when attempting to write > > ANA_FLOODING[1]. There is no ANA_FLOODING[1] in sevile, just ANA_FLOODING. > > > > And when ANA_FLOODING_IPMC is overwritten with a bogus value, the effect > > is that ANA_FLOODING_IPMC gets the value of 0x0003CF7D: > > MC6_DATA = 61, > > MC6_CTRL = 61, > > MC4_DATA = 60, > > MC4_CTRL = 0. > > Because MC4_CTRL is zero, this means that IPv4 multicast control packets > > are not flooded, but dropped. An invalid configuration, and this is how > > the issue was actually spotted. > > > > Reported-by: Eldar Gasanov <eldargasanov2@gmail.com> > > Reported-by: Maxim Kochetkov <fido_max@inbox.ru> > > Tested-by: Eldar Gasanov <eldargasanov2@gmail.com> > > Fixes: 84705fc16552 ("net: dsa: felix: introduce support for Seville VSC9953 switch") > > Fixes: 3c7b51bd39b2 ("net: dsa: felix: allow flooding for all traffic classes") > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> Applied, thanks!
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index ada75fa15861..7dc230677b78 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -588,7 +588,6 @@ static int felix_setup(struct dsa_switch *ds) struct ocelot *ocelot = ds->priv; struct felix *felix = ocelot_to_felix(ocelot); int port, err; - int tc; err = felix_init_structs(felix, ds->num_ports); if (err) @@ -627,12 +626,6 @@ static int felix_setup(struct dsa_switch *ds) ocelot_write_rix(ocelot, ANA_PGID_PGID_PGID(GENMASK(ocelot->num_phys_ports, 0)), ANA_PGID_PGID, PGID_UC); - /* Setup the per-traffic class flooding PGIDs */ - for (tc = 0; tc < FELIX_NUM_TC; tc++) - ocelot_write_rix(ocelot, ANA_FLOODING_FLD_MULTICAST(PGID_MC) | - ANA_FLOODING_FLD_BROADCAST(PGID_MC) | - ANA_FLOODING_FLD_UNICAST(PGID_UC), - ANA_FLOODING, tc); ds->mtu_enforcement_ingress = true; ds->configure_vlan_while_not_filtering = true; diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c index 3e925b8d5306..2e5bbdca5ea4 100644 --- a/drivers/net/dsa/ocelot/felix_vsc9959.c +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c @@ -1429,6 +1429,7 @@ static int felix_pci_probe(struct pci_dev *pdev, pci_set_drvdata(pdev, felix); ocelot = &felix->ocelot; ocelot->dev = &pdev->dev; + ocelot->num_flooding_pgids = FELIX_NUM_TC; felix->info = &felix_info_vsc9959; felix->switch_base = pci_resource_start(pdev, felix->info->switch_pci_bar); diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c index 1d420c4a2f0f..ebbaf6817ec8 100644 --- a/drivers/net/dsa/ocelot/seville_vsc9953.c +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c @@ -1210,6 +1210,7 @@ static int seville_probe(struct platform_device *pdev) ocelot = &felix->ocelot; ocelot->dev = &pdev->dev; + ocelot->num_flooding_pgids = 1; felix->info = &seville_info_vsc9953; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c index 2632fe2d2448..abea8dd2b0cb 100644 --- a/drivers/net/ethernet/mscc/ocelot.c +++ b/drivers/net/ethernet/mscc/ocelot.c @@ -1551,10 +1551,11 @@ int ocelot_init(struct ocelot *ocelot) SYS_FRM_AGING_MAX_AGE(307692), SYS_FRM_AGING); /* Setup flooding PGIDs */ - ocelot_write_rix(ocelot, ANA_FLOODING_FLD_MULTICAST(PGID_MC) | - ANA_FLOODING_FLD_BROADCAST(PGID_MC) | - ANA_FLOODING_FLD_UNICAST(PGID_UC), - ANA_FLOODING, 0); + for (i = 0; i < ocelot->num_flooding_pgids; i++) + ocelot_write_rix(ocelot, ANA_FLOODING_FLD_MULTICAST(PGID_MC) | + ANA_FLOODING_FLD_BROADCAST(PGID_MC) | + ANA_FLOODING_FLD_UNICAST(PGID_UC), + ANA_FLOODING, i); ocelot_write(ocelot, ANA_FLOODING_IPMC_FLD_MC6_DATA(PGID_MCIPV6) | ANA_FLOODING_IPMC_FLD_MC6_CTRL(PGID_MC) | ANA_FLOODING_IPMC_FLD_MC4_DATA(PGID_MCIPV4) | diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index dc00772950e5..1e7729421a82 100644 --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -1254,6 +1254,7 @@ static int mscc_ocelot_probe(struct platform_device *pdev) } ocelot->num_phys_ports = of_get_child_count(ports); + ocelot->num_flooding_pgids = 1; ocelot->vcap = vsc7514_vcap_props; ocelot->inj_prefix = OCELOT_TAG_PREFIX_NONE; diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h index ea1de185f2e4..731116611390 100644 --- a/include/soc/mscc/ocelot.h +++ b/include/soc/mscc/ocelot.h @@ -621,6 +621,9 @@ struct ocelot { /* Keep track of the vlan port masks */ u32 vlan_mask[VLAN_N_VID]; + /* Switches like VSC9959 have flooding per traffic class */ + int num_flooding_pgids; + /* In tables like ANA:PORT and the ANA:PGID:PGID mask, * the CPU is located after the physical ports (at the * num_phys_ports index).