[PATCH net-next v3] net: dsa: realtek: rtl8366rb: Fix up port isolation

[PATCH net-next v3] net: dsa: realtek: rtl8366rb: Fix up port isolation

[Linus Walleij]

Sashiko reports that we incorrectly disable isolation in the setup
loop while what we want to do is to enable it.

Enable it by setting all isolation bits in the isolation mask
for all ports in the switch.

Fix up the comments so it is clear what is going on, including a
missing word in the helper function.

Reported-by: Paolo Abeni <pabeni@redhat.com>
Closes: https://sashiko.dev/#/patchset/20260630-rtl8366rb-improvements-v2-0-05eb9d6a37f5%40kernel.org
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
Changes in v3:
- Fixup the erroneous shifted bitmask (I did not have my coffee...)
- Use GENMASK(ds->num_ports - 1, 0) top properly create a bitmask of
  all the ports, and isolate every port from every port.
- Link to v2: https://patch.msgid.link/20260721-rtl8366rb-fixes-v2-1-24464bd1efa4@kernel.org

Changes in v2:
- Properly fix the issue by setting all isolation bits to 1.
- Link to v1: https://patch.msgid.link/20260711-rtl8366rb-fixes-v1-1-12b9210b0cce@kernel.org
---
 drivers/net/dsa/realtek/rtl8366rb.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index d2fa8ff6a5d0..61c30d2c6802 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -794,8 +794,8 @@ static int rtl8366rb_setup_all_leds_off(struct realtek_priv *priv)
 static int rtl8366rb_port_set_isolation(struct realtek_priv *priv, int port,
 					u32 mask)
 {
-	/* Bit 0 enables isolation so set this if we enable isolation
-	 * any of the ports an clear it if we disable on all of them.
+	/* Bit 0 enables isolation so set this if we enable isolation on
+	 * any of the ports and clear it if we disable on all of them.
 	 */
 	if (mask)
 		mask = RTL8366RB_PORT_ISO_PORTS(mask) | RTL8366RB_PORT_ISO_EN;
@@ -951,7 +951,8 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 		rtl8366rb_port_stp_state_set(ds, dp->index, BR_STATE_DISABLED);
 
 		/* Start with all ports completely isolated */
-		ret = rtl8366rb_port_set_isolation(priv, dp->index, 0);
+		ret = rtl8366rb_port_set_isolation(priv, dp->index,
+						   GENMASK(ds->num_ports - 1, 0));
 		if (ret)
 			return ret;
 
@@ -974,7 +975,7 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 		if (!dsa_port_is_user(dp))
 			continue;
 
-		/* Forward only to the CPU */
+		/* Forward only to the CPU, isolate from all other ports */
 		ret = rtl8366rb_port_set_isolation(priv, dp->index, upports_mask);
 		if (ret)
 			return ret;

---
base-commit: 23dad2d088dfc82cae1f5a936f8ff7ffebb38dd9
change-id: 20260702-rtl8366rb-fixes-a93f831ec2a4

Best regards,
--  
Linus Walleij <linusw@kernel.org>

Re: [PATCH net-next v3] net: dsa: realtek: rtl8366rb: Fix up port isolation

[Jakub Kicinski]

On Sat, 25 Jul 2026 15:46:21 +0200 Linus Walleij wrote:
> Sashiko reports that we incorrectly disable isolation in the setup
> loop while what we want to do is to enable it.
> 
> Enable it by setting all isolation bits in the isolation mask
> for all ports in the switch.
> 
> Fix up the comments so it is clear what is going on, including a
> missing word in the helper function.

Sorry to report that both Sashikos remain skeptical.

https://sashiko.dev/#/patchset/20260725-rtl8366rb-fixes-v3-1-57557b39aaa5@kernel.org
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260725-rtl8366rb-fixes-v3-1-57557b39aaa5@kernel.org

The mask is the allowed mask so the write of 0 was correct,
but we want EN + mask(0) ? The problem being that
rtl8366rb_port_set_isolation() does:

 if (mask)
	reg |= EN

so with mask of 0 we can't enable.

Maybe pass 

	rtl8366rb_port_set_isolation(priv, dp->index, BIT(dp->index));

?

Of course neither I nor the LLMs actually know this HW :S
But comments on other calls to rtl8366rb_port_set_isolation()
suggest that indeed the mask is for allowed ports.