Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
References: <20230908213148.3610108-1-rmoar@google.com> <20230908213148.3610108-2-rmoar@google.com>
 <CABVgOS=0coPxNE12EsOb-y-VGQ4hafSiNiVj8sLfCCG8LLz8Vw@mail.gmail.com>
In-Reply-To: <CABVgOS=0coPxNE12EsOb-y-VGQ4hafSiNiVj8sLfCCG8LLz8Vw@mail.gmail.com>
From:   Rae Moar <rmoar@google.com>
Date:   Wed, 4 Oct 2023 17:02:20 -0400
Message-ID: <CA+GJov4c0dXjsvC5EoE52RayEvU8KTYMyCue+SuRjx9RHMzoUQ@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] Documentation: Add debugfs docs with run after boot
To:     David Gow <davidgow@google.com>
Cc:     shuah@kernel.org, dlatypov@google.com, brendan.higgins@linux.dev,
        sadiyakazi@google.com, linux-kselftest@vger.kernel.org,
        kunit-dev@googlegroups.com, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Sep 14, 2023 at 5:06=E2=80=AFAM David Gow <davidgow@google.com> wro=
te:
>
> On Sat, 9 Sept 2023 at 05:32, Rae Moar <rmoar@google.com> wrote:
> >
> > Expand the documentation on the KUnit debugfs filesystem on the
> > run_manual.rst page.
> >
> > Add section describing how to access results using debugfs.
> >
> > Add section describing how to run tests after boot using debugfs.
> >
> > Signed-off-by: Rae Moar <rmoar@google.com>
> > Co-developed-by: Sadiya Kazi <sadiyakazi@google.com>
> > Signed-off-by: Sadiya Kazi <sadiyakazi@google.com>
> > ---
>
> Looks good to me, a few nitpicks, and the fact that we'll probably
> need to add something about init section suites when those are
> implemented.
>
> (Also, since you sent the email, your sign off should be at the bottom
> of the list above.)

Hello!

Thanks for the comments! Sorry about the Signed-off order. I will
change that for next time.

>
> >  Documentation/dev-tools/kunit/run_manual.rst | 45 ++++++++++++++++++--
> >  1 file changed, 41 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/dev-tools/kunit/run_manual.rst b/Documentati=
on/dev-tools/kunit/run_manual.rst
> > index e7b46421f247..613385c5ba5b 100644
> > --- a/Documentation/dev-tools/kunit/run_manual.rst
> > +++ b/Documentation/dev-tools/kunit/run_manual.rst
> > @@ -49,9 +49,46 @@ loaded.
> >
> >  The results will appear in TAP format in ``dmesg``.
> >
> > +debugfs
> > +=3D=3D=3D=3D=3D=3D=3D
> > +
> > +``debugfs`` is a file system that enables user interaction with the fi=
les to
> > +make kernel information available to user space. A user can interact w=
ith
> > +the debugfs filesystem using a variety of file operations, such as ope=
n,
> > +read, and write.
> > +
> > +By default, only the root user has access to the debugfs directory.
>
> These two paragraphs are probably a bit excessive: we want to focus on
> what KUnit can do with debugfs, not describing what debugfs is as a
> whole (which is best left to pages like
> Documentation/filesystems/debugfs.rst )

Got it. Maybe I should just leave the first sentence and then link to
../debugfs.rst.

>
> > +
> > +If ``CONFIG_KUNIT_DEBUGFS`` is enabled, you can use KUnit debugfs
> > +filesystem to perform the following actions.
> > +
> > +Retrieve Test Results
> > +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > +
> > +You can use debugfs to retrieve KUnit test results. The test results a=
re
> > +accessible from the debugfs filesystem in the following read-only file=
:
> > +
> > +.. code-block :: bash
> > +
> > +       /sys/kernel/debug/kunit/<test_suite>/results
> > +
> > +The test results are available in KTAP format.
>
> We _could_ mention that this is a separate KTAP document (i.e., the
> numbering starts at 1), though it may be obvious.
>
> > +
> > +Run Tests After Kernel Has Booted
> > +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > +
> > +You can use the debugfs filesystem to trigger built-in tests to run af=
ter
> > +boot. To run the test suite, you can use the following command to writ=
e to
> > +the ``/sys/kernel/debug/kunit/<test_suite>/run`` file:
> > +
> > +.. code-block :: bash
> > +
> > +       echo "any string" > /sys/kernel/debugfs/kunit/<test_suite>/run
> > +
> > +As a result, the test suite runs and the results are printed to the ke=
rnel
> > +log.
> > +
> >  .. note ::
> >
> > -       If ``CONFIG_KUNIT_DEBUGFS`` is enabled, KUnit test results will
> > -       be accessible from the ``debugfs`` filesystem (if mounted).
> > -       They will be in ``/sys/kernel/debug/kunit/<test_suite>/results`=
`, in
> > -       TAP format.
> > +       The contents written to the debugfs file
> > +       ``/sys/kernel/debug/kunit/<test_suite>/run`` are not saved.
>
> This is possibly a bit obvious. Maybe it'd be more useful with a bit
> more context, e.g., "The contents written to the file ... are
> discarded; it is the act of writing which triggers the test, not the
> specific contents written."?

I will try to add more context here in the next version.

>
> It might be worth having a note that tests cannot run concurrently, so
> this may block or fail.
>
> Equally, it may be worth having a note for test authors, that their
> tests will need to correctly initialise and/or clean up any data, so
> the test runs correctly a second time.
>

Yes these are two good points. I will add notes on tests not being
able to run concurrently, cleaning up data, and also init tests.

>
> > --
> > 2.42.0.283.g2d96d420d3-goog
> >