Re: [RFC v2 PATCH 3/3] selftests/devmem: initial testset

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Sep 10, 2025 at 07:00:00PM +0200, Gabriele Paoloni wrote:
> From: Alessandro Carminati <acarmina@redhat.com>
> 
> This patch introduces a new series of tests for devmem.
> Test cases are mapped against the tested Function's expectations
> defined in /drivers/char/mem.c.

Cool, but:

> 
> Signed-off-by: Alessandro Carminati <acarmina@redhat.com>
> ---
>  tools/testing/selftests/Makefile         |   1 +
>  tools/testing/selftests/devmem/Makefile  |  13 +
>  tools/testing/selftests/devmem/debug.c   |  25 +
>  tools/testing/selftests/devmem/debug.h   |  14 +
>  tools/testing/selftests/devmem/devmem.c  | 200 ++++++++
>  tools/testing/selftests/devmem/ram_map.c | 250 ++++++++++
>  tools/testing/selftests/devmem/ram_map.h |  38 ++
>  tools/testing/selftests/devmem/secret.c  |  46 ++
>  tools/testing/selftests/devmem/secret.h  |  13 +
>  tools/testing/selftests/devmem/tests.c   | 569 +++++++++++++++++++++++
>  tools/testing/selftests/devmem/tests.h   |  45 ++
>  tools/testing/selftests/devmem/utils.c   | 379 +++++++++++++++
>  tools/testing/selftests/devmem/utils.h   | 119 +++++

That's a lot of files for a "simple" test.  Doesn't LTP have tests for
this api already?  Why not use that here instead?

Also, this is userspace testing, not kunit testing, right, is that
intentional?  You are documenting internal apis and then writing
userspace tests for those apis, which feels a bit odd.

Also /dev/mem should not be used on "modern" systems, so how was this
tested?

> +// SPDX-License-Identifier: GPL-2.0+

Are you _sure_ you want GPLv2+?  I have to ask, sorry.

> +/*
> + * devmem test debug.c
> + *
> + * Copyright (C) 2025 Red Hat, Inc. All Rights Reserved.
> + * Written by Alessandro Carminati (acarmina@redhat.com)
> + */
> +
> +#include <stdio.h>
> +#include <stdarg.h>
> +
> +#define DEBUG_FLAG 0
> +int pdebug = DEBUG_FLAG;

That's a funny define that is never used elsewhere.  I'm guessing this
was cut/pasted from some other userspace code somewhere?

> +
> +void deb_printf(const char *fmt, ...)

Who is "deb"?  You have more letters, always use them :)

Also, why debugging for just this one set of tests?  Don't kselftests
already have debugging logic?  if not, why is this unique to require it?

And am I missing something, or does this new tool not tie into the
kselftest framework properly?  I see lots of printing to output, but not
in the proper test framework format, am I just missing that somewhere?

thanks,

greg k-h