[Linux-kernel-mentees] [PATCH] checkpatch: add fixes for BAD_SIGN_OFF

[Linux-kernel-mentees] [PATCH] checkpatch: add fixes for BAD_SIGN_OFF

[Aditya Srivastava]

Currently, checkpatch warns if the author signs-off two or more times
for the same role.
E.g., for commit 6dd47d9754ff ("mac80211: fix
ieee80211_txq_setup_flows() failure path") we get warning:

WARNING: Duplicate signature
Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Here there are two Signed-off-by lines present for the same user. So,
we get a warning to remove one.

Similarly, we get warning if the author of the commit signs-off under
co-developed-by.
E.g. for commit 6e88559470f5 ("Documentation: Add section about
CPU vulnerabilities for Spectre") we get:

WARNING: Co-developed-by: should not be used to attribute nominal
patch author 'Tim Chen <tim.c.chen@linux.intel.com>'
Co-developed-by: Tim Chen <tim.c.chen@linux.intel.com>

Provide fixes by removing the duplicate signature line and the
co-developed-by line from the commit

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 58095d9d8f34..4e6cbca495eb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2818,8 +2818,11 @@ sub process {
 			$sig_nospace =~ s/\s//g;
 			$sig_nospace = lc($sig_nospace);
 			if (defined $signatures{$sig_nospace}) {
-				WARN("BAD_SIGN_OFF",
-				     "Duplicate signature\n" . $herecurr);
+				if (WARN("BAD_SIGN_OFF",
+					 "Duplicate signature\n" . $herecurr) &&
+				    $fix) {
+					fix_delete_line($fixlinenr, $rawline);
+				}
 			} else {
 				$signatures{$sig_nospace} = 1;
 			}
@@ -2827,8 +2830,11 @@ sub process {
 # Check Co-developed-by: immediately followed by Signed-off-by: with same name and email
 			if ($sign_off =~ /^co-developed-by:$/i) {
 				if ($email eq $author) {
-					WARN("BAD_SIGN_OFF",
-					      "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline);
+					if (WARN("BAD_SIGN_OFF",
+						 "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline) &&
+					    $fix) {
+						fix_delete_line($fixlinenr, $rawline);
+					}
 				}
 				if (!defined $lines[$linenr]) {
 					WARN("BAD_SIGN_OFF",
-- 
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 fixes for BAD_SIGN_OFF

[Lukas Bulwahn]

On Tue, Nov 3, 2020 at 2:31 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
>
> Currently, checkpatch warns if the author signs-off two or more times
> for the same role.
> E.g., for commit 6dd47d9754ff ("mac80211: fix
> ieee80211_txq_setup_flows() failure path") we get warning:
>
> WARNING: Duplicate signature
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>
> Here there are two Signed-off-by lines present for the same user. So,
> we get a warning to remove one.
>
> Similarly, we get warning if the author of the commit signs-off under
> co-developed-by.
> E.g. for commit 6e88559470f5 ("Documentation: Add section about
> CPU vulnerabilities for Spectre") we get:
>
> WARNING: Co-developed-by: should not be used to attribute nominal
> patch author 'Tim Chen <tim.c.chen@linux.intel.com>'
> Co-developed-by: Tim Chen <tim.c.chen@linux.intel.com>
>
> Provide fixes by removing the duplicate signature line and the
> co-developed-by line from the commit
>

A patch shall only do ONE thing. I see two here.

The duplicate signature fix is simply wrong, because the indication of
the duplicate signature is imprecise.

Probably, we would first really need to understand the valid cases of
duplicate signature warnings compared to the invalid ones.

Please rework.


Lukas

> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
>  scripts/checkpatch.pl | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 58095d9d8f34..4e6cbca495eb 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2818,8 +2818,11 @@ sub process {
>                         $sig_nospace =~ s/\s//g;
>                         $sig_nospace = lc($sig_nospace);
>                         if (defined $signatures{$sig_nospace}) {
> -                               WARN("BAD_SIGN_OFF",
> -                                    "Duplicate signature\n" . $herecurr);
> +                               if (WARN("BAD_SIGN_OFF",
> +                                        "Duplicate signature\n" . $herecurr) &&
> +                                   $fix) {
> +                                       fix_delete_line($fixlinenr, $rawline);
> +                               }
>                         } else {
>                                 $signatures{$sig_nospace} = 1;
>                         }
> @@ -2827,8 +2830,11 @@ sub process {
>  # Check Co-developed-by: immediately followed by Signed-off-by: with same name and email
>                         if ($sign_off =~ /^co-developed-by:$/i) {
>                                 if ($email eq $author) {
> -                                       WARN("BAD_SIGN_OFF",
> -                                             "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline);
> +                                       if (WARN("BAD_SIGN_OFF",
> +                                                "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline) &&
> +                                           $fix) {
> +                                               fix_delete_line($fixlinenr, $rawline);
> +                                       }
>                                 }
>                                 if (!defined $lines[$linenr]) {
>                                         WARN("BAD_SIGN_OFF",
> --
> 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 fixes for BAD_SIGN_OFF

[Aditya]

On 3/11/20 7:09 pm, Lukas Bulwahn wrote:
> On Tue, Nov 3, 2020 at 2:31 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
>>
>> Currently, checkpatch warns if the author signs-off two or more times
>> for the same role.
>> E.g., for commit 6dd47d9754ff ("mac80211: fix
>> ieee80211_txq_setup_flows() failure path") we get warning:
>>
>> WARNING: Duplicate signature
>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>>
>> Here there are two Signed-off-by lines present for the same user. So,
>> we get a warning to remove one.
>>
>> Similarly, we get warning if the author of the commit signs-off under
>> co-developed-by.
>> E.g. for commit 6e88559470f5 ("Documentation: Add section about
>> CPU vulnerabilities for Spectre") we get:
>>
>> WARNING: Co-developed-by: should not be used to attribute nominal
>> patch author 'Tim Chen <tim.c.chen@linux.intel.com>'
>> Co-developed-by: Tim Chen <tim.c.chen@linux.intel.com>
>>
>> Provide fixes by removing the duplicate signature line and the
>> co-developed-by line from the commit
>>
> 
> 
> The duplicate signature fix is simply wrong, because the indication of
> the duplicate signature is imprecise.
> 
> Probably, we would first really need to understand the valid cases of
> duplicate signature warnings compared to the invalid ones.
> 
> Please rework.
> 
> 
> Lukas
> 

Oh, Alright. I get it now.
I ran a script over WARNING: Duplicate signature.
I found out that it is occurring with these prefixes and their
frequencies:

Acked-by -- 6
Signed-off-by -- 218
Tested-by -- 3
Reported-by -- 1
Reviewed-by -- 8
Cc -- 28

IMO largely Signed-off-by warnings in this frequency might be false
positives. This is so because we are simply checking for duplicate
occurrences in the sign-offs in checkpatch.pl

> Probably, we would first really need to understand the valid cases
> of duplicate signature warnings compared to the invalid ones.
>

Is there any way I can know that the second sign-off is intended?

In my observation, this particular commit was prefixed by '^Link:.*$'
Alternatively, we could use maintainers list to ensure that the signer
is maintainer and avoid this warning.

How about other prefixes. Can we remove them?

I observed one of the 'Tested-by' warnings and it was because of these
sign offs:

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Tested-by:Liang Yang <liang.yang@amlogic.com>
Acked-by: Liang Yang <liang.yang@amlogic.com>
Tested-by:Liang Yang <liang.yang@amlogic.com>
Acked-by: Liang Yang <liang.yang@amlogic.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Here tested-by is occurring 2 times.


> A patch shall only do ONE thing. I see two here.
I'll remove the fix for Duplicate Signature from this patch.


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: add fixes for BAD_SIGN_OFF

[Lukas Bulwahn]

On Tue, Nov 3, 2020 at 3:47 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 3/11/20 7:09 pm, Lukas Bulwahn wrote:
> > On Tue, Nov 3, 2020 at 2:31 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
> >>
> >> Currently, checkpatch warns if the author signs-off two or more times
> >> for the same role.
> >> E.g., for commit 6dd47d9754ff ("mac80211: fix
> >> ieee80211_txq_setup_flows() failure path") we get warning:
> >>
> >> WARNING: Duplicate signature
> >> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> >>
> >> Here there are two Signed-off-by lines present for the same user. So,
> >> we get a warning to remove one.
> >>
> >> Similarly, we get warning if the author of the commit signs-off under
> >> co-developed-by.
> >> E.g. for commit 6e88559470f5 ("Documentation: Add section about
> >> CPU vulnerabilities for Spectre") we get:
> >>
> >> WARNING: Co-developed-by: should not be used to attribute nominal
> >> patch author 'Tim Chen <tim.c.chen@linux.intel.com>'
> >> Co-developed-by: Tim Chen <tim.c.chen@linux.intel.com>
> >>
> >> Provide fixes by removing the duplicate signature line and the
> >> co-developed-by line from the commit
> >>
> >
> >
> > The duplicate signature fix is simply wrong, because the indication of
> > the duplicate signature is imprecise.
> >
> > Probably, we would first really need to understand the valid cases of
> > duplicate signature warnings compared to the invalid ones.
> >
> > Please rework.
> >
> >
> > Lukas
> >
>
> Oh, Alright. I get it now.
> I ran a script over WARNING: Duplicate signature.
> I found out that it is occurring with these prefixes and their
> frequencies:
>
> Acked-by -- 6
> Signed-off-by -- 218
> Tested-by -- 3
> Reported-by -- 1
> Reviewed-by -- 8
> Cc -- 28
>
> IMO largely Signed-off-by warnings in this frequency might be false
> positives. This is so because we are simply checking for duplicate
> occurrences in the sign-offs in checkpatch.pl
>
> > Probably, we would first really need to understand the valid cases
> > of duplicate signature warnings compared to the invalid ones.
> >
>
> Is there any way I can know that the second sign-off is intended?
>
> In my observation, this particular commit was prefixed by '^Link:.*$'
> Alternatively, we could use maintainers list to ensure that the signer
> is maintainer and avoid this warning.
>

That is somehow difficult to say, but probably it is best to
deactivate this multiple Signed-off-by: warning on git commits.

On patches it makes sense to warn about duplicate Signed-off-by: ; on
commits, not so much.

> How about other prefixes. Can we remove them?
>

Can you show the duplicate Tested-by, Acked-by, Reported-by,
Reviewed-by, Cc in some structured format to quickly check those?

Probably for those, we can really assess them to be all true positives
(maybe we can find out the reasons as well) and hence a fix for them
would make sense.

> I observed one of the 'Tested-by' warnings and it was because of these
> sign offs:
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Tested-by:Liang Yang <liang.yang@amlogic.com>
> Acked-by: Liang Yang <liang.yang@amlogic.com>
> Tested-by:Liang Yang <liang.yang@amlogic.com>
> Acked-by: Liang Yang <liang.yang@amlogic.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>
> Here tested-by is occurring 2 times.
>

That example seems to be a clear true positive. It would be
interesting to see how that happened to be added twice in a commit.
What was the state on the mailing list and what did the maintainer do
to add these tags twice (was this done manually or is there some
automatic tool involved?).


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: add fixes for BAD_SIGN_OFF

[Aditya]

On 3/11/20 9:04 pm, Lukas Bulwahn wrote:
> On Tue, Nov 3, 2020 at 3:47 PM Aditya <yashsri421@gmail.com> wrote:
>>
>> On 3/11/20 7:09 pm, Lukas Bulwahn wrote:
>>> On Tue, Nov 3, 2020 at 2:31 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
>>>>
>>>> Currently, checkpatch warns if the author signs-off two or more times
>>>> for the same role.
>>>> E.g., for commit 6dd47d9754ff ("mac80211: fix
>>>> ieee80211_txq_setup_flows() failure path") we get warning:
>>>>
>>>> WARNING: Duplicate signature
>>>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>>>>
>>>> Here there are two Signed-off-by lines present for the same user. So,
>>>> we get a warning to remove one.
>>>>
>>>> Similarly, we get warning if the author of the commit signs-off under
>>>> co-developed-by.
>>>> E.g. for commit 6e88559470f5 ("Documentation: Add section about
>>>> CPU vulnerabilities for Spectre") we get:
>>>>
>>>> WARNING: Co-developed-by: should not be used to attribute nominal
>>>> patch author 'Tim Chen <tim.c.chen@linux.intel.com>'
>>>> Co-developed-by: Tim Chen <tim.c.chen@linux.intel.com>
>>>>
>>>> Provide fixes by removing the duplicate signature line and the
>>>> co-developed-by line from the commit
>>>>
>>>
>>>
>>> The duplicate signature fix is simply wrong, because the indication of
>>> the duplicate signature is imprecise.
>>>
>>> Probably, we would first really need to understand the valid cases of
>>> duplicate signature warnings compared to the invalid ones.
>>>
>>> Please rework.
>>>
>>>
>>> Lukas
>>>
>>
>> Oh, Alright. I get it now.
>> I ran a script over WARNING: Duplicate signature.
>> I found out that it is occurring with these prefixes and their
>> frequencies:
>>
>> Acked-by -- 6
>> Signed-off-by -- 218
>> Tested-by -- 3
>> Reported-by -- 1
>> Reviewed-by -- 8
>> Cc -- 28
>>
>> IMO largely Signed-off-by warnings in this frequency might be false
>> positives. This is so because we are simply checking for duplicate
>> occurrences in the sign-offs in checkpatch.pl
>>
>>> Probably, we would first really need to understand the valid cases
>>> of duplicate signature warnings compared to the invalid ones.
>>>
>>
>> Is there any way I can know that the second sign-off is intended?
>>
>> In my observation, this particular commit was prefixed by '^Link:.*$'
>> Alternatively, we could use maintainers list to ensure that the signer
>> is maintainer and avoid this warning.
>>
> 
> That is somehow difficult to say, but probably it is best to
> deactivate this multiple Signed-off-by: warning on git commits.
> 
> On patches it makes sense to warn about duplicate Signed-off-by: ; on
> commits, not so much.
> 
>> How about other prefixes. Can we remove them?
>>
> 
> Can you show the duplicate Tested-by, Acked-by, Reported-by,
> Reviewed-by, Cc in some structured format to quickly check those?
> 
> Probably for those, we can really assess them to be all true positives
> (maybe we can find out the reasons as well) and hence a fix for them
> would make sense.
> 

I generated git logs of all such commits, which might help us get some
idea about mailing list or sign offs as well.

Acked-by:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/acked_by.txt

Reported-by:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/reported_by.txt

Reviewed by:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/reviewed_by.txt

Tested-by:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/tested_by.txt

Cc: https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/cc.txt


>> I observed one of the 'Tested-by' warnings and it was because of these
>> sign offs:
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Tested-by:Liang Yang <liang.yang@amlogic.com>
>> Acked-by: Liang Yang <liang.yang@amlogic.com>
>> Tested-by:Liang Yang <liang.yang@amlogic.com>
>> Acked-by: Liang Yang <liang.yang@amlogic.com>
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>
>> Here tested-by is occurring 2 times.
>>
> 
> That example seems to be a clear true positive. It would be
> interesting to see how that happened to be added twice in a commit.
> What was the state on the mailing list and what did the maintainer do
> to add these tags twice (was this done manually or is there some
> automatic tool involved?).
> 

I found the mailing list state corresponding to that day. It indeed
seems like a true positive.

Link1:
https://lore.kernel.org/linux-mtd/CAFBinCB9Z_H4wX3_g2hVL-XnS6Oq-KtPHa4Z1ws79FGp5jCapQ@mail.gmail.com/

Link2 (reply):
https://lore.kernel.org/linux-mtd/8ff232c3-f325-5b91-4de1-a39e63939df2@amlogic.com/

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: add fixes for BAD_SIGN_OFF

[Lukas Bulwahn]

On Tue, 3 Nov 2020, Aditya wrote:

> On 3/11/20 9:04 pm, Lukas Bulwahn wrote:
> > On Tue, Nov 3, 2020 at 3:47 PM Aditya <yashsri421@gmail.com> wrote:
> >>
> >> On 3/11/20 7:09 pm, Lukas Bulwahn wrote:
> >>> On Tue, Nov 3, 2020 at 2:31 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
> >>>>
> >>>> Currently, checkpatch warns if the author signs-off two or more times
> >>>> for the same role.
> >>>> E.g., for commit 6dd47d9754ff ("mac80211: fix
> >>>> ieee80211_txq_setup_flows() failure path") we get warning:
> >>>>
> >>>> WARNING: Duplicate signature
> >>>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> >>>>
> >>>> Here there are two Signed-off-by lines present for the same user. So,
> >>>> we get a warning to remove one.
> >>>>
> >>>> Similarly, we get warning if the author of the commit signs-off under
> >>>> co-developed-by.
> >>>> E.g. for commit 6e88559470f5 ("Documentation: Add section about
> >>>> CPU vulnerabilities for Spectre") we get:
> >>>>
> >>>> WARNING: Co-developed-by: should not be used to attribute nominal
> >>>> patch author 'Tim Chen <tim.c.chen@linux.intel.com>'
> >>>> Co-developed-by: Tim Chen <tim.c.chen@linux.intel.com>
> >>>>
> >>>> Provide fixes by removing the duplicate signature line and the
> >>>> co-developed-by line from the commit
> >>>>
> >>>
> >>>
> >>> The duplicate signature fix is simply wrong, because the indication of
> >>> the duplicate signature is imprecise.
> >>>
> >>> Probably, we would first really need to understand the valid cases of
> >>> duplicate signature warnings compared to the invalid ones.
> >>>
> >>> Please rework.
> >>>
> >>>
> >>> Lukas
> >>>
> >>
> >> Oh, Alright. I get it now.
> >> I ran a script over WARNING: Duplicate signature.
> >> I found out that it is occurring with these prefixes and their
> >> frequencies:
> >>
> >> Acked-by -- 6
> >> Signed-off-by -- 218
> >> Tested-by -- 3
> >> Reported-by -- 1
> >> Reviewed-by -- 8
> >> Cc -- 28
> >>
> >> IMO largely Signed-off-by warnings in this frequency might be false
> >> positives. This is so because we are simply checking for duplicate
> >> occurrences in the sign-offs in checkpatch.pl
> >>
> >>> Probably, we would first really need to understand the valid cases
> >>> of duplicate signature warnings compared to the invalid ones.
> >>>
> >>
> >> Is there any way I can know that the second sign-off is intended?
> >>
> >> In my observation, this particular commit was prefixed by '^Link:.*$'
> >> Alternatively, we could use maintainers list to ensure that the signer
> >> is maintainer and avoid this warning.
> >>
> > 
> > That is somehow difficult to say, but probably it is best to
> > deactivate this multiple Signed-off-by: warning on git commits.
> > 
> > On patches it makes sense to warn about duplicate Signed-off-by: ; on
> > commits, not so much.
> > 
> >> How about other prefixes. Can we remove them?
> >>
> > 
> > Can you show the duplicate Tested-by, Acked-by, Reported-by,
> > Reviewed-by, Cc in some structured format to quickly check those?
> > 
> > Probably for those, we can really assess them to be all true positives
> > (maybe we can find out the reasons as well) and hence a fix for them
> > would make sense.
> > 
> 
> I generated git logs of all such commits, which might help us get some
> idea about mailing list or sign offs as well.
> 
> Acked-by:
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/acked_by.txt
> 
> Reported-by:
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/reported_by.txt
> 
> Reviewed by:
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/reviewed_by.txt
> 
> Tested-by:
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/tested_by.txt
> 
> Cc: https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/cc.txt
> 
> 
> >> I observed one of the 'Tested-by' warnings and it was because of these
> >> sign offs:
> >>
> >> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> >> Tested-by:Liang Yang <liang.yang@amlogic.com>
> >> Acked-by: Liang Yang <liang.yang@amlogic.com>
> >> Tested-by:Liang Yang <liang.yang@amlogic.com>
> >> Acked-by: Liang Yang <liang.yang@amlogic.com>
> >> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >>
> >> Here tested-by is occurring 2 times.
> >>
> > 
> > That example seems to be a clear true positive. It would be
> > interesting to see how that happened to be added twice in a commit.
> > What was the state on the mailing list and what did the maintainer do
> > to add these tags twice (was this done manually or is there some
> > automatic tool involved?).
> > 
> 
> I found the mailing list state corresponding to that day. It indeed
> seems like a true positive.
> 
> Link1:
> https://lore.kernel.org/linux-mtd/CAFBinCB9Z_H4wX3_g2hVL-XnS6Oq-KtPHa4Z1ws79FGp5jCapQ@mail.gmail.com/
> 
> Link2 (reply):
> https://lore.kernel.org/linux-mtd/8ff232c3-f325-5b91-4de1-a39e63939df2@amlogic.com/
>

Yes, and it suggest to fix patchwork :)

How are your python programming skills?

Do you want to try fixing patchwork for this issue?

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: add fixes for BAD_SIGN_OFF

[Aditya]

On 4/11/20 12:27 pm, Lukas Bulwahn wrote:
> 
> 
> On Tue, 3 Nov 2020, Aditya wrote:
> 
>> On 3/11/20 9:04 pm, Lukas Bulwahn wrote:
>>> On Tue, Nov 3, 2020 at 3:47 PM Aditya <yashsri421@gmail.com> wrote:
>>>>
>>>> On 3/11/20 7:09 pm, Lukas Bulwahn wrote:
>>>>> On Tue, Nov 3, 2020 at 2:31 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
>>>>>>
>>>>>> Currently, checkpatch warns if the author signs-off two or more times
>>>>>> for the same role.
>>>>>> E.g., for commit 6dd47d9754ff ("mac80211: fix
>>>>>> ieee80211_txq_setup_flows() failure path") we get warning:
>>>>>>
>>>>>> WARNING: Duplicate signature
>>>>>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>>>>>>
>>>>>> Here there are two Signed-off-by lines present for the same user. So,
>>>>>> we get a warning to remove one.
>>>>>>
>>>>>> Similarly, we get warning if the author of the commit signs-off under
>>>>>> co-developed-by.
>>>>>> E.g. for commit 6e88559470f5 ("Documentation: Add section about
>>>>>> CPU vulnerabilities for Spectre") we get:
>>>>>>
>>>>>> WARNING: Co-developed-by: should not be used to attribute nominal
>>>>>> patch author 'Tim Chen <tim.c.chen@linux.intel.com>'
>>>>>> Co-developed-by: Tim Chen <tim.c.chen@linux.intel.com>
>>>>>>
>>>>>> Provide fixes by removing the duplicate signature line and the
>>>>>> co-developed-by line from the commit
>>>>>>
>>>>>
>>>>>
>>>>> The duplicate signature fix is simply wrong, because the indication of
>>>>> the duplicate signature is imprecise.
>>>>>
>>>>> Probably, we would first really need to understand the valid cases of
>>>>> duplicate signature warnings compared to the invalid ones.
>>>>>
>>>>> Please rework.
>>>>>
>>>>>
>>>>> Lukas
>>>>>
>>>>
>>>> Oh, Alright. I get it now.
>>>> I ran a script over WARNING: Duplicate signature.
>>>> I found out that it is occurring with these prefixes and their
>>>> frequencies:
>>>>
>>>> Acked-by -- 6
>>>> Signed-off-by -- 218
>>>> Tested-by -- 3
>>>> Reported-by -- 1
>>>> Reviewed-by -- 8
>>>> Cc -- 28
>>>>
>>>> IMO largely Signed-off-by warnings in this frequency might be false
>>>> positives. This is so because we are simply checking for duplicate
>>>> occurrences in the sign-offs in checkpatch.pl
>>>>
>>>>> Probably, we would first really need to understand the valid cases
>>>>> of duplicate signature warnings compared to the invalid ones.
>>>>>
>>>>
>>>> Is there any way I can know that the second sign-off is intended?
>>>>
>>>> In my observation, this particular commit was prefixed by '^Link:.*$'
>>>> Alternatively, we could use maintainers list to ensure that the signer
>>>> is maintainer and avoid this warning.
>>>>
>>>
>>> That is somehow difficult to say, but probably it is best to
>>> deactivate this multiple Signed-off-by: warning on git commits.
>>>
>>> On patches it makes sense to warn about duplicate Signed-off-by: ; on
>>> commits, not so much.
>>>
>>>> How about other prefixes. Can we remove them?
>>>>
>>>
>>> Can you show the duplicate Tested-by, Acked-by, Reported-by,
>>> Reviewed-by, Cc in some structured format to quickly check those?
>>>
>>> Probably for those, we can really assess them to be all true positives
>>> (maybe we can find out the reasons as well) and hence a fix for them
>>> would make sense.
>>>
>>
>> I generated git logs of all such commits, which might help us get some
>> idea about mailing list or sign offs as well.
>>
>> Acked-by:
>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/acked_by.txt
>>
>> Reported-by:
>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/reported_by.txt
>>
>> Reviewed by:
>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/reviewed_by.txt
>>
>> Tested-by:
>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/tested_by.txt
>>
>> Cc: https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/cc.txt
>>
>>
>>>> I observed one of the 'Tested-by' warnings and it was because of these
>>>> sign offs:
>>>>
>>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>>> Tested-by:Liang Yang <liang.yang@amlogic.com>
>>>> Acked-by: Liang Yang <liang.yang@amlogic.com>
>>>> Tested-by:Liang Yang <liang.yang@amlogic.com>
>>>> Acked-by: Liang Yang <liang.yang@amlogic.com>
>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>
>>>> Here tested-by is occurring 2 times.
>>>>
>>>
>>> That example seems to be a clear true positive. It would be
>>> interesting to see how that happened to be added twice in a commit.
>>> What was the state on the mailing list and what did the maintainer do
>>> to add these tags twice (was this done manually or is there some
>>> automatic tool involved?).
>>>
>>
>> I found the mailing list state corresponding to that day. It indeed
>> seems like a true positive.
>>
>> Link1:
>> https://lore.kernel.org/linux-mtd/CAFBinCB9Z_H4wX3_g2hVL-XnS6Oq-KtPHa4Z1ws79FGp5jCapQ@mail.gmail.com/
>>
>> Link2 (reply):
>> https://lore.kernel.org/linux-mtd/8ff232c3-f325-5b91-4de1-a39e63939df2@amlogic.com/
>>
> 
> Yes, and it suggest to fix patchwork :)
> 
> How are your python programming skills?
> 
> Do you want to try fixing patchwork for this issue?
> 
> Lukas
> 

Sure, I'd like to try. Please guide me for it.

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: add fixes for BAD_SIGN_OFF

[Aditya]

On 4/11/20 1:17 pm, Aditya wrote:
> On 4/11/20 12:27 pm, Lukas Bulwahn wrote:
>>
>>
>> On Tue, 3 Nov 2020, Aditya wrote:
>>
>>> On 3/11/20 9:04 pm, Lukas Bulwahn wrote:
>>>> On Tue, Nov 3, 2020 at 3:47 PM Aditya <yashsri421@gmail.com> wrote:
>>>>>
>>>>> On 3/11/20 7:09 pm, Lukas Bulwahn wrote:
>>>>>> On Tue, Nov 3, 2020 at 2:31 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
>>>>>>>
>>>>>>> Currently, checkpatch warns if the author signs-off two or more times
>>>>>>> for the same role.
>>>>>>> E.g., for commit 6dd47d9754ff ("mac80211: fix
>>>>>>> ieee80211_txq_setup_flows() failure path") we get warning:
>>>>>>>
>>>>>>> WARNING: Duplicate signature
>>>>>>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>>>>>>>
>>>>>>> Here there are two Signed-off-by lines present for the same user. So,
>>>>>>> we get a warning to remove one.
>>>>>>>
>>>>>>> Similarly, we get warning if the author of the commit signs-off under
>>>>>>> co-developed-by.
>>>>>>> E.g. for commit 6e88559470f5 ("Documentation: Add section about
>>>>>>> CPU vulnerabilities for Spectre") we get:
>>>>>>>
>>>>>>> WARNING: Co-developed-by: should not be used to attribute nominal
>>>>>>> patch author 'Tim Chen <tim.c.chen@linux.intel.com>'
>>>>>>> Co-developed-by: Tim Chen <tim.c.chen@linux.intel.com>
>>>>>>>
>>>>>>> Provide fixes by removing the duplicate signature line and the
>>>>>>> co-developed-by line from the commit
>>>>>>>
>>>>>>
>>>>>>
>>>>>> The duplicate signature fix is simply wrong, because the indication of
>>>>>> the duplicate signature is imprecise.
>>>>>>
>>>>>> Probably, we would first really need to understand the valid cases of
>>>>>> duplicate signature warnings compared to the invalid ones.
>>>>>>
>>>>>> Please rework.
>>>>>>
>>>>>>
>>>>>> Lukas
>>>>>>
>>>>>
>>>>> Oh, Alright. I get it now.
>>>>> I ran a script over WARNING: Duplicate signature.
>>>>> I found out that it is occurring with these prefixes and their
>>>>> frequencies:
>>>>>
>>>>> Acked-by -- 6
>>>>> Signed-off-by -- 218
>>>>> Tested-by -- 3
>>>>> Reported-by -- 1
>>>>> Reviewed-by -- 8
>>>>> Cc -- 28
>>>>>
>>>>> IMO largely Signed-off-by warnings in this frequency might be false
>>>>> positives. This is so because we are simply checking for duplicate
>>>>> occurrences in the sign-offs in checkpatch.pl
>>>>>
>>>>>> Probably, we would first really need to understand the valid cases
>>>>>> of duplicate signature warnings compared to the invalid ones.
>>>>>>
>>>>>
>>>>> Is there any way I can know that the second sign-off is intended?
>>>>>
>>>>> In my observation, this particular commit was prefixed by '^Link:.*$'
>>>>> Alternatively, we could use maintainers list to ensure that the signer
>>>>> is maintainer and avoid this warning.
>>>>>
>>>>
>>>> That is somehow difficult to say, but probably it is best to
>>>> deactivate this multiple Signed-off-by: warning on git commits.
>>>>
>>>> On patches it makes sense to warn about duplicate Signed-off-by: ; on
>>>> commits, not so much.
>>>>
>>>>> How about other prefixes. Can we remove them?
>>>>>
>>>>
>>>> Can you show the duplicate Tested-by, Acked-by, Reported-by,
>>>> Reviewed-by, Cc in some structured format to quickly check those?
>>>>
>>>> Probably for those, we can really assess them to be all true positives
>>>> (maybe we can find out the reasons as well) and hence a fix for them
>>>> would make sense.
>>>>
>>>
>>> I generated git logs of all such commits, which might help us get some
>>> idea about mailing list or sign offs as well.
>>>
>>> Acked-by:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/acked_by.txt
>>>
>>> Reported-by:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/reported_by.txt
>>>
>>> Reviewed by:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/reviewed_by.txt
>>>
>>> Tested-by:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/tested_by.txt
>>>
>>> Cc: https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/cc.txt
>>>
>>>
>>>>> I observed one of the 'Tested-by' warnings and it was because of these
>>>>> sign offs:
>>>>>
>>>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>>>> Tested-by:Liang Yang <liang.yang@amlogic.com>
>>>>> Acked-by: Liang Yang <liang.yang@amlogic.com>
>>>>> Tested-by:Liang Yang <liang.yang@amlogic.com>
>>>>> Acked-by: Liang Yang <liang.yang@amlogic.com>
>>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>>
>>>>> Here tested-by is occurring 2 times.
>>>>>
>>>>
>>>> That example seems to be a clear true positive. It would be
>>>> interesting to see how that happened to be added twice in a commit.
>>>> What was the state on the mailing list and what did the maintainer do
>>>> to add these tags twice (was this done manually or is there some
>>>> automatic tool involved?).
>>>>
>>>
>>> I found the mailing list state corresponding to that day. It indeed
>>> seems like a true positive.
>>>
>>> Link1:
>>> https://lore.kernel.org/linux-mtd/CAFBinCB9Z_H4wX3_g2hVL-XnS6Oq-KtPHa4Z1ws79FGp5jCapQ@mail.gmail.com/
>>>
>>> Link2 (reply):
>>> https://lore.kernel.org/linux-mtd/8ff232c3-f325-5b91-4de1-a39e63939df2@amlogic.com/
>>>
>>
>> Yes, and it suggest to fix patchwork :)
>>
>> How are your python programming skills?
>>
>> Do you want to try fixing patchwork for this issue?
>>
>> Lukas
>>
> 
> Sure, I'd like to try. Please guide me for it.
> 

I might have got it wrong. Did you mean fixing the patch we are
working on currently?
I must be intermediate with Python, not too expert though(like 3.5/5)

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: add fixes for BAD_SIGN_OFF

[Lukas Bulwahn]

On Wed, Nov 4, 2020 at 10:29 AM Aditya <yashsri421@gmail.com> wrote:
>
> On 4/11/20 1:17 pm, Aditya wrote:
> > On 4/11/20 12:27 pm, Lukas Bulwahn wrote:
> >>
> >>
> >> On Tue, 3 Nov 2020, Aditya wrote:
> >>
> >>> On 3/11/20 9:04 pm, Lukas Bulwahn wrote:
> >>>> On Tue, Nov 3, 2020 at 3:47 PM Aditya <yashsri421@gmail.com> wrote:
> >>>>>
> >>>>> On 3/11/20 7:09 pm, Lukas Bulwahn wrote:
> >>>>>> On Tue, Nov 3, 2020 at 2:31 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
> >>>>>>>
> >>>>>>> Currently, checkpatch warns if the author signs-off two or more times
> >>>>>>> for the same role.
> >>>>>>> E.g., for commit 6dd47d9754ff ("mac80211: fix
> >>>>>>> ieee80211_txq_setup_flows() failure path") we get warning:
> >>>>>>>
> >>>>>>> WARNING: Duplicate signature
> >>>>>>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> >>>>>>>
> >>>>>>> Here there are two Signed-off-by lines present for the same user. So,
> >>>>>>> we get a warning to remove one.
> >>>>>>>
> >>>>>>> Similarly, we get warning if the author of the commit signs-off under
> >>>>>>> co-developed-by.
> >>>>>>> E.g. for commit 6e88559470f5 ("Documentation: Add section about
> >>>>>>> CPU vulnerabilities for Spectre") we get:
> >>>>>>>
> >>>>>>> WARNING: Co-developed-by: should not be used to attribute nominal
> >>>>>>> patch author 'Tim Chen <tim.c.chen@linux.intel.com>'
> >>>>>>> Co-developed-by: Tim Chen <tim.c.chen@linux.intel.com>
> >>>>>>>
> >>>>>>> Provide fixes by removing the duplicate signature line and the
> >>>>>>> co-developed-by line from the commit
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>> The duplicate signature fix is simply wrong, because the indication of
> >>>>>> the duplicate signature is imprecise.
> >>>>>>
> >>>>>> Probably, we would first really need to understand the valid cases of
> >>>>>> duplicate signature warnings compared to the invalid ones.
> >>>>>>
> >>>>>> Please rework.
> >>>>>>
> >>>>>>
> >>>>>> Lukas
> >>>>>>
> >>>>>
> >>>>> Oh, Alright. I get it now.
> >>>>> I ran a script over WARNING: Duplicate signature.
> >>>>> I found out that it is occurring with these prefixes and their
> >>>>> frequencies:
> >>>>>
> >>>>> Acked-by -- 6
> >>>>> Signed-off-by -- 218
> >>>>> Tested-by -- 3
> >>>>> Reported-by -- 1
> >>>>> Reviewed-by -- 8
> >>>>> Cc -- 28
> >>>>>
> >>>>> IMO largely Signed-off-by warnings in this frequency might be false
> >>>>> positives. This is so because we are simply checking for duplicate
> >>>>> occurrences in the sign-offs in checkpatch.pl
> >>>>>
> >>>>>> Probably, we would first really need to understand the valid cases
> >>>>>> of duplicate signature warnings compared to the invalid ones.
> >>>>>>
> >>>>>
> >>>>> Is there any way I can know that the second sign-off is intended?
> >>>>>
> >>>>> In my observation, this particular commit was prefixed by '^Link:.*$'
> >>>>> Alternatively, we could use maintainers list to ensure that the signer
> >>>>> is maintainer and avoid this warning.
> >>>>>
> >>>>
> >>>> That is somehow difficult to say, but probably it is best to
> >>>> deactivate this multiple Signed-off-by: warning on git commits.
> >>>>
> >>>> On patches it makes sense to warn about duplicate Signed-off-by: ; on
> >>>> commits, not so much.
> >>>>
> >>>>> How about other prefixes. Can we remove them?
> >>>>>
> >>>>
> >>>> Can you show the duplicate Tested-by, Acked-by, Reported-by,
> >>>> Reviewed-by, Cc in some structured format to quickly check those?
> >>>>
> >>>> Probably for those, we can really assess them to be all true positives
> >>>> (maybe we can find out the reasons as well) and hence a fix for them
> >>>> would make sense.
> >>>>
> >>>
> >>> I generated git logs of all such commits, which might help us get some
> >>> idea about mailing list or sign offs as well.
> >>>
> >>> Acked-by:
> >>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/acked_by.txt
> >>>
> >>> Reported-by:
> >>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/reported_by.txt
> >>>
> >>> Reviewed by:
> >>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/reviewed_by.txt
> >>>
> >>> Tested-by:
> >>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/tested_by.txt
> >>>
> >>> Cc: https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/cc.txt
> >>>
> >>>
> >>>>> I observed one of the 'Tested-by' warnings and it was because of these
> >>>>> sign offs:
> >>>>>
> >>>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> >>>>> Tested-by:Liang Yang <liang.yang@amlogic.com>
> >>>>> Acked-by: Liang Yang <liang.yang@amlogic.com>
> >>>>> Tested-by:Liang Yang <liang.yang@amlogic.com>
> >>>>> Acked-by: Liang Yang <liang.yang@amlogic.com>
> >>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >>>>>
> >>>>> Here tested-by is occurring 2 times.
> >>>>>
> >>>>
> >>>> That example seems to be a clear true positive. It would be
> >>>> interesting to see how that happened to be added twice in a commit.
> >>>> What was the state on the mailing list and what did the maintainer do
> >>>> to add these tags twice (was this done manually or is there some
> >>>> automatic tool involved?).
> >>>>
> >>>
> >>> I found the mailing list state corresponding to that day. It indeed
> >>> seems like a true positive.
> >>>
> >>> Link1:
> >>> https://lore.kernel.org/linux-mtd/CAFBinCB9Z_H4wX3_g2hVL-XnS6Oq-KtPHa4Z1ws79FGp5jCapQ@mail.gmail.com/
> >>>
> >>> Link2 (reply):
> >>> https://lore.kernel.org/linux-mtd/8ff232c3-f325-5b91-4de1-a39e63939df2@amlogic.com/
> >>>
> >>
> >> Yes, and it suggest to fix patchwork :)
> >>
> >> How are your python programming skills?
> >>
> >> Do you want to try fixing patchwork for this issue?
> >>
> >> Lukas
> >>
> >
> > Sure, I'd like to try. Please guide me for it.
> >
>
> I might have got it wrong. Did you mean fixing the patch we are
> working on currently?
> I must be intermediate with Python, not too expert though(like 3.5/5)
>

No, I meant the program patchwork; read the thread and learn how the
problem was picked up in this patch.

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