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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 9/21/20 9:13 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>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

[snip]

> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 70295d7..7d5e81b 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1310,6 +1310,9 @@ struct rte_eth_conf {
>  #define RTE_ETH_DEV_FALLBACK_RX_NBQUEUES 1
>  #define RTE_ETH_DEV_FALLBACK_TX_NBQUEUES 1
>  
> +/* Translate from FEC mode to FEC capa */
> +#define RTE_ETH_FEC_MODE_TO_CAPA(x)	(1U << (x))
> +

It should not be so far from rte_eth_fec_mode. Please, add just
after it.

May be it should be:
#define RTE_ETH_FEC_MODE_TO_CAPA(x) (1U << (RTE_ETH_FEC_ ## x))

>  /**
>   * Preferred Rx/Tx port parameters.
>   * There are separate instances of this structure for transmission
> @@ -1511,6 +1514,24 @@ 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_eth_fec_mode {
> +	RTE_ETH_FEC_NOFEC = 0,      /**< FEC is off */
> +	RTE_ETH_FEC_AUTO,	    /**< FEC autonegotiation modes */
> +	RTE_ETH_FEC_BASER,          /**< FEC using common algorithm */
> +	RTE_ETH_FEC_RS,             /**< FEC using RS algorithm */
> +};
> +
> +/* This indicates FEC capabilities */
> +#define RTE_ETH_FEC_CAPA_NOFEC  (1U << RTE_ETH_FEC_NOFEC)
> +#define RTE_ETH_FEC_CAPA_AUTO   (1U << RTE_ETH_FEC_AUTO)
> +#define RTE_ETH_FEC_CAPA_BASER  (1U << RTE_ETH_FEC_BASER)
> +#define RTE_ETH_FEC_CAPA_RS     (1U << RTE_ETH_FEC_RS)

Shouldn't RTE_ETH_FEC_MODE_TO_CAPA be used as definition
values?

> +
> +
>  #define RTE_ETH_ALL RTE_MAX_ETHPORTS
>  
>  /* Macros to check for valid port */
> @@ -3328,6 +3349,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_AUTO
> + *   RTE_ETH_FEC_CAPA_BASER
> + *   RTE_ETH_FEC_CAPA_RS
> + * @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);

The API does not allow to report capabilities per link speed:
which FEC mode is supported at which link speed?

What about something like:

struct rte_eth_fec_capa {
  uint32_t speed; /**< Link speed (see ETH_SPEED_NUM_*) */
  uint32_t capa;  /**< FEC capabilities bitmask (see RTE_FEC_CAPA_*) */
};

__rte_experimental
int rte_eth_fec_get_capability(uint16_t port_id, uint32_t *num, struct
rte_eth_fec_capa *speed_capa);

where:
 - num is in/out with a number of elements in an array
 - speed_capa is out only with per-speed capabilities

> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Get current Forward Error Correction(FEC) mode.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param mode
> + *   returns the FEC mode from the device.
> + * @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(uint16_t port_id, enum rte_eth_fec_mode *mode);

Please, specify what should be reported if link is down.
E.g. if set to RS, but link is down.

Does AUTO make sense here?

> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Set Forward Error Correction(FEC) mode.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param mode
> + *   the FEC mode.
> + * @return
> + *   - (0) if successful.
> + *   - (-EINVAL) if the FEC mode is not valid.
> + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
> + *   - (-EIO) if device is removed.
> + *   - (-ENODEV)  if *port_id* invalid.
> + */
> +__rte_experimental
> +int rte_eth_fec_set(uint16_t port_id, enum rte_eth_fec_mode mode);

It does not allow to tweak autoneg facilities.
E.g. "I know that RS is buggy, so I want to exclude it from
auto-negotiation".
So, I suggest to change mode to capa bitmask.
If AUTO is set, other bits may be set and specify allowed
options. E.g. AUTO|RS|BASER will require FEC, i.e. NOFEC is
not allowed. If just RS, it means that auto-negotiation is
disabled and RS must be used.
If AUTO is unset, only one bit may be set in capabilities.
Since we don't do it per speed, I think it is safe to ignore
unsupported mode bits. I.e. do not return error if unsupported
capa is requested to together with AUTO, however it could be
a problem if no modes are allowed for negotiated link speed.
Thoughts are welcome.

> +
> +/**
>   * Get current status of the Ethernet link flow control for Ethernet device
>   *
>   * @param port_id

[snip]