Re: [PATCH] drm/client: Convert to VISIBLE_IF_KUNIT

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 02.02.23 um 12:03 schrieb Maxime Ripard:
> Commit 8fc0380f6ba7 ("drm/client: Add some tests for
> drm_connector_pick_cmdline_mode()") was meant to introduce unit tests
> for the static drm_connector_pick_cmdline_mode() function.
>
> In such a case, the kunit documentation recommended to import the tests
> source file directly from the source file with the static function to
> test.
>
> While it was working, it's generally frowned upon. Fortunately, commit
> 9c988fae6f6a ("kunit: add macro to allow conditionally exposing static
> symbols to tests") introduced macros to easily deal with that case. We
> can thus remove our include and use those macros instead.

I like that this include statements is going away. But changing symbol 
visibility for tests is likewise awkward.

Maybe i'm askin gtoo miuch for this simple patch, but can't we have a 
helper macro that generates an exported wrapper for Kunit tests? 
Something like this:

EXPORT_KUNIT_WRAPPER(struct drm_display_mode *\
			drm_connector_pick_cmdline_mode,
			struct drm_connector *connector);

which then generates something like this:

struct drm_display_mode * drm_connector_pick_cmdline_mode_kunit(
	struct drm_connector *connector)
{
	return drm_connector_pick_cmdline_mode(connector);
}

I know that the macro for generating this code is more complex than 
illustrated here. But this solution separates Kunit and functions 
cleanly. The static functions that are exported for Kunit testing still 
need to be declared in a header file. That could also be done via such a 
macro.

Best regards
Thomas

> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> ---
>   drivers/gpu/drm/drm_client_modeset.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index 1b12a3c201a3..f48882941852 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -8,6 +8,9 @@
>    */
>   
>   #include "drm/drm_modeset_lock.h"
> +
> +#include <kunit/visibility.h>
> +
>   #include <linux/module.h>
>   #include <linux/mutex.h>
>   #include <linux/slab.h>
> @@ -159,7 +162,8 @@ drm_connector_has_preferred_mode(struct drm_connector *connector, int width, int
>   	return NULL;
>   }
>   
> -static struct drm_display_mode *drm_connector_pick_cmdline_mode(struct drm_connector *connector)
> +VISIBLE_IF_KUNIT struct drm_display_mode *
> +drm_connector_pick_cmdline_mode(struct drm_connector *connector)
>   {
>   	struct drm_cmdline_mode *cmdline_mode;
>   	struct drm_display_mode *mode;
> @@ -215,6 +219,7 @@ static struct drm_display_mode *drm_connector_pick_cmdline_mode(struct drm_conne
>   
>   	return NULL;
>   }
> +EXPORT_SYMBOL_IF_KUNIT(drm_connector_pick_cmdline_mode);
>   
>   static bool drm_connector_enabled(struct drm_connector *connector, bool strict)
>   {
> @@ -1233,7 +1238,3 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode)
>   	return ret;
>   }
>   EXPORT_SYMBOL(drm_client_modeset_dpms);
> -
> -#ifdef CONFIG_DRM_KUNIT_TEST
> -#include "tests/drm_client_modeset_test.c"
> -#endif

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature