DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] app/procinfo: add device registers dump
@ 2021-04-25 13:02 Min Hu (Connor)
  2021-05-19 10:16 ` Thomas Monjalon
                   ` (5 more replies)
  0 siblings, 6 replies; 15+ messages in thread
From: Min Hu (Connor) @ 2021-04-25 13:02 UTC (permalink / raw)
  To: dev; +Cc: maryam.tahhan, reshma.pattan

From: Chengchang Tang <tangchengchang@huawei.com>

This patch add support for dump the device registers from a running
application. It can help developers locate the problem.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 app/proc-info/main.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 91 insertions(+), 1 deletion(-)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index b9587f7..c555be5 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -94,6 +94,9 @@ static char *mempool_name;
 /**< Enable iter mempool. */
 static uint32_t enable_iter_mempool;
 static char *mempool_iter_name;
+/**< Enable dump regs. */
+static uint32_t enable_dump_regs;
+static char *dump_regs_file_prefix;
 
 /**< display usage */
 static void
@@ -119,7 +122,8 @@ proc_info_usage(const char *prgname)
 		"  --show-crypto: to display crypto information\n"
 		"  --show-ring[=name]: to display ring information\n"
 		"  --show-mempool[=name]: to display mempool information\n"
-		"  --iter-mempool=name: iterate mempool elements to display content\n",
+		"  --iter-mempool=name: iterate mempool elements to display content\n"
+		"  --dump-regs=file-prefix: dump reg to file with the file-prefix\n",
 		prgname);
 }
 
@@ -226,6 +230,7 @@ proc_info_parse_args(int argc, char **argv)
 		{"show-ring", optional_argument, NULL, 0},
 		{"show-mempool", optional_argument, NULL, 0},
 		{"iter-mempool", required_argument, NULL, 0},
+		{"dump-regs", required_argument, NULL, 0},
 		{NULL, 0, 0, 0}
 	};
 
@@ -288,6 +293,10 @@ proc_info_parse_args(int argc, char **argv)
 					"iter-mempool", MAX_LONG_OPT_SZ)) {
 				enable_iter_mempool = 1;
 				mempool_iter_name = optarg;
+			} else if (!strncmp(long_option[option_index].name,
+					"dump-regs", MAX_LONG_OPT_SZ)) {
+				enable_dump_regs = 1;
+				dump_regs_file_prefix = optarg;
 			}
 			break;
 		case 1:
@@ -1349,6 +1358,85 @@ iter_mempool(char *name)
 	}
 }
 
+static void
+dump_regs(char *file_prefix)
+{
+#define MAX_FILE_NAME_SZ (MAX_LONG_OPT_SZ + 10)
+	char file_name[MAX_FILE_NAME_SZ];
+	struct rte_dev_reg_info reg_info;
+	struct rte_eth_dev_info dev_info;
+	unsigned char *buf_data;
+	FILE *fp_regs;
+	int buf_size;
+	uint16_t i;
+	int ret;
+
+	snprintf(bdr_str, MAX_STRING_LEN, " dump - Port REG");
+	STATS_BDR_STR(10, bdr_str);
+
+	RTE_ETH_FOREACH_DEV(i) {
+		/* Skip if port is not in mask */
+		if ((enabled_port_mask & (1ul << i)) == 0)
+			continue;
+
+		snprintf(bdr_str, MAX_STRING_LEN, " Port (%u)", i);
+		STATS_BDR_STR(5, bdr_str);
+
+		memset(&reg_info, 0, sizeof(reg_info));
+		memset(&dev_info, 0, sizeof(dev_info));
+
+		ret = rte_eth_dev_info_get(i, &dev_info);
+		if (ret) {
+			printf("Error getting device info: %d\n", ret);
+			continue;
+		}
+
+		ret = rte_eth_dev_get_reg_info(i, &reg_info);
+		if (ret) {
+			printf("Error getting device reg info: %d\n", ret);
+			continue;
+		}
+
+		buf_size = reg_info.length * reg_info.width;
+		buf_data = malloc(buf_size);
+		if (buf_data == NULL) {
+			printf("Error allocating %d bytes buffer\n", buf_size);
+			continue;
+		}
+
+		reg_info.data = buf_data;
+		reg_info.length = 0;
+		ret = rte_eth_dev_get_reg_info(i, &reg_info);
+		if (ret) {
+			printf("Error getting regs from device: %d\n", ret);
+			free(buf_data);
+			continue;
+		}
+
+		snprintf(file_name, MAX_FILE_NAME_SZ, "%s-port%u",
+				file_prefix, i);
+		fp_regs = fopen(file_name, "wb");
+		if (fp_regs == NULL) {
+			printf("Error during opening '%s' for writing\n",
+					file_name);
+		} else {
+			if ((int)fwrite(buf_data, 1, buf_size, fp_regs) !=
+					buf_size)
+				printf("Error during writing %s\n",
+						file_prefix);
+			else
+				printf("dump device (%s) regs successfully, "
+					"driver:%s version:0X%08X\n",
+					dev_info.device->name,
+					dev_info.driver_name, reg_info.version);
+
+			fclose(fp_regs);
+		}
+
+		free(buf_data);
+	}
+}
+
 int
 main(int argc, char **argv)
 {
@@ -1454,6 +1542,8 @@ main(int argc, char **argv)
 		show_mempool(mempool_name);
 	if (enable_iter_mempool)
 		iter_mempool(mempool_iter_name);
+	if (enable_dump_regs)
+		dump_regs(dump_regs_file_prefix);
 
 	RTE_ETH_FOREACH_DEV(i)
 		rte_eth_dev_close(i);
-- 
2.7.4


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] app/procinfo: add device registers dump
  2021-04-25 13:02 [dpdk-dev] [PATCH] app/procinfo: add device registers dump Min Hu (Connor)
