Re: [igt-dev] [PATCH i-g-t 7/8] tests/sriov_basic: validate driver binding to VFs

["Laguna, Lukasz" <lukasz.laguna@intel.com>]

On 11/10/2023 20:44, Michal Wajdeczko wrote:
>
> On 09.11.2023 08:06, Laguna, Lukasz wrote:
>> On 11/6/2023 23:59, Michal Wajdeczko wrote:
>>> On 06.11.2023 20:59, Lukasz Laguna wrote:
>>>> From: Katarzyna Dec <katarzyna.dec@intel.com>
>>>>
>>>> Test enables VFs in range <1..totalvfs>, bind driver to all of them and
>>>> then unbind driver from all of them.
>>> commit message seems outdated
>> What do you mean? I don't see anything wrong
>>>> Signed-off-by: Katarzyna Dec <katarzyna.dec@intel.com>
>>>> Reviewed-by: Lukasz Laguna <lukasz.laguna@intel.com>
>>>> Signed-off-by: Lukasz Laguna <lukasz.laguna@intel.com>
>>>> Reviewed-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>>>> ---
>>>>    tests/sriov_basic.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 51 insertions(+)
>>>>
>>>> diff --git a/tests/sriov_basic.c b/tests/sriov_basic.c
>>>> index fc0914962..179731daf 100644
>>>> --- a/tests/sriov_basic.c
>>>> +++ b/tests/sriov_basic.c
>>>> @@ -61,6 +61,38 @@ static void probe_disable_vfs(int pf_fd, unsigned
>>>> int num_vfs)
>>>>        igt_assert(!err);
>>>>    }
>>>>    +/**
>>>> + * SUBTEST: enable-vfs-bind-all-unbind-all
>>>> + * Description:
>>>> + *   Verify VFs enabling, binding the driver and then unbinding it
>>>> from all of them
>>>> + */
>>>> +static void enable_vfs_bind_all_unbind_all(int pf_fd, unsigned int
>>>> num_vfs)
>>>> +{
>>>> +    igt_debug("Using num_vfs=%u\n", num_vfs);
>>> nit: "Testing %u VFs" ?
>> Done
>>>> +
>>>> +    igt_require(igt_sriov_get_enabled_vfs(pf_fd) == 0);
>>> duplicates main fixture
>> As already answered in different patch - first fixtureis not executed
>> between dynamic subtests.
>>>> +    igt_warn_on(!igt_sriov_disable_driver_autoprobe(pf_fd));
>>>> +    igt_skip_on(igt_sriov_is_driver_autoprobe_enabled(pf_fd));
>>> why do we need warn/skip here ?
>>> can't we just assert that 'disable' worked ?
>> Done
>>>> +
>>>> +    igt_warn_on(!igt_sriov_enable_vfs(pf_fd, num_vfs));
>>> can't we just assert ?
>> Done
>>>> +    igt_assert_eq(num_vfs, igt_sriov_get_enabled_vfs(pf_fd));
>>> why we care here ? if not all are enabled then we fail just later
>>> and this is not a test for "enable VFs" that enabled==requested
>> Done
>>>> +    igt_warn_on(!igt_sriov_enable_driver_autoprobe(pf_fd));
>>>> +    igt_assert(igt_sriov_is_driver_autoprobe_enabled(pf_fd));
>>> can't we just warn ?
>>> if that we fail to enable then probe below will fail anyway
>> Done
>>>> +
>>>> +    for (int i = 1; i <= num_vfs; i++) {
>>>> +        igt_assert(!igt_sriov_is_vf_drm_driver_probed(pf_fd, i));
>>>> +        igt_assert(igt_sriov_bind_vf_drm_driver(pf_fd, i));
>>>> +        igt_assert(igt_sriov_is_vf_drm_driver_probed(pf_fd, i));
>>> shouldn't we just "expect" to make sure to call "disable VFs" ?
>> VFs will be disabled in exit fixture. VFs disabling in subtest is needed
>> between dynamic subtests.
> but if test passed, then it should do a proper cleanup
>
> maybe problem is that if something went wrong, your igt_assert() aborts
> current test which doesn't have a chance to do proper cleanup ?
>
> or maybe that cleanup should be in some mid-test fixture ?
>
> anyway, just seems broken that we need to duplicate the code/logic every
> time
>
>>>> +    }
>>>> +
>>>> +    for (int i = 1; i <= num_vfs; i++) {
>>>> +        igt_assert(igt_sriov_unbind_vf_drm_driver(pf_fd, i));
>>>> +        igt_assert(!igt_sriov_is_vf_drm_driver_probed(pf_fd, i));
>>> do we need to have all VFs loaded ?
>>> maybe for BAT test we can just bind/unload one VF at the time ?
>> We have such test as well:
>> [PATCH i-g-t 8/8] tests/sriov_basic: add more tests for VF driver binding
>>      SUBTEST: enable-vfs-bind-unbind-each
>>      SUBTEST: bind-unbind-vf
> that's good
>
> but the question is still open?
> what the benefit of having this test which just open-coded the VF probe
> loop that would be otherwise done by the PCI subsystem ?
>
> and again, like autoprobe-on, this doesn't seem good candidate for "BAT"
> runs, more like a "STRESS"
Done
>>> otherwise it will be almost the same level of stress as in
>>> "enable-vfs-autoprobe-on" but with 'manual probe' loop of all VFs
>>>
>>>> +    }
>>>> +
>>>> +    igt_assert(igt_sriov_disable_vfs(pf_fd));
>>>> +}
>>>> +
>>>>    igt_main
>>>>    {
>>>>        int pf_fd;
>>>> @@ -113,6 +145,25 @@ igt_main
>>>>            }
>>>>        }
>>>>    +    igt_describe("Verify VFs enabling, binding the driver and then
>>>> unbinding it from all of them");
>>>> +    igt_subtest_with_dynamic("enable-vfs-bind-all-unbind-all") {
>>>> +        for_each_num_vfs(pf_fd, num_vfs) {
>>>> +            igt_dynamic_f("numvfs-%u", num_vfs) {
>>>> +                enable_vfs_bind_all_unbind_all(pf_fd, num_vfs);
>>>> +            }
>>>> +        }
>>>> +        for_random_num_vfs(pf_fd, num_vfs) {
>>>> +            igt_dynamic_f("numvfs-random") {
>>>> +                enable_vfs_bind_all_unbind_all(pf_fd, num_vfs);
>>>> +            }
>>>> +        }
>>>> +        for_max_num_vfs(pf_fd, num_vfs) {
>>>> +            igt_dynamic_f("numvfs-all") {
>>>> +                enable_vfs_bind_all_unbind_all(pf_fd, num_vfs);
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +
>>>>        igt_fixture {
>>>>            igt_sriov_disable_vfs(pf_fd);
>>>>            /* abort to avoid execution of next tests with enabled VFs */