Re: [PATCH 3/3] net/sfc: support FEC feature

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 6/2/23 01:23, Denis Pryazhennikov wrote:
> Support ethdev methods to query and set FEC information.
> Limitations: ignoring rte_eth_fec_get_capability() results
> can lead to NOFEC if the device is not strated.
> 
> Signed-off-by: Denis Pryazhennikov <denis.pryazhennikov@arknetworks.am>
> Reviewed-by: Ivan Malov <ivan.malov@arknetworks.am>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@amd.com>
> ---
>   doc/guides/nics/features/sfc.ini |   1 +
>   drivers/net/sfc/sfc.h            |   2 +
>   drivers/net/sfc/sfc_ethdev.c     | 337 +++++++++++++++++++++++++++++++
>   drivers/net/sfc/sfc_port.c       |  24 ++-
>   4 files changed, 353 insertions(+), 11 deletions(-)

Let's advertise it in release notes.

> 
> diff --git a/doc/guides/nics/features/sfc.ini b/doc/guides/nics/features/sfc.ini
> index f5ac644278ae..e0b9bfb7f7bf 100644
> --- a/doc/guides/nics/features/sfc.ini
> +++ b/doc/guides/nics/features/sfc.ini
> @@ -24,6 +24,7 @@ RSS reta update      = Y
>   SR-IOV               = Y
>   Flow control         = Y
>   VLAN offload         = P
> +FEC                  = Y
>   L3 checksum offload  = Y
>   L4 checksum offload  = Y
>   Inner L3 checksum    = Y
> diff --git a/drivers/net/sfc/sfc.h b/drivers/net/sfc/sfc.h
> index 730d054aea74..e42abe42cb8a 100644
> --- a/drivers/net/sfc/sfc.h
> +++ b/drivers/net/sfc/sfc.h
> @@ -68,6 +68,8 @@ struct sfc_port {
>   
>   	uint32_t			phy_adv_cap_mask;
>   	uint32_t			phy_adv_cap;
> +	uint32_t			fec_cfg;
> +	bool				fec_auto;
>   
>   	unsigned int			flow_ctrl;
>   	boolean_t			flow_ctrl_autoneg;
> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
> index 6d41eb000345..05fc70541b10 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -2343,6 +2343,340 @@ sfc_rx_metadata_negotiate(struct rte_eth_dev *dev, uint64_t *features)
>   	return 0;
>   }
>   
> +static unsigned int
> +sfc_fec_get_capa_speed_to_fec(uint32_t supported_caps,
> +			      struct rte_eth_fec_capa *speed_fec_capa)
> +{
> +	int num = 0;
> +
> +	if (supported_caps & (1u << EFX_PHY_CAP_10000FDX)) {
> +		if (speed_fec_capa) {

Compare vs NULL as DPDK coding style says. Here and in similar cases below.

> +			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_10G;
> +			speed_fec_capa[num].capa =
> +				RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
> +				RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> +				RTE_ETH_FEC_MODE_CAPA_MASK(BASER);

I'd like to see a comment which explicilty says that supported
FEC depends on supported link speed only. So, there is no
special capability bits.

> +		}
> +		num++;
> +	}
> +	if (supported_caps & (1u << EFX_PHY_CAP_25000FDX)) {
> +		if (speed_fec_capa) {
> +			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_25G;
> +			speed_fec_capa[num].capa =
> +				RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
> +				RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> +				RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
> +				RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +		}
> +		num++;
> +	}
> +	if (supported_caps & (1u << EFX_PHY_CAP_40000FDX)) {
> +		if (speed_fec_capa) {
> +			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_40G;
> +			speed_fec_capa[num].capa =
> +				RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
> +				RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> +				RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> +		}
> +		num++;
> +	}
> +	if (supported_caps & (1u << EFX_PHY_CAP_50000FDX)) {
> +		if (speed_fec_capa) {
> +			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_50G;
> +			speed_fec_capa[num].capa =
> +				RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
> +				RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> +				RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
> +				RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +		}
> +		num++;
> +	}
> +	if (supported_caps & (1u << EFX_PHY_CAP_100000FDX)) {
> +		if (speed_fec_capa) {
> +			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_100G;
> +			speed_fec_capa[num].capa =
> +				RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
> +				RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> +				RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +		}
> +		num++;
> +	}
> +
> +	return num;
> +}
> +
> +static int
> +sfc_fec_get_capability(struct rte_eth_dev *dev,
> +		       struct rte_eth_fec_capa *speed_fec_capa,
> +		       unsigned int num)
> +{
> +	struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
> +	unsigned int num_entries;
> +	uint32_t supported_caps;
> +
> +	sfc_adapter_lock(sa);
> +
> +	efx_phy_adv_cap_get(sa->nic, EFX_PHY_CAP_PERM, &supported_caps);
> +
> +	num_entries = sfc_fec_get_capa_speed_to_fec(supported_caps, NULL);
> +	if (!speed_fec_capa || num < num_entries)

Compare vs NULL as DPDK coding style says.

> +		goto adapter_unlock;
> +
> +	num_entries = sfc_fec_get_capa_speed_to_fec(supported_caps,
> +						    speed_fec_capa);
> +
> +adapter_unlock:
> +	sfc_adapter_unlock(sa);
> +
> +	return num_entries;
> +}
> +
> +static uint32_t
> +sfc_efx_caps_to_fec(uint32_t caps, bool is_25g)
> +{
> +	bool rs_req = caps & EFX_PHY_CAP_FEC_BIT(RS_FEC_REQUESTED);
> +	bool rs = caps & EFX_PHY_CAP_FEC_BIT(RS_FEC);
> +	uint32_t fec_capa = 0;
> +	bool baser_req;
> +	bool baser;
> +
> +	if (is_25g) {
> +		baser = caps & EFX_PHY_CAP_FEC_BIT(25G_BASER_FEC);
> +		baser_req = caps & EFX_PHY_CAP_FEC_BIT(25G_BASER_FEC_REQUESTED);
> +	} else {
> +		baser = caps & EFX_PHY_CAP_FEC_BIT(BASER_FEC);
> +		baser_req = caps & EFX_PHY_CAP_FEC_BIT(BASER_FEC_REQUESTED);
> +	}
> +
> +	if (!baser && !rs)
> +		return RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
> +
> +	if (rs_req)
> +		fec_capa |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
> +
> +	if (baser_req)
> +		fec_capa |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
> +
> +

two many empty lines, just one is sufficient

> +	return fec_capa;
> +}
> +
> +static int
> +sfc_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
> +{
> +	struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
> +	struct sfc_port *port = &sa->port;
> +	struct rte_eth_link current_link;
> +	efx_phy_fec_type_t active_fec;
> +	bool is_25g = false;
> +	int rc = 0;
> +
> +	sfc_adapter_lock(sa);
> +
> +	sfc_dev_get_rte_link(dev, 1, &current_link);
> +
> +	if (current_link.link_status == RTE_ETH_LINK_DOWN) {
> +		uint32_t speed = current_link.link_speed;
> +
> +		if (port->fec_auto) {
> +			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO);
> +			goto adapter_unlock;
> +		}
> +
> +		is_25g = (speed == RTE_ETH_SPEED_NUM_25G ||
> +			  speed == RTE_ETH_SPEED_NUM_50G);
> +
> +		*fec_capa = sfc_efx_caps_to_fec(port->fec_cfg, is_25g);
> +
> +		goto adapter_unlock;
> +	}
> +
> +	rc = efx_phy_fec_type_get(sa->nic, &active_fec);
> +	if (rc != 0)
> +		goto adapter_unlock;
> +
> +	switch (active_fec) {
> +	case EFX_PHY_FEC_NONE:
> +		*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
> +		break;
> +	case EFX_PHY_FEC_BASER:
> +		*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
> +		break;
> +	case EFX_PHY_FEC_RS:
> +		*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
> +		break;
> +	default:

Don't we need an error message here and return failure?

> +		break;
> +	}
> +
> +adapter_unlock:
> +	sfc_adapter_unlock(sa);
> +
> +	return rc;
> +}
> +
> +static int
> +sfc_fec_capa_check(struct rte_eth_dev *dev, uint32_t fec_capa,
> +		   uint32_t supported_caps)
> +{
> +	struct rte_eth_fec_capa *speed_fec_capa;
> +	struct rte_eth_link current_link;
> +	bool is_supported = false;
> +	unsigned int num_entries;
> +	bool auto_fec = false;
> +	unsigned int i;
> +
> +	struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
> +
> +	if (sa->state != SFC_ETHDEV_STARTED)
> +		return 0;
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO)) {
> +		auto_fec = true;
> +		fec_capa &= ~RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO);
> +	}
> +
> +	/*
> +	 * If only the AUTO bit is set, the decision on which FEC
> +	 * mode to use will be made by HW/FW or driver.
> +	 */
> +	if (auto_fec && fec_capa == 0)
> +		return 0;
> +
> +	sfc_dev_get_rte_link(dev, 1, &current_link);
> +
> +	num_entries = sfc_fec_get_capa_speed_to_fec(supported_caps, NULL);
> +	if (num_entries == 0)
> +		return -ENOTSUP;
> +
> +	speed_fec_capa = rte_calloc("fec_capa", num_entries,
> +				    sizeof(*speed_fec_capa), 0);
> +	num_entries = sfc_fec_get_capa_speed_to_fec(supported_caps,
> +						    speed_fec_capa);
> +
> +	for (i = 0; i < num_entries; i++) {
> +		if (speed_fec_capa[i].speed == current_link.link_speed) {
> +			if ((fec_capa & speed_fec_capa[i].capa) != 0)
> +				is_supported = true;
> +
> +			break;
> +		}
> +	}
> +
> +	rte_free(speed_fec_capa);
> +
> +	if (is_supported)
> +		return 0;
> +
> +	return -ENOTSUP;
> +}
> +
> +static int
> +sfc_fec_capa_to_efx(uint32_t supported_caps, uint32_t fec_capa,
> +		    uint32_t *efx_fec_caps)
> +{
> +	bool fec_is_set = false;
> +	bool auto_fec = false;
> +	bool nofec = false;
> +	uint32_t ret = 0;
> +
> +	if (efx_fec_caps == NULL)
> +		return -EINVAL;
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO))
> +		auto_fec = true;
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC))
> +		nofec = true;
> +
> +	if (fec_capa == RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO)) {
> +		ret |= (EFX_PHY_CAP_FEC_BIT(BASER_FEC) |
> +			EFX_PHY_CAP_FEC_BIT(25G_BASER_FEC) |
> +			EFX_PHY_CAP_FEC_BIT(RS_FEC)) & supported_caps;
> +		goto done;
> +	}
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS)) {
> +		fec_is_set = true;
> +
> +		if (supported_caps & EFX_PHY_CAP_FEC_BIT(RS_FEC)) {
> +			ret |= EFX_PHY_CAP_FEC_BIT(RS_FEC) |
> +			       EFX_PHY_CAP_FEC_BIT(RS_FEC_REQUESTED);
> +		}
> +	}
> +	if (fec_capa & RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER)) {
> +		if (!auto_fec && fec_is_set)
> +			return -EINVAL;
> +
> +		if (supported_caps & EFX_PHY_CAP_FEC_BIT(BASER_FEC)) {
> +			ret |= EFX_PHY_CAP_FEC_BIT(BASER_FEC) |
> +			       EFX_PHY_CAP_FEC_BIT(BASER_FEC_REQUESTED);
> +		}
> +		if (supported_caps & EFX_PHY_CAP_FEC_BIT(25G_BASER_FEC)) {
> +			ret |= EFX_PHY_CAP_FEC_BIT(25G_BASER_FEC) |
> +			       EFX_PHY_CAP_FEC_BIT(25G_BASER_FEC_REQUESTED);
> +		}
> +	}
> +
> +	if (ret == 0 && !nofec)
> +		return -ENOTSUP;
> +
> +done:
> +	*efx_fec_caps = ret;
> +	return 0;
> +}
> +
> +static int
> +sfc_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
> +{
> +	struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
> +	struct sfc_port *port = &sa->port;
> +	uint32_t supported_caps;
> +	uint32_t efx_fec_caps;
> +	uint32_t updated_caps;
> +	int rc = 0;
> +
> +	sfc_adapter_lock(sa);
> +
> +	efx_phy_adv_cap_get(sa->nic, EFX_PHY_CAP_PERM, &supported_caps);
> +
> +	rc = sfc_fec_capa_check(dev, fec_capa, supported_caps);
> +	if (rc != 0)
> +		goto adapter_unlock;
> +
> +	rc = sfc_fec_capa_to_efx(supported_caps, fec_capa, &efx_fec_caps);
> +	if (rc != 0)
> +		goto adapter_unlock;
> +
> +	if (sa->state == SFC_ETHDEV_STARTED) {
> +		efx_phy_adv_cap_get(sa->nic, EFX_PHY_CAP_CURRENT,
> +				    &updated_caps);
> +		updated_caps = updated_caps & ~EFX_PHY_CAP_FEC_MASK;
> +		updated_caps |= efx_fec_caps;
> +
> +		rc = efx_phy_adv_cap_set(sa->nic, updated_caps);
> +		if (rc != 0)
> +			goto adapter_unlock;
> +	}
> +
> +	port->fec_cfg = efx_fec_caps;
> +	/*
> +	 * There is no chance to recognize AUTO mode from the
> +	 * saved FEC capabilities as AUTO mode can have the same
> +	 * set of bits as any other mode from the EFX point of view.
> +	 * Save it in the proper variable.
> +	 */
> +	if (fec_capa & RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO))
> +		port->fec_auto = true;
> +	else
> +		port->fec_auto = false;
> +
> +adapter_unlock:
> +	sfc_adapter_unlock(sa);
> +
> +	return rc;
> +}
> +
>   static const struct eth_dev_ops sfc_eth_dev_ops = {
>   	.dev_configure			= sfc_dev_configure,
>   	.dev_start			= sfc_dev_start,
> @@ -2392,6 +2726,9 @@ static const struct eth_dev_ops sfc_eth_dev_ops = {
>   	.pool_ops_supported		= sfc_pool_ops_supported,
>   	.representor_info_get		= sfc_representor_info_get,
>   	.rx_metadata_negotiate		= sfc_rx_metadata_negotiate,
> +	.fec_get_capability		= sfc_fec_get_capability,
> +	.fec_get			= sfc_fec_get,
> +	.fec_set			= sfc_fec_set,
>   };
>   
>   struct sfc_ethdev_init_data {

[snip]