@ 2021-05-19 10:16 ` Thomas Monjalon
  2021-06-04 15:04 ` Pattan, Reshma
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Thomas Monjalon @ 2021-05-19 10:16 UTC (permalink / raw)
  To: maryam.tahhan, reshma.pattan, Min Hu (Connor); +Cc: dev

Ping for review.

It looks too late for 21.05, moved as Deferred in patchwork.

25/04/2021 15:02, Min Hu (Connor):
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> This patch add support for dump the device registers from a running
> application. It can help developers locate the problem.
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] app/procinfo: add device registers dump
  2021-04-25 13:02 [dpdk-dev] [PATCH] app/procinfo: add device registers dump Min Hu (Connor)
  2021-05-19 10:16 ` Thomas Monjalon
@ 2021-06-04 15:04 ` Pattan, Reshma
  2021-06-05  3:15   ` Chengchang Tang
  2021-06-21  2:17 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Pattan, Reshma @ 2021-06-04 15:04 UTC (permalink / raw)
  To: Min Hu (Connor), dev; +Cc: Tahhan, Maryam



> -----Original Message-----
> From: Min Hu (Connor) <humin29@huawei.com>

<snip> 

> +		ret = rte_eth_dev_get_reg_info(i, &reg_info);
> +		if (ret) {
> +			printf("Error getting device reg info: %d\n", ret);
> +			continue;
> +		}
> +
> +		buf_size = reg_info.length * reg_info.width;


If it is to get the regs length, you can directly call  "rte_ethtool_get_regs_len(uint16_t port_id)" API , instead of  again writing the above logic.
And use the returned length in below malloc.


> +		fp_regs = fopen(file_name, "wb");
> +		if (fp_regs == NULL) {
> +			printf("Error during opening '%s' for writing\n",
> +					file_name);

Better to print error string from fopen() errno on failure , to indicate the exact error.

> +		} else {
> +			if ((int)fwrite(buf_data, 1, buf_size, fp_regs) !=

Better have "((int)fwrite(buf_data, 1, buf_size, fp_regs)" In separate line and use the returned value inside if check.

> +					buf_size)
> +				printf("Error during writing %s\n",
> +						file_prefix);

Better to print error string from fwrite errno on failure , to indicate the exact error.

> +			else
> +				printf("dump device (%s) regs successfully, "

Reframe the sente to "Device regs dumped successfully"

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] app/procinfo: add device registers dump
  2021-06-04 15:04 ` Pattan, Reshma
@ 2021-06-05  3:15   ` Chengchang Tang
  2021-06-10 16:25     ` Pattan, Reshma
  0 siblings, 1 reply; 15+ messages in thread
From: Chengchang Tang @ 2021-06-05  3:15 UTC (permalink / raw)
  To: Pattan, Reshma, Min Hu (Connor), dev; +Cc: Tahhan, Maryam


On 2021/6/4 23:04, Pattan, Reshma wrote:
> 
> 
>> -----Original Message-----
>> From: Min Hu (Connor) <humin29@huawei.com>
> 
> <snip> 
> 
>> +		ret = rte_eth_dev_get_reg_info(i, &reg_info);
>> +		if (ret) {
>> +			printf("Error getting device reg info: %d\n", ret);
>> +			continue;
>> +		}
>> +
>> +		buf_size = reg_info.length * reg_info.width;
> 
> 
> If it is to get the regs length, you can directly call  "rte_ethtool_get_regs_len(uint16_t port_id)" API , instead of  again writing the above logic.
> And use the returned length in below malloc.

This logic is indeed identical to the logic of the "rte_ethtool_get_regs_len" API of Ethtool,
but the method of using the "rte_eth_dev_get_reg_info" API is the case. All users will have
similar code logic when using this API.

And use "rte_ethtool_get_regs_len" API here only reduces "buf_size = reg_info.length * reg_info.width;"
this logic. But at the same time, it introduces the dependence of example/ethtool in procinfo. The code in
example is not compiled by default, which seems not appropriate to import it in app/procinfo.

So, I think it is fine to keep this.
>
> 
>> +		fp_regs = fopen(file_name, "wb");
>> +		if (fp_regs == NULL) {
>> +			printf("Error during opening '%s' for writing\n",
>> +					file_name);
> 
> Better to print error string from fopen() errno on failure , to indicate the exact error.

Agree, I will fix it in the next version.

> 
>> +		} else {
>> +			if ((int)fwrite(buf_data, 1, buf_size, fp_regs) !=
> 
> Better have "((int)fwrite(buf_data, 1, buf_size, fp_regs)" In separate line and use the returned value inside if check.

Agree, I will fix it in the next version.

>
>> +					buf_size)
>> +				printf("Error during writing %s\n",
>> +						file_prefix);
> 
> Better to print error string from fwrite errno on failure , to indicate the exact error.
> 
>> +			else
>> +				printf("dump device (%s) regs successfully, "
> 
> Reframe the sente to "Device regs dumped successfully"
> 

Agree, I will fix it in the next version.
> .
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] app/procinfo: add device registers dump
  2021-06-05  3:15   ` Chengchang Tang
@ 2021-06-10 16:25     ` Pattan, Reshma
  0 siblings, 0 replies; 15+ messages in thread
From: Pattan, Reshma @ 2021-06-10 16:25 UTC (permalink / raw)
  To: Chengchang Tang, Min Hu (Connor), dev; +Cc: Tahhan, Maryam



> -----Original Message-----
> From: Chengchang Tang <tangchengchang@huawei.com>


> >> +		ret = rte_eth_dev_get_reg_info(i, &reg_info);
> >> +		if (ret) {
> >> +			printf("Error getting device reg info: %d\n", ret);
> >> +			continue;
> >> +		}
> >> +
> >> +		buf_size = reg_info.length * reg_info.width;
> >
> >
> > If it is to get the regs length, you can directly call
> "rte_ethtool_get_regs_len(uint16_t port_id)" API , instead of  again writing the
> above logic.
> > And use the returned length in below malloc.
> 
> This logic is indeed identical to the logic of the "rte_ethtool_get_regs_len" API
> of Ethtool, but the method of using the "rte_eth_dev_get_reg_info" API is the
> case. All users will have similar code logic when using this API.
> 

Oh yes, my bad, I overlooked it. It makes complete sense now. 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v2] app/procinfo: add device registers dump
  2021-04-25 13:02 [dpdk-dev] [PATCH] app/procinfo: add device registers dump Min Hu (Connor)
  2021-05-19 10:16 ` Thomas Monjalon
  2021-06-04 15:04 ` Pattan, Reshma
