Message-ID: <20230315155430.5873cdb6@fixe.home>
Date:   Wed, 15 Mar 2023 15:54:30 +0100
From:   Clément Léger <clement.leger@...tlin.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Herve Codina <herve.codina@...tlin.com>,
        Miquèl Raynal <miquel.raynal@...tlin.com>,
        Milan Stevanovic <milan.stevanovic@...com>,
        Jimmy Lalande <jimmy.lalande@...com>,
        Pascal Eberhard <pascal.eberhard@...com>,
        Arun Ramadoss <Arun.Ramadoss@...rochip.com>,
        linux-renesas-soc@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND net-next v4 3/3] net: dsa: rzn1-a5psw: add vlan
 support

Le Wed, 15 Mar 2023 01:34:54 +0200,
Vladimir Oltean <olteanv@...il.com> a écrit :

> 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@...tlin.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.

Yes, A5PSW_VLAN_DISC_SHIFT stands for "discard" which means the packet
is discarded if the port is not a member of the VLAN.
A5PSW_VLAN_VERI_SHIFT is meant to enable VLAN lookup for packet
flooding (instead of the default lookup).

> 
> - 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?

Yes, the VLAN membership is used for packet flooding. The flooding
registers are used when the packets come has a src MAC that is not in
the FDB. For more infiormation, see section 4.5.3.9, paragraph 3.c
which describe the whole lookup process.

Regarding your other question, by default, there is no default VLAN
installed but indeed, I see what you mean, a default VLAN could be used
to isolate each ports rather than setting the rule to forward only to
root CPU port + disabling of flooding. I guess a unique VLAN ID per port
should be used to isolate each of them and added to the root port to
untag the input frames tagged with the PVID ?

> 
> - 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.

Indeed, the lookup table does not contain the VLAN ID and thus it is
unused. We talked about it in a previous review and you already
mentionned that there is no hope to implement fdb_isolation. Ok for
disabling learning on standalone ports, and indeed, by default, it's
enabled. Regarding ds->ops->port_fast_age(), it is already implemented.

> 
> - 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.

The port PVID itself is not used to filter the flooding mask. But each
time a PVID is set, the port must also be programmed as a membership of
the PVID VLAN ID in the VLAN resolution table. So actually, the PVID is
just here to tag (or not) the input packet, it does not take a role in
packet forwading. This is entirely done by the VLAN resolution table
content (VLAN_RES_TABLE register). Does this means I don't have to be
extra careful when programming it ?

> 
> > +	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?

It means it disable the input tagging of packets with this PVID.
Incoming packets will not be modified and passed as-is.

> 
> > +	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?

The name of the mode is probably missleading. When setting VLAN_IN_MODE
with A5PSW_VLAN_IN_MODE_TAG_ALWAYS, the input packet will be tagged
_only_ if VLAN_IN_MODE_ENA port bit is set. If this bit is not set for
the port, packet will passthrough transparently. This bit is actually
enabled in a5psw_port_vlan_add() when a PVID is set and unset when the
PVID is removed. Maybe the comment above these lines was not clear
enough.

There are actually 3 modes (excerpt of the documentation):

0) Single Tagging with Passthrough/VID Overwrite:
Insert tag if untagged frame. Leave frame unmodified if tagged and VID
> 0. If tagged with VID = 0 (priority tagged), then the VID will be
overwritten with the VID from SYSTEM_TAGINFO and priority is kept.

1) Single Tagging with Replace:
If untagged, add the tag, if single tagged, overwrite the tag.

2) Tag always:
Insert a tag always. This results in a single tagged frame when an
untagged is received, and a double tagged frame, when a single tagged
frame is received (or triple tagged if double-tagged received etc.).

This mode is then enforced (or not) using VLAN_IN_MODE. Input
manipulation can be enabled per port with register VLAN_IN_MODE_ENA and
its port individual mode is configured in register VLAN_IN_MODE.
Moreover, the tag that will be inserted is stored in the
SYSTEM_TAGINFO[port] register.
> 
> > +	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?

Here is a description of the 4 modes (excerpt of the documentation):

0) Disabled:
No frame manipulation occurs, frame is output as-is.

1) Strip mode:
All the tags (single or double) are removed from frame before sending
it.

2) Tag through mode:
Always removes first tag from frame only. In Tag Through mode, the
inner Tag is passed through while the outer Tag is removed for a double
Tagged frame. The following rules apply:
  ● When a single tagged frame is received, strip the tag from the
   frame.
  ● When a double tagged frame is received, strip the outer tag from the
    frame

3) VLAN domain mode / transparent mode:
The first tag of a frame is removed (same as Mode 2) when the VLAN is
defined as untagged for the port. The following rules apply:
  ● If frame’s VLAN id is found in the VLAN table (see Section
    4.5.3.9(3)(b), VLAN Domain Resolution / VLAN Table) and the port is
    defined (in the table) as tagged for the VLAN, the frame is not
    modified.
  ● If frame’s VLAN id is found in the VLAN table and the port is
    defined as untagged for the VLAN, the first VLAN tag is removed from
    the frame.
  ● If frame’s VLAN id is not found in the VLAN table, the frame is not
    modified.

This last mode allows for a fine grain control oveer tagged/untagged
VLAN since each VLAN setup is in the VLAN table.

> 
> > +	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.

No worries, that's your "job" to make sure drivers are in line with
what is expected in DSA. The documentation is public and available at
[1]. Section 4.5.3 is of interest for your understanding of the VLAN
filtering support. Let's hope I answered most of your questions.


[1]
https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-r-engine-and-ethernet-peripherals?r=1054561

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Powered by blists - more mailing lists