[PATCH] app/testpmd: support dump ethdev private cmd

[PATCH] app/testpmd: support dump ethdev private cmd

[Chengwen Feng]

This patch adds 'dump_eth_priv [port_id]' which could used to dump
the specific ethdev port private info.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test-pmd/cmdline.c                      | 43 +++++++++++++++++++++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 ++++
 2 files changed, 50 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 6fa870dc32..3ac7e78aff 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -8439,6 +8439,48 @@ static cmdline_parse_inst_t cmd_dump_one = {
 	},
 };
 
+/* ******************************************************************************** */
+
+struct cmd_dump_eth_priv_result {
+	cmdline_fixed_string_t dump;
+	portid_t port_id;
+};
+
+static cmdline_parse_token_string_t cmd_dump_eth_priv_dump =
+	TOKEN_STRING_INITIALIZER(struct cmd_dump_one_result, dump,
+				 "dump_eth_priv");
+static cmdline_parse_token_num_t cmd_dump_eth_priv_port_id =
+	TOKEN_NUM_INITIALIZER(struct cmd_dump_eth_priv_result,
+			      port_id, RTE_UINT16);
+
+static void cmd_dump_eth_priv_parsed(void *parsed_result, struct cmdline *cl,
+				     __rte_unused void *data)
+{
+	struct cmd_dump_eth_priv_result *res = parsed_result;
+	int ret;
+
+	if (!rte_eth_dev_is_valid_port(res->port_id)) {
+		cmdline_printf(cl, "Invalid port id %u\n", res->port_id);
+		return;
+	}
+
+	ret = rte_eth_dev_priv_dump(res->port_id, stdout);
+	if (ret < 0)
+		cmdline_printf(cl, "Failed to dump port id %u private info with error (%d): %s\n",
+			       res->port_id, ret, strerror(-ret));
+}
+
+static cmdline_parse_inst_t cmd_dump_eth_priv = {
+	.f = cmd_dump_eth_priv_parsed,  /* function to call */
+	.data = NULL,      /* 2nd arg of func */
+	.help_str = "dump_eth_priv <port_id>: Dump one ethdev port private info",
+	.tokens = {        /* token list, NULL terminated */
+		(void *)&cmd_dump_eth_priv_dump,
+		(void *)&cmd_dump_eth_priv_port_id,
+		NULL,
+	},
+};
+
 /* *** Filters Control *** */
 
 #define IPV4_ADDR_TO_UINT(ip_addr, ip) \
@@ -12854,6 +12896,7 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_cleanup_txq_mbufs,
 	(cmdline_parse_inst_t *)&cmd_dump,
 	(cmdline_parse_inst_t *)&cmd_dump_one,
+	(cmdline_parse_inst_t *)&cmd_dump_eth_priv,
 	(cmdline_parse_inst_t *)&cmd_flow,
 	(cmdline_parse_inst_t *)&cmd_show_port_meter_cap,
 	(cmdline_parse_inst_t *)&cmd_add_port_meter_profile_srtcm,
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index 8f23847859..211f41f86d 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -605,6 +605,13 @@ Dumps the log level for all the dpdk modules::
 
    testpmd> dump_log_types
 
+dump ethdev private
+~~~~~~~~~~~~~~~~~~~
+
+Dumps the specific ethdev port private info::
+
+   testpmd> dump_eth_priv [port_id]
+
 show (raw_encap|raw_decap)
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-- 
2.17.1

Re: [PATCH] app/testpmd: support dump ethdev private cmd

[Ferruh Yigit]

On 3/14/2023 11:50 AM, Chengwen Feng wrote:
> This patch adds 'dump_eth_priv [port_id]' which could used to dump
> the specific ethdev port private info.
> 

We have already "show port ..." commands to display various port related
information, what do you think to follow same syntax for port private info.

Something like "show port private_info <port_id>"


Or it is possible to append this to the output of the existing port info
("show port info <port_id>"),
as a new section of output if port has private info?

This can be an addition parameter to existing command but not sure if we
need it, like:
"show port info <port_id> private on"

I think displaying private info as port of port info without additional
parameter is OK, it is simpler.


> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  app/test-pmd/cmdline.c                      | 43 +++++++++++++++++++++
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 ++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 6fa870dc32..3ac7e78aff 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -8439,6 +8439,48 @@ static cmdline_parse_inst_t cmd_dump_one = {
>  	},
>  };
>  
> +/* ******************************************************************************** */
> +
> +struct cmd_dump_eth_priv_result {
> +	cmdline_fixed_string_t dump;
> +	portid_t port_id;
> +};
> +
> +static cmdline_parse_token_string_t cmd_dump_eth_priv_dump =
> +	TOKEN_STRING_INITIALIZER(struct cmd_dump_one_result, dump,
> +				 "dump_eth_priv");
> +static cmdline_parse_token_num_t cmd_dump_eth_priv_port_id =
> +	TOKEN_NUM_INITIALIZER(struct cmd_dump_eth_priv_result,
> +			      port_id, RTE_UINT16);
> +
> +static void cmd_dump_eth_priv_parsed(void *parsed_result, struct cmdline *cl,
> +				     __rte_unused void *data)
> +{
> +	struct cmd_dump_eth_priv_result *res = parsed_result;
> +	int ret;
> +
> +	if (!rte_eth_dev_is_valid_port(res->port_id)) {
> +		cmdline_printf(cl, "Invalid port id %u\n", res->port_id);
> +		return;
> +	}
> +
> +	ret = rte_eth_dev_priv_dump(res->port_id, stdout);
> +	if (ret < 0)
> +		cmdline_printf(cl, "Failed to dump port id %u private info with error (%d): %s\n",
> +			       res->port_id, ret, strerror(-ret));
> +}
> +
> +static cmdline_parse_inst_t cmd_dump_eth_priv = {
> +	.f = cmd_dump_eth_priv_parsed,  /* function to call */
> +	.data = NULL,      /* 2nd arg of func */
> +	.help_str = "dump_eth_priv <port_id>: Dump one ethdev port private info",
> +	.tokens = {        /* token list, NULL terminated */
> +		(void *)&cmd_dump_eth_priv_dump,
> +		(void *)&cmd_dump_eth_priv_port_id,
> +		NULL,
> +	},
> +};
> +
>  /* *** Filters Control *** */
>  
>  #define IPV4_ADDR_TO_UINT(ip_addr, ip) \
> @@ -12854,6 +12896,7 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
>  	(cmdline_parse_inst_t *)&cmd_cleanup_txq_mbufs,
>  	(cmdline_parse_inst_t *)&cmd_dump,
>  	(cmdline_parse_inst_t *)&cmd_dump_one,
> +	(cmdline_parse_inst_t *)&cmd_dump_eth_priv,
>  	(cmdline_parse_inst_t *)&cmd_flow,
>  	(cmdline_parse_inst_t *)&cmd_show_port_meter_cap,
>  	(cmdline_parse_inst_t *)&cmd_add_port_meter_profile_srtcm,
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index 8f23847859..211f41f86d 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -605,6 +605,13 @@ Dumps the log level for all the dpdk modules::
>  
>     testpmd> dump_log_types
>  
> +dump ethdev private
> +~~~~~~~~~~~~~~~~~~~
> +
> +Dumps the specific ethdev port private info::
> +
> +   testpmd> dump_eth_priv [port_id]
> +
>  show (raw_encap|raw_decap)
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~
>  

Re: [PATCH] app/testpmd: support dump ethdev private cmd

[fengchengwen]

On 2023/3/14 21:45, Ferruh Yigit wrote:
> On 3/14/2023 11:50 AM, Chengwen Feng wrote:
>> This patch adds 'dump_eth_priv [port_id]' which could used to dump
>> the specific ethdev port private info.
>>
> 
> We have already "show port ..." commands to display various port related
> information, what do you think to follow same syntax for port private info.
> 
> Something like "show port private_info <port_id>"
> 
> 
> Or it is possible to append this to the output of the existing port info
> ("show port info <port_id>"),
> as a new section of output if port has private info?
> 
> This can be an addition parameter to existing command but not sure if we
> need it, like:
> "show port info <port_id> private on"
> 
> I think displaying private info as port of port info without additional
> parameter is OK, it is simpler.

+1 for this, will send a new patch.

Thanks.

> 
> 

...

[PATCH] app/testpmd: dump private info in 'show port info'

[Chengwen Feng]

