Re: [PATCH net-next v6 4/9] net: dsa: lan9645x: add basic dsa driver for LAN9645X

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The CPU port is excluded from multicast flooding groups, breaking IPv6 Neighbor Discovery.
- [Medium] The ingress tail drop watermark (SYS_ATOP) for the internal CPU port is left uninitialized, causing CPU-injected frames to be dropped.
- [Medium] A Time-of-Check to Time-of-Use (TOCTOU) race condition in RGMII collision handling allows mutually exclusive ports to be enabled simultaneously.
--

commit 663f05e1c5287dac7978b55faa5152a3c2ee95f8
Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>

net: dsa: lan9645x: add basic dsa driver for LAN9645X

This commit adds the LAN9645X basic DSA driver with initialization, parent
regmap requests, port module initialization, and phylink integration.

> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
[ ... ]
> @@ -112,6 +112,323 @@ static int lan9645x_setup(struct dsa_switch *ds)
> +	lan9645x_port_cpu_init(lan9645x);
> +
> +	/* Multicast to all front ports */
> +	lan_wr(front_ports, lan9645x, ANA_PGID(PGID_MC));
> +
> +	/* IP multicast to all front ports */
> +	lan_wr(front_ports, lan9645x, ANA_PGID(PGID_MCIPV4));
> +	lan_wr(front_ports, lan9645x, ANA_PGID(PGID_MCIPV6));

[Severity: High]
Will this configuration exclude the CPU port from multicast flooding groups?

Since front_ports only contains the user ports, BIT(CPU_PORT) is omitted
from the PGID_MC, PGID_MCIPV4, and PGID_MCIPV6 masks. Failing to flood IPv6
multicast traffic to the CPU could break IPv6 Neighbor Discovery (Router and
Neighbor Solicitations).

> +
> +	/* Unicast to all front ports */
> +	lan_wr(front_ports, lan9645x, ANA_PGID(PGID_UC));
> +
> +	/* Broadcast to cpu and all front ports */
> +	lan_wr(BIT(CPU_PORT) | front_ports, lan9645x, ANA_PGID(PGID_BC));
> +

> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c
[ ... ]
> @@ -103,6 +103,50 @@ static int lan9645x_phylink_mac_prepare(struct phylink_config *config,
> +					unsigned int mode,
> +					phy_interface_t iface)
> +{
[ ... ]
> +	/* RGMII_0 is a 2:1 mux shared between port 4 and port 7. Reject
> +	 * collisions by looking at current hardware state: the driver
> +	 * maintains the invariant that RGMII_0_CFG=1 iff port 4 owns
> +	 * RGMII_0, and for port 7 when GMII_ENA bit 7 is set this implies port
> +	 * 7 owns RGMII_0.
> +	 */
> +	if (is_rgmii) {
> +		hw_cfg = lan_rd(lan9645x, HSIO_HW_CFG);
> +		gmii_ena = HSIO_HW_CFG_GMII_ENA_GET(hw_cfg);
> +
> +		if (port == 4 && (gmii_ena & BIT(7)))
> +			return -EBUSY;
> +		if (port == 7 && (gmii_ena & BIT(4)) &&
> +		    HSIO_HW_CFG_RGMII_0_CFG_GET(hw_cfg))
> +			return -EBUSY;
> +	}
> +
> +	lan_rmw(HSIO_HW_CFG_GMII_ENA_SET(BIT(port)),
> +		HSIO_HW_CFG_GMII_ENA_SET(BIT(port)), lan9645x, HSIO_HW_CFG);

[Severity: Medium]
Is there a race condition between reading and writing the HSIO_HW_CFG register?

Phylink mac_prepare callbacks can be invoked concurrently from per-port
workqueues. If ports 4 and 7 negotiate link simultaneously, both could pass the
busy check before either writes its GMII_ENA bit, leading to conflicting
hardware states on the shared RGMII_0 mux. Does this read-modify-write sequence
need switch-level locking?

> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
[ ... ]
> @@ -77,6 +77,20 @@ void lan9645x_port_set_tail_drop_wm(struct lan9645x *lan9645x)
> +{
> +	int shared_per_port;
> +	struct dsa_port *dp;
> +
> +	/* Configure tail dropping watermark */
> +	shared_per_port =
> +		lan9645x->shared_queue_sz / (lan9645x->num_phys_ports + 1);
> +
> +	/* The total memory size is divided by number of front ports plus CPU
> +	 * port.
> +	 */
> +	dsa_switch_for_each_available_port(dp, lan9645x->ds)
> +		lan_wr(lan9645x_wm_enc(shared_per_port), lan9645x,
> +		       SYS_ATOP(dp->index));

[Severity: Medium]
Will this leave the tail drop watermark uninitialized for the CPU port?

The memory size calculation correctly accounts for lan9645x->num_phys_ports + 1
to include the CPU port (index 9). However, dsa_switch_for_each_available_port
only iterates over DSA-known ports (indices 0 to 8). Since SYS_ATOP(9) is
skipped, its watermark remains 0. Could this cause CPU-injected jumbo frames to
be dropped on ingress?

> +
> +	/* Tail dropping active based only on per port ATOP wm */
> +	lan_wr(lan9645x_wm_enc(lan9645x->shared_queue_sz), lan9645x,
> +	       SYS_ATOP_TOT_CFG);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527-dsa_lan9645x_switch_driver_base-v6-0-4d409ae64f3c@microchip.com?part=4