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

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