This patch adds dump private info in 'show port info [port_id]' cmd.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test-pmd/config.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 018536f177..ab5199f223 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -938,6 +938,11 @@ port_infos_display(portid_t port_id)
 		printf("unknown\n");
 		break;
 	}
+	printf("Device private info:\n");
+	ret = rte_eth_dev_priv_dump(port_id, stdout);
+	if (ret < 0)
+		fprintf(stderr, "  Failed to dump private info with error (%d): %s\n",
+			ret, strerror(-ret));
 }
 
 void
-- 
2.17.1

Re: [PATCH] app/testpmd: dump private info in 'show port info'

[Singh, Aman Deep]

On 3/15/2023 8:03 AM, Chengwen Feng wrote:
> This patch adds dump private info in 'show port info [port_id]' cmd.
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

Acked-by: Aman Singh <aman.deep.singh@intel.com>

> ---
>   app/test-pmd/config.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 018536f177..ab5199f223 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -938,6 +938,11 @@ port_infos_display(portid_t port_id)
>   		printf("unknown\n");
>   		break;
>   	}
> +	printf("Device private info:\n");
> +	ret = rte_eth_dev_priv_dump(port_id, stdout);
> +	if (ret < 0)
> +		fprintf(stderr, "  Failed to dump private info with error (%d): %s\n",
> +			ret, strerror(-ret));
>   }
>   
>   void

Re: [PATCH] app/testpmd: dump private info in 'show port info'

[Ferruh Yigit]

On 3/15/2023 2:33 AM, Chengwen Feng wrote:
> This patch adds dump private info in 'show port info [port_id]' cmd.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  app/test-pmd/config.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 018536f177..ab5199f223 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -938,6 +938,11 @@ port_infos_display(portid_t port_id)
>  		printf("unknown\n");
>  		break;
>  	}
> +	printf("Device private info:\n");

Not all drivers support device private info, above should print only
when it is supported.

> +	ret = rte_eth_dev_priv_dump(port_id, stdout);
> +	if (ret < 0)
> +		fprintf(stderr, "  Failed to dump private info with error (%d): %s\n",
> +			ret, strerror(-ret));

Similarly, if driver doesn't support '.eth_dev_priv_dump' it shouldn't
print the above error log.

Re: [PATCH] app/testpmd: dump private info in 'show port info'

[fengchengwen]

On 2023/3/15 18:12, Ferruh Yigit wrote:
> On 3/15/2023 2:33 AM, Chengwen Feng wrote:
>> This patch adds dump private info in 'show port info [port_id]' cmd.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>>  app/test-pmd/config.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index 018536f177..ab5199f223 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -938,6 +938,11 @@ port_infos_display(portid_t port_id)
>>  		printf("unknown\n");
>>  		break;
>>  	}
>> +	printf("Device private info:\n");
> 
> Not all drivers support device private info, above should print only
> when it is supported.
> 
>> +	ret = rte_eth_dev_priv_dump(port_id, stdout);
>> +	if (ret < 0)
>> +		fprintf(stderr, "  Failed to dump private info with error (%d): %s\n",
>> +			ret, strerror(-ret));
> 
> Similarly, if driver doesn't support '.eth_dev_priv_dump' it shouldn't
> print the above error log.
> 

Because we have no API to know the PMD whether impl specific ops, we could only knowed by invoking.
Except above impl, I also consider the other two:
1. just invoke rte_eth_dev_priv_dump without previous printf("Device private info") and later error printf.
   and I think people may curious about the extra output without a prompt just like "Device private info".
2. use fmemopen (the below code), this way will perfect process the PMD which not imp ops.
   FILE *f = fmemopen(buf, max-size(e.g. 128KB));
   ret = rte_eth_dev_priv_dump(port_id, f);
   if (ret == 0) {
      printf("Device private info:\n");
      printf("%s", buf);
   }
   But the windows platform don't support fmemopen.

Hope for more feedback.

Thanks

> .
> 

Re: [PATCH] app/testpmd: dump private info in 'show port info'

[Thomas Monjalon]

