Re: [PATCH net-next 1/2] net: pcs: xpcs: Add hooks for xpcs configuration of rsfec

[Jakub Kicinski]

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 hooks for xpcs configuration of rsfec

This commit adds hooks to configure the DW PCS vendor registers for RS-FEC
encoding when operating at speeds above 10Gbps. It introduces lane-specific
and vendor-specific configuration values for 25G, 50G, and 100G links.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
[ ... ]
> @@ -219,6 +219,12 @@ int xpcs_write(struct dw_xpcs *xpcs, int dev, u32 reg, u16 val)
>  	return mdiodev_c45_write(xpcs->mdiodev, dev, reg, val);
>  }
>  
> +static int
> +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);
> +}
> +
[ ... ]
> +static int
> +xpcs_config_rsfec_pma(struct dw_xpcs *xpcs, const struct pma_pcs_values *v)
> +{
> +	int ret = 0, i, 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++) {
> +		ret = xpcs_bus_write(xpcs, i, pma_mmd, MDIO_PMA_RSFEC_CTRL,
> +				     v->rsfec_ctrl);

Could passing i as the port address directly overwrite unrelated PHYs on a 
shared MDIO bus?

Looking at how the write maps down to the bus:

xpcs_bus_write()
  mdiobus_c45_write(xpcs->mdiodev->bus, prtad, ...)

If the loop index i is passed directly as prtad, it starts at 0 and writes to
absolute physical port addresses 0, 1, etc., ignoring the device's actual base
MDIO address. Should this offset the lanes using the base address, like
xpcs->mdiodev->addr + i?

Additionally, is it safe to do an unconditional write to the standard
MDIO_PMA_RSFEC_CTRL register here? 

Using a direct write instead of a read-modify-write operation will wipe out
any other existing bits in this register. For instance, global RS-FEC enable
bits or bypass correction settings configured earlier by the networking stack,
bootloader, or phylink might be silently clobbered.
    
Would it be better to use a modify operation to safely update only the
specific vendor or lane-mode bits?

> +	}
> +	return ret;
> +}
-- 
pw-bot: cr