From: Vladimir Oltean <olteanv@gmail.com>
To: "Clément Léger" <clement.leger@bootlin.com>
Cc: "Andrew Lunn" <andrew@lunn.ch>,
"Florian Fainelli" <f.fainelli@gmail.com>,
"David S. Miller" <davem@davemloft.net>,
"Eric Dumazet" <edumazet@google.com>,
"Jakub Kicinski" <kuba@kernel.org>,
"Paolo Abeni" <pabeni@redhat.com>,
"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
"Herve Codina" <herve.codina@bootlin.com>,
"Miquèl Raynal" <miquel.raynal@bootlin.com>,
"Milan Stevanovic" <milan.stevanovic@se.com>,
"Jimmy Lalande" <jimmy.lalande@se.com>,
"Pascal Eberhard" <pascal.eberhard@se.com>,
"Arun Ramadoss" <Arun.Ramadoss@microchip.com>,
linux-renesas-soc@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH RESEND net-next v4 3/3] net: dsa: rzn1-a5psw: add vlan support
Date: Wed, 15 Mar 2023 01:34:54 +0200 [thread overview]
Message-ID: <20230314233454.3zcpzhobif475hl2@skbuf> (raw)
In-Reply-To: <20230314163651.242259-4-clement.leger@bootlin.com> <20230314163651.242259-4-clement.leger@bootlin.com>
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@bootlin.com>
> ---
> 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.
next prev parent reply other threads:[~2023-03-14 23:35 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-14 16:36 [PATCH RESEND net-next v4 0/3] net: dsa: rzn1-a5psw: add support for vlan and .port_bridge_flags Clément Léger
2023-03-14 16:36 ` [PATCH RESEND net-next v4 1/3] net: dsa: rzn1-a5psw: use a5psw_reg_rmw() to modify flooding resolution Clément Léger
2023-03-14 22:54 ` Vladimir Oltean
2023-03-14 16:36 ` [PATCH RESEND net-next v4 2/3] net: dsa: rzn1-a5psw: add support for .port_bridge_flags Clément Léger
2023-03-14 22:56 ` Vladimir Oltean
2023-03-14 23:08 ` Vladimir Oltean
2023-03-16 11:53 ` Clément Léger
2023-03-24 22:10 ` Vladimir Oltean
2023-03-14 16:36 ` [PATCH RESEND net-next v4 3/3] net: dsa: rzn1-a5psw: add vlan support Clément Léger
2023-03-14 23:34 ` Vladimir Oltean [this message]
2023-03-15 14:54 ` Clément Léger
2023-03-24 22:00 ` Vladimir Oltean
2023-03-28 8:44 ` Clément Léger
2023-03-29 13:16 ` Vladimir Oltean
2023-03-30 9:09 ` Clément Léger
2023-03-30 14:40 ` Vladimir Oltean
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230314233454.3zcpzhobif475hl2@skbuf \
--to=olteanv@gmail.com \
--cc=Arun.Ramadoss@microchip.com \
--cc=andrew@lunn.ch \
--cc=clement.leger@bootlin.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=f.fainelli@gmail.com \
--cc=herve.codina@bootlin.com \
--cc=jimmy.lalande@se.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=milan.stevanovic@se.com \
--cc=miquel.raynal@bootlin.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=pascal.eberhard@se.com \
--cc=thomas.petazzoni@bootlin.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).