16/03/2023 02:10, fengchengwen:
> On 2023/3/15 18:12, Ferruh Yigit wrote:
> > On 3/15/2023 2:33 AM, Chengwen Feng wrote:
> >> This patch adds dump private info in 'show port info [port_id]' cmd.
> >>
> >> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >> ---
> >>  app/test-pmd/config.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> >> index 018536f177..ab5199f223 100644
> >> --- a/app/test-pmd/config.c
> >> +++ b/app/test-pmd/config.c
> >> @@ -938,6 +938,11 @@ port_infos_display(portid_t port_id)
> >>  		printf("unknown\n");
> >>  		break;
> >>  	}
> >> +	printf("Device private info:\n");
> > 
> > Not all drivers support device private info, above should print only
> > when it is supported.
> > 
> >> +	ret = rte_eth_dev_priv_dump(port_id, stdout);
> >> +	if (ret < 0)
> >> +		fprintf(stderr, "  Failed to dump private info with error (%d): %s\n",
> >> +			ret, strerror(-ret));
> > 
> > Similarly, if driver doesn't support '.eth_dev_priv_dump' it shouldn't
> > print the above error log.
> > 
> 
> Because we have no API to know the PMD whether impl specific ops, we could only knowed by invoking.
> Except above impl, I also consider the other two:
> 1. just invoke rte_eth_dev_priv_dump without previous printf("Device private info") and later error printf.
>    and I think people may curious about the extra output without a prompt just like "Device private info".

You can print the error only if the return is not ENOTSUP.
You can keep the first print and then print "none" if ENOTSUP.

> 2. use fmemopen (the below code), this way will perfect process the PMD which not imp ops.
>    FILE *f = fmemopen(buf, max-size(e.g. 128KB));
>    ret = rte_eth_dev_priv_dump(port_id, f);
>    if (ret == 0) {
>       printf("Device private info:\n");
>       printf("%s", buf);
>    }
>    But the windows platform don't support fmemopen.

That's also a good solution but we need to support Windows.

Re: [PATCH] app/testpmd: dump private info in 'show port info'

[Dmitry Kozlyuk]

On Thu, Mar 16, 2023 at 4:11 AM fengchengwen <fengchengwen@huawei.com> wrote:
> Because we have no API to know the PMD whether impl specific ops, we could only knowed by invoking.
> Except above impl, I also consider the other two:
> 1. just invoke rte_eth_dev_priv_dump without previous printf("Device private info") and later error printf.
>    and I think people may curious about the extra output without a prompt just like "Device private info".
> 2. use fmemopen (the below code), this way will perfect process the PMD which not imp ops.
>    FILE *f = fmemopen(buf, max-size(e.g. 128KB));
>    ret = rte_eth_dev_priv_dump(port_id, f);
>    if (ret == 0) {
>       printf("Device private info:\n");
>       printf("%s", buf);
>    }
>    But the windows platform don't support fmemopen.
>
> Hope for more feedback.

What if rte_eth_dev_priv_dump() was a documented no-op when "f == NULL"?
This can be implemented in ethdev layer:
1) if not implemented, return ENOTSUP
2) if f == NULL, return 0
3) else call PMD
Technically, even now a null device handle can be used, but this is
cumbersome and wastes resources for running the API twice.

[PATCH v2] app/testpmd: dump private info in 'show port info'

[Chengwen Feng]

This patch adds dump private info in 'show port info [port_id]' cmd.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Aman Singh <aman.deep.singh@intel.com>

---
v2: add identify ENOTSUP retcode according Thomas's comment.
---
 app/test-pmd/config.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 018536f177..22b88c67b9 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -938,6 +938,13 @@ port_infos_display(portid_t port_id)
 		printf("unknown\n");
 		break;
 	}
+	printf("Device private info:\n");
+	ret = rte_eth_dev_priv_dump(port_id, stdout);
+	if (ret == -ENOTSUP)
+		printf("  none\n");
+	else if (ret < 0)
+		fprintf(stderr, "  Failed to dump private info with error (%d): %s\n",
+			ret, strerror(-ret));
 }
 
 void
-- 
2.17.1

Re: [PATCH] app/testpmd: dump private info in 'show port info'

[fengchengwen]

