[dpdk-dev] [PATCH v2 2/7] ethdev: add a link status textrepresentation

[Morten Brørup]

> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Friday, June 5, 2020 1:45 PM
> 
> On 5/27/2020 8:45 AM, Morten Brørup wrote:
> >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ivan Dyukov
> >> Sent: Tuesday, May 26, 2020 9:10 PM
> >>
> >> This commit add function which treat link status structure
> >> and format it to text representation.
> >>
> >> Signed-off-by: Ivan Dyukov <i.dyukov at samsung.com>
> >> ---
> >>  lib/librte_ethdev/rte_ethdev.c | 39
> ++++++++++++++++++++++++++++++++++
> >>  lib/librte_ethdev/rte_ethdev.h | 31 +++++++++++++++++++++++++++
> >>  2 files changed, 70 insertions(+)
> >>
> >> diff --git a/lib/librte_ethdev/rte_ethdev.c
> >> b/lib/librte_ethdev/rte_ethdev.c
> >> index 8e10a6fc3..8d75c2440 100644
> >> --- a/lib/librte_ethdev/rte_ethdev.c
> >> +++ b/lib/librte_ethdev/rte_ethdev.c
> >> @@ -2385,6 +2385,45 @@ rte_eth_link_get_nowait(uint16_t port_id,
> struct
> >> rte_eth_link *eth_link)
> >>  	return 0;
> >>  }
> >>
> >> +void
> >> +rte_eth_link_prepare_text(struct rte_eth_link *eth_link, uint32_t
> >> speed_unit,
> >> +			  struct rte_eth_link_text *link_text)
> >> +{
> >> +	uint32_t link_speed = 0;
> >> +	/* prepare link speed */
> >> +	if (eth_link->link_speed == ETH_SPEED_NUM_UNKNOWN)
> >> +		memcpy(link_text->link_speed, "unknown",
> >> sizeof("unknown"));
> >> +	else {
> >> +		if (speed_unit == ETH_SPEED_UNIT_GBPS)
> >> +			link_speed = eth_link->link_speed / 1000;
> >> +		else
> >> +			link_speed = eth_link->link_speed;
> >> +		snprintf(link_text->link_speed, sizeof(link_text-
> >>> link_speed),
> >> +			 "%u", link_speed);
> >> +	}
> >> +	/* prepare link duplex */
> >> +	if (eth_link->link_duplex == ETH_LINK_FULL_DUPLEX)
> >> +		memcpy(link_text->link_duplex, "full-duplex",
> >> +			sizeof("full-duplex"));
> >> +	else
> >> +		memcpy(link_text->link_duplex, "half-duplex",
> >> +			sizeof("half-duplex"));
> >> +	/* prepare autoneg */
> >> +	if (eth_link->link_autoneg == ETH_LINK_AUTONEG)
> >> +		memcpy(link_text->link_autoneg, "autoneg",
> >> +			sizeof("autoneg"));
> >> +	else
> >> +		memcpy(link_text->link_autoneg, "fixed",
> >> +			sizeof("fixed"));
> >> +	/* prepare status */
> >> +	if (eth_link->link_status == ETH_LINK_DOWN)
> >> +		memcpy(link_text->link_status, "down",
> >> +			sizeof("down"));
> >> +	else
> >> +		memcpy(link_text->link_status, "up",
> >> +			sizeof("up"));
> >> +}
> >> +
> >>  int
> >>  rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
> >>  {
> >> diff --git a/lib/librte_ethdev/rte_ethdev.h
> >> b/lib/librte_ethdev/rte_ethdev.h
> >> index 2090af501..53d2f0c78 100644
> >> --- a/lib/librte_ethdev/rte_ethdev.h
> >> +++ b/lib/librte_ethdev/rte_ethdev.h
> >> @@ -316,6 +316,19 @@ struct rte_eth_link {
> >>  	uint16_t link_status  : 1;  /**< ETH_LINK_[DOWN/UP] */
> >>  } __rte_aligned(8);      /**< aligned for atomic64 read/write */
> >>
> >> +/**
> >> + * Link speed units
> >> + */
> >> +#define ETH_SPEED_UNIT_GBPS 0
> >> +#define ETH_SPEED_UNIT_MBPS 1
> >> +
> >> +
> >> +struct rte_eth_link_text {
> >> +	char link_speed[14];  /** link speed */
> >> +	char link_duplex[12];  /** full-duplex or half-duplex */
> >> +	char link_autoneg[8];  /** autoneg or fixed */
> >> +	char link_status[5];  /** down or up */
> >> +};
> >>  /* Utility constants */
> >>  #define ETH_LINK_HALF_DUPLEX 0 /**< Half-duplex connection (see
> >> link_duplex). */
> >>  #define ETH_LINK_FULL_DUPLEX 1 /**< Full-duplex connection (see
> >> link_duplex). */
> >> @@ -2295,6 +2308,24 @@ int rte_eth_link_get(uint16_t port_id, struct
> >> rte_eth_link *link);
> >>   */
> >>  int rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link
> >> *link);
> >>
> >> +/**
> >> + * Format link status to textual representation. speed_unit is used
> to
> >> convert
> >> + * link_speed to specified unit. Also this function threats a
> special
> >> + * ETH_SPEED_NUM_UNKNOWN value of link_speed and return 'UNKNOWN'
> >> speed
> >> + * in this case.
> >> + *
> >> + * @param link
> >> + *   Link status provided by rte_eth_link_get function
> >> + * @param speed_unit
> >> + *   Target units for the speed. Following values are available:
> >> + *    - ETH_SPEED_UNIT_GBPS
> >> + *    - ETH_SPEED_UNIT_MBPS
> >> + * @param link_text
> >> + *   A pointer to an *rte_eth_link_text* structure to be filled
> with
> >> + *   textual representation of device status
> >> + */
> >> +void rte_eth_link_prepare_text(struct rte_eth_link *link, uint32_t
> >> speed_unit,
> >> +				struct rte_eth_link_text *link_text);
> >>  /**
> >>   * Retrieve the general I/O statistics of an Ethernet device.
> >>   *
> >> --
> >> 2.17.1
> >>
> >
> > Considering that this function will only be used by applications that
> don't need to follow a vendor-specific textual format for their link
> status, you can choose your own text format, and don't need the struct
> for the text strings. The function can simply write the entire
> information into a single string. It also makes speed_unit superfluous.
> E.g. like this:
> >
> > void rte_eth_link_prepare_text(char *str, const struct rte_eth_link
> *const link)
> > {
> >     if (link.link_status == ETH_LINK_DOWN) {
> >         str += sprintf(str, "Link down");
> >     } else {
> >         str += sprintf(str, "Link up at ");
> >         if (link.link_speed == ETH_SPEED_NUM_UNKNOWN) {
> >             str += sprintf("Unknown speed");
> >         } else {
> >             if (link.link_speed < ETH_SPEED_NUM_1G)
> >                 str += sprintf(str, "%u Mbit/s", link.link_speed);
> >             else if (link.link_speed == ETH_SPEED_NUM_2_5G)
> >                 str += sprintf(str, "2.5 Gbit/s");
> >             else
> >                 str += sprintf(str, "%u Gbit/s", link.link_speed /
> 1000);
> >             str += sprintf(str, " %cDX", link.link_duplex ? 'F' :
> 'H');
> >             str += sprintf(str, " %s", link.link_autoneg ? "Autoneg"
> : "Fixed");
> >         }
> >     }
> > }
> >
> >
> > And if DPDK already has an established style for "convert information
> to human readable text" functions regarding function name and parameter
> order, the function should follow such style.
> >
> 
> What about having them both, a per-formatted string that can make life
> easy for
> applications, and struct for text strings for the apps need some
> granularity.
> 

Not necessary. Application developers can easily write their own link status formatting functions.

And the formatting function provided by DPDK can be used as a reference if the application developer needs more detailed information. That is one of the beauties of Open Source Software - the source code supplements the documentation.