Re: [dpdk-dev] [PATCH] app/procinfo: add device registers dump

[Chengchang Tang <tangchengchang@huawei.com>]

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.
> .
>