Re: [Intel-gfx] [PATCH v3 19/37] drm/i915: stop using kernel-doc markups for something else

[Mauro Carvalho Chehab <mchehab@kernel.org>]

Em Mon, 12 Sep 2022 10:19:04 -0700
Matt Roper <matthew.d.roper@intel.com> escreveu:

> > Those *appear* to be kernel-doc markups, but they aren't, because
> > the structs themselves are not properly marked. See, for instance
> > struct intel_context.
> > 
> > scripts/kerneldoc will *only* consider what's there as a proper
> > comment if you add:
> > 
> > 	/**
> > 	 * struct intel_context - describes an i915 context
> > 	 * <add a proper description for it>
> > 	 */
> > 	struct intel_context {
> > 		union {
> > 			/** @ref: a kernel object reference */
> > 	                struct kref ref; /* no kref_get_unless_zero()! */
> > 			/** @rcu: a rcu header */
> > 	                struct rcu_head rcu;
> > 	        };
> > 		...
> > 	}
> > 
> > Describing all fields inside the struct. Just placing
> > 	/** something */
> > on some random places in the middle doesn't make it a kernel-doc.  
> 
> Right, what we have today is incomplete/incorrect.  But I think we
> should be fixing that by adding the missing documentation on the
> structure, rather than giving up and removing the kerneldoc.  The end
> goal should be to have proper generated documentation, not just silence
> the warnings while leaving the actual output incomplete.

The end goal is to have *real* kernel-doc markups, not fake ones.
We're aiming towards there, where the first step is to get rid of the
ones that just pretend to be kernel-doc without really being validated
in order to check if they produce a valid content.

See, what we have so far are just comments. Some examples from this
patch:

	/** Powers down the TV out block, and DAC0-3 */
	 #define CH7017_TV_POWER_DOWN_EN		(1 << 5)

Currently, kernel-doc doesn't have any markup for not-function defines.

What we do to document this at kernel-doc is to either:

1. convert to an enum;
2. embed it inside some struct (or function) definition.

Other examples: this is not a Kernel-doc markup:

	/** @file
	  * driver for the Chrontel 7xxx DVI chip over DVO.
	  */

Neither this:

	/**
 	 * active: Active tracker for the rq activity (inc. external) on this
 	 * intel_context object.
 	 */
 
In summary, what happens is that those "/**" tags removed on this patch are
just pretending to be Kernel-doc, but they aren't anything. See, when a newbie
starts programming in C code, without having a compiler, lots of syntax
issues will happen. When they try to compile their code, hundreds or errors
and warnings happen. That's basically what it is happening here.

See, the very basic syntax thing is missing: just like a C file requires
that all codes to be inside functions, a kernel-doc field description 
requires a kernel-doc markup for the struct/function/enum/union that
contains it.

-

Now, I fully agree that the end goal is to have proper kernel-doc markups.

Adding those require a lot of archaeological work, looking at the git
commits which introduced the changes. Patch 34/37, for instance, does
that for struct drm_i915_gem_object:

	https://lists.freedesktop.org/archives/intel-gfx/2022-September/305736.html

See, besides adding a real Kernel-doc markup for the struct:

	+/**
	+ * struct drm_i915_gem_object - describes an i915 GEM object
	+ */
	 struct drm_i915_gem_object {

It had to fix several sintax and semantic mistakes:

Typos:

	/**
-	 * @shared_resv_from: The object shares the resv from this vm.
+	 * @shares_resv_from: The object shares the resv from this vm.
 	 */

Invalid kernel-doc markups:

	 	/**
	-	 * @mem_flags - Mutable placement-related flags
	+	 * @mem_flags: Mutable placement-related flags

Kernel markups that miss that they're on an embedded struct inside
the main one (thus those are also invalid kernel-doc markups):

                /**
-                * @shrink_pin: Prevents the pages from being made visible to
+                * @mm.shrink_pin: Prevents the pages from being made visible to


Etc.

Thanks,
Mauro