[igt-dev] [PATCH i-g-t 4/4] lib/igt_kmod: add igt_kselftests documentation
Tales
tales.aparecida at gmail.com
Wed Aug 3 15:49:08 UTC 2022
Em qua., 3 de ago. de 2022 às 05:48, Petri Latvala
<petri.latvala at intel.com> escreveu:
>
> On Wed, Aug 03, 2022 at 02:26:54AM -0300, Tales Aparecida wrote:
> > Add documentation for igt_kselftests functions and structs. Also adds
> > comments to non trivial lines.
> >
> > Signed-off-by: Tales Aparecida <tales.aparecida at gmail.com>
> > ---
> > lib/igt_kmod.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++++
> > lib/igt_kmod.h | 18 +++++++++
> > 2 files changed, 125 insertions(+)
> >
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > index 63636243..852112cc 100644
> > --- a/lib/igt_kmod.c
> > +++ b/lib/igt_kmod.c
> > @@ -792,6 +792,12 @@ igt_amdgpu_driver_unload(void)
> > return IGT_EXIT_SUCCESS;
> > }
> >
> > +/*
> > + * kmsg_dump:
> > + * @fd: file descriptor for `/dev/kmsg` opened for reading
> > + *
> > + * Dump kmsg output into stderr.
> > + */
>
> The documentation comment needs to begin with /** (two *)
I forgot to mention in the cover letter, but I've followed the
recommendation of using normal comments (without the 2nd '*' in the
first line) for static symbols from GtkDoc.It's at the end of this
page: https://developer-old.gnome.org/gtk-doc-manual/stable/documenting_syntax.html.en
WIth that said, I rather add the second *, as well, to enable syntax
highlighting.
>
> > static void kmsg_dump(int fd)
> > {
> > char record[4096 + 1];
> > @@ -832,6 +838,13 @@ static void kmsg_dump(int fd)
> > }
> > }
> >
> > +/*
> > + * tests_add:
> > + * @tl: (in): test to be inserted
> > + * @list: (inout): ordered list of tests sorted by test->number
> > + *
> > + * Insert test at the correct position in the ordered list
> > + */
>
> Same for this.
>
> > static void tests_add(struct igt_kselftest_list *tl, struct igt_list_head *list)
> > {
> > struct igt_kselftest_list *pos;
> > @@ -843,6 +856,14 @@ static void tests_add(struct igt_kselftest_list *tl, struct igt_list_head *list)
> > igt_list_add_tail(&tl->link, &pos->link);
> > }
> >
> > +/**
> > + * igt_kselftest_get_tests:
> > + * @kmod: (in): kernel module from which to parse module_params' names into IGT tests
> > + * @filter: (in)(nullable): get only tests with a prefix matching this filter
> > + * @tests: (inout): resulting ordered list of tests
> > + *
> > + * Get IGT tests' names from module_params' names, given a module.
> > + */
> > void igt_kselftest_get_tests(struct kmod_module *kmod,
> > const char *filter,
> > struct igt_list_head *tests)
> > @@ -856,25 +877,33 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
> > if (!kmod_module_get_info(kmod, &pre))
> > return;
> >
> > + /* iter through module infos trying to find tests, which should be
> > + * named following the pattern: "igt__{test_number}__{test_name}:bool",
> > + * e.g.: "igt__18__perf_engine_cs:bool" */
>
> Multiline comments should have /* and */ on their own lines.
You're absolutely right!
>
> > kmod_list_foreach(d, pre) {
> > const char *key, *val;
> > char *colon;
> > int offset;
> >
> > + /* skip any info that is not a module param */
>
> ^^ extra space there
Nice catch!
>
> > key = kmod_module_info_get_key(d);
> > if (strcmp(key, "parmtype"))
> > continue;
> >
> > + /* skip any module param that don't have the IGT test prefix */
> > val = kmod_module_info_get_value(d);
> > if (!val || strncmp(val, param_prefix, prefix_len))
> > continue;
> >
> > + /* alloc list-node that will store the module_param name */
> > offset = strlen(val) + 1;
> > tl = malloc(sizeof(*tl) + offset);
> > if (!tl)
> > continue;
> >
> > memcpy(tl->param, val, offset);
> > +
> > + /* find and replace colon with \0 to discard module_param type */
> > colon = strchr(tl->param, ':');
> > *colon = '\0';
> >
> > @@ -884,6 +913,7 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
> > &tl->number, &offset) == 1)
> > tl->name += offset;
> >
> > + /* skip any test that doesn't match the prefix filter */
> > if (filter && strncmp(tl->name, filter, strlen(filter))) {
> > free(tl);
> > continue;
> > @@ -902,6 +932,22 @@ static int open_parameters(const char *module_name)
> > return open(path, O_RDONLY);
> > }
> >
> > +/**
> > + * igt_kselftest_init:
> > + * @tst: (inout): pointer of #igt_kselftest to be initialized
> > + * @module_name: (in): name of the module with IGT tests declared as module_params
> > + *
> > + * Initialize the given struct igt_kselftest and the inner struct kmod_module.
> > + *
> > + * @Note: This function initialize @tst->kmsg with `-1`, call
> > + * igt_kselftest_begin() to actually open the <filename>/dev/kmsg</filename> stream.
> > + * @tst->module_name should be freed by the caller with free(), @tst->kmod
> > + * refcount needs to be decremented to be released using
> > + * <function>kmod_module_unref</function>.
> > + *
> > + * Returns: 0 on success, non-zero otherwise. It fails if name is not a valid
> > + * module name or if memory allocation failed.
> > + */
> > int igt_kselftest_init(struct igt_kselftest *tst,
> > const char *module_name)
> > {
> > @@ -922,6 +968,17 @@ int igt_kselftest_init(struct igt_kselftest *tst,
> > return 0;
> > }
> >
> > +/**
> > + * igt_kselftest_begin:
> > + * @tst: (inout): pointer to #igt_kselftest with `kmod_module` handler to unload
> > + * and where the file descriptor for <filename>/dev/kmsg</filename> will be
> > + * stored.
> > + *
> > + * Unload the module that will be tested and open kmsg to start reading it.
> > + * If module is i915, unloads some of its dependencies as well.
> > + *
> > + * Returns: 0 on success, non-zero otherwise.
> > + */
> > int igt_kselftest_begin(struct igt_kselftest *tst)
> > {
> > int err;
> > @@ -939,6 +996,21 @@ int igt_kselftest_begin(struct igt_kselftest *tst)
> > return 0;
> > }
> >
> > +/**
> > + * igt_kselftest_execute:
> > + * @tst: (in): handler for module to be tested and inner kmsg file descriptor
> > + * @tl: (in): contains the name of the test to be executed
> > + * @options: (in)(nullable): Extra parameters for the module. NULL in case no
> > + * parameters are to be passed, or a '\0' terminated string otherwise.
> > + * @result: (in)(nullable): NULL if the result of loading the module should be
> > + * returned, or a '\0' terminated string with the name of the module_param
> > + * holding the test's result, if that should be returned instead.
> > + *
> > + * Execute the given test by loading the module and asserts its success.
> > + *
> > + * Returns: 0 on success, non-zero otherwise. If @result is set, returns the
> > + * value from the given module_param after running the test.
> > + */
> > int igt_kselftest_execute(struct igt_kselftest *tst,
> > struct igt_kselftest_list *tl,
> > const char *options,
> > @@ -952,6 +1024,7 @@ int igt_kselftest_execute(struct igt_kselftest *tst,
> >
> > lseek(tst->kmsg, 0, SEEK_END);
> >
> > + /* prepare modprobe parameters to enable a test with extra options */
> > snprintf(buf, sizeof(buf), "%s=1 %s", tl->param, options ?: "");
> >
> > err = modprobe(tst->kmod, buf);
> > @@ -977,18 +1050,40 @@ int igt_kselftest_execute(struct igt_kselftest *tst,
> > return err;
> > }
> >
> > +/**
> > + * igt_kselftest_end:
> > + * @tst: (in): handler for module to be unloaded and kmsg file descriptor
> > + *
> > + * Unload the module and close the kmsg file descriptor.
> > + */
> > void igt_kselftest_end(struct igt_kselftest *tst)
> > {
> > kmod_module_remove_module(tst->kmod, KMOD_REMOVE_FORCE);
> > close(tst->kmsg);
> > }
> >
> > +/**
> > + * igt_kselftest_fini:
> > + * @tst: (in): container of items to be freed
> > + *
> > + * Free memory from @tst->module_name and remove reference for @tst->kmod
> > + */
> > void igt_kselftest_fini(struct igt_kselftest *tst)
> > {
> > free(tst->module_name);
> > kmod_module_unref(tst->kmod);
> > }
> >
> > +/*
> > + * unfilter:
> > + * @filter: (in)(nullable): prefix to be removed
> > + * @name: (in): string to be trimmed
> > + *
> > + * Remove prefix @filter from @name, removing an additional non-alpha char,
> > + * if there's one.
> > + *
> > + * Returns: a pointer to the resulting string
> > + */
>
> Another doc block without /**
>
>
> --
> Petri Latvala
>
>
> > static const char *unfilter(const char *filter, const char *name)
> > {
> > if (!filter)
> > @@ -1001,6 +1096,18 @@ static const char *unfilter(const char *filter, const char *name)
> > return name;
> > }
> >
> > +/**
> > + * igt_kselftests:
> > + * @module_name: (in): name of the module to find and run tests for
> > + * @options: (in)(nullable): Extra parameters for the module. NULL in case no
> > + * parameters are to be passed, or a '\0' terminated string otherwise.
> > + * @result: (in)(nullable): NULL if the result of loading the module should be
> > + * returned, or a '\0' terminated string with the name of the module_param
> > + * holding the test's result, if that should be returned instead.
> > + * @filter: (in)(nullable): run only tests with a prefix matching this filter
> > + *
> > + * Find and execute kselftests defined as module_params of the given module.
> > + */
> > void igt_kselftests(const char *module_name,
> > const char *options,
> > const char *result,
> > diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
> > index f98dd29f..4f81641b 100644
> > --- a/lib/igt_kmod.h
> > +++ b/lib/igt_kmod.h
> > @@ -50,12 +50,30 @@ void igt_kselftests(const char *module_name,
> > const char *result_option,
> > const char *filter);
> >
> > +/**
> > + * igt_kselftest:
> > + * @kmod: reference for <structname>kmod_module</structname> used to load and
> > + * unload the module
> > + * @module_name: name of the module containing the kselftests for IGT
> > + * @kmsg: holds a file descriptor for <filename>/dev/kmsg</filename>
> > + *
> > + * Handle a kernel module with kselftests and their output
> > + */
> > struct igt_kselftest {
> > struct kmod_module *kmod;
> > char *module_name;
> > int kmsg;
> > };
> >
> > +/**
> > + * igt_kselftest_list:
> > + * @link: doubly linked list attributes
> > + * @number: defines the relative position of each element in the ordered list
> > + * @name: pointer to the test's name, a suffix of <structfield>param</structfield>
> > + * @param: name of the module_param that enables/disables the test
> > + *
> > + * Node of an ordered list of test names
> > + */
> > struct igt_kselftest_list {
> > struct igt_list_head link;
> > unsigned int number;
> > --
> > 2.37.0
> >
Thanks for the review, Petri
More information about the igt-dev
mailing list