Re: [PATCH net-next v5 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] Setting PGID_SRC to 0 for all front ports prevents traffic from reaching the CPU port.
- [High] Tail-drop watermark `SYS_ATOP` is not initialized for the internal CPU port, causing CPU extraction frames to be dropped.
- [Medium] Time-of-check to time-of-use (TOCTOU) race condition during RGMII_0 hardware mux collision check.
--

commit dd42e20749a1e5409bdf3c001b0f05f884ed6287
Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
Subject: net: dsa: lan9645x: add basic dsa driver for LAN9645X

This commit introduces a basic DSA driver for the LAN9645X switch family.
It includes initialization, port configuration, NPI setup, and phylink
integration for the internal CuPHYs and RGMII ports.

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
[ ... ]
> @@ -215,22 +215,10 @@ static int lan9645x_setup(struct dsa_switch *ds)
>  	for (int i = 0; i < PGID_ENTRIES; ++i)
>  		lan_wr(ANA_PGID_CFG_OBEY_VLAN_SET(1),
>  		       lan9645x, ANA_PGID_CFG(i));
>  
>  	/* Disable bridging by default */
>  	for (int p = 0; p < lan9645x->num_phys_ports; p++) {
>  		lan_wr(0, lan9645x, ANA_PGID(PGID_SRC + p));

Does setting this mask to 0 filter out all traffic destined for the CPU
port? If BIT(CPU_PORT) is not included in the source port mask, it appears
normal networking packets (like ARP or IP traffic) won't reach the host
network stack. Should BIT(CPU_PORT) be included here?

>  
>  		/* Do not forward BPDU frames to the front ports and copy them
>  		 * to CPU
>  		 */
>  		lan_wr(ANA_CPU_FWD_BPDU_CFG_BPDU_REDIR_ENA,
>  		       lan9645x, ANA_CPU_FWD_BPDU_CFG(p));
>  	}
[ ... ]
> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c
[ ... ]
> @@ -105,37 +105,25 @@ static int lan9645x_phylink_mac_prepare(struct phylink_config *config,
>  	/* 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);

Can this code race if phylink invokes mac_prepare asynchronously for ports
4 and 7?

Since the read of HSIO_HW_CFG and the subsequent update via lan_rmw() are
not serialized by a driver-level lock, could both threads concurrently pass
the collision check and overwrite the multiplexer ownership?

>  
>  	if (port == 4) {
>  		lan_rmw(HSIO_HW_CFG_RGMII_0_CFG_SET(is_rgmii),
[ ... ]
> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
[ ... ]
> @@ -77,21 +77,15 @@ 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));

Does this loop configure the tail drop watermark for the internal CPU port?

The division accounts for the CPU port (num_phys_ports + 1), but
dsa_switch_for_each_available_port() only iterates over the physical ports
known to the DSA core. If SYS_ATOP(9) remains uninitialized at 0, will
frames forwarded to the CPU extraction queues be immediately dropped?

>  
>  	/* 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/20260518-dsa_lan9645x_switch_driver_base-v5-0-968fbf34ffa3@microchip.com?part=4