2024-02-01 06:07:30

by David Gow

[permalink] [raw]
Subject: [PATCH] kunit: device: Unregister the kunit_bus on shutdown

If KUnit is built as a module, and it's unloaded, the kunit_bus is not
unregistered. This causes an error if it's then re-loaded later, as we
try to re-register the bus.

Unregister the bus and root_device on shutdown, if it looks valid.

In addition, be more specific about the value of kunit_bus_device. It
is:
- a valid struct device* if the kunit_bus initialised correctly.
- an ERR_PTR if it failed to initialise.
- NULL before initialisation and after shutdown.

Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
Signed-off-by: David Gow <[email protected]>
---

This will hopefully resolve some of the issues linked to from:
https://lore.kernel.org/intel-gfx/DM4PR11MB614179CB9C387842D8E8BB40B97C2@DM4PR11MB6141.namprd11.prod.outlook.com/

---
lib/kunit/device-impl.h | 2 ++
lib/kunit/device.c | 14 ++++++++++++++
lib/kunit/test.c | 3 +++
3 files changed, 19 insertions(+)

diff --git a/lib/kunit/device-impl.h b/lib/kunit/device-impl.h
index 54bd55836405..5fcd48ff0f36 100644
--- a/lib/kunit/device-impl.h
+++ b/lib/kunit/device-impl.h
@@ -13,5 +13,7 @@

// For internal use only -- registers the kunit_bus.
int kunit_bus_init(void);
+// For internal use only -- unregisters the kunit_bus.
+void kunit_bus_shutdown(void);

#endif //_KUNIT_DEVICE_IMPL_H
diff --git a/lib/kunit/device.c b/lib/kunit/device.c
index 074c6dd2e36a..644a38a1f5b1 100644
--- a/lib/kunit/device.c
+++ b/lib/kunit/device.c
@@ -54,6 +54,20 @@ int kunit_bus_init(void)
return error;
}

+/* Unregister the 'kunit_bus' in case the KUnit module is unloaded. */
+void kunit_bus_shutdown(void)
+{
+ /* Make sure the bus exists before we unregister it. */
+ if (IS_ERR_OR_NULL(kunit_bus_device))
+ return;
+
+ bus_unregister(&kunit_bus_type);
+
+ root_device_unregister(kunit_bus_device);
+
+ kunit_bus_device = NULL;
+}
+
/* Release a 'fake' KUnit device. */
static void kunit_device_release(struct device *d)
{
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 31a5a992e646..1d1475578515 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -928,6 +928,9 @@ static void __exit kunit_exit(void)
#ifdef CONFIG_MODULES
unregister_module_notifier(&kunit_mod_nb);
#endif
+
+ kunit_bus_shutdown();
+
kunit_debugfs_cleanup();
}
module_exit(kunit_exit);
--
2.43.0.429.g432eaa2c6b-goog



2024-02-02 20:16:28

by Rae Moar

[permalink] [raw]
Subject: Re: [PATCH] kunit: device: Unregister the kunit_bus on shutdown

On Thu, Feb 1, 2024 at 1:06 AM David Gow <[email protected]> wrote:
>
> If KUnit is built as a module, and it's unloaded, the kunit_bus is not
> unregistered. This causes an error if it's then re-loaded later, as we
> try to re-register the bus.
>
> Unregister the bus and root_device on shutdown, if it looks valid.
>
> In addition, be more specific about the value of kunit_bus_device. It
> is:
> - a valid struct device* if the kunit_bus initialised correctly.
> - an ERR_PTR if it failed to initialise.
> - NULL before initialisation and after shutdown.
>
> Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
> Signed-off-by: David Gow <[email protected]>

Hello,

I have tested this with modules and it looks good to me!

Thanks!
-Rae

Reviewed-by: Rae Moar <[email protected]>

> ---
>
> This will hopefully resolve some of the issues linked to from:
> https://lore.kernel.org/intel-gfx/DM4PR11MB614179CB9C387842D8E8BB40B97C2@DM4PR11MB6141.namprd11.prod.outlook.com/
>
> ---
> lib/kunit/device-impl.h | 2 ++
> lib/kunit/device.c | 14 ++++++++++++++
> lib/kunit/test.c | 3 +++
> 3 files changed, 19 insertions(+)
>
> diff --git a/lib/kunit/device-impl.h b/lib/kunit/device-impl.h
> index 54bd55836405..5fcd48ff0f36 100644
> --- a/lib/kunit/device-impl.h
> +++ b/lib/kunit/device-impl.h
> @@ -13,5 +13,7 @@
>
> // For internal use only -- registers the kunit_bus.
> int kunit_bus_init(void);
> +// For internal use only -- unregisters the kunit_bus.
> +void kunit_bus_shutdown(void);
>
> #endif //_KUNIT_DEVICE_IMPL_H
> diff --git a/lib/kunit/device.c b/lib/kunit/device.c
> index 074c6dd2e36a..644a38a1f5b1 100644
> --- a/lib/kunit/device.c
> +++ b/lib/kunit/device.c
> @@ -54,6 +54,20 @@ int kunit_bus_init(void)
> return error;
> }
>
> +/* Unregister the 'kunit_bus' in case the KUnit module is unloaded. */
> +void kunit_bus_shutdown(void)
> +{
> + /* Make sure the bus exists before we unregister it. */
> + if (IS_ERR_OR_NULL(kunit_bus_device))
> + return;
> +
> + bus_unregister(&kunit_bus_type);
> +
> + root_device_unregister(kunit_bus_device);
> +
> + kunit_bus_device = NULL;
> +}
> +
> /* Release a 'fake' KUnit device. */
> static void kunit_device_release(struct device *d)
> {
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 31a5a992e646..1d1475578515 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -928,6 +928,9 @@ static void __exit kunit_exit(void)
> #ifdef CONFIG_MODULES
> unregister_module_notifier(&kunit_mod_nb);
> #endif
> +
> + kunit_bus_shutdown();
> +
> kunit_debugfs_cleanup();
> }
> module_exit(kunit_exit);
> --
> 2.43.0.429.g432eaa2c6b-goog
>

2024-02-07 13:36:27

by Jani Nikula

[permalink] [raw]
Subject: Re: [PATCH] kunit: device: Unregister the kunit_bus on shutdown

On Fri, 02 Feb 2024, Rae Moar <[email protected]> wrote:
> On Thu, Feb 1, 2024 at 1:06 AM David Gow <[email protected]> wrote:
>>
>> If KUnit is built as a module, and it's unloaded, the kunit_bus is not
>> unregistered. This causes an error if it's then re-loaded later, as we
>> try to re-register the bus.
>>
>> Unregister the bus and root_device on shutdown, if it looks valid.
>>
>> In addition, be more specific about the value of kunit_bus_device. It
>> is:
>> - a valid struct device* if the kunit_bus initialised correctly.
>> - an ERR_PTR if it failed to initialise.
>> - NULL before initialisation and after shutdown.
>>
>> Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
>> Signed-off-by: David Gow <[email protected]>
>
> Hello,
>
> I have tested this with modules and it looks good to me!
>
> Thanks!
> -Rae
>
> Reviewed-by: Rae Moar <[email protected]>

Thanks for the patch and review!

Is this on its way to some v6.8-rc's? The regression in -rc1 is hurting
our CI.


Thanks,
Jani.


--
Jani Nikula, Intel