Messages in this thread | | | From | Paweł Dembicki <> | Date | Thu, 29 Jun 2023 22:18:08 +0200 | Subject | Re: [PATCH net-next v2 6/7] net: dsa: vsc73xx: Add vlan filtering |
| |
niedz., 25 cze 2023 o 17:05 Vladimir Oltean <olteanv@gmail.com> napisał(a): > > On Sun, Jun 25, 2023 at 01:53:41PM +0200, Pawel Dembicki wrote: > > This patch implement vlan filtering for vsc73xx driver. > > > > After vlan filtering start, switch is reconfigured from QinQ to simple > > vlan aware mode. It's required, because VSC73XX chips haven't support > > for inner vlan tag filter. > > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> > > --- > > v2: > > - no changes done > > > > drivers/net/dsa/vitesse-vsc73xx-core.c | 101 +++++++++++++++++++++++++ > > 1 file changed, 101 insertions(+) > > > > diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c > > index 457eb7fddf4c..c946464489ab 100644 > > --- a/drivers/net/dsa/vitesse-vsc73xx-core.c > > +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c > > @@ -1226,6 +1226,30 @@ static int vsc73xx_port_set_double_vlan_aware(struct dsa_switch *ds, int port) > > return ret; > > } > > > > +static int > > +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port, > > + bool vlan_filtering, struct netlink_ext_ack *extack) > > +{ > > + int ret, i; > > + > > + if (vlan_filtering) { > > + vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_VLAN_AWARE); > > + } else { > > + if (port == CPU_PORT) > > + vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_CPU_AWARE); > > + else > > + vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_AWARE); > > + } > > Why do you need ports to be double VLAN aware when vlan_filtering=0? > Isn't VLAN_TCI_IGNORE_ENA sufficient to ignore the 802.1Q header from > incoming packets, and set up the PVIDs of user ports as egress-tagged on > the CPU port? >
Because I want to forward tagged and untagged frames when vlan_filtering is off. If I set VSC73XX_DOUBLE_VLAN_AWARE, I can put all (tagged and untagged) traffic into the outer vlan, called by the datasheet as "MAN space". In QinQ mode, it is possible to ignore what goes from a particular port but it is possible to separate traffic from different ports.
> > + > > + for (i = 0; i <= 3072; i++) { > > + ret = vsc73xx_port_update_vlan_table(ds, port, i, 0); > > + if (ret) > > + return ret; > > + } > > What is the purpose of this?
I want to be sure that the table is cleared when vlan awareness is changed.
> > > + > > + return ret; > > +} > > + > > static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid, > > bool port_vlan) > > { > > @@ -1304,6 +1328,80 @@ static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid, > > return 0; > > } > > > > +static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port, > > + const struct switchdev_obj_port_vlan *vlan, > > + struct netlink_ext_ack *extack) > > +{ > > + bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED; > > + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID; > > + int ret; > > + > > + /* Be sure to deny alterations to the configuration done by tag_8021q. > > + */ > > + if (vid_is_dsa_8021q(vlan->vid)) { > > + NL_SET_ERR_MSG_MOD(extack, > > + "Range 3072-4095 reserved for dsa_8021q operation"); > > + return -EBUSY; > > + } > > + > > + if (untagged && port != CPU_PORT) { > > + ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true); > > + if (ret) > > + return ret; > > + } > > + if (pvid && port != CPU_PORT) { > > Missing logic to change hardware PVID only while VLAN-aware, and to > restore the tag_8021q PVID when the bridge VLAN awareness gets disabled. > DSA does not resolve the conflicts on resources between .port_vlan_add() > and .tag_8021q_vlan_add(), the driver must do that. > > > + ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true); > > + if (ret) > > + return ret; > > + } > > + > > + ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 1); > > + > > + return ret; > > Style: return vsc73xx_port_update_vlan_table(...) > > > +} > > + > > +static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port, > > + const struct switchdev_obj_port_vlan *vlan) > > +{ > > + struct vsc73xx *vsc = ds->priv; > > + u16 vlan_no; > > + int ret; > > + u32 val; > > + > > + ret = > > + vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 0); > > Style: single line > > > + if (ret) > > + return ret; > > + > > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val); > > + > > + if (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA) { > > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_TXUPDCFG, &val); > > + vlan_no = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >> > > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT; > > + if (vlan_no == vlan->vid) { > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_TXUPDCFG, > > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, > > + 0); > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_TXUPDCFG, > > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0); > > + } > > + } > > + > > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val); > > + vlan_no = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID; > > + if (vlan_no && vlan_no == vlan->vid) { > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_CAT_PORT_VLAN, > > + VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0); > > As documented in Documentation/networking/switchdev.rst: > > When the bridge has VLAN filtering enabled and a PVID is not configured on the > ingress port, untagged and 802.1p tagged packets must be dropped. When the bridge > has VLAN filtering enabled and a PVID exists on the ingress port, untagged and > priority-tagged packets must be accepted and forwarded according to the > bridge's port membership of the PVID VLAN. When the bridge has VLAN filtering > disabled, the presence/lack of a PVID should not influence the packet > forwarding decision. > > Setting the hardware PVID to 0 when the bridge PVID is deleted sounds > like it accomplishes none of those. >
My bad. I should just set VSC73XX_CAT_DROP_UNTAGGED_ENA here.
> > + } > > + > > + return 0; > > +} > > + > > static void vsc73xx_update_forwarding_map(struct vsc73xx *vsc) > > { > > int i; > > @@ -1524,6 +1622,9 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = { > > .port_change_mtu = vsc73xx_change_mtu, > > .port_max_mtu = vsc73xx_get_max_mtu, > > .port_stp_state_set = vsc73xx_port_stp_state_set, > > + .port_vlan_filtering = vsc73xx_port_vlan_filtering, > > + .port_vlan_add = vsc73xx_port_vlan_add, > > + .port_vlan_del = vsc73xx_port_vlan_del, > > .tag_8021q_vlan_add = vsc73xx_tag_8021q_vlan_add, > > .tag_8021q_vlan_del = vsc73xx_tag_8021q_vlan_del, > > }; > > -- > > 2.34.1 > > >
Thank you for such detailed responses and clarifying for me many issues.
-- Pawel Dembicki
| |