[Linux-kernel-mentees] [PATCH] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Linux-kernel-mentees] [PATCH] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Aditya Srivastava]

Currently, checkpatch warns us if we use an empty '/*' line for comment
and contents of the comment are in next line.

E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
the refs / unrefs from the transport") reports this warning:

WARNING: networking block comments don't use an empty /* line, use /* Comment...
+		/*
+		 * If the TXQ is active, then set the timer, if not,

Provide a fix by appending the current line contents to previous line
and deleting the current line

Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
 scripts/checkpatch.pl | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f61ac7456151..90e863d63097 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3629,8 +3629,14 @@ sub process {
 		    $prevrawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
 		    $rawline =~ /^\+[ \t]*\*/ &&
 		    $realline > 3) { # Do not warn about the initial copyright comment block after SPDX-License-Identifier
-			WARN("NETWORKING_BLOCK_COMMENT_STYLE",
-			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
+			if (WARN("NETWORKING_BLOCK_COMMENT_STYLE",
+				 "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev) &&\
+			    $fix) {
+				if ($rawline =~ /^\+[ \t]*\*\s*(.*)/) {
+					fix_delete_line($fixlinenr, $rawline);
+					$fixed[$fixlinenr - 1] .= " $1";
+				}
+			}
 		}
 
 # Block comments use * on subsequent lines
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Aditya]

On 18/11/20 10:39 pm, Aditya Srivastava wrote:
> Currently, checkpatch warns us if we use an empty '/*' line for comment
> and contents of the comment are in next line.
> 
> E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
> the refs / unrefs from the transport") reports this warning:
> 
> WARNING: networking block comments don't use an empty /* line, use /* Comment...
> +		/*
> +		 * If the TXQ is active, then set the timer, if not,
> 
> Provide a fix by appending the current line contents to previous line
> and deleting the current line
> 
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
>  scripts/checkpatch.pl | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f61ac7456151..90e863d63097 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3629,8 +3629,14 @@ sub process {
>  		    $prevrawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
>  		    $rawline =~ /^\+[ \t]*\*/ &&
>  		    $realline > 3) { # Do not warn about the initial copyright comment block after SPDX-License-Identifier
> -			WARN("NETWORKING_BLOCK_COMMENT_STYLE",
> -			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
> +			if (WARN("NETWORKING_BLOCK_COMMENT_STYLE",
> +				 "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev) &&\
> +			    $fix) {
> +				if ($rawline =~ /^\+[ \t]*\*\s*(.*)/) {
> +					fix_delete_line($fixlinenr, $rawline);
> +					$fixed[$fixlinenr - 1] .= " $1";
> +				}
> +			}
>  		}
>  
>  # Block comments use * on subsequent lines
> 

I have generated the warnings occurring for this class of warnings
(over 4.13..5.8):
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/warnings.txt

My initial tests at these warnings found the fix to be working fine.


Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Aditya Srivastava]

Currently, checkpatch warns us for files in 'net/' and 'drivers/net',
if we use an empty '/*' line for comment and contents of comments are
in next line

E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
the refs / unrefs from the transport") reports this warning:

WARNING: networking block comments don't use an empty /* line, use /* Comment...
+		/*
+		 * If the TXQ is active, then set the timer, if not,

Provide a fix by appending the current line contents to previous line
and removing the current line

Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
Changes in v2: modify commit message: add information about networking files in commit message

 scripts/checkpatch.pl | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f61ac7456151..90e863d63097 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3629,8 +3629,14 @@ sub process {
 		    $prevrawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
 		    $rawline =~ /^\+[ \t]*\*/ &&
 		    $realline > 3) { # Do not warn about the initial copyright comment block after SPDX-License-Identifier
-			WARN("NETWORKING_BLOCK_COMMENT_STYLE",
-			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
+			if (WARN("NETWORKING_BLOCK_COMMENT_STYLE",
+				 "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev) &&\
+			    $fix) {
+				if ($rawline =~ /^\+[ \t]*\*\s*(.*)/) {
+					fix_delete_line($fixlinenr, $rawline);
+					$fixed[$fixlinenr - 1] .= " $1";
+				}
+			}
 		}
 
 # Block comments use * on subsequent lines
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Lukas Bulwahn]

[-- Attachment #1.1: Type: text/plain, Size: 2317 bytes --]

On Mi., 18. Nov. 2020 at 18:40, Aditya Srivastava <yashsri421@gmail.com>
wrote:

> Currently, checkpatch warns us for files in 'net/' and 'drivers/net',
> if we use an empty '/*' line for comment and contents of comments are
> in next line
>
> E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
> the refs / unrefs from the transport") reports this warning:
>
> WARNING: networking block comments don't use an empty /* line, use /*
> Comment...
> +               /*
> +                * If the TXQ is active, then set the timer, if not,
>
> Provide a fix by appending the current line contents to previous line
> and removing the current line
>

Patch generally looks good.

Can you check how many comments in net actually follow that style and how
many follow another style?


Lukas


> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
> Changes in v2: modify commit message: add information about networking
> files in commit message
>
>  scripts/checkpatch.pl | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f61ac7456151..90e863d63097 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3629,8 +3629,14 @@ sub process {
>                     $prevrawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
>                     $rawline =~ /^\+[ \t]*\*/ &&
>                     $realline > 3) { # Do not warn about the initial
> copyright comment block after SPDX-License-Identifier
> -                       WARN("NETWORKING_BLOCK_COMMENT_STYLE",
> -                            "networking block comments don't use an empty
> /* line, use /* Comment...\n" . $hereprev);
> +                       if (WARN("NETWORKING_BLOCK_COMMENT_STYLE",
> +                                "networking block comments don't use an
> empty /* line, use /* Comment...\n" . $hereprev) &&\
> +                           $fix) {
> +                               if ($rawline =~ /^\+[ \t]*\*\s*(.*)/) {
> +                                       fix_delete_line($fixlinenr,
> $rawline);
> +                                       $fixed[$fixlinenr - 1] .= " $1";
> +                               }
> +                       }
>                 }
>
>  # Block comments use * on subsequent lines
> --
> 2.17.1
>
>

[-- Attachment #1.2: Type: text/html, Size: 3735 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Aditya]

On 19/11/20 12:30 am, Lukas Bulwahn wrote:
> On Mi., 18. Nov. 2020 at 18:40, Aditya Srivastava <yashsri421@gmail.com>
> wrote:
> 
>> Currently, checkpatch warns us for files in 'net/' and 'drivers/net',
>> if we use an empty '/*' line for comment and contents of comments are
>> in next line
>>
>> E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
>> the refs / unrefs from the transport") reports this warning:
>>
>> WARNING: networking block comments don't use an empty /* line, use /*
>> Comment...
>> +               /*
>> +                * If the TXQ is active, then set the timer, if not,
>>
>> Provide a fix by appending the current line contents to previous line
>> and removing the current line
>>
> 
> Patch generally looks good.
> 
> Can you check how many comments in net actually follow that style and how
> many follow another style?
> 
> 

In drivers/net:
Wrong style: 14695 lines
Correct style: 147961 lines (ie around 10 times)

In net/:
Wrong style: 5090 lines
Correct style: 30485 lines

There is also a documentation regarding this different comment format,
and can be found here:
https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Aditya]

On 19/11/20 4:14 pm, Aditya wrote:
> On 19/11/20 12:30 am, Lukas Bulwahn wrote:
>> On Mi., 18. Nov. 2020 at 18:40, Aditya Srivastava <yashsri421@gmail.com>
>> wrote:
>>
>>> Currently, checkpatch warns us for files in 'net/' and 'drivers/net',
>>> if we use an empty '/*' line for comment and contents of comments are
>>> in next line
>>>
>>> E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
>>> the refs / unrefs from the transport") reports this warning:
>>>
>>> WARNING: networking block comments don't use an empty /* line, use /*
>>> Comment...
>>> +               /*
>>> +                * If the TXQ is active, then set the timer, if not,
>>>
>>> Provide a fix by appending the current line contents to previous line
>>> and removing the current line
>>>
>>
>> Patch generally looks good.
>>
>> Can you check how many comments in net actually follow that style and how
>> many follow another style?
>>
>>
> 
> In drivers/net:
> Wrong style: 14695 lines
> Correct style: 147961 lines (ie around 10 times)
> 
> In net/:
> Wrong style: 5090 lines
> Correct style: 30485 lines
> 
> There is also a documentation regarding this different comment format,
> and can be found here:
> https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
> 
> Thanks
> Aditya
> 

Hi Lukas
This mail probably got missed. Please review this as well :)

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Lukas Bulwahn]

On Thu, Nov 19, 2020 at 11:44 AM Aditya <yashsri421@gmail.com> wrote:
>
> On 19/11/20 12:30 am, Lukas Bulwahn wrote:
> > On Mi., 18. Nov. 2020 at 18:40, Aditya Srivastava <yashsri421@gmail.com>
> > wrote:
> >
> >> Currently, checkpatch warns us for files in 'net/' and 'drivers/net',
> >> if we use an empty '/*' line for comment and contents of comments are
> >> in next line
> >>
> >> E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
> >> the refs / unrefs from the transport") reports this warning:
> >>
> >> WARNING: networking block comments don't use an empty /* line, use /*
> >> Comment...
> >> +               /*
> >> +                * If the TXQ is active, then set the timer, if not,
> >>
> >> Provide a fix by appending the current line contents to previous line
> >> and removing the current line
> >>
> >
> > Patch generally looks good.
> >
> > Can you check how many comments in net actually follow that style and how
> > many follow another style?
> >
> >
>
> In drivers/net:
> Wrong style: 14695 lines
> Correct style: 147961 lines (ie around 10 times)
>
> In net/:
> Wrong style: 5090 lines
> Correct style: 30485 lines
>

Can you find out where the wrong style is used?

Maybe the documentation is a bit outdated.

For example, some drivers and some subdirectories might have settled
for another commenting style.

I guess you can submit the fix option, but it could be that the whole
rule/feature is broken anyway... so I do not know if that fix is of
any good...

I think it is better to actually understand, document and encode the
current rule that applies.

Can you provide an evaluation where the different styles for
commenting are aggregated in a good way?
E.g., consistently within a file with style XYZ; mixed style but
80-90% are of style ABC; consistent within a directory.

If you think some cases are in the wrong style for some specific
files, simply send a patch correcting the commenting style and see.

Lukas

> There is also a documentation regarding this different comment format,
> and can be found here:
> https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
>
> Thanks
> Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Aditya]

On 22/11/20 12:22 pm, Lukas Bulwahn wrote:
> On Thu, Nov 19, 2020 at 11:44 AM Aditya <yashsri421@gmail.com> wrote:
>>
>> On 19/11/20 12:30 am, Lukas Bulwahn wrote:
>>> On Mi., 18. Nov. 2020 at 18:40, Aditya Srivastava <yashsri421@gmail.com>
>>> wrote:
>>>
>>>> Currently, checkpatch warns us for files in 'net/' and 'drivers/net',
>>>> if we use an empty '/*' line for comment and contents of comments are
>>>> in next line
>>>>
>>>> E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
>>>> the refs / unrefs from the transport") reports this warning:
>>>>
>>>> WARNING: networking block comments don't use an empty /* line, use /*
>>>> Comment...
>>>> +               /*
>>>> +                * If the TXQ is active, then set the timer, if not,
>>>>
>>>> Provide a fix by appending the current line contents to previous line
>>>> and removing the current line
>>>>
>>>
>>> Patch generally looks good.
>>>
>>> Can you check how many comments in net actually follow that style and how
>>> many follow another style?
>>>
>>>
>>
>> In drivers/net:
>> Wrong style: 14695 lines
>> Correct style: 147961 lines (ie around 10 times)
>>
>> In net/:
>> Wrong style: 5090 lines
>> Correct style: 30485 lines
>>
> 
> Can you find out where the wrong style is used?
> 
> Maybe the documentation is a bit outdated.
> 
> For example, some drivers and some subdirectories might have settled
> for another commenting style.
> 
> I guess you can submit the fix option, but it could be that the whole
> rule/feature is broken anyway... so I do not know if that fix is of
> any good...
> 
> I think it is better to actually understand, document and encode the
> current rule that applies.
> 
> Can you provide an evaluation where the different styles for
> commenting are aggregated in a good way?
> E.g., consistently within a file with style XYZ; mixed style but
> 80-90% are of style ABC; consistent within a directory.
> 
> If you think some cases are in the wrong style for some specific
> files, simply send a patch correcting the commenting style and see.
>
Hi Lukas
I have generated the reports regarding the comments used in various
files at net/ and drivers/net.
Directory wise reports:

net/:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative.txt

drivers/net:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative.txt

File wise reports:

net/:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative_file.txt

drivers/net:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative_file.txt

Some files in 'net/' follow the accepted syntax, such as:
'net/batman-adv => 985    0'.

However, some completely not, such as:
'net/ax25 => 0    103'.

Some even follow mixed syntax such as:
'net/netfilter => 87    125'.

Similarly for drivers/net:
drivers/net/ethernet/mellanox/mlxsw => 1071    0
completely follow accepted syntax.

drivers/net/wireless/ralink/rt2x00 => 98, 2082
follow unaccepted syntax largely.

whereas files such as drivers/net/wireless/ath/ath5k => 304, 486
follow mixed syntax.

It seems to me as the documentation might be outdated, as you had also
suggested earlier, or maybe users are unaware of the documentation and
 just use the syntax for conserving consistency of the code.

What do you think?

Thanks
Aditya

> Lukas
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Lukas Bulwahn]

On Wed, Nov 25, 2020 at 8:02 AM Aditya <yashsri421@gmail.com> wrote:
>
> On 22/11/20 12:22 pm, Lukas Bulwahn wrote:
> > On Thu, Nov 19, 2020 at 11:44 AM Aditya <yashsri421@gmail.com> wrote:
> >>
> >> On 19/11/20 12:30 am, Lukas Bulwahn wrote:
> >>> On Mi., 18. Nov. 2020 at 18:40, Aditya Srivastava <yashsri421@gmail.com>
> >>> wrote:
> >>>
> >>>> Currently, checkpatch warns us for files in 'net/' and 'drivers/net',
> >>>> if we use an empty '/*' line for comment and contents of comments are
> >>>> in next line
> >>>>
> >>>> E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
> >>>> the refs / unrefs from the transport") reports this warning:
> >>>>
> >>>> WARNING: networking block comments don't use an empty /* line, use /*
> >>>> Comment...
> >>>> +               /*
> >>>> +                * If the TXQ is active, then set the timer, if not,
> >>>>
> >>>> Provide a fix by appending the current line contents to previous line
> >>>> and removing the current line
> >>>>
> >>>
> >>> Patch generally looks good.
> >>>
> >>> Can you check how many comments in net actually follow that style and how
> >>> many follow another style?
> >>>
> >>>
> >>
> >> In drivers/net:
> >> Wrong style: 14695 lines
> >> Correct style: 147961 lines (ie around 10 times)
> >>
> >> In net/:
> >> Wrong style: 5090 lines
> >> Correct style: 30485 lines
> >>
> >
> > Can you find out where the wrong style is used?
> >
> > Maybe the documentation is a bit outdated.
> >
> > For example, some drivers and some subdirectories might have settled
> > for another commenting style.
> >
> > I guess you can submit the fix option, but it could be that the whole
> > rule/feature is broken anyway... so I do not know if that fix is of
> > any good...
> >
> > I think it is better to actually understand, document and encode the
> > current rule that applies.
> >
> > Can you provide an evaluation where the different styles for
> > commenting are aggregated in a good way?
> > E.g., consistently within a file with style XYZ; mixed style but
> > 80-90% are of style ABC; consistent within a directory.
> >
> > If you think some cases are in the wrong style for some specific
> > files, simply send a patch correcting the commenting style and see.
> >
> Hi Lukas
> I have generated the reports regarding the comments used in various
> files at net/ and drivers/net.
> Directory wise reports:
>
> net/:
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative.txt
>
> drivers/net:
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative.txt
>

Can you sort that by the overall number of comments per directory?

> File wise reports:
>
> net/:
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative_file.txt
>
> drivers/net:
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative_file.txt
>
> Some files in 'net/' follow the accepted syntax, such as:
> 'net/batman-adv => 985    0'.
>
> However, some completely not, such as:
> 'net/ax25 => 0    103'.
>
> Some even follow mixed syntax such as:
> 'net/netfilter => 87    125'.
>
> Similarly for drivers/net:
> drivers/net/ethernet/mellanox/mlxsw => 1071    0
> completely follow accepted syntax.
>
> drivers/net/wireless/ralink/rt2x00 => 98, 2082
> follow unaccepted syntax largely.
>
> whereas files such as drivers/net/wireless/ath/ath5k => 304, 486
> follow mixed syntax.
>
> It seems to me as the documentation might be outdated, as you had also
> suggested earlier, or maybe users are unaware of the documentation and
>  just use the syntax for conserving consistency of the code.
>
> What do you think?
>

I think it largely depends on the history of the code and developers
that take care...

We might just start to ask the larger areas to modify the rule for
their directory.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Lukas Bulwahn]

On Wed, Nov 25, 2020 at 8:19 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>
> On Wed, Nov 25, 2020 at 8:02 AM Aditya <yashsri421@gmail.com> wrote:
> >
> > On 22/11/20 12:22 pm, Lukas Bulwahn wrote:
> > > On Thu, Nov 19, 2020 at 11:44 AM Aditya <yashsri421@gmail.com> wrote:
> > >>
> > >> On 19/11/20 12:30 am, Lukas Bulwahn wrote:
> > >>> On Mi., 18. Nov. 2020 at 18:40, Aditya Srivastava <yashsri421@gmail.com>
> > >>> wrote:
> > >>>
> > >>>> Currently, checkpatch warns us for files in 'net/' and 'drivers/net',
> > >>>> if we use an empty '/*' line for comment and contents of comments are
> > >>>> in next line
> > >>>>
> > >>>> E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
> > >>>> the refs / unrefs from the transport") reports this warning:
> > >>>>
> > >>>> WARNING: networking block comments don't use an empty /* line, use /*
> > >>>> Comment...
> > >>>> +               /*
> > >>>> +                * If the TXQ is active, then set the timer, if not,
> > >>>>
> > >>>> Provide a fix by appending the current line contents to previous line
> > >>>> and removing the current line
> > >>>>
> > >>>
> > >>> Patch generally looks good.
> > >>>
> > >>> Can you check how many comments in net actually follow that style and how
> > >>> many follow another style?
> > >>>
> > >>>
> > >>
> > >> In drivers/net:
> > >> Wrong style: 14695 lines
> > >> Correct style: 147961 lines (ie around 10 times)
> > >>
> > >> In net/:
> > >> Wrong style: 5090 lines
> > >> Correct style: 30485 lines
> > >>
> > >
> > > Can you find out where the wrong style is used?
> > >
> > > Maybe the documentation is a bit outdated.
> > >
> > > For example, some drivers and some subdirectories might have settled
> > > for another commenting style.
> > >
> > > I guess you can submit the fix option, but it could be that the whole
> > > rule/feature is broken anyway... so I do not know if that fix is of
> > > any good...
> > >
> > > I think it is better to actually understand, document and encode the
> > > current rule that applies.
> > >
> > > Can you provide an evaluation where the different styles for
> > > commenting are aggregated in a good way?
> > > E.g., consistently within a file with style XYZ; mixed style but
> > > 80-90% are of style ABC; consistent within a directory.
> > >
> > > If you think some cases are in the wrong style for some specific
> > > files, simply send a patch correcting the commenting style and see.
> > >
> > Hi Lukas
> > I have generated the reports regarding the comments used in various
> > files at net/ and drivers/net.
> > Directory wise reports:
> >
> > net/:
> > https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative.txt
> >
> > drivers/net:
> > https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative.txt
> >
>
> Can you sort that by the overall number of comments per directory?
>
> > File wise reports:
> >
> > net/:
> > https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative_file.txt
> >
> > drivers/net:
> > https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative_file.txt
> >
> > Some files in 'net/' follow the accepted syntax, such as:
> > 'net/batman-adv => 985    0'.
> >
> > However, some completely not, such as:
> > 'net/ax25 => 0    103'.
> >
> > Some even follow mixed syntax such as:
> > 'net/netfilter => 87    125'.
> >
> > Similarly for drivers/net:
> > drivers/net/ethernet/mellanox/mlxsw => 1071    0
> > completely follow accepted syntax.
> >
> > drivers/net/wireless/ralink/rt2x00 => 98, 2082
> > follow unaccepted syntax largely.
> >
> > whereas files such as drivers/net/wireless/ath/ath5k => 304, 486
> > follow mixed syntax.
> >
> > It seems to me as the documentation might be outdated, as you had also
> > suggested earlier, or maybe users are unaware of the documentation and
> >  just use the syntax for conserving consistency of the code.
> >
> > What do you think?
> >
>
> I think it largely depends on the history of the code and developers
> that take care...
>
> We might just start to ask the larger areas to modify the rule for
> their directory.
>

I suggest we start with looking at the (let us say 10) "largest"
directories with respect to number of comment blocks and then decide
on a refinement of the rule for those subdirectories:

A. Directory follows NETWORKING COMMENT STYLE; checkpatch shall warn
to follow NETWORKING COMMENT STYLE.
B. Directory follows GENERAL KERNEL COMMENT STYLE; checkpatch shall
warn to follow GENERAL KERNEL COMMENT STYLE.
C. Directory is too mixed; checkpatch shall not warn on any comment style.

We can then start the discussion with the appropriate maintainer (and
check what else this maintainer might maintain and if that is
consistent) if they agree or disagree on that.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Aditya]

On 25/11/20 6:08 pm, Lukas Bulwahn wrote:
> On Wed, Nov 25, 2020 at 8:19 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>>
>> On Wed, Nov 25, 2020 at 8:02 AM Aditya <yashsri421@gmail.com> wrote:
>>>
>>> On 22/11/20 12:22 pm, Lukas Bulwahn wrote:
>>>> On Thu, Nov 19, 2020 at 11:44 AM Aditya <yashsri421@gmail.com> wrote:
>>>>>
>>>>> On 19/11/20 12:30 am, Lukas Bulwahn wrote:
>>>>>> On Mi., 18. Nov. 2020 at 18:40, Aditya Srivastava <yashsri421@gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Currently, checkpatch warns us for files in 'net/' and 'drivers/net',
>>>>>>> if we use an empty '/*' line for comment and contents of comments are
>>>>>>> in next line
>>>>>>>
>>>>>>> E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
>>>>>>> the refs / unrefs from the transport") reports this warning:
>>>>>>>
>>>>>>> WARNING: networking block comments don't use an empty /* line, use /*
>>>>>>> Comment...
>>>>>>> +               /*
>>>>>>> +                * If the TXQ is active, then set the timer, if not,
>>>>>>>
>>>>>>> Provide a fix by appending the current line contents to previous line
>>>>>>> and removing the current line
>>>>>>>
>>>>>>
>>>>>> Patch generally looks good.
>>>>>>
>>>>>> Can you check how many comments in net actually follow that style and how
>>>>>> many follow another style?
>>>>>>
>>>>>>
>>>>>
>>>>> In drivers/net:
>>>>> Wrong style: 14695 lines
>>>>> Correct style: 147961 lines (ie around 10 times)
>>>>>
>>>>> In net/:
>>>>> Wrong style: 5090 lines
>>>>> Correct style: 30485 lines
>>>>>
>>>>
>>>> Can you find out where the wrong style is used?
>>>>
>>>> Maybe the documentation is a bit outdated.
>>>>
>>>> For example, some drivers and some subdirectories might have settled
>>>> for another commenting style.
>>>>
>>>> I guess you can submit the fix option, but it could be that the whole
>>>> rule/feature is broken anyway... so I do not know if that fix is of
>>>> any good...
>>>>
>>>> I think it is better to actually understand, document and encode the
>>>> current rule that applies.
>>>>
>>>> Can you provide an evaluation where the different styles for
>>>> commenting are aggregated in a good way?
>>>> E.g., consistently within a file with style XYZ; mixed style but
>>>> 80-90% are of style ABC; consistent within a directory.
>>>>
>>>> If you think some cases are in the wrong style for some specific
>>>> files, simply send a patch correcting the commenting style and see.
>>>>
>>> Hi Lukas
>>> I have generated the reports regarding the comments used in various
>>> files at net/ and drivers/net.
>>> Directory wise reports:
>>>
>>> net/:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative.txt
>>>
>>> drivers/net:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative.txt
>>>
>>
>> Can you sort that by the overall number of comments per directory?
>>
>>> File wise reports:
>>>
>>> net/:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative_file.txt
>>>
>>> drivers/net:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative_file.txt
>>>
>>> Some files in 'net/' follow the accepted syntax, such as:
>>> 'net/batman-adv => 985    0'.
>>>
>>> However, some completely not, such as:
>>> 'net/ax25 => 0    103'.
>>>
>>> Some even follow mixed syntax such as:
>>> 'net/netfilter => 87    125'.
>>>
>>> Similarly for drivers/net:
>>> drivers/net/ethernet/mellanox/mlxsw => 1071    0
>>> completely follow accepted syntax.
>>>
>>> drivers/net/wireless/ralink/rt2x00 => 98, 2082
>>> follow unaccepted syntax largely.
>>>
>>> whereas files such as drivers/net/wireless/ath/ath5k => 304, 486
>>> follow mixed syntax.
>>>
>>> It seems to me as the documentation might be outdated, as you had also
>>> suggested earlier, or maybe users are unaware of the documentation and
>>>  just use the syntax for conserving consistency of the code.
>>>
>>> What do you think?
>>>
>>
>> I think it largely depends on the history of the code and developers
>> that take care...
>>
>> We might just start to ask the larger areas to modify the rule for
>> their directory.
>>
> 
> I suggest we start with looking at the (let us say 10) "largest"
> directories with respect to number of comment blocks and then decide
> on a refinement of the rule for those subdirectories:
> 
> A. Directory follows NETWORKING COMMENT STYLE; checkpatch shall warn
> to follow NETWORKING COMMENT STYLE.
> B. Directory follows GENERAL KERNEL COMMENT STYLE; checkpatch shall
> warn to follow GENERAL KERNEL COMMENT STYLE.
> C. Directory is too mixed; checkpatch shall not warn on any comment style.
> 
> We can then start the discussion with the appropriate maintainer (and
> check what else this maintainer might maintain and if that is
> consistent) if they agree or disagree on that.
> 

Hi Lukas
Here is the list I have generated with the directories sorted on the
basis of comment count and labelled the top 10 directories as
following NETWORKING, GENERIC or MIXED comment style(s):

At "drivers/net":
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative_sorted.txt

At "net":
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative_sorted.txt

What do you think?

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Lukas Bulwahn]

On Sun, Nov 29, 2020 at 3:58 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 25/11/20 6:08 pm, Lukas Bulwahn wrote:
> > On Wed, Nov 25, 2020 at 8:19 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> >>
> >> On Wed, Nov 25, 2020 at 8:02 AM Aditya <yashsri421@gmail.com> wrote:
> >>>
> >>> On 22/11/20 12:22 pm, Lukas Bulwahn wrote:
> >>>> On Thu, Nov 19, 2020 at 11:44 AM Aditya <yashsri421@gmail.com> wrote:
> >>>>>
> >>>>> On 19/11/20 12:30 am, Lukas Bulwahn wrote:
> >>>>>> On Mi., 18. Nov. 2020 at 18:40, Aditya Srivastava <yashsri421@gmail.com>
> >>>>>> wrote:
> >>>>>>
> >>>>>>> Currently, checkpatch warns us for files in 'net/' and 'drivers/net',
> >>>>>>> if we use an empty '/*' line for comment and contents of comments are
> >>>>>>> in next line
> >>>>>>>
> >>>>>>> E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
> >>>>>>> the refs / unrefs from the transport") reports this warning:
> >>>>>>>
> >>>>>>> WARNING: networking block comments don't use an empty /* line, use /*
> >>>>>>> Comment...
> >>>>>>> +               /*
> >>>>>>> +                * If the TXQ is active, then set the timer, if not,
> >>>>>>>
> >>>>>>> Provide a fix by appending the current line contents to previous line
> >>>>>>> and removing the current line
> >>>>>>>
> >>>>>>
> >>>>>> Patch generally looks good.
> >>>>>>
> >>>>>> Can you check how many comments in net actually follow that style and how
> >>>>>> many follow another style?
> >>>>>>
> >>>>>>
> >>>>>
> >>>>> In drivers/net:
> >>>>> Wrong style: 14695 lines
> >>>>> Correct style: 147961 lines (ie around 10 times)
> >>>>>
> >>>>> In net/:
> >>>>> Wrong style: 5090 lines
> >>>>> Correct style: 30485 lines
> >>>>>
> >>>>
> >>>> Can you find out where the wrong style is used?
> >>>>
> >>>> Maybe the documentation is a bit outdated.
> >>>>
> >>>> For example, some drivers and some subdirectories might have settled
> >>>> for another commenting style.
> >>>>
> >>>> I guess you can submit the fix option, but it could be that the whole
> >>>> rule/feature is broken anyway... so I do not know if that fix is of
> >>>> any good...
> >>>>
> >>>> I think it is better to actually understand, document and encode the
> >>>> current rule that applies.
> >>>>
> >>>> Can you provide an evaluation where the different styles for
> >>>> commenting are aggregated in a good way?
> >>>> E.g., consistently within a file with style XYZ; mixed style but
> >>>> 80-90% are of style ABC; consistent within a directory.
> >>>>
> >>>> If you think some cases are in the wrong style for some specific
> >>>> files, simply send a patch correcting the commenting style and see.
> >>>>
> >>> Hi Lukas
> >>> I have generated the reports regarding the comments used in various
> >>> files at net/ and drivers/net.
> >>> Directory wise reports:
> >>>
> >>> net/:
> >>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative.txt
> >>>
> >>> drivers/net:
> >>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative.txt
> >>>
> >>
> >> Can you sort that by the overall number of comments per directory?
> >>
> >>> File wise reports:
> >>>
> >>> net/:
> >>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative_file.txt
> >>>
> >>> drivers/net:
> >>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative_file.txt
> >>>
> >>> Some files in 'net/' follow the accepted syntax, such as:
> >>> 'net/batman-adv => 985    0'.
> >>>
> >>> However, some completely not, such as:
> >>> 'net/ax25 => 0    103'.
> >>>
> >>> Some even follow mixed syntax such as:
> >>> 'net/netfilter => 87    125'.
> >>>
> >>> Similarly for drivers/net:
> >>> drivers/net/ethernet/mellanox/mlxsw => 1071    0
> >>> completely follow accepted syntax.
> >>>
> >>> drivers/net/wireless/ralink/rt2x00 => 98, 2082
> >>> follow unaccepted syntax largely.
> >>>
> >>> whereas files such as drivers/net/wireless/ath/ath5k => 304, 486
> >>> follow mixed syntax.
> >>>
> >>> It seems to me as the documentation might be outdated, as you had also
> >>> suggested earlier, or maybe users are unaware of the documentation and
> >>>  just use the syntax for conserving consistency of the code.
> >>>
> >>> What do you think?
> >>>
> >>
> >> I think it largely depends on the history of the code and developers
> >> that take care...
> >>
> >> We might just start to ask the larger areas to modify the rule for
> >> their directory.
> >>
> >
> > I suggest we start with looking at the (let us say 10) "largest"
> > directories with respect to number of comment blocks and then decide
> > on a refinement of the rule for those subdirectories:
> >
> > A. Directory follows NETWORKING COMMENT STYLE; checkpatch shall warn
> > to follow NETWORKING COMMENT STYLE.
> > B. Directory follows GENERAL KERNEL COMMENT STYLE; checkpatch shall
> > warn to follow GENERAL KERNEL COMMENT STYLE.
> > C. Directory is too mixed; checkpatch shall not warn on any comment style.
> >
> > We can then start the discussion with the appropriate maintainer (and
> > check what else this maintainer might maintain and if that is
> > consistent) if they agree or disagree on that.
> >
>
> Hi Lukas
> Here is the list I have generated with the directories sorted on the
> basis of comment count and labelled the top 10 directories as
> following NETWORKING, GENERIC or MIXED comment style(s):
>
> At "drivers/net":
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative_sorted.txt
>
> At "net":
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative_sorted.txt
>
> What do you think?
>

Obviously, this existing rule needs to be refined.

Do you have a proposal? Probably, you would like to have each
maintainer state which comment style is preferred or so.

How could this be technically done?

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Aditya]

On 30/11/20 3:40 pm, Lukas Bulwahn wrote:
> On Sun, Nov 29, 2020 at 3:58 PM Aditya <yashsri421@gmail.com> wrote:
>>
>> On 25/11/20 6:08 pm, Lukas Bulwahn wrote:
>>> On Wed, Nov 25, 2020 at 8:19 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>>>>
>>>> On Wed, Nov 25, 2020 at 8:02 AM Aditya <yashsri421@gmail.com> wrote:
>>>>>
>>>>> On 22/11/20 12:22 pm, Lukas Bulwahn wrote:
>>>>>> On Thu, Nov 19, 2020 at 11:44 AM Aditya <yashsri421@gmail.com> wrote:
>>>>>>>
>>>>>>> On 19/11/20 12:30 am, Lukas Bulwahn wrote:
>>>>>>>> On Mi., 18. Nov. 2020 at 18:40, Aditya Srivastava <yashsri421@gmail.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Currently, checkpatch warns us for files in 'net/' and 'drivers/net',
>>>>>>>>> if we use an empty '/*' line for comment and contents of comments are
>>>>>>>>> in next line
>>>>>>>>>
>>>>>>>>> E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
>>>>>>>>> the refs / unrefs from the transport") reports this warning:
>>>>>>>>>
>>>>>>>>> WARNING: networking block comments don't use an empty /* line, use /*
>>>>>>>>> Comment...
>>>>>>>>> +               /*
>>>>>>>>> +                * If the TXQ is active, then set the timer, if not,
>>>>>>>>>
>>>>>>>>> Provide a fix by appending the current line contents to previous line
>>>>>>>>> and removing the current line
>>>>>>>>>
>>>>>>>>
>>>>>>>> Patch generally looks good.
>>>>>>>>
>>>>>>>> Can you check how many comments in net actually follow that style and how
>>>>>>>> many follow another style?
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> In drivers/net:
>>>>>>> Wrong style: 14695 lines
>>>>>>> Correct style: 147961 lines (ie around 10 times)
>>>>>>>
>>>>>>> In net/:
>>>>>>> Wrong style: 5090 lines
>>>>>>> Correct style: 30485 lines
>>>>>>>
>>>>>>
>>>>>> Can you find out where the wrong style is used?
>>>>>>
>>>>>> Maybe the documentation is a bit outdated.
>>>>>>
>>>>>> For example, some drivers and some subdirectories might have settled
>>>>>> for another commenting style.
>>>>>>
>>>>>> I guess you can submit the fix option, but it could be that the whole
>>>>>> rule/feature is broken anyway... so I do not know if that fix is of
>>>>>> any good...
>>>>>>
>>>>>> I think it is better to actually understand, document and encode the
>>>>>> current rule that applies.
>>>>>>
>>>>>> Can you provide an evaluation where the different styles for
>>>>>> commenting are aggregated in a good way?
>>>>>> E.g., consistently within a file with style XYZ; mixed style but
>>>>>> 80-90% are of style ABC; consistent within a directory.
>>>>>>
>>>>>> If you think some cases are in the wrong style for some specific
>>>>>> files, simply send a patch correcting the commenting style and see.
>>>>>>
>>>>> Hi Lukas
>>>>> I have generated the reports regarding the comments used in various
>>>>> files at net/ and drivers/net.
>>>>> Directory wise reports:
>>>>>
>>>>> net/:
>>>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative.txt
>>>>>
>>>>> drivers/net:
>>>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative.txt
>>>>>
>>>>
>>>> Can you sort that by the overall number of comments per directory?
>>>>
>>>>> File wise reports:
>>>>>
>>>>> net/:
>>>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative_file.txt
>>>>>
>>>>> drivers/net:
>>>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative_file.txt
>>>>>
>>>>> Some files in 'net/' follow the accepted syntax, such as:
>>>>> 'net/batman-adv => 985    0'.
>>>>>
>>>>> However, some completely not, such as:
>>>>> 'net/ax25 => 0    103'.
>>>>>
>>>>> Some even follow mixed syntax such as:
>>>>> 'net/netfilter => 87    125'.
>>>>>
>>>>> Similarly for drivers/net:
>>>>> drivers/net/ethernet/mellanox/mlxsw => 1071    0
>>>>> completely follow accepted syntax.
>>>>>
>>>>> drivers/net/wireless/ralink/rt2x00 => 98, 2082
>>>>> follow unaccepted syntax largely.
>>>>>
>>>>> whereas files such as drivers/net/wireless/ath/ath5k => 304, 486
>>>>> follow mixed syntax.
>>>>>
>>>>> It seems to me as the documentation might be outdated, as you had also
>>>>> suggested earlier, or maybe users are unaware of the documentation and
>>>>>  just use the syntax for conserving consistency of the code.
>>>>>
>>>>> What do you think?
>>>>>
>>>>
>>>> I think it largely depends on the history of the code and developers
>>>> that take care...
>>>>
>>>> We might just start to ask the larger areas to modify the rule for
>>>> their directory.
>>>>
>>>
>>> I suggest we start with looking at the (let us say 10) "largest"
>>> directories with respect to number of comment blocks and then decide
>>> on a refinement of the rule for those subdirectories:
>>>
>>> A. Directory follows NETWORKING COMMENT STYLE; checkpatch shall warn
>>> to follow NETWORKING COMMENT STYLE.
>>> B. Directory follows GENERAL KERNEL COMMENT STYLE; checkpatch shall
>>> warn to follow GENERAL KERNEL COMMENT STYLE.
>>> C. Directory is too mixed; checkpatch shall not warn on any comment style.
>>>
>>> We can then start the discussion with the appropriate maintainer (and
>>> check what else this maintainer might maintain and if that is
>>> consistent) if they agree or disagree on that.
>>>
>>
>> Hi Lukas
>> Here is the list I have generated with the directories sorted on the
>> basis of comment count and labelled the top 10 directories as
>> following NETWORKING, GENERIC or MIXED comment style(s):
>>
>> At "drivers/net":
>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative_sorted.txt
>>
>> At "net":
>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative_sorted.txt
>>
>> What do you think?
>>
> 
> Obviously, this existing rule needs to be refined.
> 
> Do you have a proposal? Probably, you would like to have each
> maintainer state which comment style is preferred or so.
> 
> How could this be technically done?
> 

We can probably Cc all the corresponding maintainers checking from the
MAINTAINERS file.
Since we are considering only the top 10 directories, this probably
shouldn't be much time consuming.

Or we can probably mail any central maintainer(s) for all net and
drivers/net directories (for eg, someone who decided the comment rule
in the beginning for both the directories) and discuss.

What do you think?

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Lukas Bulwahn]

On Mon, Nov 30, 2020 at 5:47 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 30/11/20 3:40 pm, Lukas Bulwahn wrote:
> > On Sun, Nov 29, 2020 at 3:58 PM Aditya <yashsri421@gmail.com> wrote:
> >>
> >> On 25/11/20 6:08 pm, Lukas Bulwahn wrote:
> >>> On Wed, Nov 25, 2020 at 8:19 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> >>>>
> >>>> On Wed, Nov 25, 2020 at 8:02 AM Aditya <yashsri421@gmail.com> wrote:
> >>>>>
> >>>>> On 22/11/20 12:22 pm, Lukas Bulwahn wrote:
> >>>>>> On Thu, Nov 19, 2020 at 11:44 AM Aditya <yashsri421@gmail.com> wrote:
> >>>>>>>
> >>>>>>> On 19/11/20 12:30 am, Lukas Bulwahn wrote:
> >>>>>>>> On Mi., 18. Nov. 2020 at 18:40, Aditya Srivastava <yashsri421@gmail.com>
> >>>>>>>> wrote:
> >>>>>>>>
> >>>>>>>>> Currently, checkpatch warns us for files in 'net/' and 'drivers/net',
> >>>>>>>>> if we use an empty '/*' line for comment and contents of comments are
> >>>>>>>>> in next line
> >>>>>>>>>
> >>>>>>>>> E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
> >>>>>>>>> the refs / unrefs from the transport") reports this warning:
> >>>>>>>>>
> >>>>>>>>> WARNING: networking block comments don't use an empty /* line, use /*
> >>>>>>>>> Comment...
> >>>>>>>>> +               /*
> >>>>>>>>> +                * If the TXQ is active, then set the timer, if not,
> >>>>>>>>>
> >>>>>>>>> Provide a fix by appending the current line contents to previous line
> >>>>>>>>> and removing the current line
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Patch generally looks good.
> >>>>>>>>
> >>>>>>>> Can you check how many comments in net actually follow that style and how
> >>>>>>>> many follow another style?
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>> In drivers/net:
> >>>>>>> Wrong style: 14695 lines
> >>>>>>> Correct style: 147961 lines (ie around 10 times)
> >>>>>>>
> >>>>>>> In net/:
> >>>>>>> Wrong style: 5090 lines
> >>>>>>> Correct style: 30485 lines
> >>>>>>>
> >>>>>>
> >>>>>> Can you find out where the wrong style is used?
> >>>>>>
> >>>>>> Maybe the documentation is a bit outdated.
> >>>>>>
> >>>>>> For example, some drivers and some subdirectories might have settled
> >>>>>> for another commenting style.
> >>>>>>
> >>>>>> I guess you can submit the fix option, but it could be that the whole
> >>>>>> rule/feature is broken anyway... so I do not know if that fix is of
> >>>>>> any good...
> >>>>>>
> >>>>>> I think it is better to actually understand, document and encode the
> >>>>>> current rule that applies.
> >>>>>>
> >>>>>> Can you provide an evaluation where the different styles for
> >>>>>> commenting are aggregated in a good way?
> >>>>>> E.g., consistently within a file with style XYZ; mixed style but
> >>>>>> 80-90% are of style ABC; consistent within a directory.
> >>>>>>
> >>>>>> If you think some cases are in the wrong style for some specific
> >>>>>> files, simply send a patch correcting the commenting style and see.
> >>>>>>
> >>>>> Hi Lukas
> >>>>> I have generated the reports regarding the comments used in various
> >>>>> files at net/ and drivers/net.
> >>>>> Directory wise reports:
> >>>>>
> >>>>> net/:
> >>>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative.txt
> >>>>>
> >>>>> drivers/net:
> >>>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative.txt
> >>>>>
> >>>>
> >>>> Can you sort that by the overall number of comments per directory?
> >>>>
> >>>>> File wise reports:
> >>>>>
> >>>>> net/:
> >>>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative_file.txt
> >>>>>
> >>>>> drivers/net:
> >>>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative_file.txt
> >>>>>
> >>>>> Some files in 'net/' follow the accepted syntax, such as:
> >>>>> 'net/batman-adv => 985    0'.
> >>>>>
> >>>>> However, some completely not, such as:
> >>>>> 'net/ax25 => 0    103'.
> >>>>>
> >>>>> Some even follow mixed syntax such as:
> >>>>> 'net/netfilter => 87    125'.
> >>>>>
> >>>>> Similarly for drivers/net:
> >>>>> drivers/net/ethernet/mellanox/mlxsw => 1071    0
> >>>>> completely follow accepted syntax.
> >>>>>
> >>>>> drivers/net/wireless/ralink/rt2x00 => 98, 2082
> >>>>> follow unaccepted syntax largely.
> >>>>>
> >>>>> whereas files such as drivers/net/wireless/ath/ath5k => 304, 486
> >>>>> follow mixed syntax.
> >>>>>
> >>>>> It seems to me as the documentation might be outdated, as you had also
> >>>>> suggested earlier, or maybe users are unaware of the documentation and
> >>>>>  just use the syntax for conserving consistency of the code.
> >>>>>
> >>>>> What do you think?
> >>>>>
> >>>>
> >>>> I think it largely depends on the history of the code and developers
> >>>> that take care...
> >>>>
> >>>> We might just start to ask the larger areas to modify the rule for
> >>>> their directory.
> >>>>
> >>>
> >>> I suggest we start with looking at the (let us say 10) "largest"
> >>> directories with respect to number of comment blocks and then decide
> >>> on a refinement of the rule for those subdirectories:
> >>>
> >>> A. Directory follows NETWORKING COMMENT STYLE; checkpatch shall warn
> >>> to follow NETWORKING COMMENT STYLE.
> >>> B. Directory follows GENERAL KERNEL COMMENT STYLE; checkpatch shall
> >>> warn to follow GENERAL KERNEL COMMENT STYLE.
> >>> C. Directory is too mixed; checkpatch shall not warn on any comment style.
> >>>
> >>> We can then start the discussion with the appropriate maintainer (and
> >>> check what else this maintainer might maintain and if that is
> >>> consistent) if they agree or disagree on that.
> >>>
> >>
> >> Hi Lukas
> >> Here is the list I have generated with the directories sorted on the
> >> basis of comment count and labelled the top 10 directories as
> >> following NETWORKING, GENERIC or MIXED comment style(s):
> >>
> >> At "drivers/net":
> >> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative_sorted.txt
> >>
> >> At "net":
> >> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative_sorted.txt
> >>
> >> What do you think?
> >>
> >
> > Obviously, this existing rule needs to be refined.
> >
> > Do you have a proposal? Probably, you would like to have each
> > maintainer state which comment style is preferred or so.
> >
> > How could this be technically done?
> >
>
> We can probably Cc all the corresponding maintainers checking from the
> MAINTAINERS file.
> Since we are considering only the top 10 directories, this probably
> shouldn't be much time consuming.
>
> Or we can probably mail any central maintainer(s) for all net and
> drivers/net directories (for eg, someone who decided the comment rule
> in the beginning for both the directories) and discuss.
>

It makes sense to identify the names of those people to start with; I
would suggest NOT to contact them until we have a first good patch set
to provide some technical solution.

I was asking more about how a maintainer can state their preference in
the repository and how will checkpatch then use this information?

Did you think about a technical solution for that?

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Aditya]

On 30/11/20 10:27 pm, Lukas Bulwahn wrote:
> On Mon, Nov 30, 2020 at 5:47 PM Aditya <yashsri421@gmail.com> wrote:
>>
>> On 30/11/20 3:40 pm, Lukas Bulwahn wrote:
>>> On Sun, Nov 29, 2020 at 3:58 PM Aditya <yashsri421@gmail.com> wrote:
>>>>
>>>> On 25/11/20 6:08 pm, Lukas Bulwahn wrote:
>>>>> On Wed, Nov 25, 2020 at 8:19 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>>>>>>
>>>>>> On Wed, Nov 25, 2020 at 8:02 AM Aditya <yashsri421@gmail.com> wrote:
>>>>>>>
>>>>>>> On 22/11/20 12:22 pm, Lukas Bulwahn wrote:
>>>>>>>> On Thu, Nov 19, 2020 at 11:44 AM Aditya <yashsri421@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>> On 19/11/20 12:30 am, Lukas Bulwahn wrote:
>>>>>>>>>> On Mi., 18. Nov. 2020 at 18:40, Aditya Srivastava <yashsri421@gmail.com>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Currently, checkpatch warns us for files in 'net/' and 'drivers/net',
>>>>>>>>>>> if we use an empty '/*' line for comment and contents of comments are
>>>>>>>>>>> in next line
>>>>>>>>>>>
>>>>>>>>>>> E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
>>>>>>>>>>> the refs / unrefs from the transport") reports this warning:
>>>>>>>>>>>
>>>>>>>>>>> WARNING: networking block comments don't use an empty /* line, use /*
>>>>>>>>>>> Comment...
>>>>>>>>>>> +               /*
>>>>>>>>>>> +                * If the TXQ is active, then set the timer, if not,
>>>>>>>>>>>
>>>>>>>>>>> Provide a fix by appending the current line contents to previous line
>>>>>>>>>>> and removing the current line
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Patch generally looks good.
>>>>>>>>>>
>>>>>>>>>> Can you check how many comments in net actually follow that style and how
>>>>>>>>>> many follow another style?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> In drivers/net:
>>>>>>>>> Wrong style: 14695 lines
>>>>>>>>> Correct style: 147961 lines (ie around 10 times)
>>>>>>>>>
>>>>>>>>> In net/:
>>>>>>>>> Wrong style: 5090 lines
>>>>>>>>> Correct style: 30485 lines
>>>>>>>>>
>>>>>>>>
>>>>>>>> Can you find out where the wrong style is used?
>>>>>>>>
>>>>>>>> Maybe the documentation is a bit outdated.
>>>>>>>>
>>>>>>>> For example, some drivers and some subdirectories might have settled
>>>>>>>> for another commenting style.
>>>>>>>>
>>>>>>>> I guess you can submit the fix option, but it could be that the whole
>>>>>>>> rule/feature is broken anyway... so I do not know if that fix is of
>>>>>>>> any good...
>>>>>>>>
>>>>>>>> I think it is better to actually understand, document and encode the
>>>>>>>> current rule that applies.
>>>>>>>>
>>>>>>>> Can you provide an evaluation where the different styles for
>>>>>>>> commenting are aggregated in a good way?
>>>>>>>> E.g., consistently within a file with style XYZ; mixed style but
>>>>>>>> 80-90% are of style ABC; consistent within a directory.
>>>>>>>>
>>>>>>>> If you think some cases are in the wrong style for some specific
>>>>>>>> files, simply send a patch correcting the commenting style and see.
>>>>>>>>
>>>>>>> Hi Lukas
>>>>>>> I have generated the reports regarding the comments used in various
>>>>>>> files at net/ and drivers/net.
>>>>>>> Directory wise reports:
>>>>>>>
>>>>>>> net/:
>>>>>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative.txt
>>>>>>>
>>>>>>> drivers/net:
>>>>>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative.txt
>>>>>>>
>>>>>>
>>>>>> Can you sort that by the overall number of comments per directory?
>>>>>>
>>>>>>> File wise reports:
>>>>>>>
>>>>>>> net/:
>>>>>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative_file.txt
>>>>>>>
>>>>>>> drivers/net:
>>>>>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative_file.txt
>>>>>>>
>>>>>>> Some files in 'net/' follow the accepted syntax, such as:
>>>>>>> 'net/batman-adv => 985    0'.
>>>>>>>
>>>>>>> However, some completely not, such as:
>>>>>>> 'net/ax25 => 0    103'.
>>>>>>>
>>>>>>> Some even follow mixed syntax such as:
>>>>>>> 'net/netfilter => 87    125'.
>>>>>>>
>>>>>>> Similarly for drivers/net:
>>>>>>> drivers/net/ethernet/mellanox/mlxsw => 1071    0
>>>>>>> completely follow accepted syntax.
>>>>>>>
>>>>>>> drivers/net/wireless/ralink/rt2x00 => 98, 2082
>>>>>>> follow unaccepted syntax largely.
>>>>>>>
>>>>>>> whereas files such as drivers/net/wireless/ath/ath5k => 304, 486
>>>>>>> follow mixed syntax.
>>>>>>>
>>>>>>> It seems to me as the documentation might be outdated, as you had also
>>>>>>> suggested earlier, or maybe users are unaware of the documentation and
>>>>>>>  just use the syntax for conserving consistency of the code.
>>>>>>>
>>>>>>> What do you think?
>>>>>>>
>>>>>>
>>>>>> I think it largely depends on the history of the code and developers
>>>>>> that take care...
>>>>>>
>>>>>> We might just start to ask the larger areas to modify the rule for
>>>>>> their directory.
>>>>>>
>>>>>
>>>>> I suggest we start with looking at the (let us say 10) "largest"
>>>>> directories with respect to number of comment blocks and then decide
>>>>> on a refinement of the rule for those subdirectories:
>>>>>
>>>>> A. Directory follows NETWORKING COMMENT STYLE; checkpatch shall warn
>>>>> to follow NETWORKING COMMENT STYLE.
>>>>> B. Directory follows GENERAL KERNEL COMMENT STYLE; checkpatch shall
>>>>> warn to follow GENERAL KERNEL COMMENT STYLE.
>>>>> C. Directory is too mixed; checkpatch shall not warn on any comment style.
>>>>>
>>>>> We can then start the discussion with the appropriate maintainer (and
>>>>> check what else this maintainer might maintain and if that is
>>>>> consistent) if they agree or disagree on that.
>>>>>
>>>>
>>>> Hi Lukas
>>>> Here is the list I have generated with the directories sorted on the
>>>> basis of comment count and labelled the top 10 directories as
>>>> following NETWORKING, GENERIC or MIXED comment style(s):
>>>>
>>>> At "drivers/net":
>>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative_sorted.txt
>>>>
>>>> At "net":
>>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative_sorted.txt
>>>>
>>>> What do you think?
>>>>
>>>
>>> Obviously, this existing rule needs to be refined.
>>>
>>> Do you have a proposal? Probably, you would like to have each
>>> maintainer state which comment style is preferred or so.
>>>
>>> How could this be technically done?
>>>
>>
>> We can probably Cc all the corresponding maintainers checking from the
>> MAINTAINERS file.
>> Since we are considering only the top 10 directories, this probably
>> shouldn't be much time consuming.
>>
>> Or we can probably mail any central maintainer(s) for all net and
>> drivers/net directories (for eg, someone who decided the comment rule
>> in the beginning for both the directories) and discuss.
>>
> 
> It makes sense to identify the names of those people to start with; I
> would suggest NOT to contact them until we have a first good patch set
> to provide some technical solution.
> 
> I was asking more about how a maintainer can state their preference in
> the repository and how will checkpatch then use this information?
> 
> Did you think about a technical solution for that?
> 

Yes, you are correct. We can probably create a file where we can store
the directories(names) which are inside net or drivers/net, but do not
follow NETWORKING_BLOCK_COMMENT style (something like
networking_block_comment_style.ignore file). Maintainers can add their
directories inside this file.
We can then ignore these files from this class of warning.

What do you think?

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH] checkpatch: ignore files not following NETWORKING_BLOCK_COMMENT_STYLE

[Aditya Srivastava]

Currently checkpatch.pl gives warning for NETWORKING_BLOCK_COMMENT_STYLE
for files in net/ and drivers/net which do not follow the networking
comment style.
But some of these files seem to follow the generic comment style instead
of networking style and some rather mixed style of comment.

For e.g., drivers/net/wireless/ralink/rt2x00 largely follows generic
kernel comment style in spite of being inside drivers/net.

Provide an ignore file(".networking_block_comment_styles.ignore"), where
users can add the files they want to ignore this warning.

Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
 .gitignore                              |  1 +
 .networking_block_comment_styles.ignore |  1 +
 scripts/checkpatch.pl                   | 25 ++++++++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)
 create mode 100644 .networking_block_comment_styles.ignore

diff --git a/.gitignore b/.gitignore
index d01cda8e1177..3cd6074d1fb0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -94,6 +94,7 @@ modules.order
 !.gitattributes
 !.gitignore
 !.mailmap
+!.networking_block_comment_styles.ignore
 
 #
 # Generated include files
diff --git a/.networking_block_comment_styles.ignore b/.networking_block_comment_styles.ignore
new file mode 100644
index 000000000000..2022dd619901
--- /dev/null
+++ b/.networking_block_comment_styles.ignore
@@ -0,0 +1 @@
+drivers/net/wireless/ralink/rt2x00
\ No newline at end of file
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 00085308ed9d..875e572df5dc 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -832,6 +832,28 @@ sub read_words {
 	return 0;
 }
 
+my @files_ignoring_networking_comment;
+my $ignore_file = which_conf(".networking_block_comment_styles.ignore");
+if(-f $ignore_file) {
+	open(my $ignore, '<', "$ignore_file")
+	    or warn "$P: Can't find a readable .networking_block_comment_styles.ignore file $!\n";
+	while(<$ignore>) {
+		my $line = "$_";
+		$line = trim($line);
+		next if ($line =~ /^\s*$/);
+		push(@files_ignoring_networking_comment, $line);
+	}
+	close($ignore);
+}
+
+sub ignore_networking_comment_style {
+	my ($realfile) = "@_";
+	foreach my $file (@files_ignoring_networking_comment) {
+		return 1 if ($realfile =~ m@^$file@);
+	}
+	return 0;
+}
+
 my $const_structs;
 if (show_type("CONST_STRUCT")) {
 	read_words(\$const_structs, $conststructsfile)
@@ -3701,7 +3723,8 @@ sub process {
 		if ($realfile =~ m@^(drivers/net/|net/)@ &&
 		    $prevrawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
 		    $rawline =~ /^\+[ \t]*\*/ &&
-		    $realline > 3) { # Do not warn about the initial copyright comment block after SPDX-License-Identifier
+		    $realline > 3 &&  # Do not warn about the initial copyright comment block after SPDX-License-Identifier
+		    !ignore_networking_comment_style($realfile)) {
 			WARN("NETWORKING_BLOCK_COMMENT_STYLE",
 			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
 		}
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: ignore files not following NETWORKING_BLOCK_COMMENT_STYLE

[Aditya]

On 21/12/20 12:03 am, Aditya Srivastava wrote:
> Currently checkpatch.pl gives warning for NETWORKING_BLOCK_COMMENT_STYLE
> for files in net/ and drivers/net which do not follow the networking
> comment style.
> But some of these files seem to follow the generic comment style instead
> of networking style and some rather mixed style of comment.
> 
> For e.g., drivers/net/wireless/ralink/rt2x00 largely follows generic
> kernel comment style in spite of being inside drivers/net.
> 
> Provide an ignore file(".networking_block_comment_styles.ignore"), where
> users can add the files they want to ignore this warning.
> 
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
>  .gitignore                              |  1 +
>  .networking_block_comment_styles.ignore |  1 +
>  scripts/checkpatch.pl                   | 25 ++++++++++++++++++++++++-
>  3 files changed, 26 insertions(+), 1 deletion(-)
>  create mode 100644 .networking_block_comment_styles.ignore
> 
> diff --git a/.gitignore b/.gitignore
> index d01cda8e1177..3cd6074d1fb0 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -94,6 +94,7 @@ modules.order
>  !.gitattributes
>  !.gitignore
>  !.mailmap
> +!.networking_block_comment_styles.ignore
>  
>  #
>  # Generated include files
> diff --git a/.networking_block_comment_styles.ignore b/.networking_block_comment_styles.ignore
> new file mode 100644
> index 000000000000..2022dd619901
> --- /dev/null
> +++ b/.networking_block_comment_styles.ignore
> @@ -0,0 +1 @@
> +drivers/net/wireless/ralink/rt2x00
> \ No newline at end of file
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 00085308ed9d..875e572df5dc 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -832,6 +832,28 @@ sub read_words {
>  	return 0;
>  }
>  
> +my @files_ignoring_networking_comment;
> +my $ignore_file = which_conf(".networking_block_comment_styles.ignore");
> +if(-f $ignore_file) {
> +	open(my $ignore, '<', "$ignore_file")
> +	    or warn "$P: Can't find a readable .networking_block_comment_styles.ignore file $!\n";
> +	while(<$ignore>) {
> +		my $line = "$_";
> +		$line = trim($line);
> +		next if ($line =~ /^\s*$/);
> +		push(@files_ignoring_networking_comment, $line);
> +	}
> +	close($ignore);
> +}
> +
> +sub ignore_networking_comment_style {
> +	my ($realfile) = "@_";
> +	foreach my $file (@files_ignoring_networking_comment) {
> +		return 1 if ($realfile =~ m@^$file@);
> +	}
> +	return 0;
> +}
> +
>  my $const_structs;
>  if (show_type("CONST_STRUCT")) {
>  	read_words(\$const_structs, $conststructsfile)
> @@ -3701,7 +3723,8 @@ sub process {
>  		if ($realfile =~ m@^(drivers/net/|net/)@ &&
>  		    $prevrawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
>  		    $rawline =~ /^\+[ \t]*\*/ &&
> -		    $realline > 3) { # Do not warn about the initial copyright comment block after SPDX-License-Identifier
> +		    $realline > 3 &&  # Do not warn about the initial copyright comment block after SPDX-License-Identifier
> +		    !ignore_networking_comment_style($realfile)) {
>  			WARN("NETWORKING_BLOCK_COMMENT_STYLE",
>  			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
>  		}
> 

Hi Lukas
I have made this patch as I last proposed for how certain files can
avoid this warning by adding in a common .ignore file.

For the start, I have added only one directory name ie,
"drivers/net/wireless/ralink/rt2x00".

In this time, I also found out that users can also use --ignore
NETWORKING_BLOCK_COMMENT_STYLE with checkpatch and it will also result
in ignoring this style.
I don't know though which way is more preferred for the MAINTAINERS.

What do you think?

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: ignore files not following NETWORKING_BLOCK_COMMENT_STYLE

[Lukas Bulwahn]

On Sun, Dec 20, 2020 at 7:33 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
>
> Currently checkpatch.pl gives warning for NETWORKING_BLOCK_COMMENT_STYLE
> for files in net/ and drivers/net which do not follow the networking
> comment style.
> But some of these files seem to follow the generic comment style instead
> of networking style and some rather mixed style of comment.
>
> For e.g., drivers/net/wireless/ralink/rt2x00 largely follows generic
> kernel comment style in spite of being inside drivers/net.
>
> Provide an ignore file(".networking_block_comment_styles.ignore"), where
> users can add the files they want to ignore this warning.
>

I believe that is a really bad design decision here.

Imagine that in one directory, the maintainer wants to adjust ten
rules for checkpatch.

Are we going to implement this logic for those ten rules individually?
Is the maintainer going to add

None of that is obviously acceptable: 1. because there are better
design decisions; 2. because tailoring checkpatch would simply not be
worth the cost of having all those individual files laying around in
the repository and the complication throughout the whole script.

Also, you already recognized that there is already a feature to ignore
or select rules through command-line parameters.

So, how about coming up with a proper .checkpatch config file format
and then use the already available feature to configure the checkpatch
run?

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: ignore files not following NETWORKING_BLOCK_COMMENT_STYLE

[Aditya]

On 21/12/20 10:57 am, Lukas Bulwahn wrote:
> On Sun, Dec 20, 2020 at 7:33 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
>>
>> Currently checkpatch.pl gives warning for NETWORKING_BLOCK_COMMENT_STYLE
>> for files in net/ and drivers/net which do not follow the networking
>> comment style.
>> But some of these files seem to follow the generic comment style instead
>> of networking style and some rather mixed style of comment.
>>
>> For e.g., drivers/net/wireless/ralink/rt2x00 largely follows generic
>> kernel comment style in spite of being inside drivers/net.
>>
>> Provide an ignore file(".networking_block_comment_styles.ignore"), where
>> users can add the files they want to ignore this warning.
>>
> 
> I believe that is a really bad design decision here.
> 
> Imagine that in one directory, the maintainer wants to adjust ten
> rules for checkpatch.
> 
> Are we going to implement this logic for those ten rules individually?

No, only the files which are not following the networking comment
style, and are inside net/ or drivers/net should be added. The files
which are following the networking style already, need not add
themselves anywhere.

I feel, normally maintainer will want entire directory to follow the
same style. So, they can just add their directory in the .ignore file
and it will be ignored from the warning.

> Is the maintainer going to add
> 
> None of that is obviously acceptable: 1. because there are better
> design decisions; 2. because tailoring checkpatch would simply not be
> worth the cost of having all those individual files laying around in
> the repository and the complication throughout the whole script.
> 

We have earlier used a similar approach with ".get_maintainer.ignore",
where the users add their signature to be ignored from being mentioned
by "get_maintainers.pl" script.
Link: https://lore.kernel.org/patchwork/patch/939769/

We just need one file for our purpose (i.e., not necessarily .ignore
file).
Here I have used .ignore file in the root directory, so that it is
easier for the maintainer to edit it and add their directory in it.

If not in the root directory(as the files look scattered), perhaps we
can place it in script directory as .ignore or .txt file, or maybe
just place these directories in the form of a hash in the script
itself. But perhaps we'll need a list of directories to ignore and
then we'll be able to form the hash.

> Also, you already recognized that there is already a feature to ignore
> or select rules through command-line parameters.
> 
> So, how about coming up with a proper .checkpatch config file format
> and then use the already available feature to configure the checkpatch
> run?
> 

The user can add "--ignore NETWORKING_BLOCK_COMMENT_STYLE" line in
their .checkpatch.conf file, and checkpatch will then ignore this
class of warning.

Somewhat similar to this repository here:
https://github.com/openil/u-boot/blob/master/.checkpatch.conf

But this file(".checkpatch.conf") is ignored by git and is not
committed (as different users may require different configurations).
So every user will have to add this line in their individual config files.

Also, if the user starts working on a different file, where the
networking comment style is followed, he will again have to delete
this configuration line from the config file, to get this warning.

I'm not sure, if we can add directory/file specific configuration in
.checkpatch.conf. But again, if it was possible, we'll be specifying
individual files, and telling them to be avoided, similar to the
current approach.

Using any of these solution, if the users ignore this warning, we can
perhaps add a fix for NETWORKING_BLOCK_COMMENT_STYLE warning.

What do you think?

Thanks
Aditya

> Lukas
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: ignore files not following NETWORKING_BLOCK_COMMENT_STYLE

[Lukas Bulwahn]

On Mon, Dec 21, 2020 at 4:29 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 21/12/20 10:57 am, Lukas Bulwahn wrote:
> > On Sun, Dec 20, 2020 at 7:33 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
> >>
> >> Currently checkpatch.pl gives warning for NETWORKING_BLOCK_COMMENT_STYLE
> >> for files in net/ and drivers/net which do not follow the networking
> >> comment style.
> >> But some of these files seem to follow the generic comment style instead
> >> of networking style and some rather mixed style of comment.
> >>
> >> For e.g., drivers/net/wireless/ralink/rt2x00 largely follows generic
> >> kernel comment style in spite of being inside drivers/net.
> >>
> >> Provide an ignore file(".networking_block_comment_styles.ignore"), where
> >> users can add the files they want to ignore this warning.
> >>
> >
> > I believe that is a really bad design decision here.
> >
> > Imagine that in one directory, the maintainer wants to adjust ten
> > rules for checkpatch.
> >
> > Are we going to implement this logic for those ten rules individually?
>
> No, only the files which are not following the networking comment
> style, and are inside net/ or drivers/net should be added. The files
> which are following the networking style already, need not add
> themselves anywhere.
>
> I feel, normally maintainer will want entire directory to follow the
> same style. So, they can just add their directory in the .ignore file
> and it will be ignored from the warning.
>

I understand that.

> > Is the maintainer going to add
> >
> > None of that is obviously acceptable: 1. because there are better
> > design decisions; 2. because tailoring checkpatch would simply not be
> > worth the cost of having all those individual files laying around in
> > the repository and the complication throughout the whole script.
> >
>
> We have earlier used a similar approach with ".get_maintainer.ignore",
> where the users add their signature to be ignored from being mentioned
> by "get_maintainers.pl" script.
> Link: https://lore.kernel.org/patchwork/patch/939769/

Yeah, I know that. It is known to be called the "Christoph file in the
root directory", because for years, it only contained Christoph as he
was the only person that requested that feature. It is a good example
of a terrible feature.

>
> We just need one file for our purpose (i.e., not necessarily .ignore
> file).
> Here I have used .ignore file in the root directory, so that it is
> easier for the maintainer to edit it and add their directory in it.
>

Do you know how many maintainers in this project exist?
For the kernel, this what you suggest just does not make sense...

> If not in the root directory(as the files look scattered), perhaps we
> can place it in script directory as .ignore or .txt file, or maybe
> just place these directories in the form of a hash in the script
> itself. But perhaps we'll need a list of directories to ignore and
> then we'll be able to form the hash.
>

Which problem are you trying to solve?

> > Also, you already recognized that there is already a feature to ignore
> > or select rules through command-line parameters.
> >
> > So, how about coming up with a proper .checkpatch config file format
> > and then use the already available feature to configure the checkpatch
> > run?
> >
>
> The user can add "--ignore NETWORKING_BLOCK_COMMENT_STYLE" line in
> their .checkpatch.conf file, and checkpatch will then ignore this
> class of warning.
>
> Somewhat similar to this repository here:
> https://github.com/openil/u-boot/blob/master/.checkpatch.conf
>
> But this file(".checkpatch.conf") is ignored by git and is not
> committed (as different users may require different configurations).
> So every user will have to add this line in their individual config files.
>

I do not get what you say. Isn't this just a configuration question?
You configure it to be part of the configuration.

> Also, if the user starts working on a different file, where the
> networking comment style is followed, he will again have to delete
> this configuration line from the config file, to get this warning.
>
> I'm not sure, if we can add directory/file specific configuration in
> .checkpatch.conf. But again, if it was possible, we'll be specifying
> individual files, and telling them to be avoided, similar to the
> current approach.
>

A suitable feature can be implemented.

Think about 100 people modifying the same file at the same time and
the problem this brings during merging.

Think about commits moving directories from one place to the other and
the effort of keeping this file correct.

I would suggest not to have any "checkpatch.config" at the root level
mentioning all the different directories, as you are suggesting.

I suggest instead:

Have multiple ".checkpatch.conf" files in the different directories
and let checkpatch determine the relevant rules that need to be
applied to a specific file by traverse along the file hierarchy.
Generalize the method to work for any checkpatch rule, not just the
block_comment_style rule here.

Feel free to submit your current proposal to Joe Perches and lkml if
you are convinced of your idea and willing to get more feedback
(probably just more rejection). I personally believe this will be
rejected by others as well.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees