Re: [PATCH i-g-t] i915/pmu: Add pmu tests to catch unbind cleanup issues

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

Hi Kamil,

Thanks for your inputs. I will include them in the next revision.

Regards,
Umesh

On Wed, Feb 14, 2024 at 04:08:03PM +0100, Kamil Konieczny wrote:
>Hi Umesh,
>On 2024-02-12 at 22:29:48 -0800, Umesh Nerlige Ramappa wrote:
>> Doing an flr (unbind followed by bind) on i915 device while a user holds
>> a reference to the PMU events results in null-pointer-dereferences
>> (npd). Add some tests to validate the fix for this issue.
>>
>
>Could you rename tests? Here you are testing functionality of
>unbind-bind with a user holding ref to PMU events, so what about
>naming it explicitly as such? NPD is only one of bugs you
>encountered.
>
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> ---
>>  tests/intel/perf_pmu.c | 202 +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 202 insertions(+)
>>
>> diff --git a/tests/intel/perf_pmu.c b/tests/intel/perf_pmu.c
>> index 4ae2b60ae..4d60c8bd3 100644
>> --- a/tests/intel/perf_pmu.c
>> +++ b/tests/intel/perf_pmu.c
>> @@ -249,6 +249,41 @@
>>   * Description: Test the i915 pmu perf interface
>>   * Feature: i915 pmu perf interface, pmu
>>   * Test category: Perf
>> + *
>> + * SUBTEST: flr-npd
>-------------- ^^^^^^^
>imho better name would be: rebind or unbind-bind
>I do not know of good short name for 'ref to PMU event', what
>about 'pmu-event-ref-rebind' ?
>ref-event-unbind-bind ?
>
>rebind-with-pmu-event-ref ?
>
>> + * Description: Test null pointer dereference case
>------------------ ^^^^
>Write here same description you wrote in commit description:
>
>Verify unbind-bind with holding reference to PMU event
>
>Correct also descriptions below.
>
>> + * Feature: i915 pmu perf interface, pmu
>> + * Test category: Perf
>> + *
>> + * SUBTEST: flr-npd-close-after-unbind
>> + * Description: Test null pointer dereference close perf after unbind
>----------------------- ^^^^^^^^^^^^^^^
>Same here and below, drop NPD from descriptons and write a sequence of
>user actions you are validating (here and below).
>
>> + * Feature: i915 pmu perf interface, pmu
>> + * Test category: Perf
>> + *
>> + * SUBTEST: flr-npd-event-group
>> + * Description: Test null pointer dereference case with event groups
>> + * Feature: i915 pmu perf interface, pmu
>> + * Test category: Perf
>> + *
>> + * SUBTEST: flr-npd-separate-events
>> + * Description: Test null pointer dereference case with separate events
>> + * Feature: i915 pmu perf interface, pmu
>> + * Test category: Perf
>> + *
>> + * SUBTEST: forked-flr-npd
>> + * Description: Test null pointer dereference case with forked FLR
>> + * Feature: i915 pmu perf interface, pmu
>> + * Test category: Perf
>> + *
>> + * SUBTEST: forked-flr-npd-event-group
>> + * Description: Test null pointer dereference case with forked FLR and event groups
>> + * Feature: i915 pmu perf interface, pmu
>> + * Test category: Perf
>> + *
>> + * SUBTEST: forked-flr-npd-separate-events
>> + * Description: Test null pointer dereference case with forked FLR and separate events
>> + * Feature: i915 pmu perf interface, pmu
>> + * Test category: Perf
>>   */
>>
>>  IGT_TEST_DESCRIPTION("Test the i915 pmu perf interface");
>> @@ -2417,6 +2452,151 @@ static void test_unload(unsigned int num_engines)
>>  	igt_assert_eq(__igt_i915_driver_unload(NULL), 0);
>>  }
>>
>> +static int sysfs_driver(int i915)
>> +{
>> +	int dir, drv;
>> +
>> +	dir = igt_sysfs_open(i915);
>> +	igt_assert(dir != -1);
>> +
>> +	drv = openat(dir, "device/driver", O_DIRECTORY);
>> +	close(dir);
>> +
>> +	igt_assert(drv != -1);
>> +	return drv;
>> +}
>> +
>> +#define CLOSE_FD_AFTER_UNBIND	0x00000001
>> +#define GROUP_FDS		0x00000002
>> +#define SEPARATE_FDS		0x00000004
>> +
>> +#define NPD_MAX_FDS 4
>> +static void test_flr_npd(uint32_t flags)
>----------------------- ^^^
>This should be unbind_bind or rebind:
>
>static void test_flr_unbind_bind(uint32_t flags)
>or
>static void test_flr_rebind(uint32_t flags)
>
>> +{
>> +	struct pci_device *pdev;
>> +	int fd[NPD_MAX_FDS], i915, drv;
>> +	uint64_t buf[NPD_MAX_FDS];
>> +	int count = 0, i;
>> +	char *snd = NULL;
>> +	char addr[80];
>> +
>> +	i915 = __drm_open_driver(DRIVER_INTEL);
>> +	pdev = igt_device_get_pci_device(i915);
>> +
>> +	snprintf(addr, sizeof(addr), "%04x:%02x:%02x.%d",
>> +		 pdev->domain, pdev->bus, pdev->dev, pdev->func);
>> +	igt_debug("Opened %s\n", addr);
>> +
>> +	if (flags & GROUP_FDS) {
>> +		fd[count++] = open_group(i915, I915_PMU_REQUESTED_FREQUENCY, -1);
>> +		fd[count++] = open_group(i915, I915_PMU_ACTUAL_FREQUENCY, fd[0]);
>> +		fd[count++] = open_group(i915, I915_PMU_RC6_RESIDENCY, fd[0]);
>> +		fd[count++] = open_group(i915, I915_PMU_SOFTWARE_GT_AWAKE_TIME, fd[0]);
>> +		pmu_read_multi(fd[0], count, buf);
>> +	} else if (flags & SEPARATE_FDS) {
>> +		fd[count++] = open_pmu(i915, I915_PMU_REQUESTED_FREQUENCY);
>> +		fd[count++] = open_pmu(i915, I915_PMU_ACTUAL_FREQUENCY);
>> +		fd[count++] = open_pmu(i915, I915_PMU_RC6_RESIDENCY);
>> +		fd[count++] = open_pmu(i915, I915_PMU_SOFTWARE_GT_AWAKE_TIME);
>> +		pmu_read_single(fd[0]);
>> +		pmu_read_single(fd[1]);
>> +		pmu_read_single(fd[2]);
>> +		pmu_read_single(fd[3]);
>> +	} else {
>> +		fd[0] = open_pmu(i915, I915_PMU_ACTUAL_FREQUENCY);
>> +		pmu_read_single(fd[0]);
>> +	}
>> +
>> +	drv = sysfs_driver(i915);
>> +	close(i915);
>> +	igt_audio_driver_unload(&snd);
>> +
>> +	igt_set_timeout(30, "Driver unbind timeout!");
>------------------- ^^
>Why that long? Isn't 5 seconds enough?
>
>> +	igt_assert_f(igt_sysfs_set(drv, "unbind", addr),
>> +		     "Driver unbind failure (%s)!\n", addr);
>> +	igt_reset_timeout();
>> +
>> +	if (flags & CLOSE_FD_AFTER_UNBIND)
>> +		for (i = 0; i < count; i++)
>> +			close(fd[i]);
>> +
>> +	igt_set_timeout(30, "Driver bind timeout!");
>------------------- ^^
>Same repeated value, consider making it define or const.
>
>> +	igt_assert_f(igt_sysfs_set(drv, "bind", addr),
>> +		     "Driver bind failure (%s)!\n", addr);
>> +	igt_reset_timeout();
>> +
>> +	close(drv);
>> +
>> +	if (!(flags & CLOSE_FD_AFTER_UNBIND))
>> +		for (i = 0; i < count; i++)
>> +			close(fd[i]);
>> +}
>> +
>> +static void test_forked_flr_npd(uint32_t flags)
>------------------------------ ^^^
>Same here, don't use npd in function name, use
>unbind_bind or rebind.
>
>> +{
>> +	struct pci_device *pdev;
>> +	int fd[NPD_MAX_FDS], i915, drv;
>> +	uint64_t buf[NPD_MAX_FDS];
>> +	int count = 0, i;
>> +	char *snd = NULL;
>> +	char addr[80];
>> +
>> +	/* not supported for the forked version */
>> +	igt_assert(!(flags & CLOSE_FD_AFTER_UNBIND));
>> +
>> +	i915 = __drm_open_driver(DRIVER_INTEL);
>> +	pdev = igt_device_get_pci_device(i915);
>> +
>> +	snprintf(addr, sizeof(addr), "%04x:%02x:%02x.%d",
>> +		 pdev->domain, pdev->bus, pdev->dev, pdev->func);
>> +	igt_debug("Opened %s\n", addr);
>> +
>> +	if (flags & GROUP_FDS) {
>> +		fd[count++] = open_group(i915, I915_PMU_REQUESTED_FREQUENCY, -1);
>> +		fd[count++] = open_group(i915, I915_PMU_ACTUAL_FREQUENCY, fd[0]);
>> +		fd[count++] = open_group(i915, I915_PMU_RC6_RESIDENCY, fd[0]);
>> +		fd[count++] = open_group(i915, I915_PMU_SOFTWARE_GT_AWAKE_TIME, fd[0]);
>> +		pmu_read_multi(fd[0], count, buf);
>> +	} else if (flags & SEPARATE_FDS) {
>> +		fd[count] = open_pmu(i915, I915_PMU_REQUESTED_FREQUENCY);
>> +		pmu_read_single(fd[count++]);
>> +		fd[count] = open_pmu(i915, I915_PMU_ACTUAL_FREQUENCY);
>> +		pmu_read_single(fd[count++]);
>> +		fd[count] = open_pmu(i915, I915_PMU_RC6_RESIDENCY);
>> +		pmu_read_single(fd[count++]);
>> +		fd[count] = open_pmu(i915, I915_PMU_SOFTWARE_GT_AWAKE_TIME);
>> +		pmu_read_single(fd[count++]);
>> +	} else {
>> +		fd[count] = open_pmu(i915, I915_PMU_ACTUAL_FREQUENCY);
>> +		pmu_read_single(fd[count++]);
>> +	}
>> +
>> +	drv = sysfs_driver(i915);
>> +	close(i915);
>> +	igt_audio_driver_unload(&snd);
>> +
>> +	igt_fork(child, 1) {
>> +		igt_set_timeout(30, "Driver unbind timeout!");
>------------------------^^
>Same here.
>
>> +		igt_assert_f(igt_sysfs_set(drv, "unbind", addr),
>> +				"Driver unbind failure (%s)!\n", addr);
>> +		igt_reset_timeout();
>> +	}
>> +	igt_waitchildren();
>> +
>> +
>> +	igt_fork(child, 1) {
>> +		igt_set_timeout(30, "Driver bind timeout!");
>------------------------^^
>Same here.
>
>> +		igt_assert_f(igt_sysfs_set(drv, "bind", addr),
>> +				"Driver bind failure (%s)!\n", addr);
>> +		igt_reset_timeout();
>> +	}
>> +	igt_waitchildren();
>> +
>> +	close(drv);
>> +	for (i = 0; i < count; i++)
>> +		close(fd[i]);
>> +}
>> +
>>  static void pmu_read(int i915)
>>  {
>>  	char val[128];
>> @@ -2810,4 +2990,26 @@ igt_main
>>  		for (int pass = 0; pass < 3; pass++)
>>  			test_unload(num_engines);
>>  	}
>> +
>> +	igt_subtest("flr-npd")
>-------------------- ^^^
>Please change 'npd' error you see from subtest name, use a name
>describing sequence you are testing, here and below, for example:
>
>s/npd/rebind-with-pmu-event-ref/
>
>> +		test_flr_npd(0);
>> +
>> +	igt_subtest("flr-npd-close-after-unbind")
>imho better:
>rebind-with-event-close-after-unbind
>
>> +		test_flr_npd(CLOSE_FD_AFTER_UNBIND);
>> +
>> +	igt_subtest("flr-npd-event-group")
>Same here, change test name.
>
>> +		test_flr_npd(GROUP_FDS);
>> +
>> +	igt_subtest("flr-npd-separate-events")
>Same here, change test name.
>
>> +		test_flr_npd(SEPARATE_FDS);
>> +
>> +	igt_subtest("forked-flr-npd")
>Same here, change test name.
>
>> +		test_forked_flr_npd(0);
>> +
>> +	igt_subtest("forked-flr-npd-event-group")
>Same here, change test name.
>
>> +		test_forked_flr_npd(GROUP_FDS);
>> +
>> +	igt_subtest("forked-flr-npd-separate-events")
>Same here, change test name.
>
>> +		test_forked_flr_npd(SEPARATE_FDS);
>> +


>
>Regards,
>Kamil
>
>>  }
>> --
>> 2.34.1
>>