Re: [PATCH] amd/lib: refactor function is_reset_enable

[vitaly prosyak <vprosyak@amd.com>]

On 2025-01-28 15:43, Michael Strawbridge wrote:
> Hi Vitaly,
>
> One small thing below.
>
> On 1/28/25 14:24, vitaly.prosyak@amd.com wrote:
>> From: Vitaly Prosyak <vitaly.prosyak@amd.com>
>>
>> This function takes a reset type mask (`AMDGPU_RESET_TYPE_FULL`,
>> `AMDGPU_RESET_TYPE_SOFT_RESET`, AMDGPU_RESET_TYPE_PER_PIPE
>> or `AMDGPU_RESET_TYPE_PER_QUEUE`) and attempts to open and
>> read the corresponding sysfs file, e.g.,
>> `/sys/bus/pci/devices/0000:08:00.0/gfx_reset_mask`.
>> It then iterates through predefined keywords such as `full`,
>> `soft`, and `queue`, validating whether AMDGPU exposes the
>> requested operation. If the requested operation is not available,
>> the test is skipped.
>>
>> The previous implementation included redundant steps and failed
>> in certain cases. This refactored version simplifies the logic,
>> improves reliability, and ensures consistent behavior across various
>> scenarios.
>>
>> Cc: Christian Koenig <christian.koenig@amd.com>
>> Cc: Alexander Deucher <alexander.deucher@amd.com>
>> Cc: Jesse Zhang <jesse.zhang@amd.com>
>> Cc: Michael Strawbridge <michael.strawbridge@amd.com>
>>
>> Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
>> ---
>>  lib/amdgpu/amd_ip_blocks.c | 79 +++++++++++++++++++++++++-------------
>>  1 file changed, 53 insertions(+), 26 deletions(-)
>>
>> diff --git a/lib/amdgpu/amd_ip_blocks.c b/lib/amdgpu/amd_ip_blocks.c
>> index 2b1b84414..f063ad0ab 100644
>> --- a/lib/amdgpu/amd_ip_blocks.c
>> +++ b/lib/amdgpu/amd_ip_blocks.c
>> @@ -1006,11 +1006,27 @@ asic_rings_readness(amdgpu_device_handle device_handle, uint32_t mask,
>>  bool
>>  is_reset_enable(enum amd_ip_block_type ip_type, uint32_t reset_type, const struct pci_addr *pci)
>>  {
>> -	char cmd[256];
>> -	FILE *fp, *fp2;
>> -	char buffer[100];
>> +	const struct reset_arr {
>> +		const char *name;
>> +		unsigned int reset;
>> +	} reset_arr[] = {
>> +		{"full",	AMDGPU_RESET_TYPE_FULL		},
>> +		{"soft",	AMDGPU_RESET_TYPE_SOFT_RESET},
>> +		{"queue",	AMDGPU_RESET_TYPE_PER_QUEUE },
>> +		{"pipe",	AMDGPU_RESET_TYPE_PER_PIPE },
>> +		{NULL, 0}
>> +	};
>> +
>>  	bool enable = false;
>> -	char reset_mask[100];
>> +	char cmd[256];
>> +	FILE *fp;
>> +	char buffer[128];
>> +
>> +	char *token, *buf;
>> +	char reset_mask[32];
>> +	uint32_t mask = 0;
>> +	int found = 0;
>> +	int i;
>>  
>>  	if (ip_type == AMD_IP_GFX)
>>  		snprintf(reset_mask, sizeof(reset_mask) - 1, "gfx_reset_mask");
>> @@ -1019,33 +1035,44 @@ is_reset_enable(enum amd_ip_block_type ip_type, uint32_t reset_type, const struc
>>  	else
>>  		snprintf(reset_mask, sizeof(reset_mask) - 1, "sdma_reset_mask");
>>  
>> -	if( pci)
>> -		snprintf(cmd, sizeof(cmd) - 1, "sudo cat /sys/kernel/debug/dri/%04x:%02x:%02x.%01x/name |grep -oP '(?<=dev=)[0-9:.]+'",
>> -			pci->domain, pci->bus, pci->device, pci->function);
>> -	else
>> -		snprintf(cmd, sizeof(cmd) - 1, "sudo cat /sys/kernel/debug/dri/0/name |grep -oP '(?<=dev=)[0-9:.]+'");
>> +	snprintf(cmd, sizeof(cmd) - 1, "sudo cat /sys/bus/pci/devices/%04x:%02x:%02x.%01x/%s",
>> +			pci->domain, pci->bus, pci->device, pci->function, reset_mask);
>>  
>>  	fp = popen(cmd, "r");
>> -	if (fp == NULL)
>> +	if (fp == NULL) {
>> +		igt_kmsg("***FAILURE popen %s LINE %d FILE %s\n", cmd, __LINE__, __FILE__);
>>  		return false;
>> +	}
>>  
>> -	if (fgets(buffer, 13, fp) != NULL) {
>> -		snprintf(cmd, sizeof(cmd) - 1, "sudo cat /sys/bus/pci/devices/%s/%s | grep -oP '%s'",
>> -			buffer, reset_mask,
>> -			reset_type & AMDGPU_RESET_TYPE_FULL ? "full" :
>> -			reset_type & AMDGPU_RESET_TYPE_SOFT_RESET ? "soft" :
>> -			reset_type & AMDGPU_RESET_TYPE_PER_QUEUE ? "queue" : "pipe");
>> -
>> -		fp2 = popen(cmd, "r");
>> -		if (fp2 == NULL) {
>> -			pclose(fp);
>> -			return false;
>> +	buf = fgets(buffer, sizeof(buffer)-1, fp);
>> +
>> +	//igt_kmsg("cmd: %s buf: %s LINE %d FILE %s\n",
>> +	//		cmd, buf != NULL ? buf : "NULL", __LINE__, __FILE__);
> Did you mean to keep the 2 above commented lines?
Thanks Michael , i will remove.
>
> Besides this you can add:
> Reviewed-by: Michael Strawbridge <michael.strawbridge@amd.com>
>
>> +
>> +	if (buf != NULL) {
>> +		token = strtok(buf, " \n");
>> +		while (token != NULL) {
>> +			for (i = 0; reset_arr[i].name != NULL; i++) {
>> +				if (reset_type == reset_arr[i].reset &&
>> +					strcmp(token, reset_arr[i].name) == 0) {
>> +					mask |= reset_arr[i].reset;
>> +					found = 1;
>> +					break;
>> +				}
>> +			}
>> +			if (!found)
>> +				token = strtok(NULL, " \n");
>>  		}
>> -
>> -		if (fgets(buffer, 13, fp2) != NULL)
>> -			enable = true;
>> -		pclose(fp2);
>> +	} else {
>> +		igt_kmsg("***FAILURE fgets %s LINE %d FILE %s\n",
>> +				buffer, __LINE__, __FILE__);
>>  	}
>> +	if (mask == reset_type)
>> +		enable = true;
>> +	else
>> +		igt_kmsg("***FAILURE mask found 0x%x requested 0x%x operation is not supported LINE %d FILE %s\n",
>> +				mask, reset_type, __LINE__, __FILE__);
>> +
>>  	pclose(fp);
>>  
>>  	return enable;
>> @@ -1105,7 +1132,7 @@ int get_pci_addr_from_fd(int fd, struct pci_addr *pci)
>>  
>>  	if (ret != 4) {
>>  		igt_info("error %s Unable to extract PCI device address from '%s\n",
>> -				__FUNCTION__, buf);
>> +				__func__, buf);
>>  		return -ENOENT;
>>  	}
>>