Re: [dpdk-dev] [V5 1/3] ethdev: introduce FEC API

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 9/17/20 11:52 AM, Min Hu (Connor) wrote:
> This patch adds Forward error correction(FEC) support for ethdev.
> Introduce APIs which support query and config FEC information in
> hardware.
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> Reviewed-by: Chengwen Feng <fengchengwen@huawei.com>
> Reviewed-by: Chengchang Tang <tangchengchang@huawei.com>
> ---
> v4->v5:
> Modifies FEC capa definitions using macros.
> Add RTE_ prefix for public FEC mode enum.
> add release notes about FEC for dpdk20_11.
> 
> ---
> v2->v3:
> add function return value "-ENOTSUP" for API
> 
> ---
>  doc/guides/rel_notes/release_20_11.rst   | 13 +++++
>  lib/librte_ethdev/rte_ethdev.c           | 50 +++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev.h           | 83 ++++++++++++++++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev_core.h      | 77 +++++++++++++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev_version.map |  5 ++
>  5 files changed, 228 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
> index cc72609..e4f0587 100644
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -55,6 +55,19 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =======================================================
>  
> +* **Added the FEC Library, a generic FEC query and config library.**
> +
> +  Added the FEC library which provides an API for query FEC capabilities and
> +  FEC mode from device. Also, API for configuring FEC mode is also provided.

The patch does not add a library. It adds an API get FEC
capabilities, get current configuration and allows to set
a new configuration.

> +
> +  Added hns3 FEC PMD, for supporting query and config FEC mode.

If required, it should be later in release notes in accordance
with defined order.

> +
> +* **Updated testpmd with a command for FEC.**
> +
> +  Added a FEC command to testpmd app,
> +  ``show port <port_id> fec capabilities`` which queries FEC capabilities device supports.
> +  ``show port <port_id> fec_mode`` which queries FEC mode from device.
> +  ``set port <port_id> fec_mode <auto|off|rs|baser>`` which configures FEC mode to device.

IMHO, it is not much details for release notes.
As I understand it is assumed that testpmd must be
updated for any new ethdev feature, so, the information
about testpmd is not really required.

>  
>  Removed Items
>  -------------
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 7858ad5..fde77c1 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -3642,6 +3642,56 @@ rte_eth_led_off(uint16_t port_id)
>  	return eth_err(port_id, (*dev->dev_ops->dev_led_off)(dev));
>  }
>  
> +int
> +rte_eth_fec_get_capability(uint16_t port_id, uint32_t *fec_cap)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->fec_get_capability, -ENOTSUP);
> +	return eth_err(port_id, (*dev->dev_ops->fec_get_capability)(dev,
> +								fec_cap));
> +}
> +
> +int
> +rte_eth_fec_get(uint16_t port_id, enum rte_fec_mode *mode)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->fec_get, -ENOTSUP);
> +	return eth_err(port_id, (*dev->dev_ops->fec_get)(dev, mode));
> +}
> +
> +int
> +rte_eth_fec_set(uint16_t port_id, enum rte_fec_mode mode)
> +{
> +	struct rte_eth_dev *dev;
> +	uint32_t fec_mode_mask;
> +	int ret;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);

Basically the check is duplicated since get_capabilities
does it. However, there is no much harm to keep. If so,
I'd move it below just before port_id usage as an index
in rte_eth_devices array.

> +
> +	ret = rte_eth_fec_get_capability(port_id, &fec_mode_mask);
> +	if (ret)

Please, compare with 0 explicitly as specified in DPDK coding
style.

> +		return ret;
> +
> +	/*
> +	 * Check whether the configured mode is within the FEC capability.
> +	 * If not, the configured mode will not be supported.
> +	 */
> +	if (!(fec_mode_mask & (1U << (uint32_t)mode))) {
> +		RTE_ETHDEV_LOG(ERR, "unsupported fec mode=%d\n", mode);

I'd use "FEC" (in capitals) in log message as well.
Also I think it would be useful to log port_id.
.
> +		return -EINVAL;
> +	}
> +
> +	dev = &rte_eth_devices[port_id];
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->fec_set, -ENOTSUP);
> +	return eth_err(port_id, (*dev->dev_ops->fec_set)(dev, mode));
> +}
> +
>  /*
>   * Returns index into MAC address array of addr. Use 00:00:00:00:00:00 to find
>   * an empty spot.
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 70295d7..aa79326 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1511,6 +1511,25 @@ struct rte_eth_dcb_info {
>  	struct rte_eth_dcb_tc_queue_mapping tc_queue;
>  };
>  
> +/**
> + * This enum indicates the possible (forward error correction)FEC modes
> + * of an ethdev port.
> + */
> +enum rte_fec_mode {

enum rte_eth_fec_mode
to be consistent with other enums and to have library prefix.

> +	RTE_ETH_FEC_NOFEC = 0,      /**< FEC is off */
> +	RTE_ETH_FEC_BASER,          /**< FEC using common algorithm */
> +	RTE_ETH_FEC_RS,             /**< FEC using RS algorithm */
> +	RTE_ETH_FEC_AUTO,           /**< FEC autonegotiation modes */

May I suggest to put AUTO just after NOFEC. It would look move
logical in the future when more FEC modes are added. It would
look strange with AUTO in the middle of the list.

> +	RTE_ETH_FEC_NUM

I'm not about about current policy for such items. Is it really
required? Addition of more FEC modes will be ABI breakage.

> +};
> +
> +/* This indicates FEC capabilities */
> +#define RTE_ETH_FEC_CAPA_NOFEC  (1U << RTE_ETH_FEC_NOFEC)
> +#define RTE_ETH_FEC_CAPA_BASER  (1U << RTE_ETH_FEC_BASER)
> +#define RTE_ETH_FEC_CAPA_RS     (1U << RTE_ETH_FEC_RS)
> +#define RTE_ETH_FEC_CAPA_AUTO   (1U << RTE_ETH_FEC_AUTO)
> +
> +
>  #define RTE_ETH_ALL RTE_MAX_ETHPORTS
>  
>  /* Macros to check for valid port */
> @@ -3328,6 +3347,70 @@ int  rte_eth_led_on(uint16_t port_id);
>  int  rte_eth_led_off(uint16_t port_id);
>  
>  /**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Get Forward Error Correction(FEC) capability.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param fec_cap
> + *   returns the FEC capability from the device, as follows:
> + *   RTE_ETH_FEC_CAPA_NOFEC
> + *   RTE_ETH_FEC_CAPA_BASER
> + *   RTE_ETH_FEC_CAPA_RS
> + *   RTE_ETH_FEC_CAPA_AUTO

I'd like to see thoughts about different FEC modes for
different link speed. How to report it?
(sorry if I missed it before)

> + * @return
> + *   - (0) if successful.
> + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
> + *     that operation.
> + *   - (-EIO) if device is removed.
> + *   - (-ENODEV)  if *port_id* invalid.
> + */
> +__rte_experimental
> +int rte_eth_fec_get_capability(uint16_t port_id, uint32_t *fec_cap);

[snip]