Re: [PATCH v2 4/8] drm/xe/kunit: Add xe_kunit_helper_is_live_test()

[Gustavo Sousa <gustavo.sousa@intel.com>]

Rodrigo Vivi <rodrigo.vivi@intel.com> writes:

> On Mon, May 11, 2026 at 09:30:49AM -0300, Gustavo Sousa wrote:
>> Jani Nikula <jani.nikula@linux.intel.com> writes:
>> 
>> > On Mon, 11 May 2026, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> >> Jani Nikula <jani.nikula@linux.intel.com> writes:
>> >>
>> >>> On Fri, 08 May 2026, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> >>>> +/**
>> >>>> + * xe_kunit_helper_is_live_test - Return true if @test is a live test.
>> >>>> + * @test: the &kunit test
>> >>>> + *
>> >>>> + * Return: True for a live test and false otherwise.
>> >>>> + */
>> >>>
>> >>> Pardon me for being blunt, but I think this is the worst kind of
>> >>> kernel-doc comment.
>> >>
>> >> I appreciate the bluntness! :-)
>> >>
>> >>>
>> >>> It doesn't provide any additional information to what the function name
>> >>> and signature already convey (which is to say excellent job on naming
>> >>> the function), but it fails to explain what "live test" means.
>> >>
>> >> I kind of just added this kernel-doc to fill a hole for "consistency",
>> >> but, yeah, it does not provide any new info.
>> >>
>> >>>
>> >>> The extra bits of useful information people might need after seeing the
>> >>> function xe_kunit_helper_is_live_test() in code are: What is a live
>> >>> test, and what is it if it's not live? Dead?
>> >>
>> >> Zombie? ;-)
>> >>
>> >> Joking apart, I personally tend to use "regular" to refer to non-live
>> >> tests. I do agree we are missing some documentation on the subject.  I'm
>> >> not sure though this function should be the place to do it.  I think we
>> >> would be better off with a "DOC:" section for that (and also explain
>> >> other bits in there).  I think it would be sensible to rename
>> >> xe_kunit_helpers.c to simply xe_kunit.c and add such a section.
>> >>
>> >> With that in place, this function would be kind of self-explanatory,
>> >> right? Is this a case we just drop the kernel-doc?
>> >>
>> >> Or should we try to be consistent on "every public function should have
>> >> a kernel-doc"?  Is that even a rule or am I imagining things? :-)
>> >
>> > I believe xe maintainership leans more towards requiring kernel-doc
>> > comments than we do with i915 or display. I think the hard requirement
>> > leads to a lot of unnecessary boilerplate, more geared towards filling
>> > the requirement than being informative and helpful.
>> >
>> > Personally, I value overview DOC: comments much more than kernel-doc
>> > comments. If I were to add any hard requirement for documentation, it
>> > would be for DOC: comments for each .c file.
>> >
>> > Bottom line, for xe, ask for xe maintainer opinion.
>> 
>> Cc:Xe maintainers, in case they want to chime in.
>
> I'm definitely the one to be blamed by requesting docs to every
> 'public' function in Xe. :)
>
> In my view this forces developer to see the .c,.h pair as a 'component'
> with specific entry points and a reason to exist, rather than some
> architecture like i915 where .c/.h pairs were only created when some file
> was 'too big'. With the component in mind it is easier to identify when
> something is abusing the interface and accessing specific internal
> types directly rather than having a function entry point to handle it.
>
> But well, the 'Doc: ' is actually part fundamental in this component.
> We should definitely have a 'Doc: ' as well that justifies and give
> reasoning to the component.
>
> That said, in this patch here specifically I agree with Jani. We are
> missing the 'Doc: with the reasoning for the component, and the
> 'public' function documentation could be bringing more useful information
> like Jani pointed out, instead of just stating twice the return value.

One relevant point here is that, once we have a "DOC:" section that will
explain "regular" tests and "live" tests, it will be redundant to add
that detail to the kernel-doc for xe_kunit_helper_is_live_test(), and,
IMO, it would be better not to repeat it.

That brings the question: should "every public function needs a
kernel-doc" be a hard rule?

--
Gustavo Sousa

>
> Thanks,
> Rodrigo.
>
>> 
>> --
>> Gustavo Sousa
>> 
>> >
>> >
>> > BR,
>> > Jani.
>> >
>> >>
>> >> --
>> >> Gustavo Sousa
>> >>
>> >>>
>> >>>
>> >>> BR,
>> >>> Jani.
>> >>>
>> >>>> +bool xe_kunit_helper_is_live_test(struct kunit *test)
>> >>>> +{
>> >>>> +	KUNIT_STATIC_STUB_REDIRECT(xe_kunit_helper_is_live_test, test);
>> >>>> +	return false;
>> >>>> +}
>> >>>
>> >>> -- 
>> >>> Jani Nikula, Intel
>> >
>> > -- 
>> > Jani Nikula, Intel