Re: [dpdk-dev] [PATCH V12 1/4] ethdev: introduce FEC API

[Ajit Khaparde <ajit.khaparde@broadcom.com>]

On Fri, Sep 25, 2020 at 1:47 AM Min Hu (Connor) <humin29@huawei.com> wrote:
>
> HI，Andrew，
>         I fix it in V13 according to your advice.
>         Thanks for  your patient review. Please check it out again.
>
> By the way, there is always a coding check warning, like this:
>
> WARNING:PREFER_FALLTHROUGH: Prefer 'fallthrough;' over fallthrough comment
> #264: FILE: drivers/net/hns3/hns3_ethdev.c:5601:
> +       /* fallthrough */
Try /* FALLTHROUGH */

>
> total: 0 errors, 1 warnings, 0 checks, 439 lines checked.
>
> I have tried some ways, but it does not help.
> Could you giver some advice?
>         thanks.
>
>
> 在 2020/9/24 22:46, Andrew Rybchenko 写道:
> > On 9/24/20 4:05 PM, 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.
> >
> > Almost good now. See my notes below.
> > Many thanks for hard work and patience.
> >
> >>
> >> 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>
> >
> > I think that tags for Ajit and Konstantin should be
> > dropped, since API changes significantly after their review.
> >
> >> ---
> >> v10->v11:
> >> allow to report capabilities per link speed.
> >> specify what should be reported if link is down
> >> when get FEC.
> >> change mode to capa bitmask.
> >>
> >> ---
> >> v9->v10:
> >> add macro RTE_ETH_FEC_MODE_CAPA_MASK(x) to indicate
> >> different FEC mode capa.
> >>
> >> ---
> >> v8->v9:
> >> added reviewed-by and acked-by.
> >>
> >> ---
> >> v7->v8:
> >> put AUTO just after NOFEC in rte_fec_mode definition.
> >>
> >> ---
> >> v6->v7:
> >> deleted RTE_ETH_FEC_NUM to prevent ABI breakage.
> >> add new macro to indicate translation from fec mode
> >> to capa.
> >>
> >> ---
> >> v5->v6:
> >> modified release notes.
> >> deleted check duplicated for FEC API
> >> fixed code styles according to DPDK coding style.
> >> added _eth prefix.
> >>
> >> ---
> >> 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.
> >>
> >> ---
> >>   lib/librte_ethdev/rte_ethdev.c           | 37 +++++++++++++
> >>   lib/librte_ethdev/rte_ethdev.h           | 91 ++++++++++++++++++++++++++++++++
> >>   lib/librte_ethdev/rte_ethdev_driver.h    | 82 ++++++++++++++++++++++++++++
> >>   lib/librte_ethdev/rte_ethdev_version.map |  3 ++
> >>   4 files changed, 213 insertions(+)
> >>
> >> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> >> index dfe5c1b..b614bfc 100644
> >> --- a/lib/librte_ethdev/rte_ethdev.c
> >> +++ b/lib/librte_ethdev/rte_ethdev.c
> >> @@ -3679,6 +3679,43 @@ 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 *num,
> >> +                       struct rte_eth_fec_capa *speed_fec_capa)
> >> +{
> >> +    struct rte_eth_dev *dev;
> >> +
> >> +    if (num == NULL || speed_fec_capa == NULL)
> >> +            return -EINVAL;
> >
> > I think it is OK to have speed_fec_cap==NULL if *num is 0.
> > I.e. a request to get number of required array entries.
> >
> >> +
> >> +    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, num,
> >> +                                                    speed_fec_capa));
> >> +}
> >> +
> >> +int
> >> +rte_eth_fec_get(uint16_t port_id, uint32_t *mode)
> >> +{
> >> +    struct rte_eth_dev *dev;
> >
> > I think it would be good to check that mode is not NULL here.
> >
> >> +
> >> +    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, uint32_t mode)
> >> +{
> >> +    struct rte_eth_dev *dev;
> >> +
> >> +    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 645a186..104181d 100644
> >> --- a/lib/librte_ethdev/rte_ethdev.h
> >> +++ b/lib/librte_ethdev/rte_ethdev.h
> >> @@ -1544,6 +1544,29 @@ struct rte_eth_dcb_info {
> >>      struct rte_eth_dcb_tc_queue_mapping tc_queue;
> >>   };
> >>
> >> +/**
> >> + * This enum indicates the possible (forward error correction)FEC modes
> >
> > (forward error correction)FEC -> Forward Error Correction (FEC)
> >
> >> + * 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 */
> >> +};
> >> +
> >> +/* Translate from FEC mode to FEC capa */
> >> +#define RTE_ETH_FEC_MODE_TO_CAPA(x) (1U << (x))
> >> +
> >> +/* This macro indicates FEC capa mask*/
> >
> > Add missing space before */
> >
> >> +#define RTE_ETH_FEC_MODE_CAPA_MASK(x)       (1U << (RTE_ETH_FEC_ ## x))
> >> +
> >> +/* A structure used to get capabilities per link speed */
> >> +struct rte_eth_fec_capa {
> >> +    uint32_t speed; /**< Link speed (see ETH_SPEED_NUM_*) */
> >> +    uint32_t capa;  /**< FEC capabilities bitmask (see RTE_FEC_CAPA_*) */
> >> +};
> >> +
> >>   #define RTE_ETH_ALL RTE_MAX_ETHPORTS
> >>
> >>   /* Macros to check for valid port */
> >> @@ -3397,6 +3420,74 @@ 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 num
> >> + *   the num is in/out with a number of elements in an array.
> >
> > Please, see below my notes on callback description.
> >
> >> + * @param speed_fec_capa
> >> + *   speed_fec_capa is out only with per-speed capabilities.
> >> + *
> >> + * @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.
> >> + *   - (-EINVAL)  if *num* or *speed_fec_capa* invalid
> >> + */
> >> +__rte_experimental
> >> +int rte_eth_fec_get_capability(uint16_t port_id, uint32_t *num,
> >> +                    struct rte_eth_fec_capa *speed_fec_capa);
> >> +
> >> +/**
> >> + * @warning
> >> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> >> + *
> >> + * Get current Forward Error Correction(FEC) mode.
> >> + * If link is down and AUTO is enabled, AUTO is returned, otherwise,
> >> + * configured FEC mode is returned.
> >> + * If link is up, current FEC mode is returned.
> >> + *
> >> + * @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, uint32_t *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.
> >
> > See below.
> >
> >> + * @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, uint32_t mode);
> >> +
> >> +/**
> >>    * Get current status of the Ethernet link flow control for Ethernet device
> >>    *
> >>    * @param port_id
> >> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> >> index 23cc1e0..625b8c5 100644
> >> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> >> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> >> @@ -575,6 +575,81 @@ 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 num
> >> + *   the num is in/out with a number of elements in an array.
> >
> > I'm sorry, I should do it before my previous suggestion, but:
> > Looking at rte_eth_xstats_get_names() and trying to be
> > consistent I'd like to suggest to put the argument after
> > speed_fec_capa and make it input only with a number of
> > array elements.
> > Positive return values should be used to provide number of
> > filled in array elements. If the returned value is greater
> > than 'num', just provided elements are filled in, but
> > it is indication as well, that num is too small.
> >
> >> + * @param speed_fec_capa
> >> + *   speed_fec_capa is out only with per-speed capabilities.
> >> + *
> >> + * @return
> >> + *   Negative errno value on error, 0 on success.
> >> + *
> >> + * @retval 0
> >> + *   Success, get FEC success.
> >
> > See above.
> >
> >> + * @retval -ENOTSUP
> >> + *   operation is not supported.
> >
> > Should start from upper case letter
> >
> >> + * @retval -EIO
> >> + *   device is removed.
> >
> > Should start from upper case letter
> >
> >> + * @retval -ENODEV
> >> + *   Device is gone.
> >
> > What's the difference between "device is remove" and
> > "Device is gone"
> >
> >> + * @retval -EINVAL
> >> + *   *num* or *speed_fec_capa* invalid.
> >> + */
> >> +typedef int (*eth_fec_get_capability_t)(struct rte_eth_dev *dev, uint32_t *num,
> >> +                            struct rte_eth_fec_capa *speed_fec_capa);
> >> +
> >> +/**
> >> + * @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.
> >
> > Should start from upper case letter
> >
> >> + * @retval -EIO
> >> + *   device is removed.
> >
> > Should start from upper case letter
> >
> >> + * @retval -ENODEV
> >> + *   Device is gone.
> >> + */
> >> +typedef int (*eth_fec_get_t)(struct rte_eth_dev *dev,
> >> +                         uint32_t *mode);
> >> +
> >> +/**
> >> + * @internal
> >> + *   Set Forward Error Correction(FEC) mode.
> >
> > Remove extra spaces before "Set"
> >
> >> + *
> >> + * @param dev
> >> + *   ethdev handle of port.
> >> + * @param mode
> >> + *   the FEC mode.
> >
> > The description is insufficient and misleading.
> > It should be fec_capa and described as:
> > Bitmask of allowed FEC modes. If must be only one
> > if AUTO is disabled. If AUTO is enabled, other
> > bits specify FEC modes which may be negotiated.
> >
> >> + *
> >> + * @return
> >> + *   Negative errno value on error, 0 on success.
> >> + *
> >> + * @retval 0
> >> + *   Success, set FEC success.
> >> + * @retval -ENOTSUP
> >> + *   operation is not supported.
> >
> > Should start from upper case letter
> >
> > What about -EINVAL in the case of unsupported FEC mode
> > requested? It is listed above in API function but
> > missing here.
> >
> >> + * @retval -EIO
> >> + *   device is removed.
> >
> > Should start from upper case letter
> >
> >> + * @retval -ENODEV
> >> + *   Device is gone.
> >
> > What's the difference between "device is remove" and
> > "Device is gone"
> >
> >> + */
> >> +typedef int (*eth_fec_set_t)(struct rte_eth_dev *dev, uint32_t mode);
> >> +
> >> +/**
> >>    * @internal A structure containing the functions exported by an Ethernet driver.
> >>    */
> >>   struct eth_dev_ops {
> >> @@ -713,6 +788,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; */
> >
> > It should be a dot (.) at the end, not semicolon (;).
> >
> >> +    eth_fec_get_t fec_get;
> >> +    /**< Get Forward Error Correction(FEC) mode; */
> >
> > same
> >
> >> +    eth_fec_set_t fec_set;
> >> +    /**< Set Forward Error Correction(FEC) mode; */
> >
> > same
> >
> >>   };
> >>
> >>   /**
> >> diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
> >> index c95ef51..b9ace3a 100644
> >> --- a/lib/librte_ethdev/rte_ethdev_version.map
> >> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> >> @@ -229,6 +229,9 @@ EXPERIMENTAL {
> >>      # added in 20.11
> >>      rte_eth_link_speed_to_str;
> >>      rte_eth_link_to_str;
> >> +    rte_eth_fec_get_capability;
> >> +    rte_eth_fec_get;
> >> +    rte_eth_fec_set;
> >>   };
> >>
> >>   INTERNAL {
> >>
> >
> > .
> >