netdev - Re: [net-next PATCH v13 13/14] net: dsa: Add Airoha AN8855 5-Port Gigabit DSA Switch driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <67f4e8bd.df0a0220.369157.fb3d@mx.google.com>
Date: Tue, 8 Apr 2025 11:13:29 +0200
From: Christian Marangi <ansuelsmth@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	upstream@...oha.com
Subject: Re: [net-next PATCH v13 13/14] net: dsa: Add Airoha AN8855 5-Port
 Gigabit DSA Switch driver

On Tue, Mar 18, 2025 at 05:15:40PM +0200, Vladimir Oltean wrote:
> On Sat, Mar 15, 2025 at 04:43:53PM +0100, Christian Marangi wrote:
> > +/* Similar to MT7530 also trap link local frame and special frame to CPU */
> > +static int an8855_trap_special_frames(struct an8855_priv *priv)
> > +{
> > +	int ret;
> > +
> > +	/* Trap BPDUs to the CPU port(s) and egress them
> > +	 * VLAN-untagged.
> > +	 */
> > +	ret = regmap_update_bits(priv->regmap, AN8855_BPC,
> > +				 AN8855_BPDU_BPDU_FR | AN8855_BPDU_EG_TAG |
> > +				 AN8855_BPDU_PORT_FW,
> > +				 AN8855_BPDU_BPDU_FR |
> > +				 FIELD_PREP(AN8855_BPDU_EG_TAG, AN8855_VLAN_EG_UNTAGGED) |
> > +				 FIELD_PREP(AN8855_BPDU_PORT_FW, AN8855_BPDU_CPU_ONLY));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Trap 802.1X PAE frames to the CPU port(s) and egress them
> > +	 * VLAN-untagged.
> > +	 */
> > +	ret = regmap_update_bits(priv->regmap, AN8855_PAC,
> > +				 AN8855_PAE_BPDU_FR | AN8855_PAE_EG_TAG |
> > +				 AN8855_PAE_PORT_FW,
> > +				 AN8855_PAE_BPDU_FR |
> > +				 FIELD_PREP(AN8855_PAE_EG_TAG, AN8855_VLAN_EG_UNTAGGED) |
> > +				 FIELD_PREP(AN8855_PAE_PORT_FW, AN8855_BPDU_CPU_ONLY));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Trap frames with :01 MAC DAs to the CPU port(s) and egress
> > +	 * them VLAN-untagged.
> > +	 */
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RGAC1,
> > +				 AN8855_R01_BPDU_FR | AN8855_R01_EG_TAG |
> > +				 AN8855_R01_PORT_FW,
> > +				 AN8855_R01_BPDU_FR |
> > +				 FIELD_PREP(AN8855_R01_EG_TAG, AN8855_VLAN_EG_UNTAGGED) |
> > +				 FIELD_PREP(AN8855_R01_PORT_FW, AN8855_BPDU_CPU_ONLY));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Trap frames with :02 MAC DAs to the CPU port(s) and egress
> > +	 * them VLAN-untagged.
> > +	 */
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RGAC1,
> > +				 AN8855_R02_BPDU_FR | AN8855_R02_EG_TAG |
> > +				 AN8855_R02_PORT_FW,
> > +				 AN8855_R02_BPDU_FR |
> > +				 FIELD_PREP(AN8855_R02_EG_TAG, AN8855_VLAN_EG_UNTAGGED) |
> > +				 FIELD_PREP(AN8855_R02_PORT_FW, AN8855_BPDU_CPU_ONLY));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Trap frames with :03 MAC DAs to the CPU port(s) and egress
> > +	 * them VLAN-untagged.
> > +	 */
> > +	ret = regmap_update_bits(priv->regmap, AN8855_RGAC1,
> > +				 AN8855_R03_BPDU_FR | AN8855_R03_EG_TAG |
> > +				 AN8855_R03_PORT_FW,
> > +				 AN8855_R03_BPDU_FR |
> > +				 FIELD_PREP(AN8855_R03_EG_TAG, AN8855_VLAN_EG_UNTAGGED) |
> > +				 FIELD_PREP(AN8855_R03_PORT_FW, AN8855_BPDU_CPU_ONLY));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Trap frames with :0E MAC DAs to the CPU port(s) and egress
> > +	 * them VLAN-untagged.
> > +	 */
> > +	return regmap_update_bits(priv->regmap, AN8855_RGAC1,
> > +				  AN8855_R0E_BPDU_FR | AN8855_R0E_EG_TAG |
> > +				  AN8855_R0E_PORT_FW,
> > +				  AN8855_R0E_BPDU_FR |
> > +				  FIELD_PREP(AN8855_R0E_EG_TAG, AN8855_VLAN_EG_UNTAGGED) |
> > +				  FIELD_PREP(AN8855_R0E_PORT_FW, AN8855_BPDU_CPU_ONLY));
> > +}
> 
> Is there a way in which you could group the registers a bit more?
> The function occupies 2 screens :-/
>

I will use local variable and pack it.

> There are 4 read-modify-write operations in succession to the RGAC1
> register. Maybe you can converge them into a single regmap_update_bits()
> call.
> 
> Also, for packets which reach the CPU via a trap, we shouldn't set
> skb->offload_fwd_mark = 1. In other words, if the bridge layer wants to
> forward them in software (including to other an8855 ports), let it do so.
> The common example given in commit 515853ccecc6 ("bridge: allow
> forwarding some link local frames") is 802.1X PAE (01-80-C2-00-00-03).
> 
> I notice mtk_tag_rcv() calls dsa_default_offload_fwd_mark() with no
> pre-condition. Do you know whether there exists any bit in the RX tag
> which signifies whether the packet was received because of a trap
> (or if it was autonomously forwarded by the switch to the other bridge
> ports as well)? The offload_fwd_mark bit should be set based on
> something like that.


I did some simulation checking the full tag and also yesterday Airoha
confirmed that those register doesn't affect the CPU tag.

There is an entry in the TAG that signal some kind of packet but it
doesn't react. From what I can see it only comunicate when fdb or other
really special thing.

-- 
	Ansuel