Re: [PATCH net-next v2] net: dsa: realtek: rtl8366rb: Fix up port isolation
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-07-25 00:05:19
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
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
@@ -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