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

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

[Aditya Srivastava]

Currently checkpatch warns us if there is no 'Signed-off-by' line
for the patch.

E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
Completely remove --version flag") reports this error:

ERROR: Missing Signed-off-by: line(s)

Provide a fix by adding a Signed-off-by line corresponding to the author
of the patch before the patch separator line. Also avoid this error for
the commits where some typo is present in the sign off.

E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers
for quirk") we get missing sign off as well as bad sign off for:

Siganed-off-by: Tony Lindgren <tony@atomide.com>

Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
signature and avoid MISSING_SIGN_OFF

Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
This patch was made after applying https://lore.kernel.org/linux-kernel-mentees/20201108100632.75340-1-dwaipayanray1@gmail.com/
and my last changes at https://lore.kernel.org/linux-kernel-mentees/20201108134317.25400-1-yashsri421@gmail.com/

 scripts/checkpatch.pl | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index cb46288127ac..2deffd0c091b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2404,6 +2404,8 @@ sub process {
 
 	my $last_blank_line = 0;
 	my $last_coalesced_string_linenr = -1;
+	my $patch_separator_linenr = 0;
+	my $non_standard_signature = 0;
 
 	our @report = ();
 	our $cnt_lines = 0;
@@ -2755,6 +2757,10 @@ sub process {
 		if ($line =~ /^---$/) {
 			$has_patch_separator = 1;
 			$in_commit_log = 0;
+			# to add missing sign off line before diff(s)
+			if($patch_separator_linenr == 0) {
+				$patch_separator_linenr = $linenr;
+			}
 		}
 
 # Check if MAINTAINERS is being updated.  If so, there's probably no need to
@@ -2775,6 +2781,9 @@ sub process {
 			if ($sign_off !~ /$signature_tags/) {
 				WARN("BAD_SIGN_OFF",
 				     "Non-standard signature: $sign_off\n" . $herecurr);
+
+				# to avoid missing_sign_off error as it most probably is just a typo
+				$non_standard_signature = 1;
 			}
 			if (defined $space_before && $space_before ne "") {
 				if (WARN("BAD_SIGN_OFF",
@@ -7118,9 +7127,12 @@ sub process {
 		      "Does not appear to be a unified-diff format patch\n");
 	}
 	if ($is_patch && $has_commit_log && $chk_signoff) {
-		if ($signoff == 0) {
-			ERROR("MISSING_SIGN_OFF",
-			      "Missing Signed-off-by: line(s)\n");
+		if ($signoff == 0 && !$non_standard_signature) {
+			if (ERROR("MISSING_SIGN_OFF",
+				  "Missing Signed-off-by: line(s)\n") &&
+			    $fix) {
+				fix_insert_line($patch_separator_linenr - 1, "Signed-off-by: $author");
+			}
 		} elsif ($authorsignoff != 1) {
 			# authorsignoff values:
 			# 0 -> missing 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 fix option for MISSING_SIGN_OFF

[Aditya]

On 10/11/20 6:36 pm, Aditya Srivastava wrote:
> Currently checkpatch warns us if there is no 'Signed-off-by' line
> for the patch.
> 
> E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
> Completely remove --version flag") reports this error:
> 
> ERROR: Missing Signed-off-by: line(s)
> 
> Provide a fix by adding a Signed-off-by line corresponding to the author
> of the patch before the patch separator line. Also avoid this error for
> the commits where some typo is present in the sign off.
> 
> E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers
> for quirk") we get missing sign off as well as bad sign off for:
> 
> Siganed-off-by: Tony Lindgren <tony@atomide.com>
> 
> Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
> signature and avoid MISSING_SIGN_OFF
> 
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
> This patch was made after applying https://lore.kernel.org/linux-kernel-mentees/20201108100632.75340-1-dwaipayanray1@gmail.com/
> and my last changes at https://lore.kernel.org/linux-kernel-mentees/20201108134317.25400-1-yashsri421@gmail.com/
> 
>  scripts/checkpatch.pl | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index cb46288127ac..2deffd0c091b 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2404,6 +2404,8 @@ sub process {
>  
>  	my $last_blank_line = 0;
>  	my $last_coalesced_string_linenr = -1;
> +	my $patch_separator_linenr = 0;
> +	my $non_standard_signature = 0;
>  
>  	our @report = ();
>  	our $cnt_lines = 0;
> @@ -2755,6 +2757,10 @@ sub process {
>  		if ($line =~ /^---$/) {
>  			$has_patch_separator = 1;
>  			$in_commit_log = 0;
> +			# to add missing sign off line before diff(s)
> +			if($patch_separator_linenr == 0) {
> +				$patch_separator_linenr = $linenr;
> +			}
>  		}
>  
>  # Check if MAINTAINERS is being updated.  If so, there's probably no need to
> @@ -2775,6 +2781,9 @@ sub process {
>  			if ($sign_off !~ /$signature_tags/) {
>  				WARN("BAD_SIGN_OFF",
>  				     "Non-standard signature: $sign_off\n" . $herecurr);
> +
> +				# to avoid missing_sign_off error as it most probably is just a typo
> +				$non_standard_signature = 1;
>  			}
>  			if (defined $space_before && $space_before ne "") {
>  				if (WARN("BAD_SIGN_OFF",
> @@ -7118,9 +7127,12 @@ sub process {
>  		      "Does not appear to be a unified-diff format patch\n");
>  	}
>  	if ($is_patch && $has_commit_log && $chk_signoff) {
> -		if ($signoff == 0) {
> -			ERROR("MISSING_SIGN_OFF",
> -			      "Missing Signed-off-by: line(s)\n");
> +		if ($signoff == 0 && !$non_standard_signature) {
> +			if (ERROR("MISSING_SIGN_OFF",
> +				  "Missing Signed-off-by: line(s)\n") &&
> +			    $fix) {
> +				fix_insert_line($patch_separator_linenr - 1, "Signed-off-by: $author");
> +			}
>  		} elsif ($authorsignoff != 1) {
>  			# authorsignoff values:
>  			# 0 -> missing sign off
> 

Here I have changed $fixlinenr to $linenr and later reduced
$patch_separator_linenr by 1, at the time of inserting the line.

I changed it to go along with the variable name, ie
$patch_separator_linenr, earlier it was storing the line number before it.

It produces the same outcome in either way. Verified it on patches 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] checkpatch: add fix option for MISSING_SIGN_OFF

[Lukas Bulwahn]

On Tue, Nov 10, 2020 at 2:06 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
>
> Currently checkpatch warns us if there is no 'Signed-off-by' line
> for the patch.
>
> E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
> Completely remove --version flag") reports this error:
>
> ERROR: Missing Signed-off-by: line(s)
>
> Provide a fix by adding a Signed-off-by line corresponding to the author
> of the patch before the patch separator line. Also avoid this error for
> the commits where some typo is present in the sign off.
>
> E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers
> for quirk") we get missing sign off as well as bad sign off for:
>
> Siganed-off-by: Tony Lindgren <tony@atomide.com>
>
> Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
> signature and avoid MISSING_SIGN_OFF
>
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
> This patch was made after applying https://lore.kernel.org/linux-kernel-mentees/20201108100632.75340-1-dwaipayanray1@gmail.com/
> and my last changes at https://lore.kernel.org/linux-kernel-mentees/20201108134317.25400-1-yashsri421@gmail.com/
>
>  scripts/checkpatch.pl | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index cb46288127ac..2deffd0c091b 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2404,6 +2404,8 @@ sub process {
>
>         my $last_blank_line = 0;
>         my $last_coalesced_string_linenr = -1;
> +       my $patch_separator_linenr = 0;
> +       my $non_standard_signature = 0;
>
>         our @report = ();
>         our $cnt_lines = 0;
> @@ -2755,6 +2757,10 @@ sub process {
>                 if ($line =~ /^---$/) {
>                         $has_patch_separator = 1;
>                         $in_commit_log = 0;
> +                       # to add missing sign off line before diff(s)
> +                       if($patch_separator_linenr == 0) {
> +                               $patch_separator_linenr = $linenr;

Well are you sure linenr is correct? (As I wrote, I do not know, but
you should know.)

I think you would need to create a patch for testing your feature
where multiple things are fixed within the same patch and then check
if the fix is added at the right place.

> +                       }
>                 }
>
>  # Check if MAINTAINERS is being updated.  If so, there's probably no need to
> @@ -2775,6 +2781,9 @@ sub process {
>                         if ($sign_off !~ /$signature_tags/) {
>                                 WARN("BAD_SIGN_OFF",
>                                      "Non-standard signature: $sign_off\n" . $herecurr);
> +
> +                               # to avoid missing_sign_off error as it most probably is just a typo
> +                               $non_standard_signature = 1;

The variable name now sounds much better.

>                         }
>                         if (defined $space_before && $space_before ne "") {
>                                 if (WARN("BAD_SIGN_OFF",
> @@ -7118,9 +7127,12 @@ sub process {
>                       "Does not appear to be a unified-diff format patch\n");
>         }
>         if ($is_patch && $has_commit_log && $chk_signoff) {
> -               if ($signoff == 0) {
> -                       ERROR("MISSING_SIGN_OFF",
> -                             "Missing Signed-off-by: line(s)\n");
> +               if ($signoff == 0 && !$non_standard_signature) {
> +                       if (ERROR("MISSING_SIGN_OFF",
> +                                 "Missing Signed-off-by: line(s)\n") &&
> +                           $fix) {
> +                               fix_insert_line($patch_separator_linenr - 1, "Signed-off-by: $author");
> +                       }
>                 } elsif ($authorsignoff != 1) {
>                         # authorsignoff values:
>                         # 0 -> missing 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 fix option for MISSING_SIGN_OFF

[Aditya]

On 10/11/20 7:30 pm, Lukas Bulwahn wrote:
> On Tue, Nov 10, 2020 at 2:06 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
>>
>> Currently checkpatch warns us if there is no 'Signed-off-by' line
>> for the patch.
>>
>> E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
>> Completely remove --version flag") reports this error:
>>
>> ERROR: Missing Signed-off-by: line(s)
>>
>> Provide a fix by adding a Signed-off-by line corresponding to the author
>> of the patch before the patch separator line. Also avoid this error for
>> the commits where some typo is present in the sign off.
>>
>> E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers
>> for quirk") we get missing sign off as well as bad sign off for:
>>
>> Siganed-off-by: Tony Lindgren <tony@atomide.com>
>>
>> Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
>> signature and avoid MISSING_SIGN_OFF
>>
>> Suggested-by: Joe Perches <joe@perches.com>
>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
>> ---
>> This patch was made after applying https://lore.kernel.org/linux-kernel-mentees/20201108100632.75340-1-dwaipayanray1@gmail.com/
>> and my last changes at https://lore.kernel.org/linux-kernel-mentees/20201108134317.25400-1-yashsri421@gmail.com/
>>
>>  scripts/checkpatch.pl | 18 +++++++++++++++---
>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index cb46288127ac..2deffd0c091b 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -2404,6 +2404,8 @@ sub process {
>>
>>         my $last_blank_line = 0;
>>         my $last_coalesced_string_linenr = -1;
>> +       my $patch_separator_linenr = 0;
>> +       my $non_standard_signature = 0;
>>
>>         our @report = ();
>>         our $cnt_lines = 0;
>> @@ -2755,6 +2757,10 @@ sub process {
>>                 if ($line =~ /^---$/) {
>>                         $has_patch_separator = 1;
>>                         $in_commit_log = 0;
>> +                       # to add missing sign off line before diff(s)
>> +                       if($patch_separator_linenr == 0) {
>> +                               $patch_separator_linenr = $linenr;
> 
> Well are you sure linenr is correct? (As I wrote, I do not know, but
> you should know.)
> 
> I think you would need to create a patch for testing your feature
> where multiple things are fixed within the same patch and then check
> if the fix is added at the right place.
> 

I checked the fix over following patch.

I have modified the patch such that it has co-developed-by as the
author(nominal patch author warning, which requires this line to be
deleted)

Also 'occuring', which is a typo(where the typo needs to be replaced)

It works as expected.

This is the modified patch I used for testing:

From f68e7927212fa0dbe44c00c144b643c87ab0cf43 Mon Sep 17 00:00:00 2001
From: Michael Ellerman <mpe@ellerman.id.au>
Date: Sat, 23 Feb 2019 20:30:50 +1100
Subject: [PATCH] Revert "powerpc/book3s32: Reorder _PAGE_XXX flags to
simplify
 TLB handling"

This reverts commit 78ca1108b10927b3d068c8da91352b0f4cd01fc5.

It is causing boot failures with qemu mac99 in at least some
configurations.occuring
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/book3s/32/hash.h | 8 ++++----
 arch/powerpc/kernel/head_32.S             | 5 ++++-
 arch/powerpc/mm/hash_low_32.S             | 6 ++++--
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/32/hash.h
b/arch/powerpc/include/asm/book3s/32/hash.h
index a5907ea4fb40..2a0a467d2985 100644
--- a/arch/powerpc/include/asm/book3s/32/hash.h
+++ b/arch/powerpc/include/asm/book3s/32/hash.h
@@ -17,9 +17,9 @@
  * updating the accessed and modified bits in the page table tree.
  */

-#define _PAGE_RW	0x001	/* PP = x1: user write access allowed */
-#define _PAGE_USER	0x002	/* PP = 1x: usermode access allowed */
-#define _PAGE_HASHPTE	0x004	/* software: hash_page has made an HPTE
for this pte */
+#define _PAGE_PRESENT	0x001	/* software: pte contains a translation */
+#define _PAGE_HASHPTE	0x002	/* hash_page has made an HPTE for this pte */
+#define _PAGE_USER	0x004	/* usermode access allowed */
 #define _PAGE_GUARDED	0x008	/* G: prohibit speculative access */
 #define _PAGE_COHERENT	0x010	/* M: enforce memory coherence (SMP
systems) */
 #define _PAGE_NO_CACHE	0x020	/* I: cache inhibit */
@@ -27,7 +27,7 @@
 #define _PAGE_DIRTY	0x080	/* C: page changed */
 #define _PAGE_ACCESSED	0x100	/* R: page referenced */
 #define _PAGE_EXEC	0x200	/* software: exec allowed */
-#define _PAGE_PRESENT	0x400	/* software: pte contains a translation */
+#define _PAGE_RW	0x400	/* software: user write access allowed */
 #define _PAGE_SPECIAL	0x800	/* software: Special page */

 #ifdef CONFIG_PTE_64BIT
diff --git a/arch/powerpc/kernel/head_32.S b/arch/powerpc/kernel/head_32.S
index e7a5b312a7db..fdb587c96a80 100644
--- a/arch/powerpc/kernel/head_32.S
+++ b/arch/powerpc/kernel/head_32.S
@@ -522,6 +522,7 @@ InstructionTLBMiss:
 	andc.	r1,r1,r0		/* check access & ~permission */
 	bne-	InstructionAddressInvalid /* return if access not permitted */
 	/* Convert linux-style PTE to low word of PPC-style PTE */
+	rlwimi	r0,r0,32-1,30,30	/* _PAGE_USER -> PP msb */
 	ori	r1, r1, 0xe05		/* clear out reserved bits */
 	andc	r1, r0, r1		/* PP = user? 2 : 0 */
 BEGIN_FTR_SECTION
@@ -589,7 +590,8 @@ DataLoadTLBMiss:
 	 * we would need to update the pte atomically with lwarx/stwcx.
 	 */
 	/* Convert linux-style PTE to low word of PPC-style PTE */
-	rlwinm	r1, r0, 0, 31, 31	/* _PAGE_RW -> PP lsb */
+	rlwinm	r1,r0,32-10,31,31	/* _PAGE_RW -> PP lsb */
+	rlwimi	r0,r0,32-1,30,30	/* _PAGE_USER -> PP msb */
 	rlwimi	r0,r0,32-1,31,31	/* _PAGE_USER -> PP lsb */
 	ori	r1,r1,0xe04		/* clear out reserved bits */
 	andc	r1,r0,r1		/* PP = user? rw? 2: 3: 0 */
@@ -668,6 +670,7 @@ DataStoreTLBMiss:
 	 * we would need to update the pte atomically with lwarx/stwcx.
 	 */
 	/* Convert linux-style PTE to low word of PPC-style PTE */
+	rlwimi	r0,r0,32-1,30,30	/* _PAGE_USER -> PP msb */
 	li	r1,0xe05		/* clear out reserved bits & PP lsb */
 	andc	r1,r0,r1		/* PP = user? 2: 0 */
 BEGIN_FTR_SECTION
diff --git a/arch/powerpc/mm/hash_low_32.S b/arch/powerpc/mm/hash_low_32.S
index f4294edeca9d..d94fef524ef5 100644
--- a/arch/powerpc/mm/hash_low_32.S
+++ b/arch/powerpc/mm/hash_low_32.S
@@ -310,9 +310,11 @@ Hash_msk = (((1 << Hash_bits) - 1) * 64)

 _GLOBAL(create_hpte)
 	/* Convert linux-style PTE (r5) to low word of PPC-style PTE (r8) */
+	rlwinm	r8,r5,32-10,31,31	/* _PAGE_RW -> PP lsb */
 	rlwinm	r0,r5,32-7,31,31	/* _PAGE_DIRTY -> PP lsb */
-	and	r8, r5, r0		/* writable if _RW & _DIRTY */
-	rlwimi	r5, r5, 32 - 1, 31, 31	/* _PAGE_USER -> PP lsb */
+	and	r8,r8,r0		/* writable if _RW & _DIRTY */
+	rlwimi	r5,r5,32-1,30,30	/* _PAGE_USER -> PP msb */
+	rlwimi	r5,r5,32-2,31,31	/* _PAGE_USER -> PP lsb */
 	ori	r8,r8,0xe04		/* clear out reserved bits */
 	andc	r8,r5,r8		/* PP = user? (rw&dirty? 2: 3): 0 */
 BEGIN_FTR_SECTION
-- 
2.17.1


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 fix option for MISSING_SIGN_OFF

[Lukas Bulwahn]

On Tue, Nov 10, 2020 at 4:13 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 10/11/20 7:30 pm, Lukas Bulwahn wrote:
> > On Tue, Nov 10, 2020 at 2:06 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
> >>
> >> Currently checkpatch warns us if there is no 'Signed-off-by' line
> >> for the patch.
> >>
> >> E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
> >> Completely remove --version flag") reports this error:
> >>
> >> ERROR: Missing Signed-off-by: line(s)
> >>
> >> Provide a fix by adding a Signed-off-by line corresponding to the author
> >> of the patch before the patch separator line. Also avoid this error for
> >> the commits where some typo is present in the sign off.
> >>
> >> E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers
> >> for quirk") we get missing sign off as well as bad sign off for:
> >>
> >> Siganed-off-by: Tony Lindgren <tony@atomide.com>
> >>
> >> Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
> >> signature and avoid MISSING_SIGN_OFF
> >>
> >> Suggested-by: Joe Perches <joe@perches.com>
> >> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> >> ---
> >> This patch was made after applying https://lore.kernel.org/linux-kernel-mentees/20201108100632.75340-1-dwaipayanray1@gmail.com/
> >> and my last changes at https://lore.kernel.org/linux-kernel-mentees/20201108134317.25400-1-yashsri421@gmail.com/
> >>
> >>  scripts/checkpatch.pl | 18 +++++++++++++++---
> >>  1 file changed, 15 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >> index cb46288127ac..2deffd0c091b 100755
> >> --- a/scripts/checkpatch.pl
> >> +++ b/scripts/checkpatch.pl
> >> @@ -2404,6 +2404,8 @@ sub process {
> >>
> >>         my $last_blank_line = 0;
> >>         my $last_coalesced_string_linenr = -1;
> >> +       my $patch_separator_linenr = 0;
> >> +       my $non_standard_signature = 0;
> >>
> >>         our @report = ();
> >>         our $cnt_lines = 0;
> >> @@ -2755,6 +2757,10 @@ sub process {
> >>                 if ($line =~ /^---$/) {
> >>                         $has_patch_separator = 1;
> >>                         $in_commit_log = 0;
> >> +                       # to add missing sign off line before diff(s)
> >> +                       if($patch_separator_linenr == 0) {
> >> +                               $patch_separator_linenr = $linenr;
> >
> > Well are you sure linenr is correct? (As I wrote, I do not know, but
> > you should know.)
> >
> > I think you would need to create a patch for testing your feature
> > where multiple things are fixed within the same patch and then check
> > if the fix is added at the right place.
> >
>
> I checked the fix over following patch.
>
> I have modified the patch such that it has co-developed-by as the
> author(nominal patch author warning, which requires this line to be
> deleted)
>
> Also 'occuring', which is a typo(where the typo needs to be replaced)
>
> It works as expected.
>

Well to really check if it works as expected, you need to have a test
patch where a fix adds multiple lines into the fixed patch before the
line you try to record.

Then, fixlinenr and linenr are probably more than just 1 line apart,
but really multiple lines apart and only if you record the right
variable it really works.

Other than that, it is all good.

Lukas

> This is the modified patch I used for testing:
>
> From f68e7927212fa0dbe44c00c144b643c87ab0cf43 Mon Sep 17 00:00:00 2001
> From: Michael Ellerman <mpe@ellerman.id.au>
> Date: Sat, 23 Feb 2019 20:30:50 +1100
> Subject: [PATCH] Revert "powerpc/book3s32: Reorder _PAGE_XXX flags to
> simplify
>  TLB handling"
>
> This reverts commit 78ca1108b10927b3d068c8da91352b0f4cd01fc5.
>
> It is causing boot failures with qemu mac99 in at least some
> configurations.occuring
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/include/asm/book3s/32/hash.h | 8 ++++----
>  arch/powerpc/kernel/head_32.S             | 5 ++++-
>  arch/powerpc/mm/hash_low_32.S             | 6 ++++--
>  3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/32/hash.h
> b/arch/powerpc/include/asm/book3s/32/hash.h
> index a5907ea4fb40..2a0a467d2985 100644
> --- a/arch/powerpc/include/asm/book3s/32/hash.h
> +++ b/arch/powerpc/include/asm/book3s/32/hash.h
> @@ -17,9 +17,9 @@
>   * updating the accessed and modified bits in the page table tree.
>   */
>
> -#define _PAGE_RW       0x001   /* PP = x1: user write access allowed */
> -#define _PAGE_USER     0x002   /* PP = 1x: usermode access allowed */
> -#define _PAGE_HASHPTE  0x004   /* software: hash_page has made an HPTE
> for this pte */
> +#define _PAGE_PRESENT  0x001   /* software: pte contains a translation */
> +#define _PAGE_HASHPTE  0x002   /* hash_page has made an HPTE for this pte */
> +#define _PAGE_USER     0x004   /* usermode access allowed */
>  #define _PAGE_GUARDED  0x008   /* G: prohibit speculative access */
>  #define _PAGE_COHERENT 0x010   /* M: enforce memory coherence (SMP
> systems) */
>  #define _PAGE_NO_CACHE 0x020   /* I: cache inhibit */
> @@ -27,7 +27,7 @@
>  #define _PAGE_DIRTY    0x080   /* C: page changed */
>  #define _PAGE_ACCESSED 0x100   /* R: page referenced */
>  #define _PAGE_EXEC     0x200   /* software: exec allowed */
> -#define _PAGE_PRESENT  0x400   /* software: pte contains a translation */
> +#define _PAGE_RW       0x400   /* software: user write access allowed */
>  #define _PAGE_SPECIAL  0x800   /* software: Special page */
>
>  #ifdef CONFIG_PTE_64BIT
> diff --git a/arch/powerpc/kernel/head_32.S b/arch/powerpc/kernel/head_32.S
> index e7a5b312a7db..fdb587c96a80 100644
> --- a/arch/powerpc/kernel/head_32.S
> +++ b/arch/powerpc/kernel/head_32.S
> @@ -522,6 +522,7 @@ InstructionTLBMiss:
>         andc.   r1,r1,r0                /* check access & ~permission */
>         bne-    InstructionAddressInvalid /* return if access not permitted */
>         /* Convert linux-style PTE to low word of PPC-style PTE */
> +       rlwimi  r0,r0,32-1,30,30        /* _PAGE_USER -> PP msb */
>         ori     r1, r1, 0xe05           /* clear out reserved bits */
>         andc    r1, r0, r1              /* PP = user? 2 : 0 */
>  BEGIN_FTR_SECTION
> @@ -589,7 +590,8 @@ DataLoadTLBMiss:
>          * we would need to update the pte atomically with lwarx/stwcx.
>          */
>         /* Convert linux-style PTE to low word of PPC-style PTE */
> -       rlwinm  r1, r0, 0, 31, 31       /* _PAGE_RW -> PP lsb */
> +       rlwinm  r1,r0,32-10,31,31       /* _PAGE_RW -> PP lsb */
> +       rlwimi  r0,r0,32-1,30,30        /* _PAGE_USER -> PP msb */
>         rlwimi  r0,r0,32-1,31,31        /* _PAGE_USER -> PP lsb */
>         ori     r1,r1,0xe04             /* clear out reserved bits */
>         andc    r1,r0,r1                /* PP = user? rw? 2: 3: 0 */
> @@ -668,6 +670,7 @@ DataStoreTLBMiss:
>          * we would need to update the pte atomically with lwarx/stwcx.
>          */
>         /* Convert linux-style PTE to low word of PPC-style PTE */
> +       rlwimi  r0,r0,32-1,30,30        /* _PAGE_USER -> PP msb */
>         li      r1,0xe05                /* clear out reserved bits & PP lsb */
>         andc    r1,r0,r1                /* PP = user? 2: 0 */
>  BEGIN_FTR_SECTION
> diff --git a/arch/powerpc/mm/hash_low_32.S b/arch/powerpc/mm/hash_low_32.S
> index f4294edeca9d..d94fef524ef5 100644
> --- a/arch/powerpc/mm/hash_low_32.S
> +++ b/arch/powerpc/mm/hash_low_32.S
> @@ -310,9 +310,11 @@ Hash_msk = (((1 << Hash_bits) - 1) * 64)
>
>  _GLOBAL(create_hpte)
>         /* Convert linux-style PTE (r5) to low word of PPC-style PTE (r8) */
> +       rlwinm  r8,r5,32-10,31,31       /* _PAGE_RW -> PP lsb */
>         rlwinm  r0,r5,32-7,31,31        /* _PAGE_DIRTY -> PP lsb */
> -       and     r8, r5, r0              /* writable if _RW & _DIRTY */
> -       rlwimi  r5, r5, 32 - 1, 31, 31  /* _PAGE_USER -> PP lsb */
> +       and     r8,r8,r0                /* writable if _RW & _DIRTY */
> +       rlwimi  r5,r5,32-1,30,30        /* _PAGE_USER -> PP msb */
> +       rlwimi  r5,r5,32-2,31,31        /* _PAGE_USER -> PP lsb */
>         ori     r8,r8,0xe04             /* clear out reserved bits */
>         andc    r8,r5,r8                /* PP = user? (rw&dirty? 2: 3): 0 */
>  BEGIN_FTR_SECTION
> --
> 2.17.1
>
>
> 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 fix option for MISSING_SIGN_OFF

[Aditya]

On 10/11/20 8:52 pm, Lukas Bulwahn wrote:
> On Tue, Nov 10, 2020 at 4:13 PM Aditya <yashsri421@gmail.com> wrote:
>>
>> On 10/11/20 7:30 pm, Lukas Bulwahn wrote:
>>> On Tue, Nov 10, 2020 at 2:06 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
>>>>
>>>> Currently checkpatch warns us if there is no 'Signed-off-by' line
>>>> for the patch.
>>>>
>>>> E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
>>>> Completely remove --version flag") reports this error:
>>>>
>>>> ERROR: Missing Signed-off-by: line(s)
>>>>
>>>> Provide a fix by adding a Signed-off-by line corresponding to the author
>>>> of the patch before the patch separator line. Also avoid this error for
>>>> the commits where some typo is present in the sign off.
>>>>
>>>> E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers
>>>> for quirk") we get missing sign off as well as bad sign off for:
>>>>
>>>> Siganed-off-by: Tony Lindgren <tony@atomide.com>
>>>>
>>>> Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
>>>> signature and avoid MISSING_SIGN_OFF
>>>>
>>>> Suggested-by: Joe Perches <joe@perches.com>
>>>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
>>>> ---
>>>> This patch was made after applying https://lore.kernel.org/linux-kernel-mentees/20201108100632.75340-1-dwaipayanray1@gmail.com/
>>>> and my last changes at https://lore.kernel.org/linux-kernel-mentees/20201108134317.25400-1-yashsri421@gmail.com/
>>>>
>>>>  scripts/checkpatch.pl | 18 +++++++++++++++---
>>>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>>> index cb46288127ac..2deffd0c091b 100755
>>>> --- a/scripts/checkpatch.pl
>>>> +++ b/scripts/checkpatch.pl
>>>> @@ -2404,6 +2404,8 @@ sub process {
>>>>
>>>>         my $last_blank_line = 0;
>>>>         my $last_coalesced_string_linenr = -1;
>>>> +       my $patch_separator_linenr = 0;
>>>> +       my $non_standard_signature = 0;
>>>>
>>>>         our @report = ();
>>>>         our $cnt_lines = 0;
>>>> @@ -2755,6 +2757,10 @@ sub process {
>>>>                 if ($line =~ /^---$/) {
>>>>                         $has_patch_separator = 1;
>>>>                         $in_commit_log = 0;
>>>> +                       # to add missing sign off line before diff(s)
>>>> +                       if($patch_separator_linenr == 0) {
>>>> +                               $patch_separator_linenr = $linenr;
>>>
>>> Well are you sure linenr is correct? (As I wrote, I do not know, but
>>> you should know.)
>>>
>>> I think you would need to create a patch for testing your feature
>>> where multiple things are fixed within the same patch and then check
>>> if the fix is added at the right place.
>>>
>>
>> I checked the fix over following patch.
>>
>> I have modified the patch such that it has co-developed-by as the
>> author(nominal patch author warning, which requires this line to be
>> deleted)
>>
>> Also 'occuring', which is a typo(where the typo needs to be replaced)
>>
>> It works as expected.
>>
> 
> Well to really check if it works as expected, you need to have a test
> patch where a fix adds multiple lines into the fixed patch before the
> line you try to record.
> 
> Then, fixlinenr and linenr are probably more than just 1 line apart,
> but really multiple lines apart and only if you record the right
> variable it really works.
> 

Actually we currently don't have any new line insertion fixes for
lines above patch separator.
So, I modified checkpatch a bit to insert (instead of delete) for
co-developed-by.
ie here:

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);
+	fix_insert_line($fixlinenr, $rawline);
}


When repeated a few times and removing signed-off-by, this provided an
experience of inserting multiple lines before signed-off-by (as for
each co-developed-by, it is inserting a new co-developed-by)

With this as well, the fix was working as expected, ie, got this after
some iterations of removing signed-off-by and keeping co-developed-by:

...
It is causing boot failures with qemu mac99 in at least some
configurations.occurring
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/book3s/32/hash.h | 8 ++++----


Aditya

> Other than that, it is all good.
> 
> 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 fix option for MISSING_SIGN_OFF

[Lukas Bulwahn]

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

On Di., 10. Nov. 2020 at 19:13, Aditya <yashsri421@gmail.com> wrote:

> On 10/11/20 8:52 pm, Lukas Bulwahn wrote:
> > On Tue, Nov 10, 2020 at 4:13 PM Aditya <yashsri421@gmail.com> wrote:
> >>
> >> On 10/11/20 7:30 pm, Lukas Bulwahn wrote:
> >>> On Tue, Nov 10, 2020 at 2:06 PM Aditya Srivastava <
> yashsri421@gmail.com> wrote:
> >>>>
> >>>> Currently checkpatch warns us if there is no 'Signed-off-by' line
> >>>> for the patch.
> >>>>
> >>>> E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
> >>>> Completely remove --version flag") reports this error:
> >>>>
> >>>> ERROR: Missing Signed-off-by: line(s)
> >>>>
> >>>> Provide a fix by adding a Signed-off-by line corresponding to the
> author
> >>>> of the patch before the patch separator line. Also avoid this error
> for
> >>>> the commits where some typo is present in the sign off.
> >>>>
> >>>> E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers
> >>>> for quirk") we get missing sign off as well as bad sign off for:
> >>>>
> >>>> Siganed-off-by: Tony Lindgren <tony@atomide.com>
> >>>>
> >>>> Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
> >>>> signature and avoid MISSING_SIGN_OFF
> >>>>
> >>>> Suggested-by: Joe Perches <joe@perches.com>
> >>>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> >>>> ---
> >>>> This patch was made after applying
> https://lore.kernel.org/linux-kernel-mentees/20201108100632.75340-1-dwaipayanray1@gmail.com/
> >>>> and my last changes at
> https://lore.kernel.org/linux-kernel-mentees/20201108134317.25400-1-yashsri421@gmail.com/
> >>>>
> >>>>  scripts/checkpatch.pl | 18 +++++++++++++++---
> >>>>  1 file changed, 15 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >>>> index cb46288127ac..2deffd0c091b 100755
> >>>> --- a/scripts/checkpatch.pl
> >>>> +++ b/scripts/checkpatch.pl
> >>>> @@ -2404,6 +2404,8 @@ sub process {
> >>>>
> >>>>         my $last_blank_line = 0;
> >>>>         my $last_coalesced_string_linenr = -1;
> >>>> +       my $patch_separator_linenr = 0;
> >>>> +       my $non_standard_signature = 0;
> >>>>
> >>>>         our @report = ();
> >>>>         our $cnt_lines = 0;
> >>>> @@ -2755,6 +2757,10 @@ sub process {
> >>>>                 if ($line =~ /^---$/) {
> >>>>                         $has_patch_separator = 1;
> >>>>                         $in_commit_log = 0;
> >>>> +                       # to add missing sign off line before diff(s)
> >>>> +                       if($patch_separator_linenr == 0) {
> >>>> +                               $patch_separator_linenr = $linenr;
> >>>
> >>> Well are you sure linenr is correct? (As I wrote, I do not know, but
> >>> you should know.)
> >>>
> >>> I think you would need to create a patch for testing your feature
> >>> where multiple things are fixed within the same patch and then check
> >>> if the fix is added at the right place.
> >>>
> >>
> >> I checked the fix over following patch.
> >>
> >> I have modified the patch such that it has co-developed-by as the
> >> author(nominal patch author warning, which requires this line to be
> >> deleted)
> >>
> >> Also 'occuring', which is a typo(where the typo needs to be replaced)
> >>
> >> It works as expected.
> >>
> >
> > Well to really check if it works as expected, you need to have a test
> > patch where a fix adds multiple lines into the fixed patch before the
> > line you try to record.
> >
> > Then, fixlinenr and linenr are probably more than just 1 line apart,
> > but really multiple lines apart and only if you record the right
> > variable it really works.
> >
>
> Actually we currently don't have any new line insertion fixes for
> lines above patch separator.
> So, I modified checkpatch a bit to insert (instead of delete) for
> co-developed-by.
> ie here:
>
> 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);
> +       fix_insert_line($fixlinenr, $rawline);
> }
>
>
> When repeated a few times and removing signed-off-by, this provided an
> experience of inserting multiple lines before signed-off-by (as for
> each co-developed-by, it is inserting a new co-developed-by)
>
> With this as well, the fix was working as expected, ie, got this after
> some iterations of removing signed-off-by and keeping co-developed-by:
>
> ...
> It is causing boot failures with qemu mac99 in at least some
> configurations.occurring
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/include/asm/book3s/32/hash.h | 8 ++++----
>
>

I did not look into the details. If you are 100% sure that your patch is
right, let us send it to Joe and the mailing list.

I think next a fix for bad sign-off that corrects those obvious typos would
be nice as well.

Lukas


> Aditya
>
> > Other than that, it is all good.
> >
> > Lukas
> >
>
>

[-- Attachment #1.2: Type: text/html, Size: 9849 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] checkpatch: add fix option for MISSING_SIGN_OFF

[Aditya]

On 11/11/20 12:13 am, Lukas Bulwahn wrote:
> On Di., 10. Nov. 2020 at 19:13, Aditya <yashsri421@gmail.com> wrote:
> 
>> On 10/11/20 8:52 pm, Lukas Bulwahn wrote:
>>> On Tue, Nov 10, 2020 at 4:13 PM Aditya <yashsri421@gmail.com> wrote:
>>>>
>>>> On 10/11/20 7:30 pm, Lukas Bulwahn wrote:
>>>>> On Tue, Nov 10, 2020 at 2:06 PM Aditya Srivastava <
>> yashsri421@gmail.com> wrote:
>>>>>>
>>>>>> Currently checkpatch warns us if there is no 'Signed-off-by' line
>>>>>> for the patch.
>>>>>>
>>>>>> E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
>>>>>> Completely remove --version flag") reports this error:
>>>>>>
>>>>>> ERROR: Missing Signed-off-by: line(s)
>>>>>>
>>>>>> Provide a fix by adding a Signed-off-by line corresponding to the
>> author
>>>>>> of the patch before the patch separator line. Also avoid this error
>> for
>>>>>> the commits where some typo is present in the sign off.
>>>>>>
>>>>>> E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers
>>>>>> for quirk") we get missing sign off as well as bad sign off for:
>>>>>>
>>>>>> Siganed-off-by: Tony Lindgren <tony@atomide.com>
>>>>>>
>>>>>> Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
>>>>>> signature and avoid MISSING_SIGN_OFF
>>>>>>
>>>>>> Suggested-by: Joe Perches <joe@perches.com>
>>>>>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
>>>>>> ---
>>>>>> This patch was made after applying
>> https://lore.kernel.org/linux-kernel-mentees/20201108100632.75340-1-dwaipayanray1@gmail.com/
>>>>>> and my last changes at
>> https://lore.kernel.org/linux-kernel-mentees/20201108134317.25400-1-yashsri421@gmail.com/
>>>>>>
>>>>>>  scripts/checkpatch.pl | 18 +++++++++++++++---
>>>>>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>>>>> index cb46288127ac..2deffd0c091b 100755
>>>>>> --- a/scripts/checkpatch.pl
>>>>>> +++ b/scripts/checkpatch.pl
>>>>>> @@ -2404,6 +2404,8 @@ sub process {
>>>>>>
>>>>>>         my $last_blank_line = 0;
>>>>>>         my $last_coalesced_string_linenr = -1;
>>>>>> +       my $patch_separator_linenr = 0;
>>>>>> +       my $non_standard_signature = 0;
>>>>>>
>>>>>>         our @report = ();
>>>>>>         our $cnt_lines = 0;
>>>>>> @@ -2755,6 +2757,10 @@ sub process {
>>>>>>                 if ($line =~ /^---$/) {
>>>>>>                         $has_patch_separator = 1;
>>>>>>                         $in_commit_log = 0;
>>>>>> +                       # to add missing sign off line before diff(s)
>>>>>> +                       if($patch_separator_linenr == 0) {
>>>>>> +                               $patch_separator_linenr = $linenr;
>>>>>
>>>>> Well are you sure linenr is correct? (As I wrote, I do not know, but
>>>>> you should know.)
>>>>>
>>>>> I think you would need to create a patch for testing your feature
>>>>> where multiple things are fixed within the same patch and then check
>>>>> if the fix is added at the right place.
>>>>>
>>>>
>>>> I checked the fix over following patch.
>>>>
>>>> I have modified the patch such that it has co-developed-by as the
>>>> author(nominal patch author warning, which requires this line to be
>>>> deleted)
>>>>
>>>> Also 'occuring', which is a typo(where the typo needs to be replaced)
>>>>
>>>> It works as expected.
>>>>
>>>
>>> Well to really check if it works as expected, you need to have a test
>>> patch where a fix adds multiple lines into the fixed patch before the
>>> line you try to record.
>>>
>>> Then, fixlinenr and linenr are probably more than just 1 line apart,
>>> but really multiple lines apart and only if you record the right
>>> variable it really works.
>>>
>>
>> Actually we currently don't have any new line insertion fixes for
>> lines above patch separator.
>> So, I modified checkpatch a bit to insert (instead of delete) for
>> co-developed-by.
>> ie here:
>>
>> 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);
>> +       fix_insert_line($fixlinenr, $rawline);
>> }
>>
>>
>> When repeated a few times and removing signed-off-by, this provided an
>> experience of inserting multiple lines before signed-off-by (as for
>> each co-developed-by, it is inserting a new co-developed-by)
>>
>> With this as well, the fix was working as expected, ie, got this after
>> some iterations of removing signed-off-by and keeping co-developed-by:
>>
>> ...
>> It is causing boot failures with qemu mac99 in at least some
>> configurations.occurring
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Co-developed-by: Michael Ellerman <mpe@ellerman.id.au>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>>  arch/powerpc/include/asm/book3s/32/hash.h | 8 ++++----
>>
>>
> 
> I did not look into the details. If you are 100% sure that your patch is
> right, let us send it to Joe and the mailing list.
> 
Yes, on the basis of observations, it seems to be working correctly.
Sending out the patch :)

> I think next a fix for bad sign-off that corrects those obvious typos would
> be nice as well.

Sure.

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: add fix option for MISSING_SIGN_OFF

[Aditya Srivastava]

Currently checkpatch warns us if there is no 'Signed-off-by' line
for the patch.

E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
Completely remove --version flag") reports this error:

ERROR: Missing Signed-off-by: line(s)

Provide a fix by adding a Signed-off-by line corresponding to the author
of the patch before the patch separator line. Also avoid this error for
the commits where some typo is present in the sign off.

E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers
for quirk") we get missing sign off as well as bad sign off for:

Siganed-off-by: Tony Lindgren <tony@atomide.com>

Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
signature and avoid MISSING_SIGN_OFF

Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
 scripts/checkpatch.pl | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index cb46288127ac..2deffd0c091b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2404,6 +2404,8 @@ sub process {
 
 	my $last_blank_line = 0;
 	my $last_coalesced_string_linenr = -1;
+	my $patch_separator_linenr = 0;
+	my $non_standard_signature = 0;
 
 	our @report = ();
 	our $cnt_lines = 0;
@@ -2755,6 +2757,10 @@ sub process {
 		if ($line =~ /^---$/) {
 			$has_patch_separator = 1;
 			$in_commit_log = 0;
+			# to add missing sign off line before diff(s)
+			if($patch_separator_linenr == 0) {
+				$patch_separator_linenr = $linenr;
+			}
 		}
 
 # Check if MAINTAINERS is being updated.  If so, there's probably no need to
@@ -2775,6 +2781,9 @@ sub process {
 			if ($sign_off !~ /$signature_tags/) {
 				WARN("BAD_SIGN_OFF",
 				     "Non-standard signature: $sign_off\n" . $herecurr);
+
+				# to avoid missing_sign_off error as it most probably is just a typo
+				$non_standard_signature = 1;
 			}
 			if (defined $space_before && $space_before ne "") {
 				if (WARN("BAD_SIGN_OFF",
@@ -7118,9 +7127,12 @@ sub process {
 		      "Does not appear to be a unified-diff format patch\n");
 	}
 	if ($is_patch && $has_commit_log && $chk_signoff) {
-		if ($signoff == 0) {
-			ERROR("MISSING_SIGN_OFF",
-			      "Missing Signed-off-by: line(s)\n");
+		if ($signoff == 0 && !$non_standard_signature) {
+			if (ERROR("MISSING_SIGN_OFF",
+				  "Missing Signed-off-by: line(s)\n") &&
+			    $fix) {
+				fix_insert_line($patch_separator_linenr - 1, "Signed-off-by: $author");
+			}
 		} elsif ($authorsignoff != 1) {
 			# authorsignoff values:
 			# 0 -> missing 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 fix option for MISSING_SIGN_OFF

[Joe Perches]

On Wed, 2020-11-11 at 00:36 +0530, Aditya Srivastava wrote:
> Currently checkpatch warns us if there is no 'Signed-off-by' line
> for the patch.

trivial style and a comment:

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -2755,6 +2757,10 @@ sub process {
>  		if ($line =~ /^---$/) {
>  			$has_patch_separator = 1;
>  			$in_commit_log = 0;
> +			# to add missing sign off line before diff(s)
> +			if($patch_separator_linenr == 0) {

space after if

> +				$patch_separator_linenr = $linenr;
> +			}
[]
> @@ -7118,9 +7127,12 @@ sub process {
>  		      "Does not appear to be a unified-diff format patch\n");
>  	}
>  	if ($is_patch && $has_commit_log && $chk_signoff) {
> -		if ($signoff == 0) {
> -			ERROR("MISSING_SIGN_OFF",
> -			      "Missing Signed-off-by: line(s)\n");
> +		if ($signoff == 0 && !$non_standard_signature) {
> +			if (ERROR("MISSING_SIGN_OFF",
> +				  "Missing Signed-off-by: line(s)\n") &&
> +			    $fix) {
> +				fix_insert_line($patch_separator_linenr - 1, "Signed-off-by: $author");

Perhaps this needs to test $patch_separator_linenr to
make sure it's not 0.


_______________________________________________
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: add fix option for MISSING_SIGN_OFF

[Aditya Srivastava]

Currently checkpatch warns us if there is no 'Signed-off-by' line
for the patch.

E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
Completely remove --version flag") reports this error:

ERROR: Missing Signed-off-by: line(s)

Provide a fix by adding a Signed-off-by line corresponding to the author
of the patch before the patch separator line. Also avoid this error for
the commits where some typo is present in the sign off.

E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers
for quirk") we get missing sign off as well as bad sign off for:

Siganed-off-by: Tony Lindgren <tony@atomide.com>

Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
signature and avoid MISSING_SIGN_OFF

A quick evaluation over v4.13..v5.8 found out that 4 out of 11 cases
for this error is because of missing sign offs in reverts. Add
documentation regarding it for the future users.

Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
 Documentation/process/submitting-patches.rst |  2 ++
 scripts/checkpatch.pl                        | 17 ++++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index 83d9a82055a7..ff065e1f6a25 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -404,6 +404,8 @@ then you just add a line saying::
 
 using your real name (sorry, no pseudonyms or anonymous contributions.)
 This will be done for you automatically if you use ``git commit -s``.
+Also reverts should include a Signed-off-by. ``git revert -s`` does
+that for you.
 
 Some people also put extra tags at the end.  They'll just be ignored for
 now, but you can do this to mark internal company procedures or just
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index cb46288127ac..849e4a510767 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2404,6 +2404,8 @@ sub process {
 
 	my $last_blank_line = 0;
 	my $last_coalesced_string_linenr = -1;
+	my $patch_separator_linenr = 0;
+	my $is_signed_off = 0;
 
 	our @report = ();
 	our $cnt_lines = 0;
@@ -2755,6 +2757,10 @@ sub process {
 		if ($line =~ /^---$/) {
 			$has_patch_separator = 1;
 			$in_commit_log = 0;
+			# to add missing sign off line before diff(s)
+			if($patch_separator_linenr == 0) {
+				$patch_separator_linenr = $fixlinenr;
+			}
 		}
 
 # Check if MAINTAINERS is being updated.  If so, there's probably no need to
@@ -2771,6 +2777,8 @@ sub process {
 			my $space_after = $3;
 			my $email = $4;
 			my $ucfirst_sign_off = ucfirst(lc($sign_off));
+			# to avoid missing_sign_off error as it most probably is a typo
+			$is_signed_off = 1;
 
 			if ($sign_off !~ /$signature_tags/) {
 				WARN("BAD_SIGN_OFF",
@@ -7118,9 +7126,12 @@ sub process {
 		      "Does not appear to be a unified-diff format patch\n");
 	}
 	if ($is_patch && $has_commit_log && $chk_signoff) {
-		if ($signoff == 0) {
-			ERROR("MISSING_SIGN_OFF",
-			      "Missing Signed-off-by: line(s)\n");
+		if ($signoff == 0 && !$is_signed_off) {
+			if (ERROR("MISSING_SIGN_OFF",
+				  "Missing Signed-off-by: line(s)\n") &&
+			    $fix) {
+				fix_insert_line($patch_separator_linenr, "Signed-off-by: $author");
+			}
 		} elsif ($authorsignoff != 1) {
 			# authorsignoff values:
 			# 0 -> missing 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 fix option for MISSING_SIGN_OFF

[Lukas Bulwahn]

On Tue, Nov 10, 2020 at 11:06 AM Aditya Srivastava <yashsri421@gmail.com> wrote:
>
> Currently checkpatch warns us if there is no 'Signed-off-by' line
> for the patch.
>
> E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
> Completely remove --version flag") reports this error:
>
> ERROR: Missing Signed-off-by: line(s)
>
> Provide a fix by adding a Signed-off-by line corresponding to the author
> of the patch before the patch separator line. Also avoid this error for
> the commits where some typo is present in the sign off.
>
> E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers
> for quirk") we get missing sign off as well as bad sign off for:
>
> Siganed-off-by: Tony Lindgren <tony@atomide.com>
>
> Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
> signature and avoid MISSING_SIGN_OFF
>
> A quick evaluation over v4.13..v5.8 found out that 4 out of 11 cases
> for this error is because of missing sign offs in reverts. Add
> documentation regarding it for the future users.
>
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>

Split this patch into two patches. One for documentation and one for
the checkpatch.pl change.

The documentation patch needs its own commit message to explain why we
think this should be pointed out.

> ---
>  Documentation/process/submitting-patches.rst |  2 ++
>  scripts/checkpatch.pl                        | 17 ++++++++++++++---
>  2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
> index 83d9a82055a7..ff065e1f6a25 100644
> --- a/Documentation/process/submitting-patches.rst
> +++ b/Documentation/process/submitting-patches.rst
> @@ -404,6 +404,8 @@ then you just add a line saying::
>
>  using your real name (sorry, no pseudonyms or anonymous contributions.)
>  This will be done for you automatically if you use ``git commit -s``.
> +Also reverts should include a Signed-off-by. ``git revert -s`` does
> +that for you.
>
>  Some people also put extra tags at the end.  They'll just be ignored for
>  now, but you can do this to mark internal company procedures or just
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index cb46288127ac..849e4a510767 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2404,6 +2404,8 @@ sub process {
>
>         my $last_blank_line = 0;
>         my $last_coalesced_string_linenr = -1;
> +       my $patch_separator_linenr = 0;
> +       my $is_signed_off = 0;
>
>         our @report = ();
>         our $cnt_lines = 0;
> @@ -2755,6 +2757,10 @@ sub process {
>                 if ($line =~ /^---$/) {
>                         $has_patch_separator = 1;
>                         $in_commit_log = 0;
> +                       # to add missing sign off line before diff(s)
> +                       if($patch_separator_linenr == 0) {
> +                               $patch_separator_linenr = $fixlinenr;
> +                       }

Why do you use fixlinenr here? How is that different from linenr?

>                 }
>
>  # Check if MAINTAINERS is being updated.  If so, there's probably no need to
> @@ -2771,6 +2777,8 @@ sub process {
>                         my $space_after = $3;
>                         my $email = $4;
>                         my $ucfirst_sign_off = ucfirst(lc($sign_off));
> +                       # to avoid missing_sign_off error as it most probably is a typo
> +                       $is_signed_off = 1;

Probably is_signed_off is the wrong name here.

Other than that, this change looks good.

Check if the patch applies on top of Dwaipayan's latest patches he sent.

>
>                         if ($sign_off !~ /$signature_tags/) {
>                                 WARN("BAD_SIGN_OFF",
> @@ -7118,9 +7126,12 @@ sub process {
>                       "Does not appear to be a unified-diff format patch\n");
>         }
>         if ($is_patch && $has_commit_log && $chk_signoff) {
> -               if ($signoff == 0) {
> -                       ERROR("MISSING_SIGN_OFF",
> -                             "Missing Signed-off-by: line(s)\n");
> +               if ($signoff == 0 && !$is_signed_off) {
> +                       if (ERROR("MISSING_SIGN_OFF",
> +                                 "Missing Signed-off-by: line(s)\n") &&
> +                           $fix) {
> +                               fix_insert_line($patch_separator_linenr, "Signed-off-by: $author");
> +                       }
>                 } elsif ($authorsignoff != 1) {
>                         # authorsignoff values:
>                         # 0 -> missing 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 fix option for MISSING_SIGN_OFF

[Aditya]

On 10/11/20 4:01 pm, Lukas Bulwahn wrote:
> On Tue, Nov 10, 2020 at 11:06 AM Aditya Srivastava <yashsri421@gmail.com> wrote:
>>
>> Currently checkpatch warns us if there is no 'Signed-off-by' line
>> for the patch.
>>
>> E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
>> Completely remove --version flag") reports this error:
>>
>> ERROR: Missing Signed-off-by: line(s)
>>
>> Provide a fix by adding a Signed-off-by line corresponding to the author
>> of the patch before the patch separator line. Also avoid this error for
>> the commits where some typo is present in the sign off.
>>
>> E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers
>> for quirk") we get missing sign off as well as bad sign off for:
>>
>> Siganed-off-by: Tony Lindgren <tony@atomide.com>
>>
>> Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
>> signature and avoid MISSING_SIGN_OFF
>>
>> A quick evaluation over v4.13..v5.8 found out that 4 out of 11 cases
>> for this error is because of missing sign offs in reverts. Add
>> documentation regarding it for the future users.
>>
>> Suggested-by: Joe Perches <joe@perches.com>
>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> 
> Split this patch into two patches. One for documentation and one for
> the checkpatch.pl change.
> 
> The documentation patch needs its own commit message to explain why we
> think this should be pointed out.
> 
Okay

>> ---
>>  Documentation/process/submitting-patches.rst |  2 ++
>>  scripts/checkpatch.pl                        | 17 ++++++++++++++---
>>  2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
>> index 83d9a82055a7..ff065e1f6a25 100644
>> --- a/Documentation/process/submitting-patches.rst
>> +++ b/Documentation/process/submitting-patches.rst
>> @@ -404,6 +404,8 @@ then you just add a line saying::
>>
>>  using your real name (sorry, no pseudonyms or anonymous contributions.)
>>  This will be done for you automatically if you use ``git commit -s``.
>> +Also reverts should include a Signed-off-by. ``git revert -s`` does
>> +that for you.
>>
>>  Some people also put extra tags at the end.  They'll just be ignored for
>>  now, but you can do this to mark internal company procedures or just
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index cb46288127ac..849e4a510767 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -2404,6 +2404,8 @@ sub process {
>>
>>         my $last_blank_line = 0;
>>         my $last_coalesced_string_linenr = -1;
>> +       my $patch_separator_linenr = 0;
>> +       my $is_signed_off = 0;
>>
>>         our @report = ();
>>         our $cnt_lines = 0;
>> @@ -2755,6 +2757,10 @@ sub process {
>>                 if ($line =~ /^---$/) {
>>                         $has_patch_separator = 1;
>>                         $in_commit_log = 0;
>> +                       # to add missing sign off line before diff(s)
>> +                       if($patch_separator_linenr == 0) {
>> +                               $patch_separator_linenr = $fixlinenr;
>> +                       }
> 
> Why do you use fixlinenr here? How is that different from linenr?
> 
I used $fixlinenr here as we need to later use it for
fix_insert_line(), where we are passing fixlinenrs' always. So, to
keep it consistent, I used $fixlinenr
Should I use $linenr instead? I think they differ by 1

>>                 }
>>
>>  # Check if MAINTAINERS is being updated.  If so, there's probably no need to
>> @@ -2771,6 +2777,8 @@ sub process {
>>                         my $space_after = $3;
>>                         my $email = $4;
>>                         my $ucfirst_sign_off = ucfirst(lc($sign_off));
>> +                       # to avoid missing_sign_off error as it most probably is a typo
>> +                       $is_signed_off = 1;
> 
> Probably is_signed_off is the wrong name here.
> 
> Other than that, this change looks good.
>
Okay

> Check if the patch applies on top of Dwaipayan's latest patches he sent.
> 
Yes, I have made the changes after applying this patch:
https://lore.kernel.org/linux-kernel-mentees/20201108100632.75340-1-dwaipayanray1@gmail.com/

>>
>>                         if ($sign_off !~ /$signature_tags/) {
>>                                 WARN("BAD_SIGN_OFF",
>> @@ -7118,9 +7126,12 @@ sub process {
>>                       "Does not appear to be a unified-diff format patch\n");
>>         }
>>         if ($is_patch && $has_commit_log && $chk_signoff) {
>> -               if ($signoff == 0) {
>> -                       ERROR("MISSING_SIGN_OFF",
>> -                             "Missing Signed-off-by: line(s)\n");
>> +               if ($signoff == 0 && !$is_signed_off) {
>> +                       if (ERROR("MISSING_SIGN_OFF",
>> +                                 "Missing Signed-off-by: line(s)\n") &&
>> +                           $fix) {
>> +                               fix_insert_line($patch_separator_linenr, "Signed-off-by: $author");
>> +                       }
>>                 } elsif ($authorsignoff != 1) {
>>                         # authorsignoff values:
>>                         # 0 -> missing 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 fix option for MISSING_SIGN_OFF

[Lukas Bulwahn]

On Tue, Nov 10, 2020 at 12:11 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 10/11/20 4:01 pm, Lukas Bulwahn wrote:
> > On Tue, Nov 10, 2020 at 11:06 AM Aditya Srivastava <yashsri421@gmail.com> wrote:
> >>
> >> Currently checkpatch warns us if there is no 'Signed-off-by' line
> >> for the patch.
> >>
> >> E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
> >> Completely remove --version flag") reports this error:
> >>
> >> ERROR: Missing Signed-off-by: line(s)
> >>
> >> Provide a fix by adding a Signed-off-by line corresponding to the author
> >> of the patch before the patch separator line. Also avoid this error for
> >> the commits where some typo is present in the sign off.
> >>
> >> E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers
> >> for quirk") we get missing sign off as well as bad sign off for:
> >>
> >> Siganed-off-by: Tony Lindgren <tony@atomide.com>
> >>
> >> Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
> >> signature and avoid MISSING_SIGN_OFF
> >>
> >> A quick evaluation over v4.13..v5.8 found out that 4 out of 11 cases
> >> for this error is because of missing sign offs in reverts. Add
> >> documentation regarding it for the future users.
> >>
> >> Suggested-by: Joe Perches <joe@perches.com>
> >> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> >
> > Split this patch into two patches. One for documentation and one for
> > the checkpatch.pl change.
> >
> > The documentation patch needs its own commit message to explain why we
> > think this should be pointed out.
> >
> Okay
>
> >> ---
> >>  Documentation/process/submitting-patches.rst |  2 ++
> >>  scripts/checkpatch.pl                        | 17 ++++++++++++++---
> >>  2 files changed, 16 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
> >> index 83d9a82055a7..ff065e1f6a25 100644
> >> --- a/Documentation/process/submitting-patches.rst
> >> +++ b/Documentation/process/submitting-patches.rst
> >> @@ -404,6 +404,8 @@ then you just add a line saying::
> >>
> >>  using your real name (sorry, no pseudonyms or anonymous contributions.)
> >>  This will be done for you automatically if you use ``git commit -s``.
> >> +Also reverts should include a Signed-off-by. ``git revert -s`` does
> >> +that for you.
> >>
> >>  Some people also put extra tags at the end.  They'll just be ignored for
> >>  now, but you can do this to mark internal company procedures or just
> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >> index cb46288127ac..849e4a510767 100755
> >> --- a/scripts/checkpatch.pl
> >> +++ b/scripts/checkpatch.pl
> >> @@ -2404,6 +2404,8 @@ sub process {
> >>
> >>         my $last_blank_line = 0;
> >>         my $last_coalesced_string_linenr = -1;
> >> +       my $patch_separator_linenr = 0;
> >> +       my $is_signed_off = 0;
> >>
> >>         our @report = ();
> >>         our $cnt_lines = 0;
> >> @@ -2755,6 +2757,10 @@ sub process {
> >>                 if ($line =~ /^---$/) {
> >>                         $has_patch_separator = 1;
> >>                         $in_commit_log = 0;
> >> +                       # to add missing sign off line before diff(s)
> >> +                       if($patch_separator_linenr == 0) {
> >> +                               $patch_separator_linenr = $fixlinenr;
> >> +                       }
> >
> > Why do you use fixlinenr here? How is that different from linenr?
> >
> I used $fixlinenr here as we need to later use it for
> fix_insert_line(), where we are passing fixlinenrs' always. So, to
> keep it consistent, I used $fixlinenr
> Should I use $linenr instead? I think they differ by 1
>

I do not know. I am just asking if you are sure that you record the
fixlinenr there or not.

fixlinenr sounds good, but I am not sure.

> >>                 }
> >>
> >>  # Check if MAINTAINERS is being updated.  If so, there's probably no need to
> >> @@ -2771,6 +2777,8 @@ sub process {
> >>                         my $space_after = $3;
> >>                         my $email = $4;
> >>                         my $ucfirst_sign_off = ucfirst(lc($sign_off));
> >> +                       # to avoid missing_sign_off error as it most probably is a typo
> >> +                       $is_signed_off = 1;
> >
> > Probably is_signed_off is the wrong name here.
> >
> > Other than that, this change looks good.
> >
> Okay
>
> > Check if the patch applies on top of Dwaipayan's latest patches he sent.
> >
> Yes, I have made the changes after applying this patch:
> https://lore.kernel.org/linux-kernel-mentees/20201108100632.75340-1-dwaipayanray1@gmail.com/
>

Sounds good. Mention that in the patch below the "---" so that everyone knows.

Lukas
_______________________________________________
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: add fix option for MISSING_SIGN_OFF

[Aditya Srivastava]

Currently checkpatch warns us if there is no 'Signed-off-by' line
for the patch.

E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses:
Completely remove --version flag") reports this error:

ERROR: Missing Signed-off-by: line(s)

Provide a fix by adding a Signed-off-by line corresponding to the author
of the patch before the patch separator line. Also avoid this error for
the commits where some typo is present in the sign offs.

E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers
for quirk") we get missing sign off as well as bad sign off for:
Siganed-off-by: Tony Lindgren <tony@atomide.com>

Here it is probably best to give BAD_SIGN_OFF warning for Non-standard
signature and avoid MISSING_SIGN_OFF

Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
 scripts/checkpatch.pl | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index cb46288127ac..849e4a510767 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2404,6 +2404,8 @@ sub process {
 
 	my $last_blank_line = 0;
 	my $last_coalesced_string_linenr = -1;
+	my $patch_separator_linenr = 0;
+	my $is_signed_off = 0;
 
 	our @report = ();
 	our $cnt_lines = 0;
@@ -2755,6 +2757,10 @@ sub process {
 		if ($line =~ /^---$/) {
 			$has_patch_separator = 1;
 			$in_commit_log = 0;
+			# to add missing sign off line before diff(s)
+			if($patch_separator_linenr == 0) {
+				$patch_separator_linenr = $fixlinenr;
+			}
 		}
 
 # Check if MAINTAINERS is being updated.  If so, there's probably no need to
@@ -2771,6 +2777,8 @@ sub process {
 			my $space_after = $3;
 			my $email = $4;
 			my $ucfirst_sign_off = ucfirst(lc($sign_off));
+			# to avoid missing_sign_off error as it most probably is a typo
+			$is_signed_off = 1;
 
 			if ($sign_off !~ /$signature_tags/) {
 				WARN("BAD_SIGN_OFF",
@@ -7118,9 +7126,12 @@ sub process {
 		      "Does not appear to be a unified-diff format patch\n");
 	}
 	if ($is_patch && $has_commit_log && $chk_signoff) {
-		if ($signoff == 0) {
-			ERROR("MISSING_SIGN_OFF",
-			      "Missing Signed-off-by: line(s)\n");
+		if ($signoff == 0 && !$is_signed_off) {
+			if (ERROR("MISSING_SIGN_OFF",
+				  "Missing Signed-off-by: line(s)\n") &&
+			    $fix) {
+				fix_insert_line($patch_separator_linenr, "Signed-off-by: $author");
+			}
 		} elsif ($authorsignoff != 1) {
 			# authorsignoff values:
 			# 0 -> missing 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

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

[Aditya Srivastava]

Currently checkpatch warns us if there is no 'Signed-off-by' line
for the patch.

E.g., running checkpatch on commit f68e7927212f ("Revert
"powerpc/book3s32: Reorder _PAGE_XXX flags to simplify TLB handling"")
reports this error:

ERROR: Missing Signed-off-by: line(s)

Provide a fix by adding a Signed-off-by line corresponding to the author
of the patch before the start of diff(s)

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index cb46288127ac..bc447aa4e5b0 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2404,6 +2404,7 @@ sub process {
 
 	my $last_blank_line = 0;
 	my $last_coalesced_string_linenr = -1;
+	my $diff_linenr = 0;
 
 	our @report = ();
 	our $cnt_lines = 0;
@@ -2602,6 +2603,10 @@ sub process {
 			$realfile =~ s@^([^/]*)/@@ if (!$file);
 			$in_commit_log = 0;
 			$found_file = 1;
+			# to add missing sign off line before diff(s)
+			if($diff_linenr == 0) {
+				$diff_linenr = $fixlinenr;
+			}
 		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
 			$realfile = $1;
 			$realfile =~ s@^([^/]*)/@@ if (!$file);
@@ -7119,8 +7124,11 @@ sub process {
 	}
 	if ($is_patch && $has_commit_log && $chk_signoff) {
 		if ($signoff == 0) {
-			ERROR("MISSING_SIGN_OFF",
-			      "Missing Signed-off-by: line(s)\n");
+			if (ERROR("MISSING_SIGN_OFF",
+				  "Missing Signed-off-by: line(s)\n") &&
+			    $fix) {
+				fix_insert_line($diff_linenr, "Signed-off-by: $author\n");
+			}
 		} elsif ($authorsignoff != 1) {
 			# authorsignoff values:
 			# 0 -> missing 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 fix option for MISSING_SIGN_OFF

[Aditya]

On 9/11/20 6:40 pm, Aditya Srivastava wrote:
> Currently checkpatch warns us if there is no 'Signed-off-by' line
> for the patch.
> 
> E.g., running checkpatch on commit f68e7927212f ("Revert
> "powerpc/book3s32: Reorder _PAGE_XXX flags to simplify TLB handling"")
> reports this error:
> 
> ERROR: Missing Signed-off-by: line(s)
> 
> Provide a fix by adding a Signed-off-by line corresponding to the author
> of the patch before the start of diff(s)
> 
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
>  scripts/checkpatch.pl | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index cb46288127ac..bc447aa4e5b0 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2404,6 +2404,7 @@ sub process {
>  
>  	my $last_blank_line = 0;
>  	my $last_coalesced_string_linenr = -1;
> +	my $diff_linenr = 0;
>  
>  	our @report = ();
>  	our $cnt_lines = 0;
> @@ -2602,6 +2603,10 @@ sub process {
>  			$realfile =~ s@^([^/]*)/@@ if (!$file);
>  			$in_commit_log = 0;
>  			$found_file = 1;
> +			# to add missing sign off line before diff(s)
> +			if($diff_linenr == 0) {
> +				$diff_linenr = $fixlinenr;
> +			}
>  		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
>  			$realfile = $1;
>  			$realfile =~ s@^([^/]*)/@@ if (!$file);
> @@ -7119,8 +7124,11 @@ sub process {
>  	}
>  	if ($is_patch && $has_commit_log && $chk_signoff) {
>  		if ($signoff == 0) {
> -			ERROR("MISSING_SIGN_OFF",
> -			      "Missing Signed-off-by: line(s)\n");
> +			if (ERROR("MISSING_SIGN_OFF",
> +				  "Missing Signed-off-by: line(s)\n") &&
> +			    $fix) {
> +				fix_insert_line($diff_linenr, "Signed-off-by: $author\n");
> +			}
>  		} elsif ($authorsignoff != 1) {
>  			# authorsignoff values:
>  			# 0 -> missing sign off
> 

This probably is wrong. I should add the sign-off before first patch
separator(ie '---')
Will send the modified 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 fix option for MISSING_SIGN_OFF

[Lukas Bulwahn]

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

On Mo., 9. Nov. 2020 at 14:10, Aditya Srivastava <yashsri421@gmail.com>
wrote:

> Currently checkpatch warns us if there is no 'Signed-off-by' line
> for the patch.
>
> E.g., running checkpatch on commit f68e7927212f ("Revert
> "powerpc/book3s32: Reorder _PAGE_XXX flags to simplify TLB handling"")
> reports this error:
>
> ERROR: Missing Signed-off-by: line(s)
>
> Provide a fix by adding a Signed-off-by line corresponding to the author
> of the patch before the start of diff(s)
>

Can you please provide an evaluation first?

As far as remember, the fix is often not to add a Signed-off-by: tag but it
requires something completely different.

Lukas


> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
>  scripts/checkpatch.pl | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index cb46288127ac..bc447aa4e5b0 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2404,6 +2404,7 @@ sub process {
>
>         my $last_blank_line = 0;
>         my $last_coalesced_string_linenr = -1;
> +       my $diff_linenr = 0;
>
>         our @report = ();
>         our $cnt_lines = 0;
> @@ -2602,6 +2603,10 @@ sub process {
>                         $realfile =~ s@^([^/]*)/@@ if (!$file);
>                         $in_commit_log = 0;
>                         $found_file = 1;
> +                       # to add missing sign off line before diff(s)
> +                       if($diff_linenr == 0) {
> +                               $diff_linenr = $fixlinenr;
> +                       }
>                 } elsif ($line =~ /^\+\+\+\s+(\S+)/) {
>                         $realfile = $1;
>                         $realfile =~ s@^([^/]*)/@@ if (!$file);
> @@ -7119,8 +7124,11 @@ sub process {
>         }
>         if ($is_patch && $has_commit_log && $chk_signoff) {
>                 if ($signoff == 0) {
> -                       ERROR("MISSING_SIGN_OFF",
> -                             "Missing Signed-off-by: line(s)\n");
> +                       if (ERROR("MISSING_SIGN_OFF",
> +                                 "Missing Signed-off-by: line(s)\n") &&
> +                           $fix) {
> +                               fix_insert_line($diff_linenr,
> "Signed-off-by: $author\n");
> +                       }
>                 } elsif ($authorsignoff != 1) {
>                         # authorsignoff values:
>                         # 0 -> missing sign off
> --
> 2.17.1
>
>

[-- Attachment #1.2: Type: text/html, Size: 4107 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] checkpatch: add fix option for MISSING_SIGN_OFF

[Aditya]

On 9/11/20 7:10 pm, Lukas Bulwahn wrote:
> On Mo., 9. Nov. 2020 at 14:10, Aditya Srivastava <yashsri421@gmail.com>
> wrote:
> 
>> Currently checkpatch warns us if there is no 'Signed-off-by' line
>> for the patch.
>>
>> E.g., running checkpatch on commit f68e7927212f ("Revert
>> "powerpc/book3s32: Reorder _PAGE_XXX flags to simplify TLB handling"")
>> reports this error:
>>
>> ERROR: Missing Signed-off-by: line(s)
>>
>> Provide a fix by adding a Signed-off-by line corresponding to the author
>> of the patch before the start of diff(s)
>>
> 
> Can you please provide an evaluation first?
> 
> As far as remember, the fix is often not to add a Signed-off-by: tag but it
> requires something completely different.
> 

I have generated the 'git logs' of the commits causing
MISSING_SIGN_OFF at:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task7/summary_log.txt

Actually this fix was suggested by Joe at:
https://lore.kernel.org/linux-kernel-mentees/e8fef5fbd71331b010988769b249af6a79048f48.camel@perches.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 fix option for MISSING_SIGN_OFF

[Lukas Bulwahn]

On Mon, Nov 9, 2020 at 3:33 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 9/11/20 7:10 pm, Lukas Bulwahn wrote:
> > On Mo., 9. Nov. 2020 at 14:10, Aditya Srivastava <yashsri421@gmail.com>
> > wrote:
> >
> >> Currently checkpatch warns us if there is no 'Signed-off-by' line
> >> for the patch.
> >>
> >> E.g., running checkpatch on commit f68e7927212f ("Revert
> >> "powerpc/book3s32: Reorder _PAGE_XXX flags to simplify TLB handling"")
> >> reports this error:
> >>
> >> ERROR: Missing Signed-off-by: line(s)
> >>
> >> Provide a fix by adding a Signed-off-by line corresponding to the author
> >> of the patch before the start of diff(s)
> >>
> >
> > Can you please provide an evaluation first?
> >
> > As far as remember, the fix is often not to add a Signed-off-by: tag but it
> > requires something completely different.
> >
>
> I have generated the 'git logs' of the commits causing
> MISSING_SIGN_OFF at:
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task7/summary_log.txt
>
> Actually this fix was suggested by Joe at:
> https://lore.kernel.org/linux-kernel-mentees/e8fef5fbd71331b010988769b249af6a79048f48.camel@perches.com/
>

Yes I know. Now look at the reports.

The rc and release commit tags do not get a Signed-off-by
intentionally. Other than that, reverts (for reverts is probably best
to explain how to do a revert... so that a sign-off-by is included)
and typos.


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 fix option for MISSING_SIGN_OFF

[Aditya]

On 9/11/20 8:15 pm, Lukas Bulwahn wrote:
> On Mon, Nov 9, 2020 at 3:33 PM Aditya <yashsri421@gmail.com> wrote:
>>
>> On 9/11/20 7:10 pm, Lukas Bulwahn wrote:
>>> On Mo., 9. Nov. 2020 at 14:10, Aditya Srivastava <yashsri421@gmail.com>
>>> wrote:
>>>
>>>> Currently checkpatch warns us if there is no 'Signed-off-by' line
>>>> for the patch.
>>>>
>>>> E.g., running checkpatch on commit f68e7927212f ("Revert
>>>> "powerpc/book3s32: Reorder _PAGE_XXX flags to simplify TLB handling"")
>>>> reports this error:
>>>>
>>>> ERROR: Missing Signed-off-by: line(s)
>>>>
>>>> Provide a fix by adding a Signed-off-by line corresponding to the author
>>>> of the patch before the start of diff(s)
>>>>
>>>
>>> Can you please provide an evaluation first?
>>>
>>> As far as remember, the fix is often not to add a Signed-off-by: tag but it
>>> requires something completely different.
>>>
>>
>> I have generated the 'git logs' of the commits causing
>> MISSING_SIGN_OFF at:
>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task7/summary_log.txt
>>
>> Actually this fix was suggested by Joe at:
>> https://lore.kernel.org/linux-kernel-mentees/e8fef5fbd71331b010988769b249af6a79048f48.camel@perches.com/
>>
> 
> Yes I know. Now look at the reports.
> 
> The rc and release commit tags do not get a Signed-off-by
> intentionally. Other than that, reverts (for reverts is probably best
> to explain how to do a revert... so that a sign-off-by is included)
> and typos.
> 
> 
> Lukas
> 

Yes, I agree. But for the typos, the user must get BAD_SIGN_OFF
warning for "Non-standard signature". So user will have to fix that
manually. If so, this error is already avoided.

For the release commits, I think the user(s) may not use checkpatch

For the rest (ie reverts) with no sign-offs, I think that if we just
add the sign off line, it serves our purpose.
Of course if we explain them to revert with sign-off, it will be
probably the same outcome.

Thus, I feel that we can proceed with this fix for this class of error.
What do you think/suggest?

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 fix option for MISSING_SIGN_OFF

[Lukas Bulwahn]

On Mon, Nov 9, 2020 at 5:35 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 9/11/20 8:15 pm, Lukas Bulwahn wrote:
> > On Mon, Nov 9, 2020 at 3:33 PM Aditya <yashsri421@gmail.com> wrote:
> >>
> >> On 9/11/20 7:10 pm, Lukas Bulwahn wrote:
> >>> On Mo., 9. Nov. 2020 at 14:10, Aditya Srivastava <yashsri421@gmail.com>
> >>> wrote:
> >>>
> >>>> Currently checkpatch warns us if there is no 'Signed-off-by' line
> >>>> for the patch.
> >>>>
> >>>> E.g., running checkpatch on commit f68e7927212f ("Revert
> >>>> "powerpc/book3s32: Reorder _PAGE_XXX flags to simplify TLB handling"")
> >>>> reports this error:
> >>>>
> >>>> ERROR: Missing Signed-off-by: line(s)
> >>>>
> >>>> Provide a fix by adding a Signed-off-by line corresponding to the author
> >>>> of the patch before the start of diff(s)
> >>>>
> >>>
> >>> Can you please provide an evaluation first?
> >>>
> >>> As far as remember, the fix is often not to add a Signed-off-by: tag but it
> >>> requires something completely different.
> >>>
> >>
> >> I have generated the 'git logs' of the commits causing
> >> MISSING_SIGN_OFF at:
> >> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task7/summary_log.txt
> >>
> >> Actually this fix was suggested by Joe at:
> >> https://lore.kernel.org/linux-kernel-mentees/e8fef5fbd71331b010988769b249af6a79048f48.camel@perches.com/
> >>
> >
> > Yes I know. Now look at the reports.
> >
> > The rc and release commit tags do not get a Signed-off-by
> > intentionally. Other than that, reverts (for reverts is probably best
> > to explain how to do a revert... so that a sign-off-by is included)
> > and typos.
> >
> >
> > Lukas
> >
>
> Yes, I agree. But for the typos, the user must get BAD_SIGN_OFF
> warning for "Non-standard signature". So user will have to fix that
> manually. If so, this error is already avoided.
>

But then the fix should not be applied if there is a BAD_SIGN_OFF
warning, right?

Typos can also be fixed automatically by a fuzzy match. That is a
different feature, though and a different (next) patch.

> For the release commits, I think the user(s) may not use checkpatch
>

Agree. You can filter those out of your evaluation.

> For the rest (ie reverts) with no sign-offs, I think that if we just
> add the sign off line, it serves our purpose.
> Of course if we explain them to revert with sign-off, it will be
> probably the same outcome.
>

Then do both. Provide a fix in checkpatch.pl and provide documentation.

So, how many are because of reverts and how many are because of typos?

Also, how many reverts do currently include a proper sign-off?

When the majority does not include a proper sign-off on revert commit,
maybe the convention is to not have a sign-off on reverts.
When we have some data on that, we can discuss this with some central
maintainers in the kernel community.

If that is the case, then the fix is to change checkpatch.pl not to
complain on revert commits because of a missing sign-off.

> Thus, I feel that we can proceed with this fix for this class of error.
> What do you think/suggest?
>

Please continue on the steps above.

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 fix option for MISSING_SIGN_OFF

[Aditya]

On 9/11/20 11:20 pm, Lukas Bulwahn wrote:
> On Mon, Nov 9, 2020 at 5:35 PM Aditya <yashsri421@gmail.com> wrote:
>>
>> On 9/11/20 8:15 pm, Lukas Bulwahn wrote:
>>> On Mon, Nov 9, 2020 at 3:33 PM Aditya <yashsri421@gmail.com> wrote:
>>>>
>>>> On 9/11/20 7:10 pm, Lukas Bulwahn wrote:
>>>>> On Mo., 9. Nov. 2020 at 14:10, Aditya Srivastava <yashsri421@gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Currently checkpatch warns us if there is no 'Signed-off-by' line
>>>>>> for the patch.
>>>>>>
>>>>>> E.g., running checkpatch on commit f68e7927212f ("Revert
>>>>>> "powerpc/book3s32: Reorder _PAGE_XXX flags to simplify TLB handling"")
>>>>>> reports this error:
>>>>>>
>>>>>> ERROR: Missing Signed-off-by: line(s)
>>>>>>
>>>>>> Provide a fix by adding a Signed-off-by line corresponding to the author
>>>>>> of the patch before the start of diff(s)
>>>>>>
>>>>>
>>>>> Can you please provide an evaluation first?
>>>>>
>>>>> As far as remember, the fix is often not to add a Signed-off-by: tag but it
>>>>> requires something completely different.
>>>>>
>>>>
>>>> I have generated the 'git logs' of the commits causing
>>>> MISSING_SIGN_OFF at:
>>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task7/summary_log.txt
>>>>
>>>> Actually this fix was suggested by Joe at:
>>>> https://lore.kernel.org/linux-kernel-mentees/e8fef5fbd71331b010988769b249af6a79048f48.camel@perches.com/
>>>>
>>>
>>> Yes I know. Now look at the reports.
>>>
>>> The rc and release commit tags do not get a Signed-off-by
>>> intentionally. Other than that, reverts (for reverts is probably best
>>> to explain how to do a revert... so that a sign-off-by is included)
>>> and typos.
>>>
>>>
>>> Lukas
>>>
>>
>> Yes, I agree. But for the typos, the user must get BAD_SIGN_OFF
>> warning for "Non-standard signature". So user will have to fix that
>> manually. If so, this error is already avoided.
>>
> 
> But then the fix should not be applied if there is a BAD_SIGN_OFF
> warning, right?
> 
> Typos can also be fixed automatically by a fuzzy match. That is a
> different feature, though and a different (next) patch.
> 
Okay.

>> For the release commits, I think the user(s) may not use checkpatch
>>
> 
> Agree. You can filter those out of your evaluation.
> 
>> For the rest (ie reverts) with no sign-offs, I think that if we just
>> add the sign off line, it serves our purpose.
>> Of course if we explain them to revert with sign-off, it will be
>> probably the same outcome.
>>
> 
> Then do both. Provide a fix in checkpatch.pl and provide documentation.
> 
Okay.

> So, how many are because of reverts and how many are because of typos?
> 

Reverts: 4,
Typos: 5
True positives: 2 (3c0edea9b29f9be6, 9ac060a708e05423)

> Also, how many reverts do currently include a proper sign-off?
>> When the majority does not include a proper sign-off on revert commit,
> maybe the convention is to not have a sign-off on reverts.
> When we have some data on that, we can discuss this with some central
> maintainers in the kernel community.
> 
> If that is the case, then the fix is to change checkpatch.pl not to
> complain on revert commits because of a missing sign-off.
>
So, there are 993 commits which are reverts. Out of these only 4 are
there which have missing sign offs.(over 4.13..5.8)

I have a doubt, How can I provide documentation? Couldn't find any doc
file for checkpatch

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 fix option for MISSING_SIGN_OFF

[Lukas Bulwahn]

On Tue, Nov 10, 2020 at 7:43 AM Aditya <yashsri421@gmail.com> wrote:
>
> On 9/11/20 11:20 pm, Lukas Bulwahn wrote:
> > On Mon, Nov 9, 2020 at 5:35 PM Aditya <yashsri421@gmail.com> wrote:
> >>
> >> On 9/11/20 8:15 pm, Lukas Bulwahn wrote:
> >>> On Mon, Nov 9, 2020 at 3:33 PM Aditya <yashsri421@gmail.com> wrote:
> >>>>
> >>>> On 9/11/20 7:10 pm, Lukas Bulwahn wrote:
> >>>>> On Mo., 9. Nov. 2020 at 14:10, Aditya Srivastava <yashsri421@gmail.com>
> >>>>> wrote:
> >>>>>
> >>>>>> Currently checkpatch warns us if there is no 'Signed-off-by' line
> >>>>>> for the patch.
> >>>>>>
> >>>>>> E.g., running checkpatch on commit f68e7927212f ("Revert
> >>>>>> "powerpc/book3s32: Reorder _PAGE_XXX flags to simplify TLB handling"")
> >>>>>> reports this error:
> >>>>>>
> >>>>>> ERROR: Missing Signed-off-by: line(s)
> >>>>>>
> >>>>>> Provide a fix by adding a Signed-off-by line corresponding to the author
> >>>>>> of the patch before the start of diff(s)
> >>>>>>
> >>>>>
> >>>>> Can you please provide an evaluation first?
> >>>>>
> >>>>> As far as remember, the fix is often not to add a Signed-off-by: tag but it
> >>>>> requires something completely different.
> >>>>>
> >>>>
> >>>> I have generated the 'git logs' of the commits causing
> >>>> MISSING_SIGN_OFF at:
> >>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task7/summary_log.txt
> >>>>
> >>>> Actually this fix was suggested by Joe at:
> >>>> https://lore.kernel.org/linux-kernel-mentees/e8fef5fbd71331b010988769b249af6a79048f48.camel@perches.com/
> >>>>
> >>>
> >>> Yes I know. Now look at the reports.
> >>>
> >>> The rc and release commit tags do not get a Signed-off-by
> >>> intentionally. Other than that, reverts (for reverts is probably best
> >>> to explain how to do a revert... so that a sign-off-by is included)
> >>> and typos.
> >>>
> >>>
> >>> Lukas
> >>>
> >>
> >> Yes, I agree. But for the typos, the user must get BAD_SIGN_OFF
> >> warning for "Non-standard signature". So user will have to fix that
> >> manually. If so, this error is already avoided.
> >>
> >
> > But then the fix should not be applied if there is a BAD_SIGN_OFF
> > warning, right?
> >
> > Typos can also be fixed automatically by a fuzzy match. That is a
> > different feature, though and a different (next) patch.
> >
> Okay.
>
> >> For the release commits, I think the user(s) may not use checkpatch
> >>
> >
> > Agree. You can filter those out of your evaluation.
> >
> >> For the rest (ie reverts) with no sign-offs, I think that if we just
> >> add the sign off line, it serves our purpose.
> >> Of course if we explain them to revert with sign-off, it will be
> >> probably the same outcome.
> >>
> >
> > Then do both. Provide a fix in checkpatch.pl and provide documentation.
> >
> Okay.
>
> > So, how many are because of reverts and how many are because of typos?
> >
>
> Reverts: 4,
> Typos: 5
> True positives: 2 (3c0edea9b29f9be6, 9ac060a708e05423)
>

So for Reverts and the true positives, your suggested fix works.

For the typos cases, your suggested fix does not work.

You can add this fix but maybe you can also work on how to fix the typo case.
You might consider that the fix is not suggested when there is still
an BAD_SIGNOFF warning (because then it is probably a typo).

I think for the typos in tags, it will emit the BAD_SIGNOFF warning.
You can add a fix option to correct the string by some fuzzy match and
then correct the Signed-off-by: tag.

> > Also, how many reverts do currently include a proper sign-off?
> >> When the majority does not include a proper sign-off on revert commit,
> > maybe the convention is to not have a sign-off on reverts.
> > When we have some data on that, we can discuss this with some central
> > maintainers in the kernel community.
> >
> > If that is the case, then the fix is to change checkpatch.pl not to
> > complain on revert commits because of a missing sign-off.
> >
> So, there are 993 commits which are reverts. Out of these only 4 are
> there which have missing sign offs.(over 4.13..5.8)
>
> I have a doubt, How can I provide documentation? Couldn't find any doc
> file for checkpatch
>

Add an additional hint can be added here:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

E.g., that also reverts should include a Signed-off-by: and that git
revert <some option> does that for you.


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