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

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

Just delete this setting: the next loop in the setup code, over the
user ports, isolate all the ports from each other, so only the CPU
port can see them.

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>
---
 drivers/net/dsa/realtek/rtl8366rb.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index d2fa8ff6a5d0..e615814049c1 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;
@@ -950,11 +950,6 @@ 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);
-		if (ret)
-			return ret;
-
 		/* Disable learning */
 		ret = rtl8366rb_port_set_learning(priv, dp->index, false);
 		if (ret)
@@ -974,7 +969,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] net: dsa: realtek: rtl8366rb: Fix up port isolation

[Luiz Angelo Daros de Luca]

Hi Linus,

Em sáb., 11 de jul. de 2026, 06:43, Linus Walleij <linusw@kernel.org> escreveu:
>
> Sashiko reports that we incorrectly disable isolation in the setup
> loop while what we want to do is to enable it.
>
> Just delete this setting: the next loop in the setup code, over the
> user ports, isolate all the ports from each other, so only the CPU
> port can see them.
>
> 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>
> ---
>  drivers/net/dsa/realtek/rtl8366rb.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
> index d2fa8ff6a5d0..e615814049c1 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;
> @@ -950,11 +950,6 @@ 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);
> -               if (ret)
> -                       return ret;
> -

If you don't isolate the ports, what is the default port status?
Wouldn't it allow unused ports to talk with other ports and, at least,
send packets to other ports?
I think that it will not leak packets as the
.port_enable/.port_disable is present and all other ports will be
isolated from sending traffic to unused ports. Anyway, it would be
cleaner if all unused ports are really isolated.

>                 /* Disable learning */
>                 ret = rtl8366rb_port_set_learning(priv, dp->index, false);
>                 if (ret)
> @@ -974,7 +969,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>
>
>

Regards,

Luiz

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

[Linus Walleij]

On Sat, Jul 11, 2026 at 8:03 PM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:

> > -               /* Start with all ports completely isolated */
> > -               ret = rtl8366rb_port_set_isolation(priv, dp->index, 0);
> > -               if (ret)
> > -                       return ret;
>
> If you don't isolate the ports, what is the default port status?

Hm as I wrote in the commit message:

"the next loop in the setup code, over the
user ports, isolate all the ports from each other, so only the CPU
port can see them."

It looks like this:

        /* Configure user ports */
        dsa_switch_for_each_port(dp, ds) {
                if (!dsa_port_is_user(dp))
                        continue;

                /* Forward only to the CPU, isolate from all other ports */
                ret = rtl8366rb_port_set_isolation(priv, dp->index,
upports_mask);
                if (ret)
                        return ret;

                /* If we support cascade switches, it should also include the
                 * downstream DSA ports.
                 */
                downports_mask |= BIT(dp->index);
        }

upports_mask is the bitmask for all CPU ports, so that should do what is
expected?

Yours,
Linus Walleij

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

[Luiz Angelo Daros de Luca]

> > > -               /* Start with all ports completely isolated */
> > > -               ret = rtl8366rb_port_set_isolation(priv, dp->index, 0);
> > > -               if (ret)
> > > -                       return ret;
> >
> > If you don't isolate the ports, what is the default port status?
>
> Hm as I wrote in the commit message:
>
> "the next loop in the setup code, over the
> user ports, isolate all the ports from each other, so only the CPU
> port can see them."
>
> It looks like this:
>
>         /* Configure user ports */
>         dsa_switch_for_each_port(dp, ds) {
>                 if (!dsa_port_is_user(dp))
>                         continue;
>
>                 /* Forward only to the CPU, isolate from all other ports */
>                 ret = rtl8366rb_port_set_isolation(priv, dp->index,
> upports_mask);
>                 if (ret)
>                         return ret;
>
>                 /* If we support cascade switches, it should also include the
>                  * downstream DSA ports.
>                  */
>                 downports_mask |= BIT(dp->index);
>         }
>
> upports_mask is the bitmask for all CPU ports, so that should do what is
> expected?

The first loop originally acted as a catch-all to isolate all ports
upfront (whether unused, dsa, cpu, or user). The second and third
loops then explicitly opened the forwarding paths for user and CPU
ports.
If you exclude the isolation step from the initial loop, unused ports
isolation won't be touched by the subsequent configuration loops
either. As a result, those unused ports will remain in their hardware
default isolation state.

If the hardware default is to allow forwarding (which is common for
unconfigured ports), those unused ports could potentially forward
traffic between themselves, or even leak to active ports (user or
CPU). Even if the DSA core currently disables unused ports at the
MAC/PHY level, relying on that as the sole layer of defense is risky
if that behavior ever changes or if a port is briefly enabled during
setup.

We shouldn't leave the hardware forwarding matrix in an unknown or
default state for unused ports. It should explicitly disable
forwarding by setting a 0 mask for ports that are unused (or all of
them as intended).

Looking at the current implementation,
rtl8366rb_port_set_isolation(priv, port, mask) should not use mask ==
0 as a way to disable isolation. It breaks the expected semantics and
completely prevents a caller from isolating a port entirely. This also
causes a collision of meaning, where passing a mask of 0 is equivalent
as if you passed a mask of ALL_PORTS.

The function is currently trying to do two things at once:
1) Enable or disable the hardware isolation feature itself.
2) Set the isolation destination bitmask.

We should either remove the feature's enable/disable logic from this
function entirely, or expand its signature to include an explicit
boolean argument, like: rtl8366rb_port_set_isolation(priv, port, bool
enable, u32 mask)

For the newer RTL8367C family (and some other variants), Realtek
actually dropped this dedicated "enable" bit entirely, allowing
isolation to be controlled purely by setting all target port bits to 0
or 1. However, for this specific older hardware where the explicit bit
exists, we shouldn't mash both concepts into a single mask parameter.

Regards,

Luiz