Randy Dunlap
2023-Dec-25 07:40 UTC
[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
Vegard Nossum
2023-Dec-25 08:30 UTC
[PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings
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
Randy Dunlap
2023-Dec-25 17:08 UTC
[PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings
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
Apparently Analagous Threads
- [PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings
- [PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings
- [PATCH drm-next 05/14] drm/nouveau: new VM_BIND uapi interfaces
- [PATCH drm-misc-next v8 03/12] drm/nouveau: new VM_BIND uapi interfaces
- [PATCH drm-next 05/14] drm/nouveau: new VM_BIND uapi interfaces