Re: [PATCH v2 2/4] selftests/resctrl: Pass sysfs controller name of the vendor

[Moger, Babu]

Hi Reinette,

On 5/9/24 16:11, Reinette Chatre wrote:
> Hi Babu,
> 
> On 4/25/2024 1:17 PM, Babu Moger wrote:
>> Detect the vendor and pass the sysfs name for the vendor for searching
>> the controller information.
> 
> Could you please write a proper changelog?

Sure. Will do.

> 
>>
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> ---
>>  tools/testing/selftests/resctrl/resctrl_val.c | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
>> index a30cfcff605f..e3b09128ec3d 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_val.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
>> @@ -224,14 +224,24 @@ static int num_of_mem_controllers(void)
>>  {
>>  	char imc_dir[512], *temp;
>>  	unsigned int count = 0;
>> +	int ret, vendor, size;
>>  	struct dirent *ep;
>> -	int ret;
>> +	char *sysfs_name;
>>  	DIR *dp;
>>  
>> +	vendor = get_vendor();
>> +	if (vendor == ARCH_INTEL) {
>> +		sysfs_name = UNCORE_IMC;
>> +		size = sizeof(UNCORE_IMC);
> 
> Why is separate size needed? Can strlen() just be used when needed?

Yes.

> 
>> +	} else {
>> +		perror("Unsupported Vendor!\n");
> 
> ksft_perror()?
> 
> In the message, "Vendor" need not start with capital. It may also
> help to print the vendor value in this unlikely case.

ok. Sure.

> 
>> +		return -1;
>> +	}
>> +
>>  	dp = opendir(DYN_PMU_PATH);
>>  	if (dp) {
>>  		while ((ep = readdir(dp))) {
>> -			temp = strstr(ep->d_name, UNCORE_IMC);
>> +			temp = strstr(ep->d_name, sysfs_name);
>>  			if (!temp)
>>  				continue;
>>  
>> @@ -242,7 +252,7 @@ static int num_of_mem_controllers(void)
>>  			 * well and hence the last underscore character in
>>  			 * uncore_imc'_' need not be counted.
>>  			 */
>> -			temp = temp + sizeof(UNCORE_IMC);
>> +			temp = temp + size;
> 
> strlen()? (Keeping in mind the adjustment for the "_" character).

Sure.
> 
>>  
>>  			/*
>>  			 * Some directories under "DYN_PMU_PATH" could have
> 
> Reinette
> 

-- 
Thanks
Babu Moger