[v7,02/25] ethdev: add a link status text representation
diff mbox series

Message ID 20200710070226.6045-3-i.dyukov@samsung.com
State Deferred
Delegated to: Ferruh Yigit
Headers show
Series
  • ethdev: allow unknown link speed
Related show

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/checkpatch success coding style OK

Commit Message

Ivan Dyukov July 10, 2020, 7:02 a.m. UTC
This commit add function which treat link status structure
and format it to text representation.

Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
---
 MAINTAINERS                              |   1 +
 app/test/Makefile                        |   3 +
 app/test/meson.build                     |   2 +
 app/test/test_ethdev_link.c              | 299 +++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.c           | 174 +++++++++++++
 lib/librte_ethdev/rte_ethdev.h           |  54 ++++
 lib/librte_ethdev/rte_ethdev_version.map |   4 +
 7 files changed, 537 insertions(+)
 create mode 100644 app/test/test_ethdev_link.c

Comments

Ferruh Yigit July 10, 2020, 1:06 p.m. UTC | #1
On 7/10/2020 8:02 AM, Ivan Dyukov wrote:
> This commit add function which treat link status structure
> and format it to text representation.
> 
> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>

<...>

> +static int
> +rte_eth_link_strf_parser(char *str, size_t len, const char *const fmt,
> +			   const struct rte_eth_link *link)
> +{
> +	size_t offset = 0;
> +	const char *fmt_cur = fmt;
> +	char *str_cur = str;
> +	double gbits = (double)link->link_speed / 1000.;
> +	static const char autoneg_str[]       = "Autoneg";
> +	static const char fixed_str[]         = "Fixed";
> +	static const char fdx_str[]           = "FDX";
> +	static const char hdx_str[]           = "HDX";
> +	static const char unknown_str[]       = "Unknown";
> +	static const char up_str[]            = "Up";
> +	static const char down_str[]          = "Down";
> +	char gbits_str[20];
> +	char mbits_str[20];
> +
> +	/* preformat complex formatting to easily concatinate it further */
> +	snprintf(mbits_str, sizeof(mbits_str), "%u", link->link_speed);
> +	snprintf(gbits_str, sizeof(gbits_str), "%.1f", gbits);
> +	/* init str before formatting */
> +	str[0] = 0;
> +	while (*fmt_cur) {
> +		/* check str bounds */
> +		if (offset > (len - 1)) {
> +			str[len - 1] = '\0';
> +			return -1;
> +		}
> +		if (*fmt_cur == '%') {
> +			/* set null terminator to current position,
> +			 * it's required for strlcat
> +			 */
> +			*str_cur = '\0';
> +			switch (*++fmt_cur) {
> +			/* Speed in Mbits/s */
> +			case 'M':
> +				if (link->link_speed ==
> +				    ETH_SPEED_NUM_UNKNOWN)
> +					offset = strlcat(str, unknown_str,
> +							 len);
> +				else
> +					offset = strlcat(str, mbits_str, len);
> +				break;
> +			/* Speed in Gbits/s */
> +			case 'G':
> +				if (link->link_speed ==
> +				    ETH_SPEED_NUM_UNKNOWN)
> +					offset = strlcat(str, unknown_str,
> +							 len);
> +				else
> +					offset = strlcat(str, gbits_str, len);
> +				break;
> +			/* Link status */
> +			case 'S':
> +				offset = strlcat(str, link->link_status ?
> +					up_str : down_str, len);
> +				break;
> +			/* Link autoneg */
> +			case 'A':
> +				offset = strlcat(str, link->link_autoneg ?
> +					autoneg_str : fixed_str, len);
> +				break;
> +			/* Link duplex */
> +			case 'D':
> +				offset = strlcat(str, link->link_duplex ?
> +					fdx_str : hdx_str, len);
> +				break;
> +			/* ignore unknown specifier */
> +			default:
> +				*str_cur = '%';
> +				offset++;
> +				fmt_cur--;
> +				break;

What do you think ignoring the unknown specifiers and keep continue
processing the string, instead of break? Just keep unknown specifier
as it is in the output string.

> +
> +			}
> +			if (offset > (len - 1))
> +				return -1;

For me "offset >= len" is simpler than "offset > (len - 1)", also it prevents any possible error when "len == 0" ('len' is unsigned) although I can see you have check for it.
Anyway both deos same thing, up to you.

> +
> +			str_cur = str + offset;
> +		} else {
> +			*str_cur++ = *fmt_cur;
> +			offset++;

Why keeping both offset and the pointer ('str_cur'), just offset should be enough "str[offset++] = *fmt_cur;", I think this simplifies a little bit.

> +		}
> +		fmt_cur++;
> +	}
> +	*str_cur = '\0';
> +	return offset;
> +}
> +
> +int
> +rte_eth_link_printf(const char *const fmt,
> +		    const struct rte_eth_link *link)
> +{
> +	char text[200];
> +	int ret;
> +
> +	ret = rte_eth_link_strf(text, 200, fmt, link);
> +	if (ret > 0)
> +		printf("%s", text);
> +	return ret;
> +}
> +
> +int
> +rte_eth_link_strf(char *str, size_t len, const char *const fmt,
> +		    const struct rte_eth_link *link)
> +{
> +	size_t offset = 0;
> +	double gbits = (double)link->link_speed / 1000.;
> +	char speed_gbits_str[20];
> +	char speed_mbits_str[20];
> +	/* TBD: make it international? */
> +	static const char link_down_str[]     = "Link down\n";
> +	static const char link_up_str[]       = "Link up at ";
> +	static const char unknown_speed_str[] = "Unknown speed ";
> +	static const char mbits_str[]	      = "Mbit/s";
> +	static const char gbits_str[]	      = "Gbit/s";
> +	static const char fdx_str[]           = "FDX ";
> +	static const char hdx_str[]           = "HDX ";
> +	/* autoneg is latest param in default string, so add '\n' */
> +	static const char autoneg_str[]       = "Autoneg\n";
> +	static const char fixed_str[]         = "Fixed\n";

Please put an empty line after variables.

<...>

> +/**
> + * Format link status to textual representation. This function threats all
> + * special values like ETH_SPEED_NUM_UNKNOWN, ETH_LINK_DOWN etc. and convert
> + * them to textual representation.
> + *
> + * @param str
> + *   A pointer to a string to be filled with textual representation of
> + *   device status.
> + * @param len
> + *   Length of available memory at 'str' string.
> + * @param fmt
> + *   Format string which allow to format link status. If NULL is provided
> + *   , default formatting will be applied.
> + *   Following specifiers are available:
> + *    - '%M' link speed in Mbits/s
> + *    - '%G' link speed in Gbits/s
> + *    - '%S' link status. e.g. Up or Down
> + *    - '%A' link autonegotiation state
> + *    - '%D' link duplex state
> + * @param link

It should be 'eth_link' otherwise doxygen is complaining.

> + *   Link status provided by rte_eth_link_get function
> + * @return
> + *   - Number of bytes written to str array. In case of error, -1 is returned.
> + *
> + */
> +__rte_experimental
> +int rte_eth_link_strf(char *str, size_t len, const char *const fmt,
> +			const struct rte_eth_link *eth_link);
> +

For the API name, I guess 'strf' is for 'stringify' but what do you think about
'rte_eth_link_to_str()', I think more clear and simpler.
Thomas Monjalon July 10, 2020, 3:11 p.m. UTC | #2
10/07/2020 09:02, Ivan Dyukov:
> This commit add function which treat link status structure
> and format it to text representation.

It is missing an explanation about why it is required.
Which problem is it solving?

> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>

I'm surprised there is not so much review on this patch.

[...]
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> +/**
> + * print formatted link status to stdout. This function threats all
> + * special values like ETH_SPEED_NUM_UNKNOWN, ETH_LINK_DOWN etc. and convert
> + * them to textual representation.

I don't understand the need for this function.
If needed, the application can send the output of rte_eth_link_strf()
to fprintf or a log function.

> + *
> + * @param fmt
> + *   Format string which allow to format link status. If NULL is provided
> + *   , default formatting will be applied.
> + *   Following specifiers are available:
> + *    - '%M' link speed in Mbits/s
> + *    - '%G' link speed in Gbits/s
> + *    - '%S' link status. e.g. Up or Down
> + *    - '%A' link autonegotiation state
> + *    - '%D' link duplex state
> + * @param link
> + *   Link status provided by rte_eth_link_get function
> + * @return
> + *   - Number of bytes written to stdout. In case of error, -1 is returned.
> + *
> + */
> +__rte_experimental
> +int rte_eth_link_printf(const char *const fmt,
> +			const struct rte_eth_link *link);
> +
> +/**
> + * Format link status to textual representation. This function threats all

not "threats"

> + * special values like ETH_SPEED_NUM_UNKNOWN, ETH_LINK_DOWN etc. and convert
> + * them to textual representation.
> + *
> + * @param str
> + *   A pointer to a string to be filled with textual representation of
> + *   device status.
> + * @param len
> + *   Length of available memory at 'str' string.
> + * @param fmt
> + *   Format string which allow to format link status. If NULL is provided
> + *   , default formatting will be applied.

Please do not start a line with a comma,
it should be ending the previous line, like here ;)
Even better: start the new sentence on a new line.

> + *   Following specifiers are available:
> + *    - '%M' link speed in Mbits/s
> + *    - '%G' link speed in Gbits/s
> + *    - '%S' link status. e.g. Up or Down
> + *    - '%A' link autonegotiation state
> + *    - '%D' link duplex state

These specifiers look OK.

> + * @param link
> + *   Link status provided by rte_eth_link_get function
> + * @return
> + *   - Number of bytes written to str array. In case of error, -1 is returned.

Better to have error case on a separate line.

> + *
> + */
> +__rte_experimental
> +int rte_eth_link_strf(char *str, size_t len, const char *const fmt,
> +			const struct rte_eth_link *eth_link);
Stephen Hemminger July 10, 2020, 3:22 p.m. UTC | #3
On Fri, 10 Jul 2020 13:06:02 +0000
"Yigit, Ferruh" <ferruh.yigit@intel.com> wrote:

> > +			/* ignore unknown specifier */
> > +			default:
> > +				*str_cur = '%';
> > +				offset++;
> > +				fmt_cur--;
> > +				break;  
> 
> What do you think ignoring the unknown specifiers and keep continue
> processing the string, instead of break? Just keep unknown specifier
> as it is in the output string.

My comment was that the function should behave the same as
snprintf() does when handed unknown specifier. What snprintf
does is duplicate the string in the output buffer (like any
other non format character).
Ferruh Yigit July 10, 2020, 3:39 p.m. UTC | #4
> On Fri, 10 Jul 2020 13:06:02 +0000
> "Yigit, Ferruh" <ferruh.yigit@intel.com> wrote:
> 
> > > +			/* ignore unknown specifier */
> > > +			default:
> > > +				*str_cur = '%';
> > > +				offset++;
> > > +				fmt_cur--;
> > > +				break;
> >
> > What do you think ignoring the unknown specifiers and keep continue
> > processing the string, instead of break? Just keep unknown specifier
> > as it is in the output string.
> 
> My comment was that the function should behave the same as
> snprintf() does when handed unknown specifier. What snprintf does is
> duplicate the string in the output buffer (like any other non format
> character).

+1
Ivan Dyukov July 10, 2020, 6:36 p.m. UTC | #5
10.07.2020 16:06, Yigit, Ferruh пишет:
> On 7/10/2020 8:02 AM, Ivan Dyukov wrote:
>> This commit add function which treat link status structure
>> and format it to text representation.
>>
>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> <...>
>
>> +static int
>> +rte_eth_link_strf_parser(char *str, size_t len, const char *const fmt,
>> +			   const struct rte_eth_link *link)
>> +{
>> +	size_t offset = 0;
>> +	const char *fmt_cur = fmt;
>> +	char *str_cur = str;
>> +	double gbits = (double)link->link_speed / 1000.;
>> +	static const char autoneg_str[]       = "Autoneg";
>> +	static const char fixed_str[]         = "Fixed";
>> +	static const char fdx_str[]           = "FDX";
>> +	static const char hdx_str[]           = "HDX";
>> +	static const char unknown_str[]       = "Unknown";
>> +	static const char up_str[]            = "Up";
>> +	static const char down_str[]          = "Down";
>> +	char gbits_str[20];
>> +	char mbits_str[20];
>> +
>> +	/* preformat complex formatting to easily concatinate it further */
>> +	snprintf(mbits_str, sizeof(mbits_str), "%u", link->link_speed);
>> +	snprintf(gbits_str, sizeof(gbits_str), "%.1f", gbits);
>> +	/* init str before formatting */
>> +	str[0] = 0;
>> +	while (*fmt_cur) {
>> +		/* check str bounds */
>> +		if (offset > (len - 1)) {
>> +			str[len - 1] = '\0';
>> +			return -1;
>> +		}
>> +		if (*fmt_cur == '%') {
>> +			/* set null terminator to current position,
>> +			 * it's required for strlcat
>> +			 */
>> +			*str_cur = '\0';
>> +			switch (*++fmt_cur) {
>> +			/* Speed in Mbits/s */
>> +			case 'M':
>> +				if (link->link_speed ==
>> +				    ETH_SPEED_NUM_UNKNOWN)
>> +					offset = strlcat(str, unknown_str,
>> +							 len);
>> +				else
>> +					offset = strlcat(str, mbits_str, len);
>> +				break;
>> +			/* Speed in Gbits/s */
>> +			case 'G':
>> +				if (link->link_speed ==
>> +				    ETH_SPEED_NUM_UNKNOWN)
>> +					offset = strlcat(str, unknown_str,
>> +							 len);
>> +				else
>> +					offset = strlcat(str, gbits_str, len);
>> +				break;
>> +			/* Link status */
>> +			case 'S':
>> +				offset = strlcat(str, link->link_status ?
>> +					up_str : down_str, len);
>> +				break;
>> +			/* Link autoneg */
>> +			case 'A':
>> +				offset = strlcat(str, link->link_autoneg ?
>> +					autoneg_str : fixed_str, len);
>> +				break;
>> +			/* Link duplex */
>> +			case 'D':
>> +				offset = strlcat(str, link->link_duplex ?
>> +					fdx_str : hdx_str, len);
>> +				break;
>> +			/* ignore unknown specifier */
>> +			default:
>> +				*str_cur = '%';
>> +				offset++;
>> +				fmt_cur--;
>> +				break;
> What do you think ignoring the unknown specifiers and keep continue
> processing the string, instead of break? Just keep unknown specifier
> as it is in the output string.
yep. it's exactly what code do.  break exit from the switch but not from 
string processing.  I have unit tests for this case. They  work fine. 
Please review unit tests and send me more cases if they need to be tested.
>
>> +
>> +			}
>> +			if (offset > (len - 1))
>> +				return -1;
> For me "offset >= len" is simpler than "offset > (len - 1)", also it prevents any possible error when "len == 0" ('len' is unsigned) although I can see you have check for it.
> Anyway both deos same thing, up to you.
>
>> +
>> +			str_cur = str + offset;
>> +		} else {
>> +			*str_cur++ = *fmt_cur;
>> +			offset++;
> Why keeping both offset and the pointer ('str_cur'), just offset should be enough "str[offset++] = *fmt_cur;", I think this simplifies a little bit.
>
>> +		}
>> +		fmt_cur++;
>> +	}
>> +	*str_cur = '\0';
>> +	return offset;
>> +}
>> +
>> +int
>> +rte_eth_link_printf(const char *const fmt,
>> +		    const struct rte_eth_link *link)
>> +{
>> +	char text[200];
>> +	int ret;
>> +
>> +	ret = rte_eth_link_strf(text, 200, fmt, link);
>> +	if (ret > 0)
>> +		printf("%s", text);
>> +	return ret;
>> +}
>> +
>> +int
>> +rte_eth_link_strf(char *str, size_t len, const char *const fmt,
>> +		    const struct rte_eth_link *link)
>> +{
>> +	size_t offset = 0;
>> +	double gbits = (double)link->link_speed / 1000.;
>> +	char speed_gbits_str[20];
>> +	char speed_mbits_str[20];
>> +	/* TBD: make it international? */
>> +	static const char link_down_str[]     = "Link down\n";
>> +	static const char link_up_str[]       = "Link up at ";
>> +	static const char unknown_speed_str[] = "Unknown speed ";
>> +	static const char mbits_str[]	      = "Mbit/s";
>> +	static const char gbits_str[]	      = "Gbit/s";
>> +	static const char fdx_str[]           = "FDX ";
>> +	static const char hdx_str[]           = "HDX ";
>> +	/* autoneg is latest param in default string, so add '\n' */
>> +	static const char autoneg_str[]       = "Autoneg\n";
>> +	static const char fixed_str[]         = "Fixed\n";
> Please put an empty line after variables.
>
> <...>
>
>> +/**
>> + * Format link status to textual representation. This function threats all
>> + * special values like ETH_SPEED_NUM_UNKNOWN, ETH_LINK_DOWN etc. and convert
>> + * them to textual representation.
>> + *
>> + * @param str
>> + *   A pointer to a string to be filled with textual representation of
>> + *   device status.
>> + * @param len
>> + *   Length of available memory at 'str' string.
>> + * @param fmt
>> + *   Format string which allow to format link status. If NULL is provided
>> + *   , default formatting will be applied.
>> + *   Following specifiers are available:
>> + *    - '%M' link speed in Mbits/s
>> + *    - '%G' link speed in Gbits/s
>> + *    - '%S' link status. e.g. Up or Down
>> + *    - '%A' link autonegotiation state
>> + *    - '%D' link duplex state
>> + * @param link
> It should be 'eth_link' otherwise doxygen is complaining.
>
>> + *   Link status provided by rte_eth_link_get function
>> + * @return
>> + *   - Number of bytes written to str array. In case of error, -1 is returned.
>> + *
>> + */
>> +__rte_experimental
>> +int rte_eth_link_strf(char *str, size_t len, const char *const fmt,
>> +			const struct rte_eth_link *eth_link);
>> +
> For the API name, I guess 'strf' is for 'stringify' but what do you think about
> 'rte_eth_link_to_str()', I think more clear and simpler.
Ivan Dyukov July 10, 2020, 6:51 p.m. UTC | #6
10.07.2020 18:22, Stephen Hemminger пишет:
> On Fri, 10 Jul 2020 13:06:02 +0000
> "Yigit, Ferruh" <ferruh.yigit@intel.com> wrote:
>
>>> +			/* ignore unknown specifier */
>>> +			default:
>>> +				*str_cur = '%';
>>> +				offset++;
>>> +				fmt_cur--;
>>> +				break;
>> What do you think ignoring the unknown specifiers and keep continue
>> processing the string, instead of break? Just keep unknown specifier
>> as it is in the output string.
> My comment was that the function should behave the same as
> snprintf() does when handed unknown specifier. What snprintf
> does is duplicate the string in the output buffer (like any
> other non format character).
Looks like I need to rewrite comment "ignore unkown specifier". It's not 
what code do. This function work like snprintf.
Ferruh Yigit July 10, 2020, 7:01 p.m. UTC | #7
> Subject: Re: [PATCH v7 02/25] ethdev: add a link status text representation
> 
> 10.07.2020 16:06, Yigit, Ferruh пишет:
> > On 7/10/2020 8:02 AM, Ivan Dyukov wrote:
> >> This commit add function which treat link status structure and format
> >> it to text representation.
> >>
> >> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> > <...>
> >
> >> +static int
> >> +rte_eth_link_strf_parser(char *str, size_t len, const char *const fmt,
> >> +			   const struct rte_eth_link *link) {
> >> +	size_t offset = 0;
> >> +	const char *fmt_cur = fmt;
> >> +	char *str_cur = str;
> >> +	double gbits = (double)link->link_speed / 1000.;
> >> +	static const char autoneg_str[]       = "Autoneg";
> >> +	static const char fixed_str[]         = "Fixed";
> >> +	static const char fdx_str[]           = "FDX";
> >> +	static const char hdx_str[]           = "HDX";
> >> +	static const char unknown_str[]       = "Unknown";
> >> +	static const char up_str[]            = "Up";
> >> +	static const char down_str[]          = "Down";
> >> +	char gbits_str[20];
> >> +	char mbits_str[20];
> >> +
> >> +	/* preformat complex formatting to easily concatinate it further */
> >> +	snprintf(mbits_str, sizeof(mbits_str), "%u", link->link_speed);
> >> +	snprintf(gbits_str, sizeof(gbits_str), "%.1f", gbits);
> >> +	/* init str before formatting */
> >> +	str[0] = 0;
> >> +	while (*fmt_cur) {
> >> +		/* check str bounds */
> >> +		if (offset > (len - 1)) {
> >> +			str[len - 1] = '\0';
> >> +			return -1;
> >> +		}
> >> +		if (*fmt_cur == '%') {
> >> +			/* set null terminator to current position,
> >> +			 * it's required for strlcat
> >> +			 */
> >> +			*str_cur = '\0';
> >> +			switch (*++fmt_cur) {
> >> +			/* Speed in Mbits/s */
> >> +			case 'M':
> >> +				if (link->link_speed ==
> >> +				    ETH_SPEED_NUM_UNKNOWN)
> >> +					offset = strlcat(str, unknown_str,
> >> +							 len);
> >> +				else
> >> +					offset = strlcat(str, mbits_str, len);
> >> +				break;
> >> +			/* Speed in Gbits/s */
> >> +			case 'G':
> >> +				if (link->link_speed ==
> >> +				    ETH_SPEED_NUM_UNKNOWN)
> >> +					offset = strlcat(str, unknown_str,
> >> +							 len);
> >> +				else
> >> +					offset = strlcat(str, gbits_str, len);
> >> +				break;
> >> +			/* Link status */
> >> +			case 'S':
> >> +				offset = strlcat(str, link->link_status ?
> >> +					up_str : down_str, len);
> >> +				break;
> >> +			/* Link autoneg */
> >> +			case 'A':
> >> +				offset = strlcat(str, link->link_autoneg ?
> >> +					autoneg_str : fixed_str, len);
> >> +				break;
> >> +			/* Link duplex */
> >> +			case 'D':
> >> +				offset = strlcat(str, link->link_duplex ?
> >> +					fdx_str : hdx_str, len);
> >> +				break;
> >> +			/* ignore unknown specifier */
> >> +			default:
> >> +				*str_cur = '%';
> >> +				offset++;
> >> +				fmt_cur--;
> >> +				break;
> > What do you think ignoring the unknown specifiers and keep continue
> > processing the string, instead of break? Just keep unknown specifier
> > as it is in the output string.
> yep. it's exactly what code do.  break exit from the switch but not from string
> processing.  I have unit tests for this case. They  work fine.
> Please review unit tests and send me more cases if they need to be tested.

yes it does as you explained, I miss read it, so it is good, please ignore this comment.

Patch
diff mbox series

diff --git a/MAINTAINERS b/MAINTAINERS
index 5e706cd7e..f4fb31ea2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -393,6 +393,7 @@  T: git://dpdk.org/next/dpdk-next-net
 F: lib/librte_ethdev/
 F: devtools/test-null.sh
 F: doc/guides/prog_guide/switch_representation.rst
+F: app/test/test_ethdev*
 
 Flow API
 M: Ori Kam <orika@mellanox.com>
diff --git a/app/test/Makefile b/app/test/Makefile
index e5440774b..9f43b8c3c 100644
--- a/app/test/Makefile
+++ b/app/test/Makefile
@@ -251,6 +251,9 @@  SRCS-$(CONFIG_RTE_LIBRTE_SECURITY) += test_security.c
 
 SRCS-$(CONFIG_RTE_LIBRTE_IPSEC) += test_ipsec.c test_ipsec_perf.c
 SRCS-$(CONFIG_RTE_LIBRTE_IPSEC) += test_ipsec_sad.c
+
+SRCS-$(CONFIG_RTE_LIBRTE_ETHER) += test_ethdev_link.c
+
 ifeq ($(CONFIG_RTE_LIBRTE_IPSEC),y)
 LDLIBS += -lrte_ipsec
 endif
diff --git a/app/test/meson.build b/app/test/meson.build
index 56591db4e..1e6acf701 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -39,6 +39,7 @@  test_sources = files('commands.c',
 	'test_efd.c',
 	'test_efd_perf.c',
 	'test_errno.c',
+	'test_ethdev_link.c',
 	'test_event_crypto_adapter.c',
 	'test_event_eth_rx_adapter.c',
 	'test_event_ring.c',
@@ -199,6 +200,7 @@  fast_tests = [
         ['eal_flags_misc_autotest', false],
         ['eal_fs_autotest', true],
         ['errno_autotest', true],
+        ['ethdev_link_status', true],
         ['event_ring_autotest', true],
         ['fib_autotest', true],
         ['fib6_autotest', true],
diff --git a/app/test/test_ethdev_link.c b/app/test/test_ethdev_link.c
new file mode 100644
index 000000000..8a2f133c0
--- /dev/null
+++ b/app/test/test_ethdev_link.c
@@ -0,0 +1,299 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2020 Samsung Electronics Co., Ltd All Rights Reserved
+ */
+
+#include <rte_log.h>
+#include <rte_ethdev.h>
+
+#include <rte_test.h>
+#include "test.h"
+
+
+static int32_t
+test_link_status_up_default(void)
+{
+	int ret = 0;
+	struct rte_eth_link link_status = {
+		.link_speed = ETH_SPEED_NUM_2_5G,
+		.link_status = ETH_LINK_UP,
+		.link_autoneg = ETH_LINK_AUTONEG,
+		.link_duplex = ETH_LINK_FULL_DUPLEX
+	};
+	char text[128];
+	ret = rte_eth_link_strf(text, 128, NULL, &link_status);
+	RTE_TEST_ASSERT(ret > 0, "Failed to format default string\n");
+	printf("Default link up #1: %s\n", text);
+	TEST_ASSERT_BUFFERS_ARE_EQUAL("Link up at 2.5 Gbit/s FDX Autoneg\n",
+		text, strlen(text), "Invalid default link status string");
+
+	link_status.link_duplex = ETH_LINK_HALF_DUPLEX;
+	link_status.link_autoneg = ETH_LINK_FIXED;
+	link_status.link_speed = ETH_SPEED_NUM_10M,
+	ret = rte_eth_link_strf(text, 128, NULL, &link_status);
+	printf("Default link up #2: %s\n", text);
+	RTE_TEST_ASSERT(ret > 0, "Failed to format default string\n");
+	TEST_ASSERT_BUFFERS_ARE_EQUAL("Link up at 10 Mbit/s HDX Fixed\n",
+		text, strlen(text), "Invalid default link status "
+		"string with HDX");
+
+	link_status.link_speed = ETH_SPEED_NUM_UNKNOWN,
+	ret = rte_eth_link_strf(text, 128, NULL, &link_status);
+	printf("Default link up #3: %s\n", text);
+	RTE_TEST_ASSERT(ret > 0, "Failed to format default string\n");
+	TEST_ASSERT_BUFFERS_ARE_EQUAL("Link up at Unknown speed HDX Fixed\n",
+		text, strlen(text), "Invalid default link status "
+		"string with HDX");
+	return TEST_SUCCESS;
+}
+
+static int32_t
+test_link_status_down_default(void)
+{
+	int ret = 0;
+	struct rte_eth_link link_status = {
+		.link_speed = ETH_SPEED_NUM_2_5G,
+		.link_status = ETH_LINK_DOWN,
+		.link_autoneg = ETH_LINK_AUTONEG,
+		.link_duplex = ETH_LINK_FULL_DUPLEX
+	};
+	char text[128];
+	ret = rte_eth_link_strf(text, 128, NULL, &link_status);
+	RTE_TEST_ASSERT(ret > 0, "Failed to format default string\n");
+	TEST_ASSERT_BUFFERS_ARE_EQUAL("Link down\n",
+		text, strlen(text), "Invalid default link status string");
+
+	return TEST_SUCCESS;
+}
+
+static int32_t
+test_link_status_string_overflow(void)
+{
+	int ret = 0;
+	struct rte_eth_link link_status = {
+		.link_speed = ETH_SPEED_NUM_2_5G,
+		.link_status = ETH_LINK_UP,
+		.link_autoneg = ETH_LINK_AUTONEG,
+		.link_duplex = ETH_LINK_FULL_DUPLEX
+	};
+	char text[128];
+	int i = 0;
+	for (i = 0; i < 128; i++)
+		text[i] = 'Y';
+	text[127] = '\0';
+
+	ret = rte_eth_link_strf(NULL, 2, "status %S, %G Gbits/s",
+		&link_status);
+	RTE_TEST_ASSERT(ret < 0, "Format string should fail, but it's ok\n");
+
+	ret = rte_eth_link_strf(text, 2, "status %S, %G Gbits/s",
+		&link_status);
+	RTE_TEST_ASSERT(ret < 0, "Format string should fail, but it's ok\n");
+	RTE_TEST_ASSERT(text[2] == 'Y', "String1 overflow\n");
+
+	ret = rte_eth_link_strf(text, 8, NULL,
+		&link_status);
+	RTE_TEST_ASSERT(ret < 0, "Default format string should fail,"
+			" but it's ok\n");
+	RTE_TEST_ASSERT(text[8] == 'Y', "String1 overflow\n");
+
+	ret = rte_eth_link_strf(text, 10, NULL,
+		&link_status);
+	RTE_TEST_ASSERT(ret < 0, "Default format string should fail,"
+			" but it's ok\n");
+	RTE_TEST_ASSERT(text[10] == 'Y', "String1 overflow\n");
+
+	text[1] = 'Y';
+	ret = rte_eth_link_strf(text, 1, "%S",
+		&link_status);
+	RTE_TEST_ASSERT(ret < 0, "Status string should fail, but it's ok\n");
+	RTE_TEST_ASSERT(text[1] == 'Y', "String1 overflow\n");
+
+	text[1] = 'Y';
+	ret = rte_eth_link_strf(text, 1, "%s",
+		&link_status);
+	RTE_TEST_ASSERT(ret < 0, "Status string should fail, but it's ok\n");
+	RTE_TEST_ASSERT(text[1] == 'Y', "String1 overflow\n");
+
+
+	return TEST_SUCCESS;
+}
+
+static int32_t
+test_link_status_format(void)
+{
+	int ret = 0;
+	struct rte_eth_link link_status = {
+		.link_speed = ETH_SPEED_NUM_40G,
+		.link_status = ETH_LINK_UP,
+		.link_autoneg = ETH_LINK_AUTONEG,
+		.link_duplex = ETH_LINK_FULL_DUPLEX
+	};
+	char text[128];
+	int i = 0;
+	for (i = 0; i < 128; i++)
+		text[i] = 'Y';
+	text[127] = '\0';
+	printf("status format #1: %s\n", text);
+	ret = rte_eth_link_strf(text, 128, "status = %S, duplex = %D\n",
+		&link_status);
+	printf("status format #2: %s\n", text);
+	RTE_TEST_ASSERT(ret > 0, "Failed to format string\n");
+	TEST_ASSERT_BUFFERS_ARE_EQUAL("status = Up, duplex = FDX\n",
+		text, strlen(text), "Invalid status string1.");
+
+	ret = rte_eth_link_strf(text, 128, "%A", &link_status);
+	printf("status format #3: %s\n", text);
+	RTE_TEST_ASSERT(ret > 0, "Failed to format string\n");
+	TEST_ASSERT_BUFFERS_ARE_EQUAL("Autoneg",
+		text, strlen(text), "Invalid status string2.");
+
+	ret = rte_eth_link_strf(text, 128,
+		"%G",
+		&link_status);
+	printf("status format #4: %s\n", text);
+	RTE_TEST_ASSERT(ret > 0, "Failed to format string\n");
+	TEST_ASSERT_BUFFERS_ARE_EQUAL("40.0",
+		text, strlen(text), "Invalid status string3.");
+
+	ret = rte_eth_link_strf(text, 128,
+		"%d %M %",
+		&link_status);
+	printf("status format #5: %s\n", text);
+	RTE_TEST_ASSERT(ret > 0, "Failed to format string\n");
+	TEST_ASSERT_BUFFERS_ARE_EQUAL("%d 40000 %",
+		text, strlen(text), "Invalid status string4.");
+	return TEST_SUCCESS;
+}
+
+static int32_t
+test_link_status_return_value(void)
+{
+	int ret = 0;
+	struct rte_eth_link link_status = {
+		.link_speed = ETH_SPEED_NUM_40G,
+		.link_status = ETH_LINK_UP,
+		.link_autoneg = ETH_LINK_AUTONEG,
+		.link_duplex = ETH_LINK_FULL_DUPLEX
+	};
+	char text[128];
+	int i = 0;
+	for (i = 0; i < 128; i++)
+		text[i] = 'Y';
+	text[127] = '\0';
+	ret = rte_eth_link_strf(text, 128, "status = %S, ",
+		&link_status);
+	printf("return value #1:ret=%u, text=%s\n", ret, text);
+	ret += rte_eth_link_strf(text + ret, 128 - ret,
+		"%A",
+		&link_status);
+	printf("return value #2:ret=%u, text=%s\n", ret, text);
+	ret += rte_eth_link_strf(text + ret, 128 - ret,
+		", duplex = %D\n",
+		&link_status);
+	printf("return value #3:ret=%u, text=%s\n", ret, text);
+	ret += rte_eth_link_strf(text + ret, 128 - ret,
+		"%M Mbits/s\n",
+		&link_status);
+	printf("return value #4:ret=%u, text=%s\n", ret, text);
+	RTE_TEST_ASSERT(ret > 0, "Failed to format string\n");
+	TEST_ASSERT_BUFFERS_ARE_EQUAL("status = Up, Autoneg, duplex = FDX\n"
+		"40000 Mbits/s\n",
+		text, strlen(text), "Invalid status string");
+
+	return TEST_SUCCESS;
+}
+
+static int32_t
+test_link_status_unknown_specifier(void)
+{
+	int ret = 0;
+	struct rte_eth_link link_status = {
+		.link_speed = ETH_SPEED_NUM_40G,
+		.link_status = ETH_LINK_UP,
+		.link_autoneg = ETH_LINK_AUTONEG,
+		.link_duplex = ETH_LINK_FULL_DUPLEX
+	};
+	char text[128];
+	ret = rte_eth_link_strf(text, 128, "status = %",
+		&link_status);
+	RTE_TEST_ASSERT(ret > 0, "Status string1 is failed\n");
+	TEST_ASSERT_BUFFERS_ARE_EQUAL("status = %",
+		text, strlen(text), "Invalid status string1");
+
+	ret = rte_eth_link_strf(text, 128,
+		", duplex = %d\n",
+		&link_status);
+	RTE_TEST_ASSERT(ret > 0, "Status string2 is failed\n");
+	TEST_ASSERT_BUFFERS_ARE_EQUAL(", duplex = %d\n",
+		text, strlen(text), "Invalid status string2");
+
+	ret = rte_eth_link_strf(text, 128,
+		"% Mbits/s\n",
+		&link_status);
+	RTE_TEST_ASSERT(ret > 0, "Status string3 is failed\n");
+	TEST_ASSERT_BUFFERS_ARE_EQUAL("% Mbits/s\n",
+		text, strlen(text), "Invalid status string3");
+
+	ret = rte_eth_link_strf(text, 128,
+		"%w Mbits/s\n",
+		&link_status);
+	RTE_TEST_ASSERT(ret > 0, "Status string4 should be ok, but it's fail\n");
+	TEST_ASSERT_BUFFERS_ARE_EQUAL("%w Mbits/s\n",
+		text, strlen(text), "Invalid status string4");
+	return TEST_SUCCESS;
+}
+
+static int32_t
+test_link_status_format_edges(void)
+{
+	int ret = 0;
+	struct rte_eth_link link_status = {
+		.link_speed = ETH_SPEED_NUM_UNKNOWN,
+		.link_status = ETH_LINK_DOWN,
+		.link_autoneg = ETH_LINK_AUTONEG,
+		.link_duplex = ETH_LINK_HALF_DUPLEX
+	};
+	char text[128];
+	ret = rte_eth_link_strf(text, 4, "%S", &link_status);
+	printf("format edges #1: %s\n", text);
+	RTE_TEST_ASSERT(ret < 0, "It should fail. No space for "
+				 "zero terminator\n");
+	ret = rte_eth_link_strf(text, 6, "123%D", &link_status);
+	printf("format edges #2: %s\n", text);
+	RTE_TEST_ASSERT(ret < 0, "It should fail. No space for "
+				 "zero terminator\n");
+	ret = rte_eth_link_strf(text, 7, "%A", &link_status);
+	printf("format edges #3: %s\n", text);
+	RTE_TEST_ASSERT(ret < 0, "It should fail. No space for "
+				 "zero terminator\n");
+	ret = rte_eth_link_strf(text, 8, "%A", &link_status);
+	printf("format edges #4: %s\n", text);
+	RTE_TEST_ASSERT(ret > 0, "It should ok, but it fails\n");
+	return TEST_SUCCESS;
+}
+static struct unit_test_suite link_status_testsuite = {
+	.suite_name = "link status formatting",
+	.setup = NULL,
+	.teardown = NULL,
+	.unit_test_cases = {
+		TEST_CASE(test_link_status_up_default),
+		TEST_CASE(test_link_status_down_default),
+		TEST_CASE(test_link_status_string_overflow),
+		TEST_CASE(test_link_status_format),
+		TEST_CASE(test_link_status_format_edges),
+		TEST_CASE(test_link_status_unknown_specifier),
+		TEST_CASE(test_link_status_return_value),
+		TEST_CASES_END() /**< NULL terminate unit test array */
+	}
+};
+
+static int
+test_link_status(void)
+{
+	rte_log_set_global_level(RTE_LOG_DEBUG);
+	rte_log_set_level(RTE_LOGTYPE_EAL, RTE_LOG_DEBUG);
+
+	return unit_test_suite_runner(&link_status_testsuite);
+}
+
+REGISTER_TEST_COMMAND(ethdev_link_status, test_link_status);
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index d06b7f9b1..38332b1e4 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -2383,6 +2383,180 @@  rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
 	return 0;
 }
 
+static int
+rte_eth_link_strf_parser(char *str, size_t len, const char *const fmt,
+			   const struct rte_eth_link *link)
+{
+	size_t offset = 0;
+	const char *fmt_cur = fmt;
+	char *str_cur = str;
+	double gbits = (double)link->link_speed / 1000.;
+	static const char autoneg_str[]       = "Autoneg";
+	static const char fixed_str[]         = "Fixed";
+	static const char fdx_str[]           = "FDX";
+	static const char hdx_str[]           = "HDX";
+	static const char unknown_str[]       = "Unknown";
+	static const char up_str[]            = "Up";
+	static const char down_str[]          = "Down";
+	char gbits_str[20];
+	char mbits_str[20];
+
+	/* preformat complex formatting to easily concatinate it further */
+	snprintf(mbits_str, sizeof(mbits_str), "%u", link->link_speed);
+	snprintf(gbits_str, sizeof(gbits_str), "%.1f", gbits);
+	/* init str before formatting */
+	str[0] = 0;
+	while (*fmt_cur) {
+		/* check str bounds */
+		if (offset > (len - 1)) {
+			str[len - 1] = '\0';
+			return -1;
+		}
+		if (*fmt_cur == '%') {
+			/* set null terminator to current position,
+			 * it's required for strlcat
+			 */
+			*str_cur = '\0';
+			switch (*++fmt_cur) {
+			/* Speed in Mbits/s */
+			case 'M':
+				if (link->link_speed ==
+				    ETH_SPEED_NUM_UNKNOWN)
+					offset = strlcat(str, unknown_str,
+							 len);
+				else
+					offset = strlcat(str, mbits_str, len);
+				break;
+			/* Speed in Gbits/s */
+			case 'G':
+				if (link->link_speed ==
+				    ETH_SPEED_NUM_UNKNOWN)
+					offset = strlcat(str, unknown_str,
+							 len);
+				else
+					offset = strlcat(str, gbits_str, len);
+				break;
+			/* Link status */
+			case 'S':
+				offset = strlcat(str, link->link_status ?
+					up_str : down_str, len);
+				break;
+			/* Link autoneg */
+			case 'A':
+				offset = strlcat(str, link->link_autoneg ?
+					autoneg_str : fixed_str, len);
+				break;
+			/* Link duplex */
+			case 'D':
+				offset = strlcat(str, link->link_duplex ?
+					fdx_str : hdx_str, len);
+				break;
+			/* ignore unknown specifier */
+			default:
+				*str_cur = '%';
+				offset++;
+				fmt_cur--;
+				break;
+
+			}
+			if (offset > (len - 1))
+				return -1;
+
+			str_cur = str + offset;
+		} else {
+			*str_cur++ = *fmt_cur;
+			offset++;
+		}
+		fmt_cur++;
+	}
+	*str_cur = '\0';
+	return offset;
+}
+
+int
+rte_eth_link_printf(const char *const fmt,
+		    const struct rte_eth_link *link)
+{
+	char text[200];
+	int ret;
+
+	ret = rte_eth_link_strf(text, 200, fmt, link);
+	if (ret > 0)
+		printf("%s", text);
+	return ret;
+}
+
+int
+rte_eth_link_strf(char *str, size_t len, const char *const fmt,
+		    const struct rte_eth_link *link)
+{
+	size_t offset = 0;
+	double gbits = (double)link->link_speed / 1000.;
+	char speed_gbits_str[20];
+	char speed_mbits_str[20];
+	/* TBD: make it international? */
+	static const char link_down_str[]     = "Link down\n";
+	static const char link_up_str[]       = "Link up at ";
+	static const char unknown_speed_str[] = "Unknown speed ";
+	static const char mbits_str[]	      = "Mbit/s";
+	static const char gbits_str[]	      = "Gbit/s";
+	static const char fdx_str[]           = "FDX ";
+	static const char hdx_str[]           = "HDX ";
+	/* autoneg is latest param in default string, so add '\n' */
+	static const char autoneg_str[]       = "Autoneg\n";
+	static const char fixed_str[]         = "Fixed\n";
+	if (str == NULL || len == 0)
+		return -1;
+	/* default format string, if no fmt is specified */
+	if (fmt == NULL) {
+		if (link->link_status == ETH_LINK_DOWN) {
+			if (sizeof(link_down_str) > len)
+				return -1;
+			return strlcpy(str, link_down_str, len);
+		}
+
+		/* preformat complex strings to easily concatinate it further */
+		snprintf(speed_mbits_str, 20, "%u %s ", link->link_speed,
+			 mbits_str);
+		snprintf(speed_gbits_str, 20, "%.1f %s ", gbits, gbits_str);
+
+		offset = strlcpy(str, link_up_str, len);
+		/* reserve one byte to null terminator */
+		if (offset > (len - 1))
+			return -1;
+		/* link speed */
+		if (link->link_speed == ETH_SPEED_NUM_UNKNOWN) {
+			offset = strlcat(str, unknown_speed_str, len);
+			if (offset > (len - 1))
+				return -1;
+		} else {
+			if (link->link_speed < ETH_SPEED_NUM_1G) {
+				offset = strlcat(str, speed_mbits_str, len);
+				if (offset > (len - 1))
+					return -1;
+			} else {
+				offset = strlcat(str, speed_gbits_str, len);
+				if (offset > (len - 1))
+					return -1;
+			}
+		}
+		/* link duplex */
+		offset = strlcat(str, link->link_duplex ?
+			       fdx_str : hdx_str, len);
+		if (offset > (len - 1))
+			return -1;
+		/* link autonegotiation */
+		offset = strlcat(str, link->link_autoneg ?
+			       autoneg_str : fixed_str, len);
+		if (offset > (len - 1))
+			return -1;
+	/* Formatted status */
+	} else
+		offset = rte_eth_link_strf_parser(str, len, fmt, link);
+
+	return offset;
+}
+
 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..9afb566b3 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -2295,6 +2295,60 @@  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);
 
+
+/**
+ * print formatted link status to stdout. This function threats all
+ * special values like ETH_SPEED_NUM_UNKNOWN, ETH_LINK_DOWN etc. and convert
+ * them to textual representation.
+ *
+ * @param fmt
+ *   Format string which allow to format link status. If NULL is provided
+ *   , default formatting will be applied.
+ *   Following specifiers are available:
+ *    - '%M' link speed in Mbits/s
+ *    - '%G' link speed in Gbits/s
+ *    - '%S' link status. e.g. Up or Down
+ *    - '%A' link autonegotiation state
+ *    - '%D' link duplex state
+ * @param link
+ *   Link status provided by rte_eth_link_get function
+ * @return
+ *   - Number of bytes written to stdout. In case of error, -1 is returned.
+ *
+ */
+__rte_experimental
+int rte_eth_link_printf(const char *const fmt,
+			const struct rte_eth_link *link);
+
+/**
+ * Format link status to textual representation. This function threats all
+ * special values like ETH_SPEED_NUM_UNKNOWN, ETH_LINK_DOWN etc. and convert
+ * them to textual representation.
+ *
+ * @param str
+ *   A pointer to a string to be filled with textual representation of
+ *   device status.
+ * @param len
+ *   Length of available memory at 'str' string.
+ * @param fmt
+ *   Format string which allow to format link status. If NULL is provided
+ *   , default formatting will be applied.
+ *   Following specifiers are available:
+ *    - '%M' link speed in Mbits/s
+ *    - '%G' link speed in Gbits/s
+ *    - '%S' link status. e.g. Up or Down
+ *    - '%A' link autonegotiation state
+ *    - '%D' link duplex state
+ * @param link
+ *   Link status provided by rte_eth_link_get function
+ * @return
+ *   - Number of bytes written to str array. In case of error, -1 is returned.
+ *
+ */
+__rte_experimental
+int rte_eth_link_strf(char *str, size_t len, const char *const fmt,
+			const struct rte_eth_link *eth_link);
+
 /**
  * Retrieve the general I/O statistics of an Ethernet device.
  *
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index 715505604..6c80597d1 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -241,4 +241,8 @@  EXPERIMENTAL {
 	__rte_ethdev_trace_rx_burst;
 	__rte_ethdev_trace_tx_burst;
 	rte_flow_get_aged_flows;
+
+	# added in 20.08
+	rte_eth_link_strf;
+	rte_eth_link_printf;
 };