Re: [PATCH v3 phy-next 8/8] phy: lynx-10g: use RCW override procedure for dynamic protocol change

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] An off-by-one error in the bounds check for `serdes_idx` within `fsl_guts_serdes_get_rcw_override` incorrectly rejects the final SerDes block on the SoC.
- [High] Uninitialized lanes on `ls2088a` SerDes1 have their XGMII clock selection bits blindly overwritten to GMII during RCW initialization.
--

--- Patch [8]: [PATCH v3 phy-next 8/8] phy: lynx-10g: use RCW override procedure for dynamic protocol change ---
commit 7e0651dd0ae8f0fcd028d4877a54ccdb1dcfef2d
Author: Vladimir Oltean <vladimir.oltean@nxp.com>

phy: lynx-10g: use RCW override procedure for dynamic protocol change

Up until this patch, the only protocol change supported was between
1000Base-X/SGMII and 2500Base-X. The others require an RCW override
procedure which was lacking.

Since now the guts driver provides the means of applying this procedure,
make use of it and remove any comment which mentioned the limitation.

> diff --git a/drivers/phy/freescale/phy-fsl-lynx-10g.c b/drivers/phy/freescale/phy-fsl-lynx-10g.c
> index 38def160ef1aa..5ece7889aed7b 100644
> --- a/drivers/phy/freescale/phy-fsl-lynx-10g.c
> +++ b/drivers/phy/freescale/phy-fsl-lynx-10g.c
[ ... ]
> @@ -1225,6 +1223,13 @@ static int lynx_10g_set_mode(struct phy *phy, enum phy_mode mode, int submode)
>  	if (powered_up)
>  		lynx_10g_lane_halt(phy);
>  
> +	if (lynx_10g_lane_mode_needs_rcw_override(lane, lane_mode)) {
> +		err = fsl_guts_lane_set_mode(priv->info->index, lane->id,
> +					     lane_mode);

[Severity: High]
This isn't a bug introduced by this patch, but does the underlying RCW override
procedure contain an off-by-one error that rejects the final SerDes block?

When this calls into fsl_guts_lane_set_mode(), it eventually hits a bounds
check in fsl_guts_serdes_get_rcw_override():

drivers/soc/fsl/guts.c:fsl_guts_serdes_get_rcw_override() {
    ...
    if (serdes_idx >= soc_data->num_serdes || serdes_idx <= 0)
        return -ERANGE;
    ...
}

Since serdes_idx is 1-based, and this uses >= instead of >, won't this
completely fail to apply the dynamic protocol change for any interface on
the last SerDes block (e.g. index 2 on SoCs with 2 SerDes blocks)?

[Severity: High]
This is a pre-existing issue, but does initializing the SerDes RCW
inadvertently corrupt unmanaged lanes on LS2088A?

When fsl_guts_lane_set_mode() is called for the first time on an LS2088A SoC,
it triggers ls2088a_serdes_init_rcwcr() to build a new state for the RCW
utilizing soc.lanes_initialized:

drivers/soc/fsl/guts.c:ls2088a_serdes_init_rcwcr() {
    ...
    for_each_set_bit(i, &soc.lanes_initialized[serdes_idx - 1], MAX_NUM_LANES)
        if (lynx_lane_mode_uses_xgmii_mac(soc.lane_mode[serdes_idx - 1][i]))
            reg |= LS2088A_RCWSR30_SRDS_CLK_SEL_XGMII_Ln_S1(i);

    err = fsl_guts_rcw_rmw(29, reg,
                   LS2088A_RCWSR30_SRDS_CLK_EN_SEL_XGMII_S1 |
                   LS2088A_RCWSR30_SRDS_CLK_SEL_MSK);
    ...
}

Because the blanket mask LS2088A_RCWSR30_SRDS_CLK_SEL_MSK clears the bits
for all 8 lanes but only restores them for explicitly managed lanes, won't
unmanaged lanes (such as those used by DPDK or alternative firmware) have
their hardware protocol selection forced to 0 (GMII), potentially breaking
network connectivity?

> +		if (err)
> +			goto out;
> +	}
> +
>  	err = lynx_10g_lane_disable_pcvt(lane, lane->mode);
>  	if (err)
>  		goto out;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720133642.136324-1-vladimir.oltean@nxp.com?part=8