Re: [PATCH RFC net-next 2/9] net: pcs: xpcs: add USXGMII Clause 37 autoneg support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] net: pcs: xpcs: USXGMII link up forces full-duplex and ignores negotiated duplex
- [Low] net: pcs: xpcs: unconditional disable of Clause 37 AN interrupts for USXGMII
- [Low] net: pcs: xpcs: incorrect use of standard MDIO_CTRL1 in vendor-specific register access
--

commit a4e7caa76a7f032f82d43f5626c2004e96588abf
Author: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
net: pcs: xpcs: add USXGMII Clause 37 autoneg support

This commit adds Clause 37 USXGMII auto-negotiation support to the generic
DesignWare XPCS driver. It configures the PCS for BASE-R, enables single-port
10G-SXGMII operation, and decodes the negotiated link, speed, and duplex.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 0337e2bcc0125..7f5c7a275630f 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
[ ... ]
> @@ -355,6 +356,54 @@ static int xpcs_read_fault_c73(struct dw_xpcs *xpcs,
>  	return 0;
>  }
>  
> +static int xpcs_config_aneg_c37_usxgmii(struct dw_xpcs *xpcs)
> +{
> +	u16 mask, val;
> +	int ret;
[ ... ]
> +	/* Configure USXGMII per XPCS databook section 7.6. */
> +	mask = DW_VR_MII_AN_CTRL_8BIT | DW_VR_MII_SGMII_LINK_STS |
> +	       DW_VR_MII_TX_CONFIG_MASK | DW_VR_MII_PCS_MODE_MASK |
> +	       DW_VR_MII_AN_INTR_EN;
> +	val = FIELD_PREP(DW_VR_MII_TX_CONFIG_MASK,
> +			 DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII) |
> +	      DW_VR_MII_SGMII_LINK_STS;
> +
> +	ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL,
> +			  mask, val);

[Severity: Low]
Does this unconditionally disable Clause 37 AN interrupts for USXGMII?

DW_VR_MII_AN_INTR_EN is included in the mask but is missing from val, which
explicitly and unconditionally clears the interrupt enable bit.

Although USXGMII currently defaults to polling mode, could explicitly disabling
it here break interrupt-driven operation if a MAC driver attempts to use it?

> +	if (ret < 0)
> +		return ret;
> +
> +	return xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR,
> +			   BMCR_ANENABLE, BMCR_ANENABLE);
> +}
> +
>  static void xpcs_link_up_usxgmii(struct dw_xpcs *xpcs, int speed)
>  {
>  	int ret, speed_sel;

[Severity: Low]
Is the standard MDIO_CTRL1 macro used incorrectly here for a vendor-specific
register?

In xpcs_link_up_usxgmii(), there are calls like:

    ret = xpcs_modify_vpcs(xpcs, MDIO_CTRL1, DW_USXGMII_EN, DW_USXGMII_EN);

Both MDIO_CTRL1 and DW_VR_XS_PCS_DIG_CTRL1 evaluate to 0x00, but using the
standard IEEE macro instead of the vendor-specific one in xpcs_modify_vpcs()
breaks consistency. This also occurs when applying DW_USXGMII_RST.

[ ... ]
> @@ -1158,6 +1213,58 @@ static int xpcs_get_state_2500basex(struct dw_xpcs *xpcs,
>  	return 0;
>  }
>  
> +static int xpcs_get_state_c37_usxgmii(struct dw_xpcs *xpcs,
> +				      struct phylink_link_state *state)
> +{
> +	u16 speed;
> +	int ret;
[ ... ]
> +	state->duplex = ret & DW_VR_MII_USXG_FULL ?
> +			DUPLEX_FULL : DUPLEX_HALF;
> +
> +	return 0;
> +}

[Severity: Medium]
Will xpcs_link_up_usxgmii() ignore this negotiated duplex state?

If the link negotiates half-duplex (which is supported here as DUPLEX_HALF),
the xpcs_link_up() function passes this state, but xpcs_link_up_usxgmii()
ignores the duplex parameter entirely and unconditionally sets DW_USXGMII_FULL.

Furthermore, DW_USXGMII_FULL is missing from DW_USXGMII_SS_MASK:

    ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, DW_USXGMII_SS_MASK,
                      speed_sel | DW_USXGMII_FULL);

Is it impossible to clear the full-duplex bit if it was previously set,
leading to a duplex mismatch?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-qcom_xpcs_nord_emac-v1-0-4b1c682af70f@oss.qualcomm.com?part=2