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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, May 11, 2026 at 06:01:59PM -0300, Gustavo Sousa wrote:
> 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?

I do not like repetitions either. But I like the result of the doc when
we have a doc explain the overall flow and underneath the function inputs
of that component. Gives a good overview.

So, why can't you do in a way that the doc explains most of it and overall
flow and still leaving a room for the function to explain itself?!

But perhaps we can still say that it is case by case and live without the
:internals:... 

> 
> --
> 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