Re: [PATCH net-next v2 6/7] net: dsa: vsc73xx: Add vlan filtering
From: Paweł Dembicki
Date: Thu Jun 29 2023 - 16:18:30 EST
niedz., 25 cze 2023 o 17:05 Vladimir Oltean <olteanv@xxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Pawel Dembicki <paweldembicki@xxxxxxxxx>
> > ---
> > 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