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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PHY soft reset loop unconditionally includes the MAC-only CPU port, risking a 600ms polling timeout and probe failure.
- [High] Auto-negotiation enable/disable polarity is inverted for the KSZ8995XA switch.
--

commit 9246ba5d8ed3da59b20518974c9b8a20afa8511f
Author: Linus Walleij <linusw@kernel.org>
Subject: net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA

This commit adds support for the Microchip KSZ8995XA switch to the KSZ
driver. It updates hardware detection and initialization paths, handling
specific quirks of this older switch model.

> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c

[ ... ]

> @@ -253,7 +255,35 @@ static int ksz8463_reset_switch(struct ksz_device *dev)
>  
>  static int ksz8_reset_switch(struct ksz_device *dev)
>  {
> -	if (ksz_is_ksz88x3(dev)) {
> +	if (ksz_is_ksz8995xa(dev)) {
> +		unsigned int port, reg, val;
> +		int ret;
> +
> +		/* The KSZ8995XA switch itself cannot be reset by software, but
> +		 * often boot loaders have fun with ports, so reset them. This
> +		 * reset bit is self-clearing.
> +		 */
> +		for (port = 0; port < dev->info->port_cnt; port++) {
> +			ret = ksz_prmw8(dev, port, REG_PORT_STATUS_3,
> +					PORT_PHY_SOFT_RESET,
> +					PORT_PHY_SOFT_RESET);
> +			if (ret)
> +				return ret;
> +		}

[Severity: High]
Does this loop unconditionally include the CPU port? In ksz8995xa, the CPU
port (port 4) does not have an internal PHY, so it lacks this PHY reset
register. Writing to it might have unintended consequences.

> +
> +		/* IEEE 802.3 allows up to 500 ms for a PHY reset. */
> +		for (port = 0; port < dev->info->port_cnt; port++) {
> +			reg = dev->dev_ops->get_port_addr(port, REG_PORT_STATUS_3);
> +			ret = regmap_read_poll_timeout(ksz_regmap_8(dev), reg, val,
> +						       !(val & PORT_PHY_SOFT_RESET),
> +						       1000, 600000);
> +			if (ret)
> +				return ret;
> +		}

[Severity: High]
Similarly, polling for the self-clearing bit on the MAC-only CPU port might
cause an unconditional timeout here. Since the reserved register on the CPU
port may not behave like a self-clearing PHY register, could this cause the
switch initialization to hang for 600ms and fail to probe completely?

[ ... ]

> @@ -1307,13 +1377,15 @@ static int ksz8_w_phy_bmcr(struct ksz_device *dev, u16 port, u16 val)
>  	if (val & BMCR_RESET)
>  		return 0;
>  
> -	speed = 0;
> -	if (val & KSZ886X_BMCR_HP_MDIX)
> -		speed |= PORT_HP_MDIX;
> +	if (!ksz_is_ksz8995xa(dev)) {
> +		speed = 0;
> +		if (val & KSZ886X_BMCR_HP_MDIX)
> +			speed |= PORT_HP_MDIX;
>  
> -	ret = ksz_prmw8(dev, port, regs[P_SPEED_STATUS], PORT_HP_MDIX, speed);
> -	if (ret)
> -		return ret;
> +		ret = ksz_prmw8(dev, port, regs[P_SPEED_STATUS], PORT_HP_MDIX, speed);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ctrl = 0;
>  	if (ksz_is_ksz88x3(dev)) {

[Severity: High]
Are the auto-negotiation enable and disable polarities inverted for the
KSZ8995XA switch?

In ksz8_w_phy_bmcr, right after this check, the code branches based on
ksz_is_ksz88x3. The KSZ8995XA falls into the else block which sets the
PORT_AUTO_NEG_DISABLE bit when auto-negotiation is requested to be disabled.

However, for the older KSZ8995 family, bit 7 is typically an enable bit (like
the KSZ88x3), meaning 1 = enable, whereas for KSZ8795 it means 1 = disable.
If KSZ8995XA treats it as an enable bit, this logic will disable AN when
requested to enable, and enable it when requested to disable, potentially
leading to broken network links.

This same inversion would also affect ksz8_r_phy_bmcr which reads this bit to
report the link state. Should KSZ8995XA be grouped with the KSZ88X3 logic
here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-ks8995-to-ksz8-v14-0-3d72692b3c97@kernel.org?part=3