On 2023/3/16 15:55, Thomas Monjalon wrote:
> 16/03/2023 02:10, fengchengwen:
>> On 2023/3/15 18:12, Ferruh Yigit wrote:
>>> On 3/15/2023 2:33 AM, Chengwen Feng wrote:
>>>> This patch adds dump private info in 'show port info [port_id]' cmd.
>>>>
>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>> ---
>>>>  app/test-pmd/config.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>>> index 018536f177..ab5199f223 100644
>>>> --- a/app/test-pmd/config.c
>>>> +++ b/app/test-pmd/config.c
>>>> @@ -938,6 +938,11 @@ port_infos_display(portid_t port_id)
>>>>  		printf("unknown\n");
>>>>  		break;
>>>>  	}
>>>> +	printf("Device private info:\n");
>>>
>>> Not all drivers support device private info, above should print only
>>> when it is supported.
>>>
>>>> +	ret = rte_eth_dev_priv_dump(port_id, stdout);
>>>> +	if (ret < 0)
>>>> +		fprintf(stderr, "  Failed to dump private info with error (%d): %s\n",
>>>> +			ret, strerror(-ret));
>>>
>>> Similarly, if driver doesn't support '.eth_dev_priv_dump' it shouldn't
>>> print the above error log.
>>>
>>
>> Because we have no API to know the PMD whether impl specific ops, we could only knowed by invoking.
>> Except above impl, I also consider the other two:
>> 1. just invoke rte_eth_dev_priv_dump without previous printf("Device private info") and later error printf.
>>    and I think people may curious about the extra output without a prompt just like "Device private info".
> 
> You can print the error only if the return is not ENOTSUP.
> You can keep the first print and then print "none" if ENOTSUP.

Good idea, fix in v2, thanks.

> 
>> 2. use fmemopen (the below code), this way will perfect process the PMD which not imp ops.
>>    FILE *f = fmemopen(buf, max-size(e.g. 128KB));
>>    ret = rte_eth_dev_priv_dump(port_id, f);
>>    if (ret == 0) {
>>       printf("Device private info:\n");
>>       printf("%s", buf);
>>    }
>>    But the windows platform don't support fmemopen.
> 
> That's also a good solution but we need to support Windows.
> 
> 
> 
> .
> 

Re: [PATCH] app/testpmd: dump private info in 'show port info'

[fengchengwen]

On 2023/3/16 17:19, Dmitry Kozlyuk wrote:
> On Thu, Mar 16, 2023 at 4:11 AM fengchengwen <fengchengwen@huawei.com> wrote:
>> Because we have no API to know the PMD whether impl specific ops, we could only knowed by invoking.
>> Except above impl, I also consider the other two:
>> 1. just invoke rte_eth_dev_priv_dump without previous printf("Device private info") and later error printf.
>>    and I think people may curious about the extra output without a prompt just like "Device private info".
>> 2. use fmemopen (the below code), this way will perfect process the PMD which not imp ops.
>>    FILE *f = fmemopen(buf, max-size(e.g. 128KB));
>>    ret = rte_eth_dev_priv_dump(port_id, f);
>>    if (ret == 0) {
>>       printf("Device private info:\n");
>>       printf("%s", buf);
>>    }
>>    But the windows platform don't support fmemopen.
>>
>> Hope for more feedback.
> 
> What if rte_eth_dev_priv_dump() was a documented no-op when "f == NULL"?
> This can be implemented in ethdev layer:
> 1) if not implemented, return ENOTSUP
> 2) if f == NULL, return 0
> 3) else call PMD
> Technically, even now a null device handle can be used, but this is
> cumbersome and wastes resources for running the API twice.

Thanks your advise.
V2 has sent which adopt Thomas's comment:
	printf("Device private info:\n");
	ret = rte_eth_dev_priv_dump(port_id, stdout);
	if (ret == -ENOTSUP)
		printf("  none\n");
	else if (ret < 0)
		fprintf(stderr, "  Failed to dump private info with error (%d): %s\n",
			ret, strerror(-ret));
which seem simpler.

> .
> 

Re: [PATCH] app/testpmd: dump private info in 'show port info'

[Ferruh Yigit]

