Re: [PATCH net-next 2/2] net: pcs: xpcs: Add handling for 4 channel rsfec device

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: pcs: xpcs: Add handling for 4 channel rsfec device

This commit introduces the configuration of vendor-specific registers for
alignment encoding, PCS Mode, and VL_INTVL over one or two instances. It
derives encoding tables from the IEEE 8023-2022 specification and handles
register conflicts for DW_PCS IP.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 0987621632a70..c42eacafad91e 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
[ ... ]
> @@ -1429,21 +1429,94 @@ static int xpcs_get_pma_mmd(struct dw_xpcs *xpcs)
[ ... ]
>  static int
>  xpcs_config_rsfec_pma(struct dw_xpcs *xpcs, const struct pma_pcs_values *v)
>  {
> -	int ret = 0, i, pma_mmd;
> +	int ret = 0, i, vl, pma_mmd;
>  
>  	pma_mmd = xpcs_get_pma_mmd(xpcs);
>  	if (pma_mmd < 1)
>  		return pma_mmd;
>  
>  	for (i = 0; ret >= 0 && i < v->lanes; i++) {
> +		/* code word markings */
> +		for (vl = 0; ret >= 0 && vl < 4; vl++)
> +			ret = xpcs_config_vl_markers(xpcs, vl, i,
> +						     !vl ? &v->vl0_markers[0] :
> +						     &v->vl123_markers[vl - 1]);
> +		if (ret < 0)
> +			break;
> +		/* vendor registers */
> +		ret = xpcs_write_pcs_prtad(xpcs, i, DW_VR_MII_PCS_VL_INTVL,
> +					   v->vl_intvl);

Does passing the loop index i directly as the prtad (MDIO port address) here
overwrite registers of unrelated PHY devices on the shared MDIO bus?

Looking at xpcs_write_pcs_prtad() and xpcs_bus_write(), the address is passed
directly to mdiobus_c45_write():

xpcs_bus_write(struct dw_xpcs *xpcs, int prtad, int dev, u32 reg, u16 val) {
    return mdiobus_c45_write(xpcs->mdiodev->bus, prtad, dev, reg, val);
}

Since i is just the lane index (0 to v->lanes - 1), it appears to disregard
the actual base address of the device.

Could this be offset by the base address (xpcs->mdiodev->addr) to ensure the
writes hit the correct XPCS block instead of unrelated PHYs that might happen
to reside at addresses 0-3 on the bus?

> +		if (ret < 0)
> +			break;
> +		ret = xpcs_write_pcs_prtad(xpcs, i, DW_VR_MII_PCS_PCS_MODE,
> +					   v->pcs_mode);
> +		if (ret < 0)
> +			break;
> +		/* rsfec register */
>  		ret = xpcs_bus_write(xpcs, i, pma_mmd, MDIO_PMA_RSFEC_CTRL,
>  				     v->rsfec_ctrl);
>  	}
>  	return ret;
>  }