Message-ID: <CAGegRW7qKAPre7EAh8JLkUuA4Wi0-cKs7q3_qxHAE0nJs6VHEw@mail.gmail.com>
Date: Tue, 21 Oct 2025 19:40:07 +0200
From: Alessandro Carminati <acarmina@...hat.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: Gabriele Paoloni <gpaoloni@...hat.com>, shuah@...nel.org, linux-kselftest@...r.kernel.org, 
	linux-kernel@...r.kernel.org, corbet@....net, linux-doc@...r.kernel.org, 
	linux-mm@...ck.org, safety-architecture@...ts.elisa.tech, 
	kstewart@...uxfoundation.org, chuckwolber@...il.com
Subject: Re: [RFC v2 PATCH 3/3] selftests/devmem: initial testset

Hi Greg,

On Tue, Oct 21, 2025 at 9:41 AM Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Wed, Sep 10, 2025 at 07:00:00PM +0200, Gabriele Paoloni wrote:
> > From: Alessandro Carminati <acarmina@...hat.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@...hat.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?
Indeed, at first glance the test may look simple.
However, despite the high-level concept being straightforward, there
are several corner cases and peculiar aspects that needed to be
covered, that’s why the test grew larger than expected.
For this RFC, my intent was mainly to share the full picture of what I
had in mind.
I fully agree that it can be refined and streamlined in the next iterations.
When I started working on this, I wasn’t aware of any existing LTP
tests for this interface.
After your comment, I double-checked, but I couldn’t find any relevant coverage.
As far as I can tell, neither LTP nor the upstream kernel selftests
(kselftest or KUnit) currently include tests specifically targeting
/dev/mem.

>
> 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.
Yes, that’s intentional.
The main reason is that all the interactions happen from userspace,
and the internal helper functions aren’t meant to be consumed by other
kernel components.
So I chose a kselftest-style approach.
That said, this is still an RFC, I’m happy to adapt it based on
community feedback and direction.

>
> Also /dev/mem should not be used on "modern" systems, so how was this
> tested?
That’s indeed one of the nuances I mentioned earlier.
Access to /dev/mem is often restricted for security reasons, so the
test is designed to detect and adapt to the environment, only
performing operations that are actually allowed.
>
> > +// SPDX-License-Identifier: GPL-2.0+
>
> Are you _sure_ you want GPLv2+?  I have to ask, sorry.
Yes, I used GPLv2+ deliberately, but I’m open to changing it if the
consensus is that GPLv2-only is more appropriate here.

>
> > +/*
> > + * devmem test debug.c
> > + *
> > + * Copyright (C) 2025 Red Hat, Inc. All Rights Reserved.
> > + * Written by Alessandro Carminati (acarmina@...hat.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?
It wasn’t copied, but I agree it looks unnecessary in its current form.
I can remove or rework it in the next version.
>
> > +
> > +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?
You’re right, this was a small custom debug I left in place, mainly
because I needed a way to include additional information that isn’t
normally part of the standard selftest output, such as references to
specific requirements in the code.
My intent was to make it easier to correlate test results with those
requirements.
That said, I fully agree that the implementation isn’t aligned with
the kselftest framework style, and I’ll look into integrating this
need in a more standardized and consistent way in the next iteration.

>
> thanks,
>
> greg k-h
>
Thanks a lot for the detailed feedback, it’s very helpful.
I’ll revise the test accordingly for the next round.

Best regards,
Alessandro


--
---
172

Powered by blists - more mailing lists