On 3/16/2023I 9:19 AM, Dmitry Kozlyuk wrote:
> On Thu, Mar 16, 2023 at 4:11 AM fengchengwen <fengchengwen@huawei.com> wrote:
>> Because we have no API to know the PMD whether impl specific ops, we could only knowed by invoking.
>> Except above impl, I also consider the other two:
>> 1. just invoke rte_eth_dev_priv_dump without previous printf("Device private info") and later error printf.
>>    and I think people may curious about the extra output without a prompt just like "Device private info".
>> 2. use fmemopen (the below code), this way will perfect process the PMD which not imp ops.
>>    FILE *f = fmemopen(buf, max-size(e.g. 128KB));
>>    ret = rte_eth_dev_priv_dump(port_id, f);
>>    if (ret == 0) {
>>       printf("Device private info:\n");
>>       printf("%s", buf);
>>    }
>>    But the windows platform don't support fmemopen.
>>
>> Hope for more feedback.
> 
> What if rte_eth_dev_priv_dump() was a documented no-op when "f == NULL"?
> This can be implemented in ethdev layer:
> 1) if not implemented, return ENOTSUP
> 2) if f == NULL, return 0
> 3) else call PMD
> Technically, even now a null device handle can be used, but this is
> cumbersome and wastes resources for running the API twice.


Not sure about to overload "f == NULL" condition to detect the feature
support.

It may be good to have a generic way to detect the support.

One way is to add new set of APIs just to test the dev_ops, and return
boolean like:
'bool rte_eth_dev_is_priv_dump(uint16_t port_id);'


Another option can be introducing an enum, each enum item can represent
a dev_ops and a single API can be used to detect the support. This
requires more maintenance for long term, as app needs to know more,
not sure if I like this.


If there is no objection to add new APIs, I can own the task, to have
APIs like:
'bool rte_eth_dev_is_xxx(uint16_t port_id);'

Re: [PATCH v2] app/testpmd: dump private info in 'show port info'

[Ferruh Yigit]

On 3/16/2023 9:32 AM, Chengwen Feng wrote:
> This patch adds dump private info in 'show port info [port_id]' cmd.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Acked-by: Aman Singh <aman.deep.singh@intel.com>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

Applied to dpdk-next-net/main, thanks.

Re: [PATCH] app/testpmd: dump private info in 'show port info'

[Thomas Monjalon]

16/03/2023 11:46, Ferruh Yigit:
> On 3/16/2023I 9:19 AM, Dmitry Kozlyuk wrote:
> > On Thu, Mar 16, 2023 at 4:11 AM fengchengwen <fengchengwen@huawei.com> wrote:
> >> Because we have no API to know the PMD whether impl specific ops, we could only knowed by invoking.
> >> Except above impl, I also consider the other two:
> >> 1. just invoke rte_eth_dev_priv_dump without previous printf("Device private info") and later error printf.
> >>    and I think people may curious about the extra output without a prompt just like "Device private info".
> >> 2. use fmemopen (the below code), this way will perfect process the PMD which not imp ops.
> >>    FILE *f = fmemopen(buf, max-size(e.g. 128KB));
> >>    ret = rte_eth_dev_priv_dump(port_id, f);
> >>    if (ret == 0) {
> >>       printf("Device private info:\n");
> >>       printf("%s", buf);
> >>    }
> >>    But the windows platform don't support fmemopen.
> >>
> >> Hope for more feedback.
> > 
> > What if rte_eth_dev_priv_dump() was a documented no-op when "f == NULL"?
> > This can be implemented in ethdev layer:
> > 1) if not implemented, return ENOTSUP
> > 2) if f == NULL, return 0
> > 3) else call PMD
> > Technically, even now a null device handle can be used, but this is
> > cumbersome and wastes resources for running the API twice.
> 
> 
> Not sure about to overload "f == NULL" condition to detect the feature
> support.
> 
> It may be good to have a generic way to detect the support.
> 
> One way is to add new set of APIs just to test the dev_ops, and return
> boolean like:
> 'bool rte_eth_dev_is_priv_dump(uint16_t port_id);'
> 
> 
> Another option can be introducing an enum, each enum item can represent
> a dev_ops and a single API can be used to detect the support. This
> requires more maintenance for long term, as app needs to know more,
> not sure if I like this.
> 
> 
> If there is no objection to add new APIs, I can own the task, to have
> APIs like:
> 'bool rte_eth_dev_is_xxx(uint16_t port_id);'

That's a debug function, so I don't think we need to know in advance
whether it will print something or not.
Returning ENOTSUP should be enough in my opinion.