Re: [PATCH v8 8/8] net/hns3: support filter registers by module names

[Jie Hai <haijie1@huawei.com>]

Hi, fengchengwen,

Thanks for your review.

On 2024/9/26 9:46, fengchengwen wrote:
> On 2024/9/25 14:40, Jie Hai wrote:
>> This patch support dumping registers which module name is the
>> `filter` string. The module names are in lower case and so is
>> the `filter`. Available module names are cmdq, common_pf,
>> common_vf, ring, tqp_intr, 32_bit_dfx, 64_bit_dfx, bios, igu_egu,
>> ssu, ppp, rpu, ncsi, rtc, rcb, etc.
>>
>> Signed-off-by: Jie Hai<haijie1@huawei.com>
>> ---
>>   doc/guides/nics/hns3.rst     |   7 +
>>   drivers/net/hns3/hns3_regs.c | 257 ++++++++++++++++++++---------------
>>   2 files changed, 157 insertions(+), 107 deletions(-)
>>
>> diff --git a/doc/guides/nics/hns3.rst b/doc/guides/nics/hns3.rst
>> index 97b4686bfabe..20765bd7d145 100644
>> --- a/doc/guides/nics/hns3.rst
>> +++ b/doc/guides/nics/hns3.rst
>> @@ -407,6 +407,13 @@ be provided to avoid scheduling the CPU core used by DPDK application threads fo
>>   other tasks. Before starting the Linux OS, add the kernel isolation boot parameter.
>>   For example, "isolcpus=1-18 nohz_full=1-18 rcu_nocbs=1-18".
>>   
>> +Dump registers
>> +--------------
>> +HNS3 supports dumping registers values with their names, and suppotrt filtering
> typo of suppotrt
> 
Will correct in V9.
>> +by module names. The available modules are ``bios``, ``ssu``, ``igu_egu``,
> The available modules names?
> 
>> +``rpu``, ``ncsi``, ``rtc``, ``ppp``, ``rcb``, ``tqp``, ``rtc``, ``cmdq``,
>> +``common_pf``, ``common_vf``, ``ring``, ``tqp_intr``, ``32_bit_dfx``,
>> +``64_bit_dfx``, etc.
> etc means there are more which don't list above, please don't add it because the doc should contain all modules names.
> 
Will remove 'etc' in V9.
>>   
>>   Limitations or Known issues
>>   ---------------------------

>> +static void
>> +hns3_get_module_names(char *names, uint32_t len)
>> +{
>> +	size_t i;
>> +
>> +	for (i = 0; i < RTE_DIM(hns3_module_name_map); i++) {
>> +		strlcat(names, " ", len);
>> +		strlcat(names, hns3_module_name_map[i].name, len);
>> +	}
>> +}
>> +
>> +static uint32_t
>> +hns3_parse_modules_by_filter(struct hns3_hw *hw, const char *filter)
>> +{
>> +	struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw);
>> +	char names[HNS3_MAX_MODULES_LEN] = {0};
>> +	uint32_t modules = 0;
>> +	size_t i;
>> +
>> +	if (filter == NULL) {
>> +		modules = (1 << RTE_DIM(hns3_reg_lists)) - 1;
>> +	} else {
>> +		for (i = 0; i < RTE_DIM(hns3_module_name_map); i++) {
>> +			if (strcmp(filter, hns3_module_name_map[i].name) == 0) {
>> +				modules |= hns3_module_name_map[i].module;
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>> +	if (hns->is_vf)
>> +		modules &= HNS3_VF_MODULES;
>> +	else
>> +		modules &= ~HNS3_VF_ONLY_MODULES;
>> +	if (modules == 0) {
>> +		hns3_get_module_names(names, HNS3_MAX_MODULES_LEN);
> the name is NULL.
> 
I am not sure what problem it is.
You mean the parameter names of hns3_get_module_names?
It is an array and not null for sure. And it is used for the names of 
all modules.
>> +		hns3_err(hw, "mismatched module name! Available names are:\n%s.",
> hns3_err already contain \n, please remove this.
This is a new line between the above expression and the name list
to prevent the overall output from being too long.
Not the last of the expression.
> 
>> +			 names);
>> +	}
>> +	return modules;
>>   }
>>