Re: [PATCH i-g-t 1/1] tests/intel/xe_debugfs: Add tests to read all debugfs and sysfs files

[Peter Senna Tschudin <peter.senna@linux.intel.com>]

Hi Kamil,

Please see my comments bellow.

On 18.03.2025 16:56, Kamil Konieczny wrote:
> Hi Peter,
> On 2025-03-18 at 10:53:50 +0100, Peter Senna Tschudin wrote:
>> The existing tests/intel/debugfs_test.c scans and reads all relevant
>> files from sysfs and debugfs, but it is specific to i915, leaving a gap
>> for Xe.
>>
>> A similar test exists for Xe in tests/intel/xe_debugfs.c, but it has two
> 
> Please do not write out full path nor test name, it is in
> subject, no need for repeating.

Will fix it.

> 
>> gaps compared to the i915 counterpart:
>>  - It lacks sysfs file testing.
>>  - It does not attempt to read all debugfs files.
>>
>> This commit addresses these gaps by adding two new tests to
>> tests/intel/xe_debugfs.c:
>>  - sysfs_read_all_entries
>>  - debugfs_read_all_entries
> 
> Both test names should have minus as separator, so s/_/-/g
> 
> - sysfs-read-all-entries
> - debugfs-read-all-entries

Will fix this one too.

> 
>>
>> Both tests use the function read_and_discard_sysfs_entries(), which was
>> copied from tests/intel/debugfs_test.c.
> 
> You also do not need to translate code from C into words,
> all this is in patch itself so imho drop this paragraph.

The actual value here is the confession that I am copying and pasting the
function from elsewhere. In the cover letter I redeem myself a bit stating
that I will move the cloned code to lib/.

> 
> Btw this looks pretty generic, maybe move all this into
> core_sysfs test?

You are correct in the sense that it is generic. What I am doing is
extending the existing test xe_debugfs in the same way the i915
counterpart works. May I ask what is your suggestion here?

In the cover letter I explain what I want to do for cleaning up the
mess a bit. The reason to send this first is to close the gap and
give us time to cleanup things properly.

I am not sure to understand what you want me to do here.

Thanks,

Peter

> 
> Regards,
> Kamil
> 
>>
>> Cc: rodrigo.vivi@intel.com
>> Cc: katarzyna.piecielska@intel.com
>> Signed-off-by: Peter Senna Tschudin <peter.senna@linux.intel.com>
>> ---
>>  tests/intel/xe_debugfs.c | 88 +++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 87 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/intel/xe_debugfs.c b/tests/intel/xe_debugfs.c
>> index ec8b0d0b5..0b468c4d4 100644
>> --- a/tests/intel/xe_debugfs.c
>> +++ b/tests/intel/xe_debugfs.c
>> @@ -235,15 +235,90 @@ static int opt_handler(int option, int option_index, void *input)
>>  	return IGT_OPT_HANDLER_SUCCESS;
>>  }
>>  
>> +/**
>> + * SUBTEST: sysfs_read_all_entries
>> + * Description: Read all entries from sysfs path.
>> + *
>> + * SUBTEST: debugfs_read_all_entries
>> + * Description: Read all entries from debugfs path.
>> + */
>> +static void read_and_discard_sysfs_entries(int path_fd, int indent)
>> +{
>> +	struct dirent *dirent;
>> +	DIR *dir;
>> +	char tabs[8];
>> +	int i;
>> +
>> +	igt_assert(indent < sizeof(tabs) - 1);
>> +
>> +	for (i = 0; i < indent; i++)
>> +		tabs[i] = '\t';
>> +	tabs[i] = '\0';
>> +
>> +	dir = fdopendir(path_fd);
>> +	if (!dir)
>> +		return;
>> +
>> +	while ((dirent = readdir(dir))) {
>> +		if (!strcmp(dirent->d_name, ".") ||
>> +		    !strcmp(dirent->d_name, ".."))
>> +			continue;
>> +
>> +		if (dirent->d_type == DT_DIR) {
>> +			int sub_fd;
>> +
>> +			sub_fd = openat(path_fd, dirent->d_name,
>> +					O_RDONLY | O_DIRECTORY);
>> +			if (sub_fd < 0)
>> +				continue;
>> +
>> +			igt_debug("%sEntering subdir %s\n", tabs, dirent->d_name);
>> +			read_and_discard_sysfs_entries(sub_fd, indent + 1);
>> +			close(sub_fd);
>> +		} else if (dirent->d_type == DT_REG) {
>> +			char buf[512];
>> +			int sub_fd;
>> +			ssize_t ret;
>> +
>> +			igt_kmsg(KMSG_DEBUG "Reading file \"%s\"\n", dirent->d_name);
>> +			igt_debug("%sReading file \"%s\"\n", tabs, dirent->d_name);
>> +			igt_set_timeout(5, "reading sysfs entry");
>> +
>> +			sub_fd = openat(path_fd, dirent->d_name, O_RDONLY | O_NONBLOCK);
>> +			if (sub_fd == -1) {
>> +				igt_debug("%sCould not open file \"%s\" with error: %m\n",
>> +					  tabs, dirent->d_name);
>> +				continue;
>> +			}
>> +
>> +			do {
>> +				ret = read(sub_fd, buf, sizeof(buf));
>> +			} while (ret == sizeof(buf));
>> +
>> +			if (ret == -1)
>> +				igt_debug("%sCould not read file \"%s\" with error: %m\n",
>> +					  tabs, dirent->d_name);
>> +
>> +			igt_reset_timeout();
>> +			close(sub_fd);
>> +		}
>> +	}
>> +	closedir(dir);
>> +}
>> +
>>  igt_main_args("", long_options, help_str, opt_handler, NULL)
>>  {
>>  	char devnode[PATH_MAX];
>>  	int fd;
>>  	int gt;
>> +	int debugfs;
>> +	int sysfs;
>>  
>>  	igt_fixture {
>>  		fd = drm_open_driver(DRIVER_XE);
>>  		__igt_debugfs_dump(fd, "info", IGT_LOG_INFO);
>> +		debugfs = igt_debugfs_dir(fd);
>> +		sysfs = igt_sysfs_open(fd);
>>  	}
>>  
>>  	igt_subtest("base") {
>> @@ -263,6 +338,17 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
>>  		test_forcewake(fd);
>>  	}
>>  
>> -	igt_fixture
>> +	igt_describe("Read all entries from sysfs path.");
>> +	igt_subtest("sysfs_read_all_entries")
>> +		read_and_discard_sysfs_entries(sysfs, 0);
>> +
>> +	igt_describe("Read all entries from debugfs path.");
>> +	igt_subtest("debugfs_read_all_entries")
>> +		read_and_discard_sysfs_entries(debugfs, 0);
>> +
>> +	igt_fixture {
>> +		close(sysfs);
>> +		close(debugfs);
>>  		drm_close_driver(fd);
>> +	}
>>  }
>> -- 
>> 2.34.1
>>