[PATCH i-g-t] i915/pmu: Add pmu tests to catch unbind cleanup issues
Umesh Nerlige Ramappa
umesh.nerlige.ramappa at intel.com
Wed Feb 14 20:22:10 UTC 2024
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 at 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
>>
More information about the igt-dev
mailing list