Re: [RFC net-next v2 3/4] net: dsa: mt7530: set STP state also on filter ID 1

On Mon, Aug 02, 2021 at 11:31:29PM +0800, DENG Qingfang wrote:

On Mon, Aug 02, 2021 at 04:43:36PM +0300, Vladimir Oltean wrote:

quoted

On Sun, Aug 01, 2021 at 03:10:21AM +0800, DENG Qingfang wrote:

quoted

--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h

@@ -181,7 +181,7 @@ enum mt7530_vlan_egress_attr {

 /* Register for port STP state control */
 #define MT7530_SSP_P(x)			(0x2000 + ((x) * 0x100))
-#define  FID_PST(x)			((x) & 0x3)

Shouldn't these macros have _two_ arguments, the FID and the port state?

quoted

+#define  FID_PST(x)			(((x) & 0x3) * 0x5)

"* 5": explanation?

quoted

 #define  FID_PST_MASK			FID_PST(0x3)

 enum mt7530_stp_state {
--
2.25.1

I don't exactly understand how this patch works, sorry.
Are you altering port state only on bridged ports, or also on standalone
ports after this patch? Are standalone ports in the proper STP state
(FORWARDING)?

The current code only sets FID 0's STP state. This patch sets both 0's and
1's states.

The *5 part is binary magic. [1:0] is FID 0's state, [3:2] is FID 1's state
and so on. Since 5 == 4'b0101, the value in [1:0] is copied to [3:2] after
the multiplication.

Perhaps I should only change FID 1's state.

Keep the patches dumb for us mortals please.
If you only change FID 1's state, I am concerned that the driver no
longer initializes FID 0's port state, and might leave that to the
default set by other pre-kernel initialization stage (bootloader?).
So even if you might assume that standalone ports are FORWARDING, they
might not be.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek