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

[Christian Marangi <ansuelsmth@gmail.com>]

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