Re: [PATCH v2 1/4] lib/ethdev: Add link_speed lanes support into rte lib

[Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>]

On Tue, Jun 11, 2024 at 4:39 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 6/2/2024 3:45 AM, Damodharam Ammepalli wrote:
> > Update the eth_dev_ops structure with new function vectors
> > to get and set number of speed lanes. This will help user to
> > configure  the same fixed speed with different number of lanes
> > based on the physical carrier type.
> >
> > Signed-off-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> > Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> > ---
> >  lib/ethdev/ethdev_driver.h | 49 +++++++++++++++++++++++++++++++++++
> >  lib/ethdev/rte_ethdev.c    | 26 +++++++++++++++++++
> >  lib/ethdev/rte_ethdev.h    | 52 ++++++++++++++++++++++++++++++++++++++
> >  lib/ethdev/version.map     |  2 ++
> >  4 files changed, 129 insertions(+)
> >
> > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> > index 0dbf2dd6a2..b1f473e4de 100644
> > --- a/lib/ethdev/ethdev_driver.h
> > +++ b/lib/ethdev/ethdev_driver.h
> > @@ -1179,6 +1179,51 @@ typedef int (*eth_rx_descriptor_dump_t)(const struct rte_eth_dev *dev,
> >                                       uint16_t queue_id, uint16_t offset,
> >                                       uint16_t num, FILE *file);
> >
> > +/**
> > + * @internal
> > + * Get number of current active lanes and max supported lanes
> >
>
> There is already a 'eth_speed_lanes_get_capa' dev_ops, why max supported
> lanes returned in this call, instead of capa?
>
> > + *
> > + * @param dev
> > + *   ethdev handle of port.
> > + * @param speed_lanes_capa
> > + *   Number of active lanes that the link is trained up.
> > + *   Max number of lanes supported by HW
> > + * @return
> > + *   Negative errno value on error, 0 on success.
> > + *
> > + * @retval 0
> > + *   Success, get speed_lanes data success.
> >
>
> Success, speed_lanes_capa filled.
>
> > + * @retval -ENOTSUP
> > + *   Operation is not supported.
> > + * @retval -EIO
> > + *   Device is removed.
> > + */
> > +typedef int (*eth_speed_lanes_get_t)(struct rte_eth_dev *dev,
> > +                                  struct rte_eth_speed_lanes_capa *speed_lanes_capa);
> > +
> > +/**
> > + * @internal
> > + * Set speed lanes
> >
>
> As we are on the subject, we all understand what "speed lane" is in this
> context, but I am not sure if the naming is descriptive enough, how this
> is referenced in datasheet?
>
In the data sheet there is no special description used. Ethtool says
"link modes"
Eg: caa4bbe ethtool: Add support for 200Gbps (50Gbps per lane) link mode
The cmdline is ethtool -s < int> speed 200000 lanes < int>
Taking our BRCM data sheet as reference. The user can configure
100Gb in three ways, and the hardware trains up the link with these two
user fed configs (speed + lane + drivers default phy auto params)
with the mapped signalling mode given below.

Eg: with this user config, speed 100 lanes 2 , the PHY will train up
fixed speed with
PAM4-56 signalling (if the cable and HW supports)

100Gb (NRZ: 25G per lane, 4 lanes) link speed
100Gb (PAM4-56: 50G per lane, 2 lanes) link speed
100Gb (PAM4-112: 100G per lane, 1 lane) link speed

Let me know if you have a proposal to update the function name or/and
function comments?

> > + *
> > + * @param dev
> > + *   ethdev handle of port.
> > + * @param speed_lanes_capa
> > + *   Non-negative number of lanes
> > + *
> > + * @return
> > + *   Negative errno value on error, 0 on success.
> > + *
> > + * @retval 0
> > + *   Success, set lanes success.
> > + * @retval -ENOTSUP
> > + *   Operation is not supported.
> > + * @retval -EINVAL
> > + *   Unsupported mode requested.
> > + * @retval -EIO
> > + *   Device is removed.
> > + */
> > +typedef int (*eth_speed_lanes_set_t)(struct rte_eth_dev *dev, uint32_t speed_lanes_capa);
> > +
>
> These new dev_ops seems inserted in between 'rx_descriptor_dump' &
> 'tx_descriptor_dump' dev_ops.
> Can you please move them just below 'eth_link_update_t'?
>
Is this change only into the struct eth_dev_ops or applies to the
function declarations also?
>
>
> >  /**
> >   * @internal
> >   * Dump Tx descriptor info to a file.
> > @@ -1474,6 +1519,10 @@ struct eth_dev_ops {
> >       eth_count_aggr_ports_t count_aggr_ports;
> >       /** Map a Tx queue with an aggregated port of the DPDK port */
> >       eth_map_aggr_tx_affinity_t map_aggr_tx_affinity;
> > +     /** Get number of speed lanes supported and active lanes */
> > +     eth_speed_lanes_get_t speed_lanes_get;
> > +     /** Set number of speed lanes */
> > +     eth_speed_lanes_set_t speed_lanes_set;
> >  };
> >
> >  /**
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > index f1c658f49e..45e2f7645b 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -7008,4 +7008,30 @@ int rte_eth_dev_map_aggr_tx_affinity(uint16_t port_id, uint16_t tx_queue_id,
> >       return ret;
> >  }
> >
> > +int
> > +rte_eth_speed_lanes_get(uint16_t port_id, struct rte_eth_speed_lanes_capa *capa)
> > +{
> > +     struct rte_eth_dev *dev;
> > +
> > +     RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +     dev = &rte_eth_devices[port_id];
> > +
> > +     if (*dev->dev_ops->speed_lanes_get == NULL)
> > +             return -ENOTSUP;
> > +     return eth_err(port_id, (*dev->dev_ops->speed_lanes_get)(dev, capa));
> >
>
> Shouldn't we verify if 'capa' is not NULL in API?
>
> > +}
> > +
> > +int
> > +rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa)
> > +{
> > +     struct rte_eth_dev *dev;
> > +
> > +     RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +     dev = &rte_eth_devices[port_id];
> > +
> > +     if (*dev->dev_ops->speed_lanes_set == NULL)
> > +             return -ENOTSUP;
> > +     return eth_err(port_id, (*dev->dev_ops->speed_lanes_set)(dev, speed_lanes_capa));
> > +}
> > +
> >  RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO);
> > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> > index 147257d6a2..caae1f27c6 100644
> > --- a/lib/ethdev/rte_ethdev.h
> > +++ b/lib/ethdev/rte_ethdev.h
> > @@ -1997,6 +1997,12 @@ struct rte_eth_fec_capa {
> >       uint32_t capa;  /**< FEC capabilities bitmask */
> >  };
> >
> > +/* A structure used to get and set lanes capabilities per link speed */
> > +struct rte_eth_speed_lanes_capa {
> > +     uint32_t active_lanes;
> > +     uint32_t max_lanes_cap;
> > +};
> > +
> >  #define RTE_ETH_ALL RTE_MAX_ETHPORTS
> >
> >  /* Macros to check for valid port */
> > @@ -6917,6 +6923,52 @@ rte_eth_tx_queue_count(uint16_t port_id, uint16_t queue_id)
> >       return rc;
> >  }
> >
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> > + *
> > + * Get maximum speed lanes supported by the NIC.
> >
>
> Isn't this API to get the current lane number?
>
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param speed_lanes_capa
> > + *   speed_lanes_capa is out only with max speed lanes capabilities.
> > + *   If set to NULL, then its assumed zero or not supported.
> >
>
> Why NULL 'capa' is supported?
>
> > + *
> > + * @return
> > + *   - A non-negative value of active lanes that currently link is up with.
> > + *   - A non-negative value that this HW scales up to for all speeds.
> >
>
> Isn't the return value only for status, error or success, and data
> stored in 'capa' pointer?
>
>
> > + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
> > + *     that operation.
> > + *   - (-EIO) if device is removed.
> > + *   - (-ENODEV)  if *port_id* invalid.
> > + *   - (-EINVAL)  if *speed_lanes_capa* invalid
> >
>
> This is 'get' function, how 'speed_lanes_capa' can be invalid?
>
> > + */
> > +__rte_experimental
> > +int rte_eth_speed_lanes_get(uint16_t port_id, struct rte_eth_speed_lanes_capa *capa);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> > + *
> > + * Set speed lanes supported by the NIC.
> >
>
> Should we document somewhere that this is only for the case Auto
> Negotiation (AN) is disabled, otherwise AN will figure out the lanes.
>
yes. may be in the command help, we can describe this.

> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param speed_lanes
> > + *   speed_lanes a non-zero value of number lanes for this speeds.
> >
>
> Please reword 'this speeds'
>
> > + *
> > + * @return
> > + *  - (>=0) valid input and supported by driver or hardware.
> >
>
> Lanes set successfully?
>
>
> > + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
> > + *     that operation.
> > + *   - (-EIO) if device is removed.
> > + *   - (-ENODEV)  if *port_id* invalid.
> > + *   - (-EINVAL)  if *speed_lanes* invalid
> > + */
> > +__rte_experimental
> > +int rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa);
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> > index 79f6f5293b..9c27980f3a 100644
> > --- a/lib/ethdev/version.map
> > +++ b/lib/ethdev/version.map
> > @@ -325,6 +325,8 @@ EXPERIMENTAL {
> >       rte_flow_template_table_resizable;
> >       rte_flow_template_table_resize;
> >       rte_flow_template_table_resize_complete;
> > +     rte_eth_speed_lanes_get;
> > +     rte_eth_speed_lanes_set;
> >
>
> Please follow the syntax in the file, add "# added in 24.07" comment and
> add new APIs under it alphabetically sorted way.
>

As you suggested, I have consolidated all the 4 patches (testpmd +
lib) into one single patch and addressed the comments in my local
patch.
If you can comment on the function name changes, will update and push
a new revision soon.

Thanks
Damo

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.