Re: [PATCH v2 1/5] drm/ttm: Update Makefile for KUnit

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 11 Sep 2023, Karolina Stolarek <karolina.stolarek@intel.com> wrote:
> Update Makefile so it can produce a module that consists of TTM tests.
> This will allow us to test non-exported functions when KUnit tests
> are built as a module. Remove the tests' Makefile.

I'm asking questions instead of making assertions, because I'm not 100%
confident, but I don't feel like this Makefile could work right.

>
> Signed-off-by: Karolina Stolarek <karolina.stolarek@intel.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202309010358.50gYLkmw-lkp@intel.com/
> Closes: https://lore.kernel.org/oe-kbuild-all/202309011134.bwvpuyOj-lkp@intel.com/
> Closes: https://lore.kernel.org/oe-kbuild-all/202309011935.bBpezbUQ-lkp@intel.com/
> ---
>  drivers/gpu/drm/ttm/Makefile       | 18 +++++++++++++-----
>  drivers/gpu/drm/ttm/tests/Makefile |  6 ------
>  2 files changed, 13 insertions(+), 11 deletions(-)
>  delete mode 100644 drivers/gpu/drm/ttm/tests/Makefile
>
> diff --git a/drivers/gpu/drm/ttm/Makefile b/drivers/gpu/drm/ttm/Makefile
> index dad298127226..6322a33e65ed 100644
> --- a/drivers/gpu/drm/ttm/Makefile
> +++ b/drivers/gpu/drm/ttm/Makefile
> @@ -2,10 +2,18 @@
>  #
>  # Makefile for the drm device driver.  This driver provides support for the
>  
> -ttm-y := ttm_tt.o ttm_bo.o ttm_bo_util.o ttm_bo_vm.o ttm_module.o \
> -	ttm_execbuf_util.o ttm_range_manager.o ttm_resource.o ttm_pool.o \
> -	ttm_device.o ttm_sys_manager.o
> +ttm := ttm_tt.o ttm_bo.o ttm_bo_util.o ttm_bo_vm.o ttm_module.o \
> +       ttm_execbuf_util.o ttm_range_manager.o ttm_resource.o ttm_pool.o \
> +       ttm_device.o ttm_sys_manager.o
> +obj-$(CONFIG_DRM_TTM) += $(ttm)

Does that not lead to each object in $(ttm) becoming its own module?

>  ttm-$(CONFIG_AGP) += ttm_agp_backend.o

Does this not create a ttm.o with just one object, depending on
CONFIG_AGP?

>  
> -obj-$(CONFIG_DRM_TTM) += ttm.o
> -obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += tests/
> +ttm-tests := tests/ttm_kunit_helpers.o tests/ttm_device_test.o \
> +             tests/ttm_pool_test.o

I'd preserve the one object per line syntax. It's nicer for the diffs in
subsequent updates.

> +
> +ifeq ($(CONFIG_DRM_TTM_KUNIT_TEST),m)
> +    ttm-test-objs := $(ttm) $(ttm-tests)

Isn't the -objs syntax for host/userspace programs? And if not, doesn't
it lead to objects with exported symbols being present in two places?

Confused.

BR,
Jani.

> +    obj-m := ttm-test.o
> +else
> +    obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += $(ttm-tests)
> +endif
> diff --git a/drivers/gpu/drm/ttm/tests/Makefile b/drivers/gpu/drm/ttm/tests/Makefile
> deleted file mode 100644
> index ec87c4fc1ad5..000000000000
> --- a/drivers/gpu/drm/ttm/tests/Makefile
> +++ /dev/null
> @@ -1,6 +0,0 @@
> -# SPDX-License-Identifier: GPL-2.0 AND MIT
> -
> -obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += \
> -        ttm_device_test.o \
> -        ttm_pool_test.o \
> -        ttm_kunit_helpers.o

-- 
Jani Nikula, Intel Open Source Graphics Center