@ 2021-06-21  2:17 ` Min Hu (Connor)
  2021-07-09 21:55   ` Thomas Monjalon
  2021-07-17 17:51 ` [dpdk-dev] [PATCH] " Stephen Hemminger
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Min Hu (Connor) @ 2021-06-21  2:17 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, reshma.pattan

From: Chengchang Tang <tangchengchang@huawei.com>

This patch add support for dump the device registers from a running
application. It can help developers locate the problem.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
v2:
* some logs are adjusted and error string are printed after
file operation fails.
---
 app/proc-info/main.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 93 insertions(+), 1 deletion(-)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index b9587f7..553d686 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -94,6 +94,9 @@ static char *mempool_name;
 /**< Enable iter mempool. */
 static uint32_t enable_iter_mempool;
 static char *mempool_iter_name;
+/**< Enable dump regs. */
+static uint32_t enable_dump_regs;
+static char *dump_regs_file_prefix;
 
 /**< display usage */
 static void
@@ -119,7 +122,8 @@ proc_info_usage(const char *prgname)
 		"  --show-crypto: to display crypto information\n"
 		"  --show-ring[=name]: to display ring information\n"
 		"  --show-mempool[=name]: to display mempool information\n"
-		"  --iter-mempool=name: iterate mempool elements to display content\n",
+		"  --iter-mempool=name: iterate mempool elements to display content\n"
+		"  --dump-regs=file-prefix: dump reg to file with the file-prefix\n",
 		prgname);
 }
 
@@ -226,6 +230,7 @@ proc_info_parse_args(int argc, char **argv)
 		{"show-ring", optional_argument, NULL, 0},
 		{"show-mempool", optional_argument, NULL, 0},
 		{"iter-mempool", required_argument, NULL, 0},
+		{"dump-regs", required_argument, NULL, 0},
 		{NULL, 0, 0, 0}
 	};
 
@@ -288,6 +293,10 @@ proc_info_parse_args(int argc, char **argv)
 					"iter-mempool", MAX_LONG_OPT_SZ)) {
 				enable_iter_mempool = 1;
 				mempool_iter_name = optarg;
+			} else if (!strncmp(long_option[option_index].name,
+					"dump-regs", MAX_LONG_OPT_SZ)) {
+				enable_dump_regs = 1;
+				dump_regs_file_prefix = optarg;
 			}
 			break;
 		case 1:
@@ -1349,6 +1358,87 @@ iter_mempool(char *name)
 	}
 }
 
+static void
+dump_regs(char *file_prefix)
+{
+#define MAX_FILE_NAME_SZ (MAX_LONG_OPT_SZ + 10)
+	char file_name[MAX_FILE_NAME_SZ];
+	struct rte_dev_reg_info reg_info;
+	struct rte_eth_dev_info dev_info;
+	unsigned char *buf_data;
+	size_t buf_size;
+	FILE *fp_regs;
+	uint16_t i;
+	int ret;
+
+	snprintf(bdr_str, MAX_STRING_LEN, " dump - Port REG");
+	STATS_BDR_STR(10, bdr_str);
+
+	RTE_ETH_FOREACH_DEV(i) {
+		/* Skip if port is not in mask */
+		if ((enabled_port_mask & (1ul << i)) == 0)
+			continue;
+
+		snprintf(bdr_str, MAX_STRING_LEN, " Port (%u)", i);
+		STATS_BDR_STR(5, bdr_str);
+
+		memset(&reg_info, 0, sizeof(reg_info));
+		memset(&dev_info, 0, sizeof(dev_info));
+
+		ret = rte_eth_dev_info_get(i, &dev_info);
+		if (ret) {
+			printf("Error getting device info: %d\n", ret);
+			continue;
+		}
+
+		ret = rte_eth_dev_get_reg_info(i, &reg_info);
+		if (ret) {
+			printf("Error getting device reg info: %d\n", ret);
+			continue;
+		}
+
+		buf_size = reg_info.length * reg_info.width;
+		buf_data = malloc(buf_size);
+		if (buf_data == NULL) {
+			printf("Error allocating %zu bytes buffer\n", buf_size);
+			continue;
+		}
+
+		reg_info.data = buf_data;
+		reg_info.length = 0;
+		ret = rte_eth_dev_get_reg_info(i, &reg_info);
+		if (ret) {
+			printf("Error getting regs from device: %d\n", ret);
+			free(buf_data);
+			continue;
+		}
+
+		snprintf(file_name, MAX_FILE_NAME_SZ, "%s-port%u",
+				file_prefix, i);
+		fp_regs = fopen(file_name, "wb");
+		if (fp_regs == NULL) {
+			printf("Error during opening '%s' for writing: %s\n",
+					file_name, strerror(errno));
+		} else {
+			size_t nr_written;
+
+			nr_written = fwrite(buf_data, 1, buf_size, fp_regs);
+			if (nr_written != buf_size)
+				printf("Error during writing %s: %s\n",
+						file_prefix, strerror(errno));
+			else
+				printf("Device (%s) regs dumped successfully, "
+					"driver:%s version:0X%08X\n",
+					dev_info.device->name,
+					dev_info.driver_name, reg_info.version);
+
+			fclose(fp_regs);
+		}
+
+		free(buf_data);
+	}
+}
+
 int
 main(int argc, char **argv)
 {
@@ -1454,6 +1544,8 @@ main(int argc, char **argv)
 		show_mempool(mempool_name);
 	if (enable_iter_mempool)
 		iter_mempool(mempool_iter_name);
+	if (enable_dump_regs)
+		dump_regs(dump_regs_file_prefix);
 
 	RTE_ETH_FOREACH_DEV(i)
 		rte_eth_dev_close(i);
-- 
2.7.4


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2] app/procinfo: add device registers dump
  2021-06-21  2:17 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
@ 2021-07-09 21:55   ` Thomas Monjalon
  2021-07-17  2:16     ` Min Hu (Connor)
  0 siblings, 1 reply; 15+ messages in thread
From: Thomas Monjalon @ 2021-07-09 21:55 UTC (permalink / raw)
  To: reshma.pattan, Maryam Tahhan
  Cc: dev, ferruh.yigit, Min Hu (Connor), bruce.richardson

21/06/2021 04:17, Min Hu (Connor):
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> This patch add support for dump the device registers from a running
> application. It can help developers locate the problem.
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> v2:
> * some logs are adjusted and error string are printed after
> file operation fails.

Ping for review please.




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2] app/procinfo: add device registers dump
  2021-07-09 21:55   ` Thomas Monjalon
@ 2021-07-17  2:16     ` Min Hu (Connor)
  0 siblings, 0 replies; 15+ messages in thread
From: Min Hu (Connor) @ 2021-07-17  2:16 UTC (permalink / raw)
  To: Thomas Monjalon, reshma.pattan, Maryam Tahhan
  Cc: dev, ferruh.yigit, bruce.richardson

Hi, all,
	any comments for this patch?

在 2021/7/10 5:55, Thomas Monjalon 写道:
> 21/06/2021 04:17, Min Hu (Connor):
>> From: Chengchang Tang <tangchengchang@huawei.com>
>>
>> This patch add support for dump the device registers from a running
>> application. It can help developers locate the problem.
>>
>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>> v2:
>> * some logs are adjusted and error string are printed after
>> file operation fails.
> 
> Ping for review please.
> 
> 
> 
> .
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] app/procinfo: add device registers dump
  2021-04-25 13:02 [dpdk-dev] [PATCH] app/procinfo: add device registers dump Min Hu (Connor)
                   ` (2 preceding siblings ...)
  2021-06-21  2:17 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
@ 2021-07-17 17:51 ` Stephen Hemminger
  2021-07-19  6:40   ` Chengchang Tang
  2021-07-17 17:53 ` Stephen Hemminger
  2021-07-22 10:58 ` [dpdk-dev] [PATCH v3] " Min Hu (Connor)
  5 siblings, 1 reply; 15+ messages in thread
From: Stephen Hemminger @ 2021-07-17 17:51 UTC (permalink / raw)
  To: Min Hu (Connor); +Cc: dev, maryam.tahhan, reshma.pattan

On Sun, 25 Apr 2021 21:02:22 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:

> +
> +		memset(&reg_info, 0, sizeof(reg_info));
> +		memset(&dev_info, 0, sizeof(dev_info));

This memset is redundant, rte_eth_dev_info_get already has the same memset.
For the registers, ethdev should be fixed to do it.

> +
> +		ret = rte_eth_dev_info_get(i, &dev_info);
> +		if (ret) {
> +			printf("Error getting device info: %d\n", ret);
> +			continue;
> +		}
> +
> +		ret = rte_eth_dev_get_reg_info(i, &reg_info);
> +		if (ret) {
> +			printf("Error getting device reg info: %d\n", ret);
> +			continue;
> +		}

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] app/procinfo: add device registers dump
  2021-04-25 13:02 [dpdk-dev] [PATCH] app/procinfo: add device registers dump Min Hu (Connor)
                   ` (3 preceding siblings ...)
  2021-07-17 17:51 ` [dpdk-dev] [PATCH] " Stephen Hemminger
@ 2021-07-17 17:53 ` Stephen Hemminger
  2021-07-19  7:00   ` Chengchang Tang
  2021-07-22 10:58 ` [dpdk-dev] [PATCH v3] " Min Hu (Connor)
  5 siblings, 1 reply; 15+ messages in thread
From: Stephen Hemminger @ 2021-07-17 17:53 UTC (permalink / raw)
  To: Min Hu (Connor); +Cc: dev, maryam.tahhan, reshma.pattan

On Sun, 25 Apr 2021 21:02:22 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:

> 		snprintf(file_name, MAX_FILE_NAME_SZ, "%s-port%u",
> +				file_prefix, i);
> +		fp_regs = fopen(file_name, "wb");
> +		if (fp_regs == NULL) {
> +			printf("Error during opening '%s' for writing\n",
> +					file_name);
> +		} else {
> +			if ((int)fwrite(buf_data, 1, buf_size, fp_regs) !=
> +					buf_size)
> +				printf("Error during writing %s\n",
> +						file_prefix);
> +			else
> +				printf("dump device (%s) regs successfully, "
> +					"driver:%s version:0X%08X\n",
> +					dev_info.device->name,
> +					dev_info.driver_name, reg_info.version);
> +
> +			fclose(fp_regs);
> +	

I don't like applications opening and writing an arbitrary file in the
current directory. Any file should be an argument to the application
and optional.

Also, since you are writing binary data, there is no point in using
stdio here.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] app/procinfo: add device registers dump
  2021-07-17 17:51 ` [dpdk-dev] [PATCH] " Stephen Hemminger
@ 2021-07-19  6:40   ` Chengchang Tang
  0 siblings, 0 replies; 15+ messages in thread
From: Chengchang Tang @ 2021-07-19  6:40 UTC (permalink / raw)
  To: Stephen Hemminger, Min Hu (Connor); +Cc: dev, maryam.tahhan, reshma.pattan


On 2021/7/18 1:51, Stephen Hemminger wrote:
> On Sun, 25 Apr 2021 21:02:22 +0800
> "Min Hu (Connor)" <humin29@huawei.com> wrote:
> 
>> +
>> +		memset(&reg_info, 0, sizeof(reg_info));
>> +		memset(&dev_info, 0, sizeof(dev_info));
> 
> This memset is redundant, rte_eth_dev_info_get already has the same memset.
> For the registers, ethdev should be fixed to do it.
>

The memset for dev_info is indeed redundant. But the memset for reg_info seems
reasonable, because there are some input parameters in reg_info.

E.g. If info->data is NULL the get_reg_info() fills in the width and length
fields. If non-NULL the registers are put into the buffer pointed at by the data
field.

So, I will remove the redundant memset for dev_info, and keep the memset for reg_info
in the next version.

>> +
>> +		ret = rte_eth_dev_info_get(i, &dev_info);
>> +		if (ret) {
>> +			printf("Error getting device info: %d\n", ret);
>> +			continue;
>> +		}
>> +
>> +		ret = rte_eth_dev_get_reg_info(i, &reg_info);
>> +		if (ret) {
>> +			printf("Error getting device reg info: %d\n", ret);
>> +			continue;
>> +		}
> .
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] app/procinfo: add device registers dump
  2021-07-17 17:53 ` Stephen Hemminger
