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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] DCB per-port initialization is executed for KSZ8995XA despite DCB support being explicitly disabled and skipped globally.
- [High] Inverted Auto-Negotiation Enable bit polarity for KSZ8995XA.
- [High] Corrupted port state due to incorrect `P_NEG_RESTART_CTRL` bits used for KSZ8995XA.
--

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

net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA
    
This adds support for the Microchip KSZ8995XA also known as the
Micrel KS8995XA switch to the KSZ driver.
    
Notice: there are also KSZ8995E and KSZ8995MA. These are BOTH
different from the KSZ8995XA.
    
The helper macros are named ksz_is_ksz8995xa() to make it
possible to add E and MA support in the future.

> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index 0f84b2e7d25d8..18541a8efc39c 100644
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c
[ ... ]
> @@ -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: High]
Does this code invert the Auto-Negotiation Enable bit polarity for KSZ8995XA?

The KSZ8995XA falls into the else branch of ksz_is_ksz88x3(dev) earlier in
ksz8_r_phy_bmcr(), which treats BIT(7) of the P_FORCE_CTRL register as
PORT_AUTO_NEG_DISABLE (where 1 = disable, 0 = enable). 

However, it appears the KSZ8995XA hardware uses BIT(7) for Auto-Negotiation
Enable (1 = enable, 0 = disable), matching the KSZ88X3. If software writes 0
to this bit when intending to enable auto-negotiation, will it disable it in
hardware and break PHY link negotiation?

[ ... ]
> @@ -1147,13 +1175,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]
Does this function write incorrect bits to P_NEG_RESTART_CTRL for the
KSZ8995XA?

Later in ksz8_w_phy_bmcr(), it constructs a bitmask using KSZ8895-specific
bits:

    restart_mask = PORT_LED_OFF | PORT_TX_DISABLE | PORT_AUTO_NEG_RESTART |
        PORT_POWER_DOWN | PORT_AUTO_MDIX_DISABLE | PORT_FORCE_MDIX;

For KSZ8995XA, are these features mapped to different bits? Writing these
might apply the wrong settings. 

Additionally, because KSZ8995XA is not KSZ88X3, it calls
ksz879x_set_loopback() which sets BIT(7) of REG_PORT_STATUS_3 (0x0F). Does
0x0F resolve to Port Control 15 (0x1F) on the KSZ8995XA and corrupt the 802.1p
priority mapping register instead of enabling loopback?

[ ... ]
> @@ -2416,6 +2502,34 @@ const struct ksz_dev_ops ksz88xx_dev_ops = {
>  	.pme_pwrite8 = ksz8_pme_pwrite8,
>  };
>  
> +/*
> + * Restricted operations for KSZ8995XA, so many things are not supported
> + * by this old switch that we need diet DSA operations.
> + */
> +const struct dsa_switch_ops ksz8995xa_switch_ops = {
> +	.get_tag_protocol	= ksz8995xa_get_tag_protocol,
> +	.get_phy_flags		= ksz_get_phy_flags,
> +	.setup			= ksz8_setup,
> +	.teardown		= ksz_teardown,
> +	.phy_read		= ksz8_phy_read16,
> +	.phy_write		= ksz8_phy_write16,
> +	.phylink_get_caps	= ksz8_phylink_get_caps,
> +	.port_setup		= ksz8_dsa_port_setup,

[Severity: High]
Since global DCB initialization ksz_dcb_init() is explicitly skipped for the
KSZ8995XA in ksz8_setup(), will configuring DCB at the port level cause issues
here?

ksz8_dsa_port_setup() unconditionally calls ksz_dcb_init_port(dev, port),
which enables DiffServ and 802.1p priorities on the ports while the global
DSCP mapping remains uninitialized. 

Could this cause unpredictable traffic classification, or corrupt unrelated
port configurations by writing to bits 3:6 of offset 0x00 (REG_PORT_CTRL_0)
on the KSZ8995XA?

> +	.port_bridge_join	= ksz_port_bridge_join,
> +	.port_bridge_leave	= ksz_port_bridge_leave,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-ks8995-to-ksz8-v4-0-e15149ef21e7@kernel.org?part=4