Re: [PATCH 1/2] cpumask: Fix kernel-doc formatting errors in cpumask.h

[Yury Norov]

On Fri, Mar 07, 2025 at 12:38:41AM +0900, Akira Yokosawa wrote:
> Hello Viresh,
> 
> On Thu,  6 Mar 2025 16:06:50 +0530, Viresh Kumar wrote:
> > This fixes various kernel-doc formatting errors in cpumask.h:
> > 
> > - WARNING: Inline literal start-string without end-string.
> > - ERROR: Unexpected indentation.
> > - ERROR: Unknown target name: "gfp".
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > ---
> >  include/linux/cpumask.h | 65 +++++++++++++++++++++++------------------
> >  1 file changed, 37 insertions(+), 28 deletions(-)
> > 
> > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> > index 36a890d0dd57..73ba808c559f 100644
> > --- a/include/linux/cpumask.h
> > +++ b/include/linux/cpumask.h
> > @@ -20,7 +20,7 @@
> >   * cpumask_pr_args - printf args to output a cpumask
> >   * @maskp: cpumask to be printed
> >   *
> > - * Can be used to provide arguments for '%*pb[l]' when printing a cpumask.
> > + * Can be used to provide arguments for '\%*pb[l]' when printing a cpumask.
> 
> kernel-doc (script) can recognize the pattern of %*pb but not %*pb[l].
> "%*bp [l]" should work here.
> (without quotes and a white space in front of "[").

So why not fixing kernel-doc instead?
 
> No need to escape "%".
> 
> >   */
> >  #define cpumask_pr_args(maskp)		nr_cpu_ids, cpumask_bits(maskp)
> >  
> > @@ -166,7 +166,7 @@ static __always_inline unsigned int cpumask_first_zero(const struct cpumask *src
> >  }
> >  
> >  /**
> > - * cpumask_first_and - return the first cpu from *srcp1 & *srcp2
> > + * cpumask_first_and - return the first cpu from \*srcp1 & \*srcp2
> 
> kernel-doc (script) understands the pattern of *@srcp1.
> No need to escape "*".

Yes, please don't do it. The comments first and foremost should be a
human-readable text. If we add C code, it should stay a C code.

> But it does not (yet) parse the pattern of "*@n+1".  You need to say
> "*@n +1", with a space in front of "+1", for the time being.
> 
> [...]
> > @@ -335,6 +335,9 @@ unsigned int __pure cpumask_next_wrap(int n, const struct cpumask *mask, int sta
> >   * @mask2: the second cpumask pointer
> >   *
> >   * This saves a temporary CPU mask in many places.  It is equivalent to:
> > + *
> > + * .. code-block:: c
> > + *
> >   *	struct cpumask tmp;
> >   *	cpumask_and(&tmp, &mask1, &mask2);
> >   *	for_each_cpu(cpu, &tmp)
> 
> Do you really want those code-blocks to look fancy?
> 
> In kernel-doc comments, I'd normally use plain literal blocks instead.
> 
> Something like:
> 
>  * This saves a temporary CPU mask in many places.  It is equivalent to::
>  *
>  *	struct cpumask tmp;
>  *	cpumask_and(&tmp, &mask1, &mask2);
>  *	for_each_cpu(cpu, &tmp)
> 
> should work.  Note the "::" and the empty line below it.
> 
> [...]
> > @@ -941,7 +950,7 @@ bool zalloc_cpumask_var_node(cpumask_var_t *mask, gfp_t flags, int node)
> >  /**
> >   * alloc_cpumask_var - allocate a struct cpumask
> >   * @mask: pointer to cpumask_var_t where the cpumask is returned
> > - * @flags: GFP_ flags
> > + * @flags: GFP\_ flags
> 
> You can say:
> 
>     * @flags: %GFP_ flags
> 
> instead.
> 
> >   *
> >   * Only defined when CONFIG_CPUMASK_OFFSTACK=y, otherwise is
> >   * a nop returning a constant 1 (in <linux/cpumask.h>).
> 
> [...]
> 
> Side note:
> 
> I think 1/2 would be better to be CC'ed linux-doc as well.
> Please do so in respin. 

I agree. I'm all for covering cpumasks documentation with kernel-doc,
but it should stay readable after that.

Thanks,
Yury