Re: [PATCH v3 3/9] kexec_file: Factor out kexec_locate_mem_hole from kexec_add_buffer.

[Dave Young]

On 06/23/16 at 12:37pm, Thiago Jung Bauermann wrote:
> Am Donnerstag, 23 Juni 2016, 01:44:07 schrieb Dave Young:
> > Hmm, hold on. For declaring a struct in a header file, comment should be
> > just after each fields, like below, your format is for a function instead:
> > struct pci_slot {
> >         struct pci_bus *bus;            /* The bus this slot is on */
> >         struct list_head list;          /* node in list of slots on this
> > bus */ struct hotplug_slot *hotplug;   /* Hotplug info (migrate over
> > time) */ unsigned char number;           /* PCI_SLOT(pci_dev->devfn) */
> > struct kobject kobj;
> > };
> 
> The comment style you mention above is not extractable documentation. The 
> style I used is what is described in section "kernel-doc for structs, 
> unions, enums, and typedefs" in Documentation/kernel-doc-nano-HOWTO.txt.

You are right and I was wrong!

>  
> > BTW, what is @size? there's no size field in kexec_buf. I think it is not
> > necessary to add these comment, they are easy to understand. If you really
> > want, please rewrite them correctly, for example "image" description is
> > wrong. It is not only for searching memory only, top_down description is
> > also bad.
> 
> Sorry, I moved these comments from kexec_locate_mem_hole but forgot to 
> rename the parameters to what they are called in struct kexec_buf. @size 
> should have been @memsz (other fields also have wrong names, I'll fix them 
> as well). The image description is correct in the context of where struct 
> kexec_buf is used and explains what it will be used for in the function 
> taking kexec_buf as an argument. It is not meant as a general description of 
> the purpose of struct kimage. What is bad about the description of top_down?

It is not clear enough to me, I personally think the original one in
source code is better:
/* allocate from top of memory hole */

> 
> I decided to add these comments because struct kexec_buf is now part of the 
> kernel API for kexec. kernel-doc-nano-HOWTO.txt says:
> 
> > We definitely need kernel-doc formatted documentation for functions
> > that are exported to loadable modules using EXPORT_SYMBOL.
> > 
> > We also look to provide kernel-doc formatted documentation for
> > functions externally visible to other kernel files (not marked
> > "static").
> > 
> > We also recommend providing kernel-doc formatted documentation
> > for private (file "static") routines, for consistency of kernel
> > source code layout.  But this is lower priority and at the
> > discretion of the MAINTAINER of that kernel source file.
> 
> If you think they are not necessary or just add clutter I can leave them 
> out.

I'm fine with either way.

THanks
Dave