@ 2021-07-19  7:00   ` Chengchang Tang
  0 siblings, 0 replies; 15+ messages in thread
From: Chengchang Tang @ 2021-07-19  7:00 UTC (permalink / raw)
  To: Stephen Hemminger, Min Hu (Connor); +Cc: dev, maryam.tahhan, reshma.pattan



On 2021/7/18 1:53, Stephen Hemminger wrote:
> On Sun, 25 Apr 2021 21:02:22 +0800
> "Min Hu (Connor)" <humin29@huawei.com> wrote:
> 
>> 		snprintf(file_name, MAX_FILE_NAME_SZ, "%s-port%u",
>> +				file_prefix, i);
>> +		fp_regs = fopen(file_name, "wb");
>> +		if (fp_regs == NULL) {
>> +			printf("Error during opening '%s' for writing\n",
>> +					file_name);
>> +		} else {
>> +			if ((int)fwrite(buf_data, 1, buf_size, fp_regs) !=
>> +					buf_size)
>> +				printf("Error during writing %s\n",
>> +						file_prefix);
>> +			else
>> +				printf("dump device (%s) regs successfully, "
>> +					"driver:%s version:0X%08X\n",
>> +					dev_info.device->name,
>> +					dev_info.driver_name, reg_info.version);
>> +
>> +			fclose(fp_regs);
>> +	
> 
> I don't like applications opening and writing an arbitrary file in the
> current directory. Any file should be an argument to the application
> and optional.

I think the use of the file here does not seem to be exactly the same as
the behavior you object to. The directories of the file and its name prefix
is passed by user. some suffixes has been added based on user's input.

This is mainly considering the scenario where the user does not specify a
device. If no device is specified, this command will dump regs for all
devices, and their dumped files are distinguished by the suffixes. If the
user is required to give a name to the dumped file of each device, the ease
of use of this command may decreases
> 
> Also, since you are writing binary data, there is no point in using
> stdio here.
> .
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v3] app/procinfo: add device registers dump
  2021-04-25 13:02 [dpdk-dev] [PATCH] app/procinfo: add device registers dump Min Hu (Connor)
                   ` (4 preceding siblings ...)
  2021-07-17 17:53 ` Stephen Hemminger
@ 2021-07-22 10:58 ` Min Hu (Connor)
  2021-07-29 14:14   ` Pattan, Reshma
  5 siblings, 1 reply; 15+ messages in thread
From: Min Hu (Connor) @ 2021-07-22 10:58 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, andrew.rybchenko, stephen

From: Chengchang Tang <tangchengchang@huawei.com>

This patch add support for dump the device registers from a running
application. It can help developers locate the problem.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
v3:
* delete memset of dev_info.

v2:
* some logs are adjusted and error string are printed after
file operation fails.
---
 app/proc-info/main.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 91 insertions(+), 1 deletion(-)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index b9587f7..e85d997 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -94,6 +94,9 @@ static char *mempool_name;
 /**< Enable iter mempool. */
 static uint32_t enable_iter_mempool;
 static char *mempool_iter_name;
+/**< Enable dump regs. */
+static uint32_t enable_dump_regs;
+static char *dump_regs_file_prefix;
 
 /**< display usage */
 static void
@@ -119,7 +122,8 @@ proc_info_usage(const char *prgname)
 		"  --show-crypto: to display crypto information\n"
 		"  --show-ring[=name]: to display ring information\n"
 		"  --show-mempool[=name]: to display mempool information\n"
-		"  --iter-mempool=name: iterate mempool elements to display content\n",
+		"  --iter-mempool=name: iterate mempool elements to display content\n"
+		"  --dump-regs=file-prefix: dump reg to file with the file-prefix\n",
 		prgname);
 }
 
@@ -226,6 +230,7 @@ proc_info_parse_args(int argc, char **argv)
 		{"show-ring", optional_argument, NULL, 0},
 		{"show-mempool", optional_argument, NULL, 0},
 		{"iter-mempool", required_argument, NULL, 0},
+		{"dump-regs", required_argument, NULL, 0},
 		{NULL, 0, 0, 0}
 	};
 
@@ -288,6 +293,10 @@ proc_info_parse_args(int argc, char **argv)
 					"iter-mempool", MAX_LONG_OPT_SZ)) {
 				enable_iter_mempool = 1;
 				mempool_iter_name = optarg;
+			} else if (!strncmp(long_option[option_index].name,
+					"dump-regs", MAX_LONG_OPT_SZ)) {
+				enable_dump_regs = 1;
+				dump_regs_file_prefix = optarg;
 			}
 			break;
 		case 1:
@@ -1349,6 +1358,85 @@ iter_mempool(char *name)
 	}
 }
 
+static void
+dump_regs(char *file_prefix)
+{
+#define MAX_FILE_NAME_SZ (MAX_LONG_OPT_SZ + 10)
+	char file_name[MAX_FILE_NAME_SZ];
+	struct rte_dev_reg_info reg_info;
+	struct rte_eth_dev_info dev_info;
+	unsigned char *buf_data;
+	size_t buf_size;
+	FILE *fp_regs;
+	uint16_t i;
+	int ret;
+
+	snprintf(bdr_str, MAX_STRING_LEN, " dump - Port REG");
+	STATS_BDR_STR(10, bdr_str);
+
+	RTE_ETH_FOREACH_DEV(i) {
+		/* Skip if port is not in mask */
+		if ((enabled_port_mask & (1ul << i)) == 0)
+			continue;
+
+		snprintf(bdr_str, MAX_STRING_LEN, " Port (%u)", i);
+		STATS_BDR_STR(5, bdr_str);
+
+		ret = rte_eth_dev_info_get(i, &dev_info);
+		if (ret) {
+			printf("Error getting device info: %d\n", ret);
+			continue;
+		}
+
+		memset(&reg_info, 0, sizeof(reg_info));
+		ret = rte_eth_dev_get_reg_info(i, &reg_info);
+		if (ret) {
+			printf("Error getting device reg info: %d\n", ret);
+			continue;
+		}
+
+		buf_size = reg_info.length * reg_info.width;
+		buf_data = malloc(buf_size);
+		if (buf_data == NULL) {
+			printf("Error allocating %zu bytes buffer\n", buf_size);
+			continue;
+		}
+
+		reg_info.data = buf_data;
+		reg_info.length = 0;
+		ret = rte_eth_dev_get_reg_info(i, &reg_info);
+		if (ret) {
+			printf("Error getting regs from device: %d\n", ret);
+			free(buf_data);
+			continue;
+		}
+
+		snprintf(file_name, MAX_FILE_NAME_SZ, "%s-port%u",
+				file_prefix, i);
+		fp_regs = fopen(file_name, "wb");
+		if (fp_regs == NULL) {
+			printf("Error during opening '%s' for writing: %s\n",
+					file_name, strerror(errno));
+		} else {
+			size_t nr_written;
+
+			nr_written = fwrite(buf_data, 1, buf_size, fp_regs);
+			if (nr_written != buf_size)
+				printf("Error during writing %s: %s\n",
+						file_prefix, strerror(errno));
+			else
+				printf("Device (%s) regs dumped successfully, "
+					"driver:%s version:0X%08X\n",
+					dev_info.device->name,
+					dev_info.driver_name, reg_info.version);
+
+			fclose(fp_regs);
+		}
+
+		free(buf_data);
+	}
+}
+
 int
 main(int argc, char **argv)
 {
@@ -1454,6 +1542,8 @@ main(int argc, char **argv)
 		show_mempool(mempool_name);
 	if (enable_iter_mempool)
 		iter_mempool(mempool_iter_name);
+	if (enable_dump_regs)
+		dump_regs(dump_regs_file_prefix);
 
 	RTE_ETH_FOREACH_DEV(i)
 		rte_eth_dev_close(i);
-- 
2.7.4


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v3] app/procinfo: add device registers dump
  2021-07-22 10:58 ` [dpdk-dev] [PATCH v3] " Min Hu (Connor)
