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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Standalone switch ports drop all normal data frames because the source port masks (`PGID_SRC`) and flooding masks exclude the CPU port, preventing dataplane forwarding to the host.
- [High] SYS_ATOP tail-drop watermark for the internal CPU port is left uninitialized.
- [High] Time-of-check to time-of-use (TOCTOU) race condition in RGMII multiplexing configuration when configuring port 4 and port 7 concurrently.
- [Low] Incompatible pointer type when passing `val` to `regmap_read_poll_timeout()`.
--

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

This patch adds a basic DSA driver for the LAN9645X switch. It implements
initialization, NPI port setup, MAC phylink configuration, and basic port
modules.

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> @@ -95,9 +95,9 @@ static int lan9645x_reset_switch(struct lan9645x *lan9645x)
>  {
>  	int val = 0;
>  	int err;
>  
>  	lan_wr(SYS_RESET_CFG_CORE_ENA_SET(0), lan9645x, SYS_RESET_CFG);
>  	lan_wr(SYS_RAM_INIT_RAM_INIT_SET(1), lan9645x, SYS_RAM_INIT);
>  	err = lan9645x_rd_poll_timeout(lan9645x, SYS_RAM_INIT, val,
>  				       SYS_RAM_INIT_RAM_INIT_GET(val) == 0);

[Severity: Low]
Does passing an int variable to a macro that expects an unsigned int pointer
cause an incompatible pointer type regression?

The variable val is declared as an int, but it is passed to
lan9645x_rd_poll_timeout() which expands to regmap_read_poll_timeout().
Since regmap_read() expects an unsigned int pointer, this violates pointer
compatibility rules. Should val be declared as u32 instead?

[ ... ]

> @@ -211,7 +211,6 @@ static int lan9645x_setup(struct dsa_switch *ds)
>  		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));

[Severity: High]
Does hardcoding the source port mask to 0 permanently block dataplane
forwarding for standalone ports?

For standalone ports that are not part of a bridge, DSA expects data frames
to be forwarded exclusively to the CPU port. Because the source mask is set
to 0 here, it appears all forwarding from these ports will be dropped at
the destination analysis stage.

Even known unicast frames destined for the host might be dropped because
BIT(CPU_PORT) masked with 0 is 0. Does this prevent standalone ports from
communicating with the network stack?

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c
> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c
> @@ -120,9 +120,7 @@ static int lan9645x_phylink_mac_prepare(struct phylink_config *config,
>  
>  	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: High]
Could concurrent configuration of ports 4 and 7 lead to a race condition
when accessing HSIO_HW_CFG?

The lan9645x_phylink_mac_prepare() function is called by phylink, which
serializes operations per-port but does not synchronize across different
ports. If ports 4 and 7 are brought up at the same time, they might both
read HSIO_HW_CFG simultaneously and race to modify it via lan_rmw().

Is a switch-level lock needed to protect accesses to this shared hardware
resource?

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> @@ -85,5 +85,5 @@ void lan9645x_port_set_tail_drop_wm(struct lan9645x *lan9645x)
>  	/* 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: High]
Will the SYS_ATOP watermark for the internal CPU port remain uninitialized
or zero due to this loop?

The dsa_switch_for_each_available_port() macro iterates over DSA user
ports, which correspond to indices 0 to 8, but the internal CPU port has an
index of 9.

Since the CPU port is skipped, it seems its tail-drop watermark is never
written. Could this cause frames injected by the CPU to be immediately
tail-dropped upon ingress into the queue system?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-dsa_lan9645x_switch_driver_base-v7-0-b2f90e676707@microchip.com?part=4