Nouveau - Dec 2023 - [PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings

On 12/24/23 22:51, Vegard Nossum wrote:
> As of commit b77fdd6a48e6 ("scripts/kernel-doc: restore warning for
> Excess struct/union"), we see the following warnings when running
'make
> htmldocs':
> 
>   ./include/uapi/drm/nouveau_drm.h:292: warning: Excess struct member
'DRM_NOUVEAU_VM_BIND_OP_MAP' description in
'drm_nouveau_vm_bind_op'
>   ./include/uapi/drm/nouveau_drm.h:292: warning: Excess struct member
'DRM_NOUVEAU_VM_BIND_OP_UNMAP' description in
'drm_nouveau_vm_bind_op'
>   ./include/uapi/drm/nouveau_drm.h:292: warning: Excess struct member
'DRM_NOUVEAU_VM_BIND_SPARSE' description in
'drm_nouveau_vm_bind_op'
>   ./include/uapi/drm/nouveau_drm.h:336: warning: Excess struct member
'DRM_NOUVEAU_VM_BIND_RUN_ASYNC' description in
'drm_nouveau_vm_bind'
> 
> The problem is that these values are #define constants, but had kerneldoc
> comments attached to them as if they were actual struct members.
> 
> There are a number of ways we could fix this, but I chose to draw
> inspiration from include/uapi/drm/i915_drm.h, which pulls them into the
> corresponding kerneldoc comment for the struct member that they are
> intended to be used with.
> 
> To keep the diff readable, there are a number of things I _didn't_ do
in
> this patch, but which we should also consider:
> 
> - This is pretty good documentation, but it ends up in gpu/driver-uapi,
>   which is part of subsystem-apis/ when it really ought to display under
>   userspace-api/ (the "Linux kernel user-space API guide" book of
the
>   documentation).
> 
> - More generally, we might want a warning if include/uapi/ files are
>   kerneldoc'd outside userspace-api/.
> 
> - I'd consider it cleaner if the #defines appeared between the
kerneldoc
>   for the member and the member itself (which is something other DRM-
>   related UAPI docs do).
> 
> - The %IDENTIFIER kerneldoc syntax is intended for "constants",
and is
>   more appropriate in this context than ``IDENTIFIER`` or &IDENTIFIER.
>   The DRM docs aren't very consistent on this.
> 
> Cc: Randy Dunlap <rdunlap at infradead.org>
> Cc: Jonathan Corbet <corbet at lwn.net>
> Signed-off-by: Vegard Nossum <vegard.nossum at oracle.com>

This all looks good to me. Thanks for your help.

Reviewed-by: Randy Dunlap <rdunlap at infradead.org>
Tested-by: Randy Dunlap <rdunlap at infradead.org>

I do see one thing that I don't like in the generated html output.
It's not a problem with this patch.
The #defines for DRM_NOUVEAU_VM_BIND_OP_MAP etc. have a ';' at the
end of each line:

struct drm_nouveau_vm_bind_op {
    __u32 op;
#define DRM_NOUVEAU_VM_BIND_OP_MAP 0x0;
#define DRM_NOUVEAU_VM_BIND_OP_UNMAP 0x1;
    __u32 flags;
#define DRM_NOUVEAU_VM_BIND_SPARSE (1 << 8);
    __u32 handle;
    __u32 pad;
    __u64 addr;
    __u64 bo_offset;
    __u64 range;
};

so something else to look into one of these days....

> ---
>  include/uapi/drm/nouveau_drm.h | 56 ++++++++++++++++------------------
>  1 file changed, 27 insertions(+), 29 deletions(-)
> 
> diff --git a/include/uapi/drm/nouveau_drm.h
b/include/uapi/drm/nouveau_drm.h
> index 0bade1592f34..c95ef8a4d94a 100644

-- 
#Randy
https://people.kernel.org/tglx/notes-about-netiquette
https://subspace.kernel.org/etiquette.html

On 25/12/2023 08:40, Randy Dunlap wrote:
> I do see one thing that I don't like in the generated html output.
> It's not a problem with this patch.
> The #defines for DRM_NOUVEAU_VM_BIND_OP_MAP etc. have a ';' at the
> end of each line:
> 
> struct drm_nouveau_vm_bind_op {
>      __u32 op;
> #define DRM_NOUVEAU_VM_BIND_OP_MAP 0x0;
> #define DRM_NOUVEAU_VM_BIND_OP_UNMAP 0x1;
>      __u32 flags;
> #define DRM_NOUVEAU_VM_BIND_SPARSE (1 << 8);
>      __u32 handle;
>      __u32 pad;
>      __u64 addr;
>      __u64 bo_offset;
>      __u64 range;
> };
Do we actually ever want preprocessor directives to appear inside
definitions in the output? If not, I think this should work:

diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 3cdc7dba37e3..61425fc9645e 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -1259,6 +1259,8 @@ sub dump_struct($$) {
                 $clause =~ s/\s+$//;
                 $clause =~ s/\s+/ /;
                 next if (!$clause);
+               # skip preprocessor directives
+               next if $clause =~ m/^#/;
                 $level-- if ($clause =~ m/(\})/ && $level > 1);
                 if (!($clause =~ m/^\s*#/)) {
                         $declaration .= "\t" x $level;


Vegard

On 12/25/23 00:30, Vegard Nossum wrote:
> 
> On 25/12/2023 08:40, Randy Dunlap wrote:
>> I do see one thing that I don't like in the generated html output.
>> It's not a problem with this patch.
>> The #defines for DRM_NOUVEAU_VM_BIND_OP_MAP etc. have a ';' at
the
>> end of each line:
>>
>> struct drm_nouveau_vm_bind_op {
>> ???? __u32 op;
>> #define DRM_NOUVEAU_VM_BIND_OP_MAP 0x0;
>> #define DRM_NOUVEAU_VM_BIND_OP_UNMAP 0x1;
>> ???? __u32 flags;
>> #define DRM_NOUVEAU_VM_BIND_SPARSE (1 << 8);
>> ???? __u32 handle;
>> ???? __u32 pad;
>> ???? __u64 addr;
>> ???? __u64 bo_offset;
>> ???? __u64 range;
>> };
> 
> Do we actually ever want preprocessor directives to appear inside
> definitions in the output? If not, I think this should work:
Not necessarily.
> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> index 3cdc7dba37e3..61425fc9645e 100755
> --- a/scripts/kernel-doc
> +++ b/scripts/kernel-doc
> @@ -1259,6 +1259,8 @@ sub dump_struct($$) {
> ??????????????? $clause =~ s/\s+$//;
> ??????????????? $clause =~ s/\s+/ /;
> ??????????????? next if (!$clause);
> +?????????????? # skip preprocessor directives
> +?????????????? next if $clause =~ m/^#/;
> ??????????????? $level-- if ($clause =~ m/(\})/ && $level > 1);
> ??????????????? if (!($clause =~ m/^\s*#/)) {
> ??????????????????????? $declaration .= "\t" x $level;
> 
> 
but that didn't work for me.
I don't have time to look into it any more today.  :)

Thanks.

-- 
#Randy
https://people.kernel.org/tglx/notes-about-netiquette
https://subspace.kernel.org/etiquette.html