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) <humi...@huawei.com>
> Reviewed-by: Wei Hu (Xavier) <xavier.hu...@huawei.com>
> Reviewed-by: Chengwen Feng <fengcheng...@huawei.com>
> Reviewed-by: Chengchang Tang <tangchengch...@huawei.com>
> Reviewed-by: Ajit Khaparde <ajit.khapa...@broadcom.com>
> Acked-by: Konstantin Ananyev <konstantin.anan...@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]
