[V5,1/3] ethdev: introduce FEC API
Checks
Commit Message
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(+)
Comments
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]
>
> 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.
> +
> + Added hns3 FEC PMD, for supporting query and config FEC mode.
> +
> +* **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.
>
> 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);
> +
> + ret = rte_eth_fec_get_capability(port_id, &fec_mode_mask);
> + if (ret)
> + 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);
> + 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 {
> + 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 */
> + RTE_ETH_FEC_NUM
It is better not to have RTE_ETH_FEC_NUM here:
Any future additions to that enum would overwrite _NUM values and would
considered as ABI breakage.
Aprart from that:
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> +};
> +
> +/* 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
> + * @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);
> +
> +/**
> + * @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_fec_mode *mode);
> +
> +/**
> + * @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_fec_mode mode);
> +
> +/**
> * Get current status of the Ethernet link flow control for Ethernet device
> *
> * @param port_id
> diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
> index 32407dd..cd1acf1 100644
> --- a/lib/librte_ethdev/rte_ethdev_core.h
> +++ b/lib/librte_ethdev/rte_ethdev_core.h
> @@ -604,6 +604,76 @@ typedef int (*eth_tx_hairpin_queue_setup_t)
> const struct rte_eth_hairpin_conf *hairpin_conf);
>
> /**
> + * @internal
> + * Get Forward Error Correction(FEC) capability.
> + *
> + * @param dev
> + * ethdev handle of port.
> + * @param fec_cap
> + * returns the FEC capability from the device.
> + *
> + * @return
> + * Negative errno value on error, 0 on success.
> + *
> + * @retval 0
> + * Success, get FEC success.
> + * @retval -ENOTSUP
> + * operation is not supported.
> + * @retval -EIO
> + * device is removed.
> + * @retval -ENODEV
> + * Device is gone.
> + */
> +typedef int (*eth_fec_get_capability_t)(struct rte_eth_dev *dev,
> + uint32_t *fec_cap);
> +
> +/**
> + * @internal
> + * Get Forward Error Correction(FEC) mode.
> + *
> + * @param dev
> + * ethdev handle of port.
> + * @param mode
> + * returns the FEC mode from the device.
> + *
> + * @return
> + * Negative errno value on error, 0 on success.
> + *
> + * @retval 0
> + * Success, get FEC success.
> + * @retval -ENOTSUP
> + * operation is not supported.
> + * @retval -EIO
> + * device is removed.
> + * @retval -ENODEV
> + * Device is gone.
> + */
> +typedef int (*eth_fec_get_t)(struct rte_eth_dev *dev, enum rte_fec_mode *mode);
> +
> +/**
> + * @internal
> + * Set Forward Error Correction(FEC) mode.
> + *
> + * @param dev
> + * ethdev handle of port.
> + * @param mode
> + * the FEC mode.
> + *
> + * @return
> + * Negative errno value on error, 0 on success.
> + *
> + * @retval 0
> + * Success, set FEC success.
> + * @retval -ENOTSUP
> + * operation is not supported.
> + * @retval -EIO
> + * device is removed.
> + * @retval -ENODEV
> + * Device is gone.
> + */
> +typedef int (*eth_fec_set_t)(struct rte_eth_dev *dev, enum rte_fec_mode mode);
> +
> +/**
> * @internal A structure containing the functions exported by an Ethernet driver.
> */
> struct eth_dev_ops {
> @@ -752,6 +822,13 @@ struct eth_dev_ops {
> /**< Set up device RX hairpin queue. */
> eth_tx_hairpin_queue_setup_t tx_hairpin_queue_setup;
> /**< Set up device TX hairpin queue. */
> +
> + eth_fec_get_capability_t fec_get_capability;
> + /**< Get Forward Error Correction(FEC) capability; */
> + eth_fec_get_t fec_get;
> + /**< Get Forward Error Correction(FEC) mode; */
> + eth_fec_set_t fec_set;
> + /**< Set Forward Error Correction(FEC) mode; */
> };
>
> /**
> diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
> index 02081d9..9f3fb8f 100644
> --- a/lib/librte_ethdev/rte_ethdev_version.map
> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> @@ -239,6 +239,11 @@ EXPERIMENTAL {
> __rte_ethdev_trace_rx_burst;
> __rte_ethdev_trace_tx_burst;
> rte_flow_get_aged_flows;
> +
> + # added in 20.11
> + rte_eth_fec_get_capability;
> + rte_eth_fec_get;
> + rte_eth_fec_set;
> };
>
> INTERNAL {
> --
> 2.7.4
在 2020/9/17 20:49, Ananyev, Konstantin 写道:
>
>>
>> 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.
>> +
>> + Added hns3 FEC PMD, for supporting query and config FEC mode.
>> +
>> +* **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.
>>
>> 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);
>> +
>> + ret = rte_eth_fec_get_capability(port_id, &fec_mode_mask);
>> + if (ret)
>> + 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);
>> + 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 {
>> + 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 */
>> + RTE_ETH_FEC_NUM
>
> It is better not to have RTE_ETH_FEC_NUM here:
> Any future additions to that enum would overwrite _NUM values and would
> considered as ABI breakage.
> Aprart from that:
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>
HI,
it does not matter even though RTE_ETH_FEC_NUM value is changed
when adding new element to that enum. RTE_ETH_FEC_NUM is used as the MAX
vlaue of FEC modes, not one FEC mode itself.
One of the application scenarios is as follows,set "testpmd" as
an example:
show_fec_capability(uint32_t fec_cap)
{
uint32_t i;
if (fec_cap == 0) {
printf("FEC is not supported\n");
return;
}
printf("FEC capabilities: ");
for (i = RTE_ETH_FEC_BASER; i < RTE_ETH_FEC_NUM; i++) {
if (fec_cap & 1U << i)
printf("%s ", fec_mode_name[i].name);
}
printf("\n");
}
Hope for your reply.
>> +};
>> +
>> +/* 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
>> + * @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);
>> +
>> +/**
>> + * @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_fec_mode *mode);
>> +
>> +/**
>> + * @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_fec_mode mode);
>> +
>> +/**
>> * Get current status of the Ethernet link flow control for Ethernet device
>> *
>> * @param port_id
>> diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
>> index 32407dd..cd1acf1 100644
>> --- a/lib/librte_ethdev/rte_ethdev_core.h
>> +++ b/lib/librte_ethdev/rte_ethdev_core.h
>> @@ -604,6 +604,76 @@ typedef int (*eth_tx_hairpin_queue_setup_t)
>> const struct rte_eth_hairpin_conf *hairpin_conf);
>>
>> /**
>> + * @internal
>> + * Get Forward Error Correction(FEC) capability.
>> + *
>> + * @param dev
>> + * ethdev handle of port.
>> + * @param fec_cap
>> + * returns the FEC capability from the device.
>> + *
>> + * @return
>> + * Negative errno value on error, 0 on success.
>> + *
>> + * @retval 0
>> + * Success, get FEC success.
>> + * @retval -ENOTSUP
>> + * operation is not supported.
>> + * @retval -EIO
>> + * device is removed.
>> + * @retval -ENODEV
>> + * Device is gone.
>> + */
>> +typedef int (*eth_fec_get_capability_t)(struct rte_eth_dev *dev,
>> + uint32_t *fec_cap);
>> +
>> +/**
>> + * @internal
>> + * Get Forward Error Correction(FEC) mode.
>> + *
>> + * @param dev
>> + * ethdev handle of port.
>> + * @param mode
>> + * returns the FEC mode from the device.
>> + *
>> + * @return
>> + * Negative errno value on error, 0 on success.
>> + *
>> + * @retval 0
>> + * Success, get FEC success.
>> + * @retval -ENOTSUP
>> + * operation is not supported.
>> + * @retval -EIO
>> + * device is removed.
>> + * @retval -ENODEV
>> + * Device is gone.
>> + */
>> +typedef int (*eth_fec_get_t)(struct rte_eth_dev *dev, enum rte_fec_mode *mode);
>> +
>> +/**
>> + * @internal
>> + * Set Forward Error Correction(FEC) mode.
>> + *
>> + * @param dev
>> + * ethdev handle of port.
>> + * @param mode
>> + * the FEC mode.
>> + *
>> + * @return
>> + * Negative errno value on error, 0 on success.
>> + *
>> + * @retval 0
>> + * Success, set FEC success.
>> + * @retval -ENOTSUP
>> + * operation is not supported.
>> + * @retval -EIO
>> + * device is removed.
>> + * @retval -ENODEV
>> + * Device is gone.
>> + */
>> +typedef int (*eth_fec_set_t)(struct rte_eth_dev *dev, enum rte_fec_mode mode);
>> +
>> +/**
>> * @internal A structure containing the functions exported by an Ethernet driver.
>> */
>> struct eth_dev_ops {
>> @@ -752,6 +822,13 @@ struct eth_dev_ops {
>> /**< Set up device RX hairpin queue. */
>> eth_tx_hairpin_queue_setup_t tx_hairpin_queue_setup;
>> /**< Set up device TX hairpin queue. */
>> +
>> + eth_fec_get_capability_t fec_get_capability;
>> + /**< Get Forward Error Correction(FEC) capability; */
>> + eth_fec_get_t fec_get;
>> + /**< Get Forward Error Correction(FEC) mode; */
>> + eth_fec_set_t fec_set;
>> + /**< Set Forward Error Correction(FEC) mode; */
>> };
>>
>> /**
>> diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
>> index 02081d9..9f3fb8f 100644
>> --- a/lib/librte_ethdev/rte_ethdev_version.map
>> +++ b/lib/librte_ethdev/rte_ethdev_version.map
>> @@ -239,6 +239,11 @@ EXPERIMENTAL {
>> __rte_ethdev_trace_rx_burst;
>> __rte_ethdev_trace_tx_burst;
>> rte_flow_get_aged_flows;
>> +
>> + # added in 20.11
>> + rte_eth_fec_get_capability;
>> + rte_eth_fec_get;
>> + rte_eth_fec_set;
>> };
>>
>> INTERNAL {
>> --
>> 2.7.4
>
> .
>
在 2020/9/17 17:58, Andrew Rybchenko 写道:
> 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.
I fixed it in V6，thanks.
>
>> +
>> + 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.
>
I fixed it in V6，thanks.
>> +
>> +* **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.
>
I fixed it in V6，thanks.
>>
>> 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.
>
I fixed it in V6，just delete the duplicated check, thanks.
>> +
>> + ret = rte_eth_fec_get_capability(port_id, &fec_mode_mask);
>> + if (ret)
>
> Please, compare with 0 explicitly as specified in DPDK coding
> style.
>
I fixed it in V6，thanks.
>> + 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.
I fixed it in V6，thanks.
> .
>> + 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.
>
I fixed it in V6，thanks.
>> + 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.
>
hns3 PMD is the first to implement FEC function. The FEC mode order
is in accordance with the bit definition in hardware FEC module.
in other ways, AUTO is flexible mode, which should not be differently
treated,the location in last item of enum is ok.
>> + 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.
it does not matter even though RTE_ETH_FEC_NUM value is changed when
adding new element to that enum.
RTE_ETH_FEC_NUM is used as the MAX vlaue of FEC modes, not one FEC
mode itself.
One of the application scenarios is as follows,set "testpmd" as an
example:
show_fec_capability(uint32_t fec_cap)
{
uint32_t i;
if (fec_cap == 0) {
printf("FEC is not supported\n");
return;
}
printf("FEC capabilities: ");
for (i = RTE_ETH_FEC_BASER; i < RTE_ETH_FEC_NUM; i++) {
if (fec_cap & 1U << i)
printf("%s ", fec_mode_name[i].name);
}
printf("\n");
}
>> +};
>> +
>> +/* 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)
the relation between FEC mode and link speed, we can refer to
hns3 PMD code, as follows:
switch (mac->link_speed) {
case ETH_SPEED_NUM_10G:
case ETH_SPEED_NUM_40G:
mode = RTE_ETH_FEC_CAPA_NOFEC | RTE_ETH_FEC_CAPA_BASER |
RTE_ETH_FEC_CAPA_AUTO;
break;
case ETH_SPEED_NUM_25G:
case ETH_SPEED_NUM_50G:
mode = RTE_ETH_FEC_CAPA_NOFEC | RTE_ETH_FEC_CAPA_BASER |
RTE_ETH_FEC_CAPA_RS | RTE_ETH_FEC_CAPA_AUTO;
break;
case ETH_SPEED_NUM_100G:
case ETH_SPEED_NUM_200G:
mode = RTE_ETH_FEC_CAPA_NOFEC | RTE_ETH_FEC_CAPA_RS |
RTE_ETH_FEC_CAPA_AUTO;
break;
default:
mode = 0;
break;
}
>
>> + * @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]
> .
>
> >>
> >> +/**
> >> + * This enum indicates the possible (forward error correction)FEC modes
> >> + * of an ethdev port.
> >> + */
> >> +enum rte_fec_mode {
> >> + 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 */
> >> + RTE_ETH_FEC_NUM
> >
> > It is better not to have RTE_ETH_FEC_NUM here:
> > Any future additions to that enum would overwrite _NUM values and would
> > considered as ABI breakage.
> > Aprart from that:
> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> >
> HI,
> it does not matter even though RTE_ETH_FEC_NUM value is changed
> when adding new element to that enum. RTE_ETH_FEC_NUM is used as the MAX
> vlaue of FEC modes, not one FEC mode itself.
I understand that, but when in future you'll try to add some new mode,
it will cause a change of RTE_ETH_FEC_NUM value.
As I remember, abicheck will report it as ABI breakage.
So adding new values to that enum will be really problematic.
> One of the application scenarios is as follows,set "testpmd" as
> an example:
> show_fec_capability(uint32_t fec_cap)
> {
> uint32_t i;
>
> if (fec_cap == 0) {
> printf("FEC is not supported\n");
> return;
> }
>
> printf("FEC capabilities: ");
> for (i = RTE_ETH_FEC_BASER; i < RTE_ETH_FEC_NUM; i++) {
I think you can use RTE_DIM(fec_mode_name) here instead of RTE_ETH_FEC_NUM.
> if (fec_cap & 1U << i)
BTW, one more thing - as you translate from mode to capa all over the place,
I think it deserves a separate macro for it.
Something like:
#define RTE_ETH_FEC_MODE_TO_CAPA(x) (1U << (x))
> printf("%s ", fec_mode_name[i].name);
> }
> printf("\n");
> }
>
> Hope for your reply.
>
> >> +};
> >> +
> >> +/* 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)
> >> +
> >> +
On 9/18/20 12:28 PM, Min Hu (Connor) wrote:
>
>
> 在 2020/9/17 17:58, Andrew Rybchenko 写道:
>> 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
[snip]
>>> 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.
>>
> I fixed it in V6，thanks.
>>> + 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.
>>
> hns3 PMD is the first to implement FEC function. The FEC mode order
> is in accordance with the bit definition in hardware FEC module.
Sorry, it is not a good justification.
It does not matter who is the first and who will be the last.
> in other ways, AUTO is flexible mode, which should not be differently
> treated,the location in last item of enum is ok.
I still disagree. If it is OK for other ethdev maintainer - no problem,
but I'd like to see explicit confirmation.
[snip]
>>> @@ -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)
> the relation between FEC mode and link speed, we can refer to
> hns3 PMD code, as follows:
>
> switch (mac->link_speed) {
> case ETH_SPEED_NUM_10G:
> case ETH_SPEED_NUM_40G:
> mode = RTE_ETH_FEC_CAPA_NOFEC | RTE_ETH_FEC_CAPA_BASER |
> RTE_ETH_FEC_CAPA_AUTO;
> break;
> case ETH_SPEED_NUM_25G:
> case ETH_SPEED_NUM_50G:
> mode = RTE_ETH_FEC_CAPA_NOFEC | RTE_ETH_FEC_CAPA_BASER |
> RTE_ETH_FEC_CAPA_RS | RTE_ETH_FEC_CAPA_AUTO;
> break;
> case ETH_SPEED_NUM_100G:
> case ETH_SPEED_NUM_200G:
> mode = RTE_ETH_FEC_CAPA_NOFEC | RTE_ETH_FEC_CAPA_RS |
> RTE_ETH_FEC_CAPA_AUTO;
> break;
> default:
> mode = 0;
> break;
> }
I'm sorry, but it does not answer my question.
I think we should change the API to report supported FEC modes per
supported link speed.
>>> + * @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]
>> .
>>
在 2020/9/19 16:42, Andrew Rybchenko 写道:
> On 9/18/20 12:28 PM, Min Hu (Connor) wrote:
>>
>>
>> 在 2020/9/17 17:58, Andrew Rybchenko 写道:
>>> 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
>
> [snip]
>
>>>> 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.
>>>
>> I fixed it in V6，thanks.
>>>> + 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.
>>>
>> hns3 PMD is the first to implement FEC function. The FEC mode order
>> is in accordance with the bit definition in hardware FEC module.
>
> Sorry, it is not a good justification.
> It does not matter who is the first and who will be the last.
>
>> in other ways, AUTO is flexible mode, which should not be
>> differently treated,the location in last item of enum is ok.
>
> I still disagree. If it is OK for other ethdev maintainer - no problem,
> but I'd like to see explicit confirmation.
OK,thanks, I will try to fix it as you suggested.
>
> [snip]
>
>>>> @@ -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)
>> the relation between FEC mode and link speed, we can refer to
>> hns3 PMD code, as follows:
>>
>> switch (mac->link_speed) {
>> case ETH_SPEED_NUM_10G:
>> case ETH_SPEED_NUM_40G:
>> mode = RTE_ETH_FEC_CAPA_NOFEC | RTE_ETH_FEC_CAPA_BASER |
>> RTE_ETH_FEC_CAPA_AUTO;
>> break;
>> case ETH_SPEED_NUM_25G:
>> case ETH_SPEED_NUM_50G:
>> mode = RTE_ETH_FEC_CAPA_NOFEC | RTE_ETH_FEC_CAPA_BASER |
>> RTE_ETH_FEC_CAPA_RS | RTE_ETH_FEC_CAPA_AUTO;
>> break;
>> case ETH_SPEED_NUM_100G:
>> case ETH_SPEED_NUM_200G:
>> mode = RTE_ETH_FEC_CAPA_NOFEC | RTE_ETH_FEC_CAPA_RS |
>> RTE_ETH_FEC_CAPA_AUTO;
>> break;
>> default:
>> mode = 0;
>> break;
>> }
>
> I'm sorry, but it does not answer my question.
> I think we should change the API to report supported FEC modes per
> supported link speed.
Sorry, maybe I do not get your meaning. By this API
(rte_eth_fec_get_capability)，we can get FEC capabilityes from device.
we (as user )do not care about the link speed, because this is done in PMD.
So, how to change the API. Could your make it more clearer?
Hope for your reply.
>
>>>> + * @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]
>>> .
>>>
>
> .
@@ -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.
+
+ Added hns3 FEC PMD, for supporting query and config FEC mode.
+
+* **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.
Removed Items
-------------
@@ -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);
+
+ ret = rte_eth_fec_get_capability(port_id, &fec_mode_mask);
+ if (ret)
+ 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);
+ 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.
@@ -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 {
+ 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 */
+ RTE_ETH_FEC_NUM
+};
+
+/* 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
+ * @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);
+
+/**
+ * @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_fec_mode *mode);
+
+/**
+ * @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_fec_mode mode);
+
+/**
* Get current status of the Ethernet link flow control for Ethernet device
*
* @param port_id
@@ -604,6 +604,76 @@ typedef int (*eth_tx_hairpin_queue_setup_t)
const struct rte_eth_hairpin_conf *hairpin_conf);
/**
+ * @internal
+ * Get Forward Error Correction(FEC) capability.
+ *
+ * @param dev
+ * ethdev handle of port.
+ * @param fec_cap
+ * returns the FEC capability from the device.
+ *
+ * @return
+ * Negative errno value on error, 0 on success.
+ *
+ * @retval 0
+ * Success, get FEC success.
+ * @retval -ENOTSUP
+ * operation is not supported.
+ * @retval -EIO
+ * device is removed.
+ * @retval -ENODEV
+ * Device is gone.
+ */
+typedef int (*eth_fec_get_capability_t)(struct rte_eth_dev *dev,
+ uint32_t *fec_cap);
+
+/**
+ * @internal
+ * Get Forward Error Correction(FEC) mode.
+ *
+ * @param dev
+ * ethdev handle of port.
+ * @param mode
+ * returns the FEC mode from the device.
+ *
+ * @return
+ * Negative errno value on error, 0 on success.
+ *
+ * @retval 0
+ * Success, get FEC success.
+ * @retval -ENOTSUP
+ * operation is not supported.
+ * @retval -EIO
+ * device is removed.
+ * @retval -ENODEV
+ * Device is gone.
+ */
+typedef int (*eth_fec_get_t)(struct rte_eth_dev *dev, enum rte_fec_mode *mode);
+
+/**
+ * @internal
+ * Set Forward Error Correction(FEC) mode.
+ *
+ * @param dev
+ * ethdev handle of port.
+ * @param mode
+ * the FEC mode.
+ *
+ * @return
+ * Negative errno value on error, 0 on success.
+ *
+ * @retval 0
+ * Success, set FEC success.
+ * @retval -ENOTSUP
+ * operation is not supported.
+ * @retval -EIO
+ * device is removed.
+ * @retval -ENODEV
+ * Device is gone.
+ */
+typedef int (*eth_fec_set_t)(struct rte_eth_dev *dev, enum rte_fec_mode mode);
+
+/**
* @internal A structure containing the functions exported by an Ethernet driver.
*/
struct eth_dev_ops {
@@ -752,6 +822,13 @@ struct eth_dev_ops {
/**< Set up device RX hairpin queue. */
eth_tx_hairpin_queue_setup_t tx_hairpin_queue_setup;
/**< Set up device TX hairpin queue. */
+
+ eth_fec_get_capability_t fec_get_capability;
+ /**< Get Forward Error Correction(FEC) capability; */
+ eth_fec_get_t fec_get;
+ /**< Get Forward Error Correction(FEC) mode; */
+ eth_fec_set_t fec_set;
+ /**< Set Forward Error Correction(FEC) mode; */
};
/**
@@ -239,6 +239,11 @@ EXPERIMENTAL {
__rte_ethdev_trace_rx_burst;
__rte_ethdev_trace_tx_burst;
rte_flow_get_aged_flows;
+
+ # added in 20.11
+ rte_eth_fec_get_capability;
+ rte_eth_fec_get;
+ rte_eth_fec_set;
};
INTERNAL {