Re: [PATCH RFC v3 04/12] net: pcs: add NXP SerDes XPCS shared core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: pcs: Link flap and incorrect speed/duplex resolution in SGMII AN
- [High] net: pcs: USXGMII full-duplex configuration dropped by regmap mask
- [Medium] net: pcs: Kernel panic on NULL pointer in nxp_serdes_xpcs_destroy
--

commit fefa07f062a827745ea578ec9b3b8e4d9a1e9739
Author: Clark Wang <xiaoning.wang@nxp.com>

net: pcs: add NXP SerDes XPCS shared core

This commit adds a transport agnostic phylink PCS core for the Synopsys
DesignWare Ethernet XPCS and Ethernet PHY found on NXP S32G and i.MX SoCs.
It provides configuration and link state handling, operating on regmaps
supplied by platform-specific glue drivers.

> diff --git a/drivers/net/pcs/pcs-nxp-serdes-xpcs.c b/drivers/net/pcs/pcs-nxp-serdes-xpcs.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-nxp-serdes-xpcs.c
[ ... ]
> +static int nxp_serdes_xpcs_get_state_c37_sgmii(struct nxp_serdes_xpcs *xpcs,
> +					       unsigned int neg_mode,
> +					       struct phylink_link_state *state)
> +{
[ ... ]
> +	if (ret & DW_VR_MII_C37_ANSGM_SP_LNKSTS) {
[ ... ]
> +	} else if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
> +		int speed, duplex;
> +
> +		state->link = true;
> +
> +		speed = nxp_serdes_xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
> +		if (speed < 0)
> +			return speed;
> +
> +		speed &= BMCR_SPEED100 | BMCR_SPEED1000;
> +		if (speed == BMCR_SPEED1000)
> +			state->speed = SPEED_1000;
> +		else if (speed == BMCR_SPEED100)
> +			state->speed = SPEED_100;
> +		else if (speed == 0)
> +			state->speed = SPEED_10;
> +
> +		duplex = nxp_serdes_xpcs_read(xpcs, MDIO_MMD_VEND2,
> +					      MII_ADVERTISE);
> +		if (duplex < 0)
> +			return duplex;
> +
> +		if (duplex & ADVERTISE_1000XFULL)
> +			state->duplex = DUPLEX_FULL;
> +		else if (duplex & ADVERTISE_1000XHALF)
> +			state->duplex = DUPLEX_HALF;
> +
> +		nxp_serdes_xpcs_write(xpcs, MDIO_MMD_VEND2,
> +				      DW_VR_MII_AN_INTR_STS, 0);
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Does this code override a valid link-down status and clear the interrupt,
which could cause persistent link flaps?

In nxp_serdes_xpcs_get_state_c37_sgmii(), if the link is actually down 
(DW_VR_MII_C37_ANSGM_SP_LNKSTS is clear) but AN completed 
(DW_VR_MII_AN_STS_C37_ANCMPLT_INTR is set), this block forces state->link
to true and writes 0 to DW_VR_MII_AN_INTR_STS. The next poll might read 0
and report the link as down, creating an endless link flap.

Additionally, can the auto-negotiated duplex be resolved incorrectly here
by reading the local advertisement (MII_ADVERTISE) rather than the link
partner's ability?

[ ... ]
> +static void nxp_serdes_xpcs_link_up_usxgmii(struct nxp_serdes_xpcs *xpcs,
> +					    int speed)
> +{
[ ... ]
> +	ret = nxp_serdes_xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR,
> +				     DW_USXGMII_SS_MASK,
> +				     speed_sel | DW_USXGMII_FULL);

[Severity: High]
Does this regmap modification drop the DW_USXGMII_FULL configuration?

Looking at nxp_serdes_xpcs_link_up_usxgmii(), the code attempts to set the
duplex by passing DW_USXGMII_FULL to nxp_serdes_xpcs_modify().

However, DW_USXGMII_FULL is not included in DW_USXGMII_SS_MASK. Because
nxp_serdes_xpcs_modify() masks the target value with the provided mask,
the full-duplex configuration will be filtered out and never written to
the hardware.

[ ... ]
> +void nxp_serdes_xpcs_destroy(struct phylink_pcs *pcs)
> +{
> +	struct nxp_serdes_xpcs *xpcs = phylink_pcs_to_nxp_serdes_xpcs(pcs);
> +
> +	if (!xpcs)
> +		return;
> +
> +	kfree(xpcs);
> +}

[Severity: Medium]
Can this regression cause a panic due to evaluating container_of() before
checking for NULL?

In nxp_serdes_xpcs_destroy(), xpcs is extracted from pcs using
phylink_pcs_to_nxp_serdes_xpcs() which is a container_of() wrapper. If a
caller passes a NULL pcs pointer, container_of() will yield a negative
offset. This bypasses the if (!xpcs) check, resulting in kfree() being
called with an invalid pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919-s32g_serdes-v3-0-9d68868c1e89@oss.nxp.com?part=4