Currently, kernel-doc start parsing the comment as a kernel-doc comment if
it starts with '/**', but does not take into account if the content inside
the comment too, adheres with the expected format.
This results in unexpected and unclear warnings for the user.
E.g., running scripts/kernel-doc -none mm/memcontrol.c emits:
"mm/memcontrol.c:961: warning: expecting prototype for do not fallback to current(). Prototype was for get_mem_cgroup_from_current() instead"
Here kernel-doc parses the corresponding comment as a kernel-doc comment
and expects prototype for it in the next lines, and as a result causing
this warning.
Provide a clearer warning message to the users regarding the same, if the
content inside the comment does not follow the kernel-doc expected format.
Signed-off-by: Aditya Srivastava <[email protected]>
---
scripts/kernel-doc | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index cb92d0e1e932..b1d71a7b721f 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -2103,15 +2103,17 @@ sub process_name($$) {
}
} elsif (/$doc_decl/o) {
$identifier = $1;
- if (/\s*([\w\s]+?)(\(\))?\s*([-:].*)?$/) {
+ my $is_kernel_comment = 0;
+ if (/^\s*\*\s*([\w\s]+?)(\(\))?\s*([-:].*)?$/) {
$identifier = $1;
+ $decl_type = 'function';
+ $identifier =~ s/^define\s+//;
+ $is_kernel_comment = 1;
}
if ($identifier =~ m/^(struct|union|enum|typedef)\b\s*(\S*)/) {
$decl_type = $1;
$identifier = $2;
- } else {
- $decl_type = 'function';
- $identifier =~ s/^define\s+//;
+ $is_kernel_comment = 1;
}
$identifier =~ s/\s+$//;
@@ -2133,6 +2135,13 @@ sub process_name($$) {
$declaration_purpose = "";
}
+ if (!$is_kernel_comment) {
+ print STDERR "${file}:$.: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst\n";
+ print STDERR $_;
+ ++$warnings;
+ $state = STATE_NORMAL;
+ }
+
if (($declaration_purpose eq "") && $verbose) {
print STDERR "${file}:$.: warning: missing initial short description on line:\n";
print STDERR $_;
--
2.17.1
Hi Aditya,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on lwn/docs-next]
[also build test WARNING on linus/master v5.12-rc5 next-20210329]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Aditya-Srivastava/scripts-kernel-doc-add-warning-for-comment-not-following-kernel-doc-syntax/20210329-173213
base: git://git.lwn.net/linux-2.6 docs-next
config: i386-tinyconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/274a7b0af9aa2846ed0a5b2096e07fe76e47fe29
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Aditya-Srivastava/scripts-kernel-doc-add-warning-for-comment-not-following-kernel-doc-syntax/20210329-173213
git checkout 274a7b0af9aa2846ed0a5b2096e07fe76e47fe29
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> arch/x86/kernel/i8259.c:234: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* ELCR registers (0x4d0, 0x4d1) control edge/level of IRQ
--
>> arch/x86/lib/cmdline.c:17: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* Find a boolean option (like quiet,noapic,nosmp....)
--
arch/x86/lib/msr.c:40: warning: expecting prototype for Read an MSR with error handling(). Prototype was for msr_read() instead
arch/x86/lib/msr.c:58: warning: expecting prototype for Write an MSR with error handling(). Prototype was for msr_write() instead
>> arch/x86/lib/msr.c:91: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* Set @bit in a MSR @msr.
arch/x86/lib/msr.c:104: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* Clear @bit in a MSR @msr.
vim +234 arch/x86/kernel/i8259.c
4305df947ca1fd Thomas Gleixner 2010-09-28 231
21fd5132b223a1 Pavel Machek 2008-05-21 232 static char irq_trigger[2];
21fd5132b223a1 Pavel Machek 2008-05-21 233 /**
21fd5132b223a1 Pavel Machek 2008-05-21 @234 * ELCR registers (0x4d0, 0x4d1) control edge/level of IRQ
21fd5132b223a1 Pavel Machek 2008-05-21 235 */
21fd5132b223a1 Pavel Machek 2008-05-21 236 static void restore_ELCR(char *trigger)
21fd5132b223a1 Pavel Machek 2008-05-21 237 {
21fd5132b223a1 Pavel Machek 2008-05-21 238 outb(trigger[0], 0x4d0);
21fd5132b223a1 Pavel Machek 2008-05-21 239 outb(trigger[1], 0x4d1);
21fd5132b223a1 Pavel Machek 2008-05-21 240 }
21fd5132b223a1 Pavel Machek 2008-05-21 241
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Aditya Srivastava <[email protected]> writes:
> Currently, kernel-doc start parsing the comment as a kernel-doc comment if
> it starts with '/**', but does not take into account if the content inside
> the comment too, adheres with the expected format.
> This results in unexpected and unclear warnings for the user.
>
> E.g., running scripts/kernel-doc -none mm/memcontrol.c emits:
> "mm/memcontrol.c:961: warning: expecting prototype for do not fallback to current(). Prototype was for get_mem_cgroup_from_current() instead"
>
> Here kernel-doc parses the corresponding comment as a kernel-doc comment
> and expects prototype for it in the next lines, and as a result causing
> this warning.
>
> Provide a clearer warning message to the users regarding the same, if the
> content inside the comment does not follow the kernel-doc expected format.
>
> Signed-off-by: Aditya Srivastava <[email protected]>
> ---
> scripts/kernel-doc | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
This is definitely a capability we want, but I really don't think that
we can turn it on by default - for now. Experience shows that if you
create a blizzard of warnings, nobody sees any of them. How many
warnings does this add to a full docs build?
For now I think we need a flag to turn this warning on, which perhaps
can be set for a W=1 build.
Thanks,
jon
Hi Aditya,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on lwn/docs-next]
[also build test WARNING on linus/master v5.12-rc5 next-20210329]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Aditya-Srivastava/scripts-kernel-doc-add-warning-for-comment-not-following-kernel-doc-syntax/20210329-173213
base: git://git.lwn.net/linux-2.6 docs-next
config: csky-randconfig-r023-20210329 (attached as .config)
compiler: csky-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/274a7b0af9aa2846ed0a5b2096e07fe76e47fe29
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Aditya-Srivastava/scripts-kernel-doc-add-warning-for-comment-not-following-kernel-doc-syntax/20210329-173213
git checkout 274a7b0af9aa2846ed0a5b2096e07fe76e47fe29
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=csky
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/scsi/pm8001/pm8001_ctl.c:313: warning: expecting prototype for pm8001_ctl_sas_address_show(). Prototype was for pm8001_ctl_host_sas_address_show() instead
drivers/scsi/pm8001/pm8001_ctl.c:530: warning: expecting prototype for pm8001_ctl_aap_log_show(). Prototype was for pm8001_ctl_iop_log_show() instead
>> drivers/scsi/pm8001/pm8001_ctl.c:559: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
** pm8001_ctl_fatal_log_show - fatal error logging
drivers/scsi/pm8001/pm8001_ctl.c:579: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
** non_fatal_log_show - non fatal error logging
drivers/scsi/pm8001/pm8001_ctl.c:624: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
** pm8001_ctl_gsm_log_show - gsm dump collection
vim +559 drivers/scsi/pm8001/pm8001_ctl.c
dbf9bfe615717d jack wang 2009-10-14 557
d078b5117f18dc Anand Kumar Santhanam 2013-09-04 558 /**
d078b5117f18dc Anand Kumar Santhanam 2013-09-04 @559 ** pm8001_ctl_fatal_log_show - fatal error logging
d078b5117f18dc Anand Kumar Santhanam 2013-09-04 560 ** @cdev:pointer to embedded class device
e1c3e0f8a2ae15 Lee Jones 2020-07-13 561 ** @attr: device attribute
d078b5117f18dc Anand Kumar Santhanam 2013-09-04 562 ** @buf: the buffer returned
d078b5117f18dc Anand Kumar Santhanam 2013-09-04 563 **
d078b5117f18dc Anand Kumar Santhanam 2013-09-04 564 ** A sysfs 'read-only' shost attribute.
d078b5117f18dc Anand Kumar Santhanam 2013-09-04 565 **/
d078b5117f18dc Anand Kumar Santhanam 2013-09-04 566
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On 29/3/21 7:26 pm, Jonathan Corbet wrote:
> Aditya Srivastava <[email protected]> writes:
>
>> Currently, kernel-doc start parsing the comment as a kernel-doc comment if
>> it starts with '/**', but does not take into account if the content inside
>> the comment too, adheres with the expected format.
>> This results in unexpected and unclear warnings for the user.
>>
>> E.g., running scripts/kernel-doc -none mm/memcontrol.c emits:
>> "mm/memcontrol.c:961: warning: expecting prototype for do not fallback to current(). Prototype was for get_mem_cgroup_from_current() instead"
>>
>> Here kernel-doc parses the corresponding comment as a kernel-doc comment
>> and expects prototype for it in the next lines, and as a result causing
>> this warning.
>>
>> Provide a clearer warning message to the users regarding the same, if the
>> content inside the comment does not follow the kernel-doc expected format.
>>
>> Signed-off-by: Aditya Srivastava <[email protected]>
>> ---
>> scripts/kernel-doc | 17 +++++++++++++----
>> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> This is definitely a capability we want, but I really don't think that
> we can turn it on by default - for now. Experience shows that if you
> create a blizzard of warnings, nobody sees any of them. How many
> warnings does this add to a full docs build?
>
Hi Jonathan, here's the diff I have created for the warnings before
and after the changes:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/kernel-doc/kernel_doc_comment_syntax.txt
Around ~1320 new warnings of this type are added to the kernel tree,
and around ~1580 warnings are removed.
Thanks
Aditya
> For now I think we need a flag to turn this warning on, which perhaps
> can be set for a W=1 build.
> > Thanks,
>
> jon
>
Aditya Srivastava <[email protected]> writes:
> On 29/3/21 7:26 pm, Jonathan Corbet wrote:
>> Aditya Srivastava <[email protected]> writes:
>>
>>> Currently, kernel-doc start parsing the comment as a kernel-doc comment if
>>> it starts with '/**', but does not take into account if the content inside
>>> the comment too, adheres with the expected format.
>>> This results in unexpected and unclear warnings for the user.
>>>
>>> E.g., running scripts/kernel-doc -none mm/memcontrol.c emits:
>>> "mm/memcontrol.c:961: warning: expecting prototype for do not fallback to current(). Prototype was for get_mem_cgroup_from_current() instead"
>>>
>>> Here kernel-doc parses the corresponding comment as a kernel-doc comment
>>> and expects prototype for it in the next lines, and as a result causing
>>> this warning.
>>>
>>> Provide a clearer warning message to the users regarding the same, if the
>>> content inside the comment does not follow the kernel-doc expected format.
>>>
>>> Signed-off-by: Aditya Srivastava <[email protected]>
>>> ---
>>> scripts/kernel-doc | 17 +++++++++++++----
>>> 1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> This is definitely a capability we want, but I really don't think that
>> we can turn it on by default - for now. Experience shows that if you
>> create a blizzard of warnings, nobody sees any of them. How many
>> warnings does this add to a full docs build?
>>
>
> Hi Jonathan, here's the diff I have created for the warnings before
> and after the changes:
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/kernel-doc/kernel_doc_comment_syntax.txt
>
> Around ~1320 new warnings of this type are added to the kernel tree,
> and around ~1580 warnings are removed.
So I finally got around to looking at this again... How did you
generate that file?
I tried applying the patch and doing a normal full htmldocs build and
got all of four warnings:
./include/linux/seqlock.h:829: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* DEFINE_SEQLOCK(sl) - Define a statically allocated seqlock_t
./fs/jbd2/journal.c:1391: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* journal_t * jbd2_journal_init_dev() - creates and initialises a journal structure
./fs/jbd2/journal.c:1422: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* journal_t * jbd2_journal_init_inode () - creates a journal which maps to a inode.
./include/linux/dcache.h:309: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* dget, dget_dlock - get a reference to a dentry
Two observations:
- This is not an awful lot of warnings - not the blizzard I had
feared. At this level, I think we can just merge the patch and
then, hopefully, fix those cases.
- All of the warned-about places are *attempts* to write real kerneldoc
comments, they just got the syntax wrong in one way or another. It's
probably not worth the effort to try to detect this case - the
warning is enough to draw attention to the comment in question.
Thanks,
jon
On 1/4/21 1:02 am, Jonathan Corbet wrote:
> Aditya Srivastava <[email protected]> writes:
>
>> On 29/3/21 7:26 pm, Jonathan Corbet wrote:
>>> Aditya Srivastava <[email protected]> writes:
>>>
>>>> Currently, kernel-doc start parsing the comment as a kernel-doc comment if
>>>> it starts with '/**', but does not take into account if the content inside
>>>> the comment too, adheres with the expected format.
>>>> This results in unexpected and unclear warnings for the user.
>>>>
>>>> E.g., running scripts/kernel-doc -none mm/memcontrol.c emits:
>>>> "mm/memcontrol.c:961: warning: expecting prototype for do not fallback to current(). Prototype was for get_mem_cgroup_from_current() instead"
>>>>
>>>> Here kernel-doc parses the corresponding comment as a kernel-doc comment
>>>> and expects prototype for it in the next lines, and as a result causing
>>>> this warning.
>>>>
>>>> Provide a clearer warning message to the users regarding the same, if the
>>>> content inside the comment does not follow the kernel-doc expected format.
>>>>
>>>> Signed-off-by: Aditya Srivastava <[email protected]>
>>>> ---
>>>> scripts/kernel-doc | 17 +++++++++++++----
>>>> 1 file changed, 13 insertions(+), 4 deletions(-)
>>>
>>> This is definitely a capability we want, but I really don't think that
>>> we can turn it on by default - for now. Experience shows that if you
>>> create a blizzard of warnings, nobody sees any of them. How many
>>> warnings does this add to a full docs build?
>>>
>>
>> Hi Jonathan, here's the diff I have created for the warnings before
>> and after the changes:
>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/kernel-doc/kernel_doc_comment_syntax.txt
>>
>> Around ~1320 new warnings of this type are added to the kernel tree,
>> and around ~1580 warnings are removed.
>
> So I finally got around to looking at this again... How did you
> generate that file?
>
I ran scripts/kernel-doc -none on all the files in the kernel tree
before and after appying the changes, and then generated their diff to
find the warnings removed and added.
> I tried applying the patch and doing a normal full htmldocs build and
> got all of four warnings:
>
> ./include/linux/seqlock.h:829: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
> * DEFINE_SEQLOCK(sl) - Define a statically allocated seqlock_t
> ./fs/jbd2/journal.c:1391: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
> * journal_t * jbd2_journal_init_dev() - creates and initialises a journal structure
> ./fs/jbd2/journal.c:1422: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
> * journal_t * jbd2_journal_init_inode () - creates a journal which maps to a inode.
> ./include/linux/dcache.h:309: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
> * dget, dget_dlock - get a reference to a dentry
>
I think there should be more warnings. For e.g., running kernel-doc
-none ./drivers/usb/mtu3/mtu3.h gives these warnings:
./drivers/usb/mtu3/mtu3.h:75: warning: This comment starts with '/**',
but isn't a kernel-doc comment. Refer
Documentation/doc-guide/kernel-doc.rst
./drivers/usb/mtu3/mtu3.h:86: warning: This comment starts with '/**',
but isn't a kernel-doc comment. Refer
Documentation/doc-guide/kernel-doc.rst
./drivers/usb/mtu3/mtu3.h:143: warning: This comment starts with
'/**', but isn't a kernel-doc comment. Refer
Documentation/doc-guide/kernel-doc.rst
> Two observations:
>
> - This is not an awful lot of warnings - not the blizzard I had
> feared. At this level, I think we can just merge the patch and
> then, hopefully, fix those cases.
>
> - All of the warned-about places are *attempts* to write real kerneldoc
> comments, they just got the syntax wrong in one way or another. It's
> probably not worth the effort to try to detect this case - the
> warning is enough to draw attention to the comment in question.
>
I agree. Above are some of the cases which are not getting detected by
this patch.
This may be so as I am only allowing the function syntax as mentioned
in the rst file, i.e., "^\s*\*\s*([\w\s]+?)(\(\))?\s*([-:].*)?$" or
("* foo(\(\))? - description")
I probably need to check for pointers as well and other similar case(s).
Maybe I should design a separate check for functions than assigning
$decl_type = 'function' in the first check.
What do you think?
Thanks
Aditya