Re: [PATCH v4 phy-next 15/16] phy: lynx-10g: new driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver incorrectly uses 0 as a sentinel value in `lane->default_pccr[mode]`, treating a valid backed-up PCCR configuration of 0 as uninitialized data and unintentionally falling back to a hardcoded PCS muxing configuration.
--

commit 0fcab57e2d19361e4720b40d6690e12c5a5bfd0b
Author: Vladimir Oltean <vladimir.oltean@nxp.com>

phy: lynx-10g: new driver

Introduce a driver for the networking lanes of the 10G Lynx SerDes
block, present on the majority of Layerscape and QorIQ (Freescale/NXP)
SoCs. It provides the generic PHY framework to allow network MACs to
reconfigure the SerDes for different link modes based on connected modules.

> diff --git a/drivers/phy/freescale/phy-fsl-lynx-10g.c b/drivers/phy/freescale/phy-fsl-lynx-10g.c
> new file mode 100644
> index 0000000000000..7dd5d94b51cf2
> --- /dev/null
> +++ b/drivers/phy/freescale/phy-fsl-lynx-10g.c
> @@ -0,0 +1,1278 @@
[ ... ]
> +static void lynx_10g_backup_pccr_val(struct lynx_lane *lane)
> +{
> +	u32 val;
> +	int err;
> +
> +	if (lane->mode == LANE_MODE_UNKNOWN)
> +		return;
> +
> +	err = lynx_pccr_read(lane, lane->mode, &val);
> +	if (err) {
> +		dev_warn(&lane->phy->dev,
> +			 "The driver doesn't know how to access the PCCR for lane mode %s\n",
> +			 lynx_lane_mode_str(lane->mode));
> +		lane->mode = LANE_MODE_UNKNOWN;
> +		return;
> +	}
> +
> +	lane->default_pccr[lane->mode] = val;
> +
> +	switch (lane->mode) {
> +	case LANE_MODE_1000BASEX_SGMII:
> +	case LANE_MODE_2500BASEX:
> +		lane->default_pccr[LANE_MODE_1000BASEX_SGMII] = val & ~PCCR8_SGMIIa_KX;
> +		lane->default_pccr[LANE_MODE_2500BASEX] = val & ~PCCR8_SGMIIa_KX;
> +		break;
> +	default:
> +		break;
> +	}
> +}

[Severity: High]
Since the PCCR8_SGMIIa_KX bit is explicitly cleared here, could this result in
a backed-up value of exactly 0 if no other configuration bits were originally
set?

[ ... ]
> +static int lynx_10g_lane_enable_pcvt(struct lynx_lane *lane,
> +				     enum lynx_lane_mode mode)
> +{
> +	struct lynx_priv *priv = lane->priv;
> +	u32 val;
> +	int err;
> +
> +	spin_lock(&priv->pcc_lock);
> +
> +	switch (mode) {
> +	case LANE_MODE_1000BASEX_SGMII:
> +	case LANE_MODE_2500BASEX:
> +		err = lynx_pcvt_rmw(lane, mode, CR(1), SGMIIaCR1_SGPCS_EN,
> +				    SGMIIaCR1_SGPCS_EN);
> +		if (err)
> +			goto out;
> +
> +		lynx_pcvt_rmw(lane, mode, CR(0), SGMIIaCR0_RST_SGM_OFF,
> +			      SGMIIaCR0_RST_SGM | SGMIIaCR0_PD_SGM);
> +		break;
> +	case LANE_MODE_QSGMII:
> +		err = lynx_pcvt_rmw(lane, mode, CR(0), QSGMIIaCR0_RST_QSGM_OFF,
> +				    QSGMIIaCR0_RST_QSGM | QSGMIIaCR0_PD_QSGM);
> +		if (err)
> +			goto out;
> +		break;
> +	default:
> +		err = 0;
> +	}
> +
> +	if (lane->default_pccr[mode]) {
> +		err = lynx_pccr_write(lane, mode, lane->default_pccr[mode]);
> +		goto out;
> +	}

[Severity: High]
If the valid backed-up PCCR configuration is 0, will this condition evaluate
to false and skip the restoration?

It seems a configuration value of 0 is a valid state for the PCCR register,
but this check relies on 0 as a sentinel value for uninitialized data. Could
this inadvertently cause the driver to apply the hardcoded default values
below, breaking the initial muxing configured by the RCW?

> +
> +	val = 0;
> +
> +	switch (mode) {
> +	case LANE_MODE_1000BASEX_SGMII:
> +	case LANE_MODE_2500BASEX:
> +		val |= PCCR8_SGMIIa_CFG;
> +		break;

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy