Re: [PATCH net-next v2 4/5] net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] The KSZ8995XA driver incorrectly uses the KSZ879x-specific PHY loopback functions, resulting in reads and writes to a reserved port register.
- [Medium] Unconditional PHY reset writes to a reserved register for the CPU port.
- [Medium] Incorrect MTU register polarity disables hardware MTU enforcement on KSZ8995XA.

Pre-existing issues:
- [High] Race condition in driver teardown between `ksz_switch_remove` and `ksz_phylink_mac_link_down` leading to a Use-After-Free.
--

commit 3134f79144b9d51d69cd09df98e69bd4282145df
Author: Linus Walleij <linusw@kernel.org>

net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA

This commit adds support for the Microchip KSZ8995XA (also known as the
Micrel KS8995XA) switch to the KSZ driver.

> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index 825133e9ce6e6..82e528c7e8d07 100644
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c
> @@ -137,7 +138,18 @@ static int ksz8_pme_pwrite8(struct ksz_device *dev, int port, int offset, u8 dat
>  
>  static int ksz8_reset_switch(struct ksz_device *dev)
>  {
> -	if (ksz_is_ksz88x3(dev)) {
> +	if (ksz_is_ksz8995xa(dev)) {
> +		unsigned int port;
> +
> +		/* KSZ8995XA is "reset" by stopping the switch and starting it */
> +		ksz_cfg(dev, REG_SW_ID_1, SW_START_SWITCH, false);
> +		usleep_range(10, 100);
> +		ksz_cfg(dev, REG_SW_ID_1, SW_START_SWITCH, true);
> +
> +		/* Often boot loaders have fun with ports, so reset them */
> +		for (port = 0; port < dev->info->port_cnt; port++)
> +			ksz_port_cfg(dev, port, 0x0f, BIT(4), true);

[Severity: Medium]
Since port 4 (the CPU port) lacks an internal PHY, will this loop attempt to
write to the reserved register 0x5F (0x0f + 0x10 + 4 * 0x10) for that port?
Should this check dev->info->internal_phy[port] before performing the reset
to avoid undefined hardware behavior?

[ ... ]

> @@ -161,8 +173,14 @@ static int ksz8_reset_switch(struct ksz_device *dev)
>  static int ksz8863_change_mtu(struct ksz_device *dev, int frame_size)
>  {
>  	u8 ctrl2 = 0;
> +	unsigned int legalsz;
> +
> +	if (ksz_is_ksz8995xa(dev))
> +		legalsz = KSZ8995XA_LEGAL_PACKET_SIZE;
> +	else
> +		legalsz = KSZ8_LEGAL_PACKET_SIZE;
>  
> -	if (frame_size <= KSZ8_LEGAL_PACKET_SIZE)
> +	if (frame_size <= legalsz)
>  		ctrl2 |= KSZ8863_LEGAL_PACKET_ENABLE;

[Severity: Medium]
Is the polarity of KSZ8863_LEGAL_PACKET_ENABLE (BIT 1 of Reg 4) inverted on
the KSZ8995XA? If it functions as Legal Maximum Packet Size Check Disable,
does setting this bit to 1 actually disable the hardware MTU enforcement
instead of enabling it?

[ ... ]

> @@ -924,8 +947,10 @@ static int ksz8_r_phy_bmcr(struct ksz_device *dev, u16 port, u16 *val)
>  	if (ctrl & PORT_FORCE_FULL_DUPLEX)
>  		*val |= BMCR_FULLDPLX;
>  
> -	if (speed & PORT_HP_MDIX)
> -		*val |= KSZ886X_BMCR_HP_MDIX;
> +	if (!ksz_is_ksz8995xa(dev)) {
> +		if (speed & PORT_HP_MDIX)
> +			*val |= KSZ886X_BMCR_HP_MDIX;
> +	}
>  
>  	if (restart & PORT_FORCE_MDIX)
>  		*val |= KSZ886X_BMCR_FORCE_MDI;

[Severity: Medium]
In ksz8_r_phy_bmcr() and ksz8_w_phy_bmcr(), the driver uses
if (ksz_is_ksz88x3(dev)) to separate KSZ88x3 and KSZ879x logic. For the
KSZ8995XA, this evaluates to false and falls through to the KSZ879x block.

Does this cause the KSZ8995XA to incorrectly use the KSZ879x-specific PHY
loopback functions (ksz879x_get_loopback and ksz879x_set_loopback) which
access REG_PORT_STATUS_3 (relative offset 0x0F)? I noticed the KSZ8995XA
MAC loopback control is actually located in P_NEG_RESTART_CTRL (0x0D),
matching the KSZ88x3. Could this break loopback functionality and write to
reserved hardware registers?

> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 4a7bcd1a93927..7f514064116a1 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c

[ ... ]

> @@ -4948,11 +4996,13 @@ int ksz_switch_register(struct ksz_device *dev)
>  		return ret;
>  	}
>  
> -	/* Read MIB counters every 30 seconds to avoid overflow. */
> -	dev->mib_read_interval = msecs_to_jiffies(5000);
> +	if (!ksz_is_ksz8995xa(dev)) {
> +		/* Read MIB counters every 30 seconds to avoid overflow. */
> +		dev->mib_read_interval = msecs_to_jiffies(5000);
>  
> -	/* Start the MIB timer. */
> -	schedule_delayed_work(&dev->mib_read, 0);
> +		/* Start the MIB timer. */
> +		schedule_delayed_work(&dev->mib_read, 0);
> +	}
>  
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition in
the teardown path involving dev->mib_read_interval.

In ksz_switch_remove(), the driver zeroes out dev->mib_read_interval and
calls cancel_delayed_work_sync(&dev->mib_read) before unregistering the
switch with dsa_unregister_switch().

If ksz_phylink_mac_link_down() runs concurrently, could it read a non-zero
dev->mib_read_interval, get preempted while ksz_switch_remove() finishes
canceling the work, and then resume to unconditionally call
schedule_delayed_work(&dev->mib_read, 0)? This would re-arm the work item
on a structure that is about to be freed, leading to a Use-After-Free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522-ks8995-to-ksz8-v2-0-5712c0dc9e75@kernel.org?part=4