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

Aditya yashsri421 at gmail.com
Tue Nov 3 14:47:22 UTC 2020


On 3/11/20 7:09 pm, Lukas Bulwahn wrote:
> On Tue, Nov 3, 2020 at 2:31 PM Aditya Srivastava <yashsri421 at 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 at 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 at linux.intel.com>'
>> Co-developed-by: Tim Chen <tim.c.chen at 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 at googlemail.com>
Tested-by:Liang Yang <liang.yang at amlogic.com>
Acked-by: Liang Yang <liang.yang at amlogic.com>
Tested-by:Liang Yang <liang.yang at amlogic.com>
Acked-by: Liang Yang <liang.yang at amlogic.com>
Signed-off-by: Miquel Raynal <miquel.raynal at 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


More information about the Linux-kernel-mentees mailing list