On Tue, Mar 14, 2023 at 05:36:51PM +0100, Clément Léger wrote:
> Add support for vlan operation (add, del, filtering) on the RZN1
> driver. The a5psw switch supports up to 32 VLAN IDs with filtering,
> tagged/untagged VLANs and PVID for each ports.
>
> Signed-off-by: Clément Léger <clement.leger@xxxxxxxxxxx>
> ---
> drivers/net/dsa/rzn1_a5psw.c | 164 +++++++++++++++++++++++++++++++++++
> drivers/net/dsa/rzn1_a5psw.h | 8 +-
> 2 files changed, 169 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
> index 5059b2814cdd..a9a42a8bc7e3 100644
> --- a/drivers/net/dsa/rzn1_a5psw.c
> +++ b/drivers/net/dsa/rzn1_a5psw.c
> @@ -583,6 +583,144 @@ static int a5psw_port_fdb_dump(struct dsa_switch *ds, int port,
> return ret;
> }
>
> +static int a5psw_port_vlan_filtering(struct dsa_switch *ds, int port,
> + bool vlan_filtering,
> + struct netlink_ext_ack *extack)
> +{
> + u32 mask = BIT(port + A5PSW_VLAN_VERI_SHIFT) |
> + BIT(port + A5PSW_VLAN_DISC_SHIFT);
I'm curious what the A5PSW_VLAN_VERI_SHIFT and A5PSW_VLAN_DISC_SHIFT
bits do. Also curious in general what does this hardware do w.r.t.
VLANs. There would be several things on the checklist:
- can it drop a VLAN which isn't present in the port membership list?
I guess this is what A5PSW_VLAN_DISC_SHIFT does.
- can it use VLAN information from the packet (with a fallback on the
port PVID) to determine where to send, and where *not* to send the
packet? How does this relate to the flooding registers? Is the flood
mask restricted by the VLAN mask? Is there a default VLAN installed in
the hardware tables, which is also the PVID of all ports, and all
ports are members of it? Could you implement standalone/bridged port
forwarding isolation based on VLANs, rather than the flimsy and most
likely buggy implementation done based on flooding domains, from this
patch set?
- is the FDB looked up per {MAC DA, VLAN ID} or just MAC DA? Looking at
a5psw_port_fdb_add(), there's absolutely no sign of "vid" being used,
so I guess it's Shared VLAN Learning. In that case, there's absolutely
no hope to implement ds->fdb_isolation for this hardware. But at the
*very* least, please disable address learning on standalone ports,
*and* implement ds->ops->port_fast_age() so that ports quickly forget
their learned MAC adddresses after leaving a bridge and become
standalone again.
- if the port PVID is indeed used to filter the flooding mask of
untagged packets, then I speculate that when A5PSW_VLAN_VERI_SHIFT
is set, the hardware searches for a VLAN tag in the packet, whereas if
it's unset, all packets will be forwarded according just to the port
PVID (A5PSW_SYSTEM_TAGINFO). That would be absolutely magnificent if
true, but it also means that you need to be *a lot* more careful when
programming this register. See the "Address databases" section from
Documentation/networking/dsa/dsa.rst for an explanation of the
asynchronous nature of .port_vlan_add() relative to .port_vlan_filtering().
Also see the call paths of sja1105_commit_pvid() and mv88e6xxx_port_commit_pvid()
for an example of how this should be managed correctly, and how the
bridge PVID should be committed to hardware only when the port is
currently VLAN-aware.
> + u32 val = vlan_filtering ? mask : 0;
> + struct a5psw *a5psw = ds->priv;
> +
> + a5psw_reg_rmw(a5psw, A5PSW_VLAN_VERIFY, mask, val);
> +
> + return 0;
> +}
> +
> +static int a5psw_port_vlan_del(struct dsa_switch *ds, int port,
> + const struct switchdev_obj_port_vlan *vlan)
> +{
> + struct a5psw *a5psw = ds->priv;
> + u16 vid = vlan->vid;
> + int vlan_res_id;
> +
> + dev_dbg(a5psw->dev, "Removing VLAN %d on port %d\n", vid, port);
> +
> + vlan_res_id = a5psw_find_vlan_entry(a5psw, vid);
> + if (vlan_res_id < 0)
> + return -EINVAL;
> +
> + a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, false);
> + a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port, false);
> +
> + /* Disable PVID if the vid is matching the port one */
What does it mean to disable PVID?
> + if (vid == a5psw_reg_readl(a5psw, A5PSW_SYSTEM_TAGINFO(port)))
> + a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port), 0);
> +
> + return 0;
> +}
> +
> static u64 a5psw_read_stat(struct a5psw *a5psw, u32 offset, int port)
> {
> u32 reg_lo, reg_hi;
> @@ -700,6 +838,27 @@ static void a5psw_get_eth_ctrl_stats(struct dsa_switch *ds, int port,
> ctrl_stats->MACControlFramesReceived = stat;
> }
>
> +static void a5psw_vlan_setup(struct a5psw *a5psw, int port)
> +{
> + u32 reg;
> +
> + /* Enable TAG always mode for the port, this is actually controlled
> + * by VLAN_IN_MODE_ENA field which will be used for PVID insertion
> + */
What does the "tag always" mode do, and what are the alternatives?
> + reg = A5PSW_VLAN_IN_MODE_TAG_ALWAYS;
> + reg <<= A5PSW_VLAN_IN_MODE_PORT_SHIFT(port);
> + a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE, A5PSW_VLAN_IN_MODE_PORT(port),
> + reg);
> +
> + /* Set transparent mode for output frame manipulation, this will depend
> + * on the VLAN_RES configuration mode
> + */
What does the "transparent" output mode do, and how does it compare to
the "dis", "strip" and "tag through" alternatives?
> + reg = A5PSW_VLAN_OUT_MODE_TRANSPARENT;
> + reg <<= A5PSW_VLAN_OUT_MODE_PORT_SHIFT(port);
> + a5psw_reg_rmw(a5psw, A5PSW_VLAN_OUT_MODE,
> + A5PSW_VLAN_OUT_MODE_PORT(port), reg);
> +}
Sorry for asking all these questions, but VLAN configuration on a switch
such as to bring it in line with the bridge driver expectations is a
rather tricky thing, so I'd like to have as clear of a mental model of
this hardware as possible, if public documentation isn't available.
