Re: [PATCH v4] kernel.h: add comments for system_states

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

Em Fri, 5 Sep 2025 15:07:31 -0700
Randy Dunlap <rdunlap@infradead.org> escreveu:

> Hi,
> 
> On 9/5/25 6:38 AM, Mauro Carvalho Chehab wrote:
> > On Fri, Sep 05, 2025 at 04:06:31PM +0300, Jani Nikula wrote:  
> >> On Fri, 05 Sep 2025, Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:  
> >>> Em Fri, 05 Sep 2025 12:02:37 +0300
> >>> Jani Nikula <jani.nikula@linux.intel.com> escreveu:
> >>>  
> >>>> On Wed, 03 Sep 2025, Randy Dunlap <rdunlap@infradead.org> wrote:  
> >>>>> Provide some basic comments about the system_states and what they imply.
> >>>>> Also convert the comments to kernel-doc format.
> >>>>>
> >>>>> However, kernel-doc does not support kernel-doc notation on extern
> >>>>> struct/union/typedef/enum/etc. So I made this a DOC: block so that
> >>>>> I can use (insert) it into a Documentation (.rst) file and have it
> >>>>> look decent.    
> >>>>
> >>>> The simple workaround is to separate the enum type and the variable:
> >>>>
> >>>> /**
> >>>>  * kernel-doc for the enum
> >>>>  */
> >>>> enum system_states {
> >>>> 	...
> >>>> };
> >>>>
> >>>> /**
> >>>>  * kernel-doc for the variable
> >>>>  */
> >>>> extern enum system_states system_state;
> >>>>
> >>>> BR,
> >>>> Jani.
> >>>>  
> >>>>>
> >>>>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> >>>>> Acked-by: Rafael J. Wysocki <rafael@kernel.org> # v1
> >>>>> ---  
> 
> [snip]
> >>> If the problem is with "extern" before enum, fixing kdoc be
> >>> fairly trivial.  
> >>
> >> The non-trivial part is deciding whether you're documenting the enum
> >> type or the variable. Both are equally valid options.  
> > 
> > True.
> > 
> > I'd say that, if the variable is under EXPORT_SYMBOL macros, it
> > should be documented.  
> 
> Do you mean documented with kernel-doc? How do we do that?
> I was under the impression that we don't currently have a way to
> use kernel-doc for variables (definitions, only for declarations).

No, but it shouldn't be hard to add something like:

	/**
	 * global system_state - stores system state
	 * <some description>
	 */
	extern enum system_states system_state;  

and place a dump_global() function at kdoc parser. Ok, one might use
DOC:, but IMHO the best is to have a proper handler for it that would
automatically pick the type.

> > The same applies to the enum: if it is used by kAPI, it should
> > also be documented.
> > 
> > So, yeah, I suspect that on most kAPI cases, the best is to split,
> > having documentation for both.
> >   
> >>
> >> BR,
> >> Jani.
> >>  
> >>>
> >>> If you see:
> >>>
> >>> 	def dump_function(self, ln, prototype):
> >>> 	
> >>>      	# Prefixes that would be removed
> >>>         sub_prefixes = [
> >>> 	    ...
> >>> 	    (r"^extern +", "", 0),	
> >>> 	    ...
> >>> 	}
> >>>
> >>>         for search, sub, flags in sub_prefixes:
> >>>             prototype = KernRe(search, flags).sub(sub, prototype)
> >>>
> >>>
> >>> we need something similar to that at:
> >>> 	def dump_enum(self, ln, proto):
> >>>
> >>> alternatively, we could use a simpler approach modifying adding a
> >>> non-group optional match for "extern". E.g:
> >>>
> >>> -	r = KernRe(r'enum\s+(\w*)\s*\{(.*)\}')
> >>> +       r = KernRe(r'(?:extern\s+)?enum\s+(\w*)\s*\{(.*)\}')
> >>>
> >>> (untested)  
> 
> Yes, this might work. It might also lead to documenting more than
> was intended. It seems safer just to do the enum declaration and
> variable definition split light Jani suggested.

Yeah, the extra var at the end may be problematic, although it
wouldn't be hard to drop or parse it, like:

	/**
	 * enum system_states - Values used for system_state.
	 *
	 * @SYSTEM_BOOTING:	%0, no init needed
	 * @SYSTEM_SCHEDULING: system is ready for scheduling; OK to use RCU
	 * @SYSTEM_FREEING_INITMEM: system is freeing all of initmem; almost running
	 * @SYSTEM_RUNNING:	system is up and running
	 * @SYSTEM_HALT:	system entered clean system halt state
	 * @SYSTEM_POWER_OFF:	system entered shutdown/clean power off state
	 * @SYSTEM_RESTART:	system entered emergency power off or normal restart
	 * @SYSTEM_SUSPEND:	system entered suspend or hibernate state
	 *
	 * @system_state: global var describing the current system state.
 	 *
	 * Note:
	 * Ordering of the states must not be changed
	 * as code checks for <, <=, >, >= STATE.
	 */
	extern enum system_states {
	 	SYSTEM_BOOTING,
	 	SYSTEM_SCHEDULING,
	 	SYSTEM_FREEING_INITMEM,
		SYSTEM_RUNNING,
		SYSTEM_HALT,
	 	SYSTEM_POWER_OFF,
	 	SYSTEM_RESTART,
	 	SYSTEM_SUSPEND,
	} system_state;

making it create two separate entries: one for the enum, and another one for
the global var - internally calling dump_global() for the var.

Thanks,
Mauro