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

Lukas Bulwahn lukas.bulwahn at gmail.com
Tue Nov 10 15:22:11 UTC 2020


On Tue, Nov 10, 2020 at 4:13 PM Aditya <yashsri421 at 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 at 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 at 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 at perches.com>
> >> Signed-off-by: Aditya Srivastava <yashsri421 at 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 at 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 at 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


More information about the Linux-kernel-mentees mailing list