Skip to content
Snippets Groups Projects
  • Dan Carpenter's avatar
    d5d6281a
    checkpatch: check for missing Fixes tags · d5d6281a
    Dan Carpenter authored
    This check looks for common words that probably indicate a patch
    is a fix.  For now the regex is:
    
    	(?:(?:BUG: K.|UB)SAN: |Call Trace:|stable\@|syzkaller)/)
    
    Why are stable patches encouraged to have a fixes tag?  Some people mark
    their stable patches as "# 5.10" etc.  This is useful but a Fixes tag is
    still a good idea.  For example, the Fixes tag helps in review.  It
    helps people to not cherry-pick buggy patches without also
    cherry-picking the fix.
    
    Also if a bug affects the 5.7 kernel some people will round it up to
    5.10+ because 5.7 is not supported on kernel.org.  It's possible the Bad
    Binder bug was caused by this sort of gap where companies outside of
    kernel.org are supporting different kernels from kernel.org.
    
    Should it be counted as a Fix when a patch just silences harmless
    WARN_ON() stack trace.  Yes.  Definitely.
    
    Is silencing compiler warnings a fix?  It seems unfair to the original
    authors, but we use -Werror now, and warnings break the build so let's
    just add Fixes tags.  I tell people that silencing static checker
    warnings is not a fix but the rules on this vary by subsystem.
    
    Is fixing a minor LTP issue (Linux Test Project) a fix?  Probably?  It's
    hard to know what to do if the LTP test has technically always been
    broken.
    
    One clear false positive from this check is when someone updated their
    debug output and included before and after Call Traces.  Or when crashes
    are introduced deliberately for testing.  In those cases, you should
    just ignore checkpatch.
    
    Link: https://lkml.kernel.org/r/ZmhUgZBKeF_8ixA6@moroto
    
    
    Signed-off-by: default avatarDan Carpenter <dan.carpenter@linaro.org>
    Acked-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
    Reviewed-by: default avatarKees Cook <keescook@chromium.org>
    Cc: Andy Whitcroft <apw@canonical.com>
    Cc: Arnd Bergmann <arnd@arndb.de>
    Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
    Cc: Joe Perches <joe@perches.com>
    Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
    Cc: Sasha Levin <sashal@kernel.org>
    Cc: Thorsten Leemhuis <linux@leemhuis.info>
    Signed-off-by: default avatarAndrew Morton <akpm@linux-foundation.org>
    d5d6281a
    History
    checkpatch: check for missing Fixes tags
    Dan Carpenter authored
    This check looks for common words that probably indicate a patch
    is a fix.  For now the regex is:
    
    	(?:(?:BUG: K.|UB)SAN: |Call Trace:|stable\@|syzkaller)/)
    
    Why are stable patches encouraged to have a fixes tag?  Some people mark
    their stable patches as "# 5.10" etc.  This is useful but a Fixes tag is
    still a good idea.  For example, the Fixes tag helps in review.  It
    helps people to not cherry-pick buggy patches without also
    cherry-picking the fix.
    
    Also if a bug affects the 5.7 kernel some people will round it up to
    5.10+ because 5.7 is not supported on kernel.org.  It's possible the Bad
    Binder bug was caused by this sort of gap where companies outside of
    kernel.org are supporting different kernels from kernel.org.
    
    Should it be counted as a Fix when a patch just silences harmless
    WARN_ON() stack trace.  Yes.  Definitely.
    
    Is silencing compiler warnings a fix?  It seems unfair to the original
    authors, but we use -Werror now, and warnings break the build so let's
    just add Fixes tags.  I tell people that silencing static checker
    warnings is not a fix but the rules on this vary by subsystem.
    
    Is fixing a minor LTP issue (Linux Test Project) a fix?  Probably?  It's
    hard to know what to do if the LTP test has technically always been
    broken.
    
    One clear false positive from this check is when someone updated their
    debug output and included before and after Call Traces.  Or when crashes
    are introduced deliberately for testing.  In those cases, you should
    just ignore checkpatch.
    
    Link: https://lkml.kernel.org/r/ZmhUgZBKeF_8ixA6@moroto
    
    
    Signed-off-by: default avatarDan Carpenter <dan.carpenter@linaro.org>
    Acked-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
    Reviewed-by: default avatarKees Cook <keescook@chromium.org>
    Cc: Andy Whitcroft <apw@canonical.com>
    Cc: Arnd Bergmann <arnd@arndb.de>
    Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
    Cc: Joe Perches <joe@perches.com>
    Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
    Cc: Sasha Levin <sashal@kernel.org>
    Cc: Thorsten Leemhuis <linux@leemhuis.info>
    Signed-off-by: default avatarAndrew Morton <akpm@linux-foundation.org>