| Message ID | 20230625115343.1603330-6-paweldembicki@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2,1/7] net: dsa: vsc73xx: use read_poll_timeout instead delay loop | expand |
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? > + > + 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? > + > + 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. > + } > + > + 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 >
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
On Thu, Jun 29, 2023 at 10:18:08PM +0200, Paweł Dembicki wrote: > niedz., 25 cze 2023 o 17:05 Vladimir Oltean <olteanv@gmail.com> napisał(a): > > 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. I think we may have some problem in finding common terminology. Opening the manual and seeing the table "Customer Port Sample Configuration", I think it's indeed what you need. But I wouldn't call it "double VLAN aware". The port is actually configured to be VLAN *unaware* from the perspective of classification, and always encapsulate all packets in one more VLAN (the port PVID). This switch's analyzer is always aware only of the outer VLAN header, and that's not "double VLAN aware" (it can perform no action based on the inner VLAN, if that exists), but it's fine for what is needed of it. You might be mixing these with MAC_CFG::VLAN_AWR and MAC_CFG::VLAN_DBLAWR, which essentially are only there to allow single- and double-VLAN-tagged frames to be longer by 4 and 8 bytes, respectively, than the max frame size. I don't think that these 2 fields have any reason to depend upon the bridge VLAN awareness state of the port. They can be unconditionally enabled. After all, Linux only cares about MTU, and that is the size of the L2 payload, excluding any VLAN headers, if present. I would suggest that if you exclude the MAC_CFG registers from vsc73xx_port_set_vlan_conf(), you end up with not as many VLAN awareness modes as you think. 2, to be precise: on or off. So you don't need the enum. Also, AFAIU, I don't see a reason to modify CAT_VLAN_MISC::VLAN_KEEP_TAG_ENA from the value of 1 at all. You could always keep frames in the queue system with the VID attached, and strip that VID on egress, if necessary, via TXUPDCFG. Not sure if you're noticed this, but drivers/net/ethernet/mscc/ and drivers/net/dsa/ocelot/ contain a driver for a newer generation of hardware than the VSC73xx, but many of the concepts apply. Maybe you can take a look at how some things were done there. > > > + > > > + 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. Yes, but why? That should specifically not be done, since there is no code in the kernel to replay the port_vlan_add() and tag_8021q_vlan_add() calls for you when the VLAN awareness state changes. If you delete the VLANs, they're gone, even though in software they're still there.
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); + } + + for (i = 0; i <= 3072; i++) { + ret = vsc73xx_port_update_vlan_table(ds, port, i, 0); + if (ret) + return ret; + } + + 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) { + 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; +} + +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); + 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); + } + + 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, };