[PATCH i-g-t 1/3] lib/igt_kmod: stop using KMOD_REMOVE_FORCE
Kamil Konieczny
kamil.konieczny at linux.intel.com
Tue Feb 6 17:27:04 UTC 2024
Hi Lucas,
On 2024-02-01 at 08:03:15 -0600, Lucas De Marchi wrote:
> On Thu, Feb 01, 2024 at 02:05:10PM +0100, Kamil Konieczny wrote:
> > Hi Lucas,
> > On 2024-01-31 at 16:50:28 -0800, Lucas De Marchi wrote:
> >
> > Missing commit description - please add one.
> >
> > Do we really need this remove?
>
> oops, I was supposed to add the explanation why that I used in a
> patch review and forgot to paste.
>
> I'd swap the question: do we really need the flag? No, we never need
> the flag. If the kernel ever resorts to honoring the flag we will
> have a tainted kernel, potential leaks and memory corruption. It's
> better to just handle the error returned by libkmod and fix the root
> cause.
>
> Lucas De Marchi
>
If it is ignored on kernel side it can be ignored also in igt,
Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> >
> > Regards,
> > Kamil
> >
> > +cc Janusz Krzysztofik
> >
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> > > ---
> > > lib/igt_kmod.c | 42 ++++++++++++++++------------------
> > > lib/igt_kmod.h | 2 +-
> > > tests/intel/i915_module_load.c | 4 ++--
> > > tests/kms_content_protection.c | 2 +-
> > > tests/vgem_basic.c | 2 +-
> > > 5 files changed, 25 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > > index 5709138c0..2fed6ed54 100644
> > > --- a/lib/igt_kmod.c
> > > +++ b/lib/igt_kmod.c
> > > @@ -258,7 +258,7 @@ out:
> > > return err < 0 ? err : 0;
> > > }
> > >
> > > -static int igt_kmod_unload_r(struct kmod_module *kmod, unsigned int flags)
> > > +static int igt_kmod_unload_r(struct kmod_module *kmod)
> > > {
> > > #define MAX_TRIES 20
> > > #define SLEEP_DURATION 500000
> > > @@ -272,7 +272,7 @@ static int igt_kmod_unload_r(struct kmod_module *kmod, unsigned int flags)
> > > holders = kmod_module_get_holders(kmod);
> > > kmod_list_foreach(pos, holders) {
> > > struct kmod_module *it = kmod_module_get_module(pos);
> > > - err = igt_kmod_unload_r(it, flags);
> > > + err = igt_kmod_unload_r(it);
> > > kmod_module_unref(it);
> > > if (err < 0)
> > > break;
> > > @@ -292,7 +292,7 @@ static int igt_kmod_unload_r(struct kmod_module *kmod, unsigned int flags)
> > > }
> > >
> > > for (tries = 0; tries < MAX_TRIES; tries++) {
> > > - err = kmod_module_remove_module(kmod, flags);
> > > + err = kmod_module_remove_module(kmod, 0);
> > >
> > > /* Only loop in the following cases */
> > > if (err != -EBUSY && err != -EAGAIN)
> > > @@ -326,8 +326,6 @@ static int igt_kmod_unload_r(struct kmod_module *kmod, unsigned int flags)
> > > /**
> > > * igt_kmod_unload:
> > > * @mod_name: Module name.
> > > - * @flags: flags are passed directly to libkmod and can be:
> > > - * KMOD_REMOVE_FORCE or KMOD_REMOVE_NOWAIT.
> > > *
> > > * Returns: 0 in case of success or -errno otherwise.
> > > *
> > > @@ -335,7 +333,7 @@ static int igt_kmod_unload_r(struct kmod_module *kmod, unsigned int flags)
> > > *
> > > */
> > > int
> > > -igt_kmod_unload(const char *mod_name, unsigned int flags)
> > > +igt_kmod_unload(const char *mod_name)
> > > {
> > > struct kmod_ctx *ctx = kmod_ctx();
> > > struct kmod_module *kmod;
> > > @@ -348,7 +346,7 @@ igt_kmod_unload(const char *mod_name, unsigned int flags)
> > > goto out;
> > > }
> > >
> > > - err = igt_kmod_unload_r(kmod, flags);
> > > + err = igt_kmod_unload_r(kmod);
> > > if (err < 0) {
> > > igt_debug("Could not remove module %s (%s)\n", mod_name,
> > > strerror(-err));
> > > @@ -525,7 +523,7 @@ static int igt_always_unload_audio_driver(char **who)
> > > if (ret)
> > > igt_warn("Failed to notify pipewire_pulse\n");
> > > kick_snd_hda_intel();
> > > - ret = igt_kmod_unload(*m, 0);
> > > + ret = igt_kmod_unload(*m);
> > > pipewire_pulse_stop_reserve();
> > > if (ret) {
> > > igt_warn("Could not unload audio driver %s\n", *m);
> > > @@ -583,7 +581,7 @@ int __igt_intel_driver_unload(char **who, const char *driver)
> > > if (!igt_kmod_is_loaded(*m))
> > > continue;
> > >
> > > - ret = igt_kmod_unload(*m, 0);
> > > + ret = igt_kmod_unload(*m);
> > > if (ret) {
> > > if (who)
> > > *who = strdup_realloc(*who, *m);
> > > @@ -593,7 +591,7 @@ int __igt_intel_driver_unload(char **who, const char *driver)
> > > }
> > >
> > > if (igt_kmod_is_loaded(driver)) {
> > > - ret = igt_kmod_unload(driver, 0);
> > > + ret = igt_kmod_unload(driver);
> > > if (ret) {
> > > if (who)
> > > *who = strdup_realloc(*who, driver);
> > > @@ -629,10 +627,10 @@ igt_intel_driver_unload(const char *driver)
> > > free(who);
> > >
> > > if (igt_kmod_is_loaded("intel-gtt"))
> > > - igt_kmod_unload("intel-gtt", 0);
> > > + igt_kmod_unload("intel-gtt");
> > >
> > > - igt_kmod_unload("drm_kms_helper", 0);
> > > - igt_kmod_unload("drm", 0);
> > > + igt_kmod_unload("drm_kms_helper");
> > > + igt_kmod_unload("drm");
> > >
> > > if (igt_kmod_is_loaded("driver")) {
> > > igt_warn("%s.ko still loaded!\n", driver);
> > > @@ -682,7 +680,7 @@ igt_amdgpu_driver_unload(void)
> > > bind_fbcon(false);
> > >
> > > if (igt_kmod_is_loaded("amdgpu")) {
> > > - if (igt_kmod_unload("amdgpu", 0)) {
> > > + if (igt_kmod_unload("amdgpu")) {
> > > igt_warn("Could not unload amdgpu\n");
> > > igt_kmod_list_loaded();
> > > igt_lsof("/dev/dri");
> > > @@ -690,8 +688,8 @@ igt_amdgpu_driver_unload(void)
> > > }
> > > }
> > >
> > > - igt_kmod_unload("drm_kms_helper", 0);
> > > - igt_kmod_unload("drm", 0);
> > > + igt_kmod_unload("drm_kms_helper");
> > > + igt_kmod_unload("drm");
> > >
> > > if (igt_kmod_is_loaded("amdgpu")) {
> > > igt_warn("amdgpu.ko still loaded!\n");
> > > @@ -1158,8 +1156,8 @@ static void kunit_get_tests(struct igt_list_head *tests,
> > > free(case_name);
> > > }
> > >
> > > - igt_skip_on(kmod_module_remove_module(tst->kmod, KMOD_REMOVE_FORCE));
> > > - igt_skip_on(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
> > > + igt_skip_on(kmod_module_remove_module(tst->kmod, 0));
> > > + igt_skip_on(igt_kmod_unload("kunit"));
> > >
> > > igt_skip_on_f(err,
> > > "KTAP parser failed while getting a list of test cases\n");
> > > @@ -1362,7 +1360,7 @@ void igt_kunit(const char *module_name, const char *name, const char *opts)
> > > * unload may fail if kunit base module is not loaded,
> > > * ignore any failures, we'll fail later if still loaded.
> > > */
> > > - igt_ignore_warn(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
> > > + igt_ignore_warn(igt_kmod_unload("kunit"));
> > >
> > > igt_assert(igt_list_empty(&tests));
> > > }
> > > @@ -1395,7 +1393,7 @@ void igt_kunit(const char *module_name, const char *name, const char *opts)
> > > kunit_results_free(&tests, &suite_name, &case_name);
> > >
> > > igt_ktest_end(&tst);
> > > - igt_debug_on(igt_kmod_unload("kunit", KMOD_REMOVE_FORCE));
> > > + igt_debug_on(igt_kmod_unload("kunit"));
> > > }
> > >
> > > igt_ktest_fini(&tst);
> > > @@ -1438,7 +1436,7 @@ int igt_ktest_begin(struct igt_ktest *tst)
> > > if (strcmp(tst->module_name, "i915") == 0)
> > > igt_i915_driver_unload();
> > >
> > > - err = kmod_module_remove_module(tst->kmod, KMOD_REMOVE_FORCE);
> > > + err = kmod_module_remove_module(tst->kmod, 0);
> > > igt_require(err == 0 || err == -ENOENT);
> > >
> > > tst->kmsg = open("/dev/kmsg", O_RDONLY | O_NONBLOCK);
> > > @@ -1486,7 +1484,7 @@ int igt_kselftest_execute(struct igt_ktest *tst,
> > >
> > > void igt_ktest_end(struct igt_ktest *tst)
> > > {
> > > - kmod_module_remove_module(tst->kmod, KMOD_REMOVE_FORCE);
> > > + kmod_module_remove_module(tst->kmod, 0);
> > > close(tst->kmsg);
> > > }
> > >
> > > diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
> > > index 990e5309d..efb46da12 100644
> > > --- a/lib/igt_kmod.h
> > > +++ b/lib/igt_kmod.h
> > > @@ -34,7 +34,7 @@ void igt_kmod_list_loaded(void);
> > > bool igt_kmod_has_param(const char *mod_name, const char *param);
> > >
> > > int igt_kmod_load(const char *mod_name, const char *opts);
> > > -int igt_kmod_unload(const char *mod_name, unsigned int flags);
> > > +int igt_kmod_unload(const char *mod_name);
> > >
> > > int igt_audio_driver_unload(char **whom);
> > >
> > > diff --git a/tests/intel/i915_module_load.c b/tests/intel/i915_module_load.c
> > > index 9fffe93d9..b72f3252a 100644
> > > --- a/tests/intel/i915_module_load.c
> > > +++ b/tests/intel/i915_module_load.c
> > > @@ -216,7 +216,7 @@ static void unload_or_die(const char *module_name)
> > >
> > > /* should be unloaded, so expect a no-op */
> > > for (loop = 0;; loop++) {
> > > - err = igt_kmod_unload(module_name, 0);
> > > + err = igt_kmod_unload(module_name);
> > > if (err == -ENOENT) /* -ENOENT == unloaded already */
> > > err = 0;
> > > if (!err || loop >= 10)
> > > @@ -259,7 +259,7 @@ inject_fault(const char *module_name, const char *opt, int fault)
> > > igt_debug("Loaded '%s %s', result=%d\n", module_name, buf, fault);
> > >
> > > if (strcmp(module_name, "i915")) /* XXX better ideas! */
> > > - igt_kmod_unload(module_name, 0);
> > > + igt_kmod_unload(module_name);
> > > else
> > > igt_i915_driver_unload();
> > >
> > > diff --git a/tests/kms_content_protection.c b/tests/kms_content_protection.c
> > > index 3fc575634..6858b22e2 100644
> > > --- a/tests/kms_content_protection.c
> > > +++ b/tests/kms_content_protection.c
> > > @@ -396,7 +396,7 @@ static void test_content_protection_on_output(igt_output_t *output,
> > > }
> > >
> > > if (data.cp_tests & CP_MEI_RELOAD) {
> > > - igt_assert_f(!igt_kmod_unload("mei_hdcp", 0),
> > > + igt_assert_f(!igt_kmod_unload("mei_hdcp"),
> > > "mei_hdcp unload failed");
> > >
> > > /* Expected to fail */
> > > diff --git a/tests/vgem_basic.c b/tests/vgem_basic.c
> > > index 63c5c0971..cfda908bc 100644
> > > --- a/tests/vgem_basic.c
> > > +++ b/tests/vgem_basic.c
> > > @@ -438,7 +438,7 @@ static void test_debugfs_read(int fd)
> > >
> > > static int module_unload(void)
> > > {
> > > - return igt_kmod_unload("vgem", 0);
> > > + return igt_kmod_unload("vgem");
> > > }
> > >
> > > static void test_unload(void)
> > > --
> > > 2.43.0
> > >
More information about the igt-dev
mailing list