@ 2021-07-29 14:14   ` Pattan, Reshma
  2021-07-30 16:46     ` Thomas Monjalon
  0 siblings, 1 reply; 15+ messages in thread
From: Pattan, Reshma @ 2021-07-29 14:14 UTC (permalink / raw)
  To: Min Hu (Connor), dev; +Cc: Yigit, Ferruh, andrew.rybchenko, stephen



> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Min Hu (Connor)
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

Acked-by: Reshma Pattan <reshma.pattan@intel.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v3] app/procinfo: add device registers dump
  2021-07-29 14:14   ` Pattan, Reshma
@ 2021-07-30 16:46     ` Thomas Monjalon
  0 siblings, 0 replies; 15+ messages in thread
From: Thomas Monjalon @ 2021-07-30 16:46 UTC (permalink / raw)
  To: Min Hu (Connor)
  Cc: dev, Yigit, Ferruh, andrew.rybchenko, stephen, Pattan, Reshma

29/07/2021 16:14, Pattan, Reshma:
> 
> > Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> > Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> 
> Acked-by: Reshma Pattan <reshma.pattan@intel.com>

Applied, thanks.




^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-07-30 16:46 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-25 13:02 [dpdk-dev] [PATCH] app/procinfo: add device registers dump Min Hu (Connor)
2021-05-19 10:16 ` Thomas Monjalon
2021-06-04 15:04 ` Pattan, Reshma
2021-06-05  3:15   ` Chengchang Tang
2021-06-10 16:25     ` Pattan, Reshma
2021-06-21  2:17 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
2021-07-09 21:55   ` Thomas Monjalon
2021-07-17  2:16     ` Min Hu (Connor)
2021-07-17 17:51 ` [dpdk-dev] [PATCH] " Stephen Hemminger
2021-07-19  6:40   ` Chengchang Tang
2021-07-17 17:53 ` Stephen Hemminger
2021-07-19  7:00   ` Chengchang Tang
2021-07-22 10:58 ` [dpdk-dev] [PATCH v3] " Min Hu (Connor)
2021-07-29 14:14   ` Pattan, Reshma
2021-07-30 16:46     ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).