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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The PHY soft reset loop incorrectly iterates over all ports including the CPU port, writing to and polling a reserved register which can lead to a 600ms timeout and probe failure.
--

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

net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA

This commit adds support for the Microchip KSZ8995XA switch to the KSZ driver, along with handling its specific PHY reset requirements, DSCP mapping, and legal packet size semantics. It also transitions the legacy KS8995 driver exclusives.

> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index 941ae9f66f70b..d7498132064e8 100644
> --- 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);

[Severity: High]
Does this code inadvertently write to a reserved register for the CPU port?

The PHY soft reset loop iterates over all ports up to dev->info->port_cnt,
which includes the CPU port (port 4). However, the CPU port lacks an internal
PHY (.internal_phy = false). Initiating a PHY reset here means writing to
REG_PORT_STATUS_3, which maps to a reserved register (offset 0x5D) for the
CPU port.

> +			if (ret)
> +				return ret;
> +		}
> +
> +		/* 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);

[Severity: High]
Can this polling loop cause a 600ms probe timeout?

Since this loop also polls the reserved register on the CPU port, if the
written bit does not self-clear as it would on actual PHYs, this could block
for the entire 600ms duration and cause the initialization to fail.

Should these loops iterate up to dev->phy_port_cnt or explicitly check
.internal_phy[port] before interacting with the PHY reset register?

> +			if (ret)
> +				return ret;
> +		}
> +
> +		/* Some PHYs need additional settling time after reset. */
> +		usleep_range(1000, 2000);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913-ks8995-to-ksz8-v13-0-327b16d309eb@kernel.org?part=3