mbox series

[v2,0/4] kunit: Add helpers for creating test-managed devices

Message ID 20231208-kunit_bus-v2-0-e95905d9b325@google.com (mailing list archive)
Headers show
Series kunit: Add helpers for creating test-managed devices | expand

Message

David Gow Dec. 8, 2023, 10:09 a.m. UTC
KUnit tests often need to provide a struct device, and thus far have
mostly been using root_device_register() or platform devices to create
a 'fake device' for use with, e.g., code which uses device-managed
resources. This has several disadvantages, including not being designed
for test use, scattering files in sysfs, and requiring manual teardown
on test exit, which may not always be possible in case of failure.

Instead, introduce a set of helper functions which allow devices
(internally a struct kunit_device) to be created and managed by KUnit --
i.e., they will be automatically unregistered on test exit. These
helpers can either use a user-provided struct device_driver, or have one
automatically created and managed by KUnit. In both cases, the device
lives on a new kunit_bus.

This is a follow-up to a previous proposal here:
https://lore.kernel.org/linux-kselftest/20230325043104.3761770-1-davidgow@google.com/

(The kunit_defer() function in the first patch there has since been
merged as the 'deferred actions' feature.)

My intention is to take this whole series in via the kselftest/kunit
branch, but I'm equally okay with splitting up the later patches which
use this to go via the various subsystem trees in case there are merge
conflicts.

I'd really appreciate any extra scrutiny that can be given to this;
particularly around the device refcounts and whether we can guarantee
that the device will be released at the correct point in the test
cleanup. I've seen a few crashes in kunit_cleanup, but only on some
already flaky/fragile UML/clang/alltests setups, which seem to go away
if I remove the devm_add_action() call (or if I enable any debugging
features / symbols, annoyingly).

Cheers,
-- David

Signed-off-by: David Gow <davidgow@google.com>
---
Changes in v2:
- Simplify device/driver/bus matching, removing the no-longer-required
  kunit_bus_match function. (Thanks, Greg)
- The return values are both more consistent (kunit_device_register now
  returns an explicit error pointer, rather than failing the test), and
  better documented.
- Add some basic documentation to the implementations as well as the
  headers. The documentation in the headers is still more complete, and
  is now properly compiled into the HTML documentation (under
  dev-tools/kunit/api/resources.html). (Thanks, Matti)
- Moved the internal-only kunit_bus_init() function to a private header,
  lib/kunit/device-impl.h to avoid polluting the public headers, and
  match other internal-only headers. (Thanks, Greg)
- Alphabetise KUnit includes in other test modules. (Thanks, Amadeusz.)
- Several code cleanups, particularly around error handling and
  allocation. (Thanks Greg, Maxime)
- Several const-correctness and casting improvements. (Thanks, Greg)
- Added a new test to verify KUnit cleanup triggers device cleanup.
  (Thanks, Maxime).
- Improved the user-specified device test to verify that probe/remove
  hooks are called correctly. (Thanks, Maxime).
- The overflow test no-longer needlessly calls
  kunit_device_unregister().
- Several other minor cleanups and documentation improvements, which
  hopefully make this a bit clearer and more robust.
- Link to v1: https://lore.kernel.org/r/20231205-kunit_bus-v1-0-635036d3bc13@google.com

---
David Gow (4):
      kunit: Add APIs for managing devices
      fortify: test: Use kunit_device
      overflow: Replace fake root_device with kunit_device
      ASoC: topology: Replace fake root_device with kunit_device in tests

 Documentation/dev-tools/kunit/api/resource.rst |   9 ++
 Documentation/dev-tools/kunit/usage.rst        |  50 +++++++
 include/kunit/device.h                         |  80 +++++++++++
 lib/fortify_kunit.c                            |   5 +-
 lib/kunit/Makefile                             |   3 +-
 lib/kunit/device.c                             | 181 +++++++++++++++++++++++++
 lib/kunit/kunit-test.c                         | 134 +++++++++++++++++-
 lib/kunit/test.c                               |   3 +
 lib/overflow_kunit.c                           |   5 +-
 sound/soc/soc-topology-test.c                  |  10 +-
 10 files changed, 465 insertions(+), 15 deletions(-)
---
base-commit: c8613be119892ccceffbc550b9b9d7d68b995c9e
change-id: 20230718-kunit_bus-ab19c4ef48dc

Best regards,

Comments

Maxime Ripard Dec. 13, 2023, 3:04 p.m. UTC | #1
Hi David,

On Fri, Dec 08, 2023 at 06:09:28PM +0800, davidgow@google.com wrote:
> KUnit tests often need to provide a struct device, and thus far have
> mostly been using root_device_register() or platform devices to create
> a 'fake device' for use with, e.g., code which uses device-managed
> resources. This has several disadvantages, including not being designed
> for test use, scattering files in sysfs, and requiring manual teardown
> on test exit, which may not always be possible in case of failure.
> 
> Instead, introduce a set of helper functions which allow devices
> (internally a struct kunit_device) to be created and managed by KUnit --
> i.e., they will be automatically unregistered on test exit. These
> helpers can either use a user-provided struct device_driver, or have one
> automatically created and managed by KUnit. In both cases, the device
> lives on a new kunit_bus.
> 
> This is a follow-up to a previous proposal here:
> https://lore.kernel.org/linux-kselftest/20230325043104.3761770-1-davidgow@google.com/
> 
> (The kunit_defer() function in the first patch there has since been
> merged as the 'deferred actions' feature.)
> 
> My intention is to take this whole series in via the kselftest/kunit
> branch, but I'm equally okay with splitting up the later patches which
> use this to go via the various subsystem trees in case there are merge
> conflicts.

Could you take (and apply eventually) that patch as part of your series?
https://lore.kernel.org/linux-kselftest/20231205090405.153140-1-mripard@kernel.org/

Thanks
Maxime
David Gow Dec. 14, 2023, 8:51 a.m. UTC | #2
On Wed, 13 Dec 2023 at 23:04, Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi David,
>
> On Fri, Dec 08, 2023 at 06:09:28PM +0800, davidgow@google.com wrote:
> > KUnit tests often need to provide a struct device, and thus far have
> > mostly been using root_device_register() or platform devices to create
> > a 'fake device' for use with, e.g., code which uses device-managed
> > resources. This has several disadvantages, including not being designed
> > for test use, scattering files in sysfs, and requiring manual teardown
> > on test exit, which may not always be possible in case of failure.
> >
> > Instead, introduce a set of helper functions which allow devices
> > (internally a struct kunit_device) to be created and managed by KUnit --
> > i.e., they will be automatically unregistered on test exit. These
> > helpers can either use a user-provided struct device_driver, or have one
> > automatically created and managed by KUnit. In both cases, the device
> > lives on a new kunit_bus.
> >
> > This is a follow-up to a previous proposal here:
> > https://lore.kernel.org/linux-kselftest/20230325043104.3761770-1-davidgow@google.com/
> >
> > (The kunit_defer() function in the first patch there has since been
> > merged as the 'deferred actions' feature.)
> >
> > My intention is to take this whole series in via the kselftest/kunit
> > branch, but I'm equally okay with splitting up the later patches which
> > use this to go via the various subsystem trees in case there are merge
> > conflicts.
>
> Could you take (and apply eventually) that patch as part of your series?
> https://lore.kernel.org/linux-kselftest/20231205090405.153140-1-mripard@kernel.org/
>

Thanks -- I've included it in v3 (which fixes a few other issues), and
will take it along with the rest of the series:
https://lore.kernel.org/linux-kselftest/20231214-kunit_bus-v3-0-7e9a287d3048@google.com/T/

Cheers,
-- David