Re: [PATCH] dsa: vsc73xx: add support for vlan filtering

czw., 21 sty 2021 o 23:45 Vladimir Oltean [off-list ref] napisał(a):

Hi Pawel,

Hi Vladimir,
Thank You for Your answer.

On Wed, Jan 20, 2021 at 07:30:18AM +0100, Pawel Dembicki wrote:

quoted

This patch adds support for vlan filtering in vsc73xx driver.

After vlan filtering enable, CPU_PORT is configured as trunk, without
non-tagged frames. This allows to avoid problems with transmit untagged
frames because vsc73xx is DSA_TAG_PROTO_NONE.

Signed-off-by: Pawel Dembicki <redacted>

What are the issues that are preventing you from getting rid of
DSA_TAG_PROTO_NONE? Not saying that making the driver VLAN aware is a
bad idea, but maybe also adding a tagging driver should really be the
path going forward. If there are hardware issues surrounding the native
tagging support, then DSA can make use of your VLAN features by
transforming them into a software-defined tagger, see
net/dsa/tag_8021q.c. But using a trunk CPU port with 8021q uppers on top
of the DSA master is a poor job of achieving that.

I was planning to prepare tagging for the next step. Without VLAN
filtering and/or tagging it is usable only as a full bridge.
Vsc73xx devices support QinQ. I can use double tagging for port
separation, but then it's impossible to filter vlans.
So, I'm planning to start working with tagging based on
net/dsa/tag_8021q.c. Should I wait with this patch and send the
corrected version with tagging support?

quoted

---
+static int
+vsc73xx_port_read_vlan_table_entry(struct dsa_switch *ds, u16 vid, u8 *portmap)
+{
+     struct vsc73xx *vsc = ds->priv;
+     u32 val;
+     int ret;
+
+     if (vid > 4095)
+             return -EPERM;

This is a paranoid check and should be removed (not only here but
everywhere).

quoted

+static int vsc73xx_port_vlan_prepare(struct dsa_switch *ds, int port,
+                                  const struct switchdev_obj_port_vlan *vlan)
+{
+     /* nothing needed */
+     return 0;
+}

Can you please rebase your work on top of the net-next/master branch?
You will see that the API has changed.
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git

quoted

+
+static void vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
+                               const struct switchdev_obj_port_vlan *vlan)
+{
+     bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+     bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+     struct vsc73xx *vsc = ds->priv;
+     int ret;
+     u32 tmp;
+
+     if (!dsa_port_is_vlan_filtering(dsa_to_port(ds, port)))
+             return;

Sorry, but no. You need to support the case where the bridge (or 8021q
module) adds a VLAN even when the port is not enforcing VLAN filtering.
See commit:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=0ee2af4ebbe3c4364429859acd571018ebfb3424

quoted

+
+     ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid_begin,
+                                          vlan->vid_end, 1);
+     if (ret)
+             return;
+
+     if (untagged && port != CPU_PORT) {
+             /* VSC73xx can have only one untagged vid per port. */
+             vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port,
+                          VSC73XX_TXUPDCFG, &tmp);
+
+             if (tmp & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA)
+                     dev_warn(vsc->dev,
+                              "Chip support only one untagged VID per port. Overwriting...\n");

Just return an error, don't overwrite, this leaves the bridge VLAN
information out of sync with the hardware otherwise, which is not a
great idea.

But it's a void return function. It always will be out of sync after
the second untagged VID attemption.
Should I give warning without changing untagged vlan?

FWIW the drivers/net/dsa/ocelot/felix.c and drivers/net/mscc/ocelot.c
files support switching chips from the same vendor. The VSC73XX family
is much older, but some of the limitations apply to both architectures
nonetheless (like this one), you can surely borrow some ideas from
ocelot - in this case search for ocelot_vlan_prepare.

As Linus said, It's impossible because vsc73xx doesn't support tags
for external cpu via rgmii or regular port. This chip is very limited.

quoted

+
+             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+                                 VSC73XX_TXUPDCFG,
+                                 VSC73XX_TXUPDCFG_TX_UNTAGGED_VID,
+                                 (vlan->vid_end <<
+                                 VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) &
+                                 VSC73XX_TXUPDCFG_TX_UNTAGGED_VID);
+             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+                                 VSC73XX_TXUPDCFG,
+                                 VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA,
+                                 VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA);
+     }
+     if (pvid && port != CPU_PORT) {
+             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+                                 VSC73XX_CAT_DROP,
+                                 VSC73XX_CAT_DROP_UNTAGGED_ENA,
+                                 ~VSC73XX_CAT_DROP_UNTAGGED_ENA);
+             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+                                 VSC73XX_CAT_PORT_VLAN,
+                                 VSC73XX_CAT_PORT_VLAN_VLAN_VID,
+                                 vlan->vid_end &
+                                 VSC73XX_CAT_PORT_VLAN_VLAN_VID);
+     }
+}

Best Regards,
Pawel Dembicki