quoted

quoted

-               /* 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