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

[PATCH net-next v2] 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.

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 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..6d5a6e7bc5f1 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,
+						   RTL8366RB_PORT_ISO_PORTS_MASK);
 		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 v2] net: dsa: realtek: rtl8366rb: Fix up port isolation

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: realtek: rtl8366rb: Fix up port isolation

The patch changes the initial port isolation setup in rtl8366rb_setup()
from passing 0 to passing RTL8366RB_PORT_ISO_PORTS_MASK, and also
tidies up two comments.

Link: https://sashiko.dev/#/patchset/20260630-rtl8366rb-improvements-v2-0-05eb9d6a37f5%40kernel.org

> diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
> index d2fa8ff6a5d0..6d5a6e7bc5f1 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;

[Medium]
Should the mask argument to rtl8366rb_port_set_isolation() be treated as
a port-index bitmap (bit N == port N) rather than a register-layout mask?

The helper unconditionally shifts its argument left by one via
RTL8366RB_PORT_ISO_PORTS():

    #define RTL8366RB_PORT_ISO_EN           BIT(0)
    #define RTL8366RB_PORT_ISO_PORTS_MASK   GENMASK(7, 1)
    #define RTL8366RB_PORT_ISO_PORTS(pmask) ((pmask) << 1)

    if (mask)
            mask = RTL8366RB_PORT_ISO_PORTS(mask) | RTL8366RB_PORT_ISO_EN;

All other callers construct mask as a port-index bitmap, e.g.:

    upports_mask |= BIT(dp->index);
    downports_mask |= BIT(dp->index);
    ret = rtl8366rb_port_set_isolation(priv, dp->index, upports_mask);

so port 0 is expected to be encoded as BIT(0) in the argument, and
RTL8366RB_PORT_ISO_PORTS() places it at the correct register bit 1.

> @@ -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,
> +						   RTL8366RB_PORT_ISO_PORTS_MASK);
>  		if (ret)
>  			return ret;

[Medium]
Given the port-index vs register-layout convention above, does passing
RTL8366RB_PORT_ISO_PORTS_MASK (GENMASK(7, 1) == 0xFE, an
already-shifted register-layout value) here double-shift the mask?

Following the helper:

    mask = (0xFE << 1) | BIT(0)
         = 0x1FC | 0x1
         = 0x1FD

which is then written to RTL8366RB_PORT_ISO(port). Two observations:

Bit 1 of the register (the isolation slot for port 0) ends up cleared,
so port 0 does not appear to be isolated in the initial setup, which
seems to contradict the "Start with all ports completely isolated"
comment and the commit log's "setting all isolation bits in the
isolation mask".

Bit 8 lies outside RTL8366RB_PORT_ISO_PORTS_MASK (GENMASK(7, 1)) and is
written as 1, touching a reserved bit in that register.

Would a port-index bitmap covering all ports, for example
GENMASK(RTL8366RB_NUM_PORTS - 1, 0), match the convention used by the
other callers and produce the intended 0xFF register value (bits 1..7
set plus the enable bit)?
-- 
pw-bot: cr