[PATCH -next v3] docs: document all error message types in checkpatch

Mon Mar 22 07:43:13 UTC 2021

On Mon, Mar 22, 2021 at 1:03 PM Lukas Bulwahn <lukas.bulwahn at gmail.com> wrote:
>
> On Sun, Mar 21, 2021 at 12:26 PM Dwaipayan Ray <dwaipayanray1 at gmail.com> wrote:
> >
> > All the error message types now have a verbose description.
> >
> > Also there are two new groups of message types:
> >
> > - Macros, Attributes and Symbols
> > - Functions and Variables
> >
> > Rearrange the message types to fit these new groups as needed.
> >
> > Signed-off-by: Dwaipayan Ray <dwaipayanray1 at gmail.com>
>
> Let us go for a wider review on linux-doc and lkml...
>
Sure will do

> Reviewed-by: Lukas Bulwahn <lukas.bulwahn at gmail.com>
>
>
> > ---
> >
> > Changes in v3:
> > - Add more explanation for LOCKDEP
> > - Minor grammar fixes
> >
> > Changes in v2:
> > - Replace 4.10 kernel doc links by latest
> >
> >  Documentation/dev-tools/checkpatch.rst | 319 ++++++++++++++++++++++---
> >  1 file changed, 281 insertions(+), 38 deletions(-)
> >
> > diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> > index 2671e54c8320..904fd5fe2484 100644
> > --- a/Documentation/dev-tools/checkpatch.rst
> > +++ b/Documentation/dev-tools/checkpatch.rst
> > @@ -280,43 +280,12 @@ API usage
> >      However this is not always the case (See signal.h).
> >      This message type is emitted only for includes from arch/.
> >
> > -  **ARRAY_SIZE**
> > -    The ARRAY_SIZE(foo) macro should be preferred over
> > -    sizeof(foo)/sizeof(foo[0]) for finding number of elements in an
> > -    array.
> > -
> > -    The macro is defined in include/linux/kernel.h::
> > -
> > -      #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > -
> >    **AVOID_BUG**
> >      BUG() or BUG_ON() should be avoided totally.
> >      Use WARN() and WARN_ON() instead, and handle the "impossible"
> >      error condition as gracefully as possible.
> >      See: https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on
> >
> > -  **AVOID_EXTERNS**
> > -    Function prototypes don't need to be declared extern in .h
> > -    files.  It's assumed by the compiler and is unnecessary.
> > -
> > -  **AVOID_L_PREFIX**
> > -    Local symbol names that are prefixed with `.L` should be avoided,
> > -    as this has special meaning for the assembler; a symbol entry will
> > -    not be emitted into the symbol table.  This can prevent `objtool`
> > -    from generating correct unwind info.
> > -
> > -    Symbols with STB_LOCAL binding may still be used, and `.L` prefixed
> > -    local symbol names are still generally usable within a function,
> > -    but `.L` prefixed local symbol names should not be used to denote
> > -    the beginning or end of code regions via
> > -    `SYM_CODE_START_LOCAL`/`SYM_CODE_END`
> > -
> > -  **BIT_MACRO**
> > -    Defines like: 1 << <digit> could be BIT(digit).
> > -    The BIT() macro is defined in include/linux/bitops.h::
> > -
> > -      #define BIT(nr)         (1UL << (nr))
> > -
> >    **CONSIDER_KSTRTO**
> >      The simple_strtol(), simple_strtoll(), simple_strtoul(), and
> >      simple_strtoull() functions explicitly ignore overflows, which
> > @@ -325,6 +294,25 @@ API usage
> >      correct replacements.
> >      See: https://www.kernel.org/doc/html/latest/process/deprecated.html#simple-strtol-simple-strtoll-simple-strtoul-simple-strtoull
> >
> > +  **LOCKDEP**
> > +    The lockdep_no_validate class was added as a temporary measure to
> > +    prevent warnings on conversion of device->sem to device->mutex.
> > +    It should not be used for any other purpose.
> > +    See: https://lore.kernel.org/lkml/1268959062.9440.467.camel@laptop/
> > +
> > +  **MALFORMED_INCLUDE**
> > +    The #include statement has a malformed path.  This has happened
> > +    because the author has included a double slash "//" in the pathname
> > +    accidentally.
> > +
> > +  **USE_LOCKDEP**
> > +    lockdep_assert_held() annotations should be preferred over
> > +    assertions based on spin_is_locked()
> > +    See: https://www.kernel.org/doc/html/latest/locking/lockdep-design.html#annotations
> > +
> > +  **UAPI_INCLUDE**
> > +    No #include statements in include/uapi should use a uapi/ path.
> > +
> >
> >  Comment style
> >  -------------
> > @@ -353,7 +341,6 @@ Comment style
> >      See: https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
> >
> >
> > -
> >  Commit message
> >  --------------
> >
> > @@ -397,6 +384,35 @@ Commit message
> >      source patch.
> >      See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
> >
> > +  **DIFF_IN_COMMIT_MSG**
> > +    Avoid having diff content in commit message.
> > +    This causes problems when one tries to apply a file containing both
> > +    the changelog and the diff because patch(1) tries to apply the diff
> > +    which it found in the changelog.
> > +    See: https://lore.kernel.org/lkml/20150611134006.9df79a893e3636019ad2759e@linux-foundation.org/
> > +
> > +  **GERRIT_CHANGE_ID**
> > +    To be picked up by gerrit, the footer of the commit message might
> > +    have a Change-Id like::
> > +
> > +      Change-Id: Ic8aaa0728a43936cd4c6e1ed590e01ba8f0fbf5b
> > +      Signed-off-by: A. U. Thor <author at example.com>
> > +
> > +    The Change-Id line must be removed before submitting.
> > +
> > +  **GIT_COMMIT_ID**
> > +    The proper way to reference a commit id is:
> > +    commit <12+ chars of sha1> ("<title line>")
> > +
> > +    An example may be::
> > +
> > +      Commit e21d2170f36602ae2708 ("video: remove unnecessary
> > +      platform_set_drvdata()") removed the unnecessary
> > +      platform_set_drvdata(), but left the variable "dev" unused,
> > +      delete it.
> > +
> > +    See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> > +
> >
> >  Comparison style
> >  ----------------
> > @@ -426,6 +442,148 @@ Comparison style
> >      side of the test should be avoided.
> >
> >
> > +Macros, Attributes and Symbols
> > +------------------------------
> > +
> > +  **ARRAY_SIZE**
> > +    The ARRAY_SIZE(foo) macro should be preferred over
> > +    sizeof(foo)/sizeof(foo[0]) for finding number of elements in an
> > +    array.
> > +
> > +    The macro is defined in include/linux/kernel.h::
> > +
> > +      #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > +
> > +  **AVOID_EXTERNS**
> > +    Function prototypes don't need to be declared extern in .h
> > +    files.  It's assumed by the compiler and is unnecessary.
> > +
> > +  **AVOID_L_PREFIX**
> > +    Local symbol names that are prefixed with `.L` should be avoided,
> > +    as this has special meaning for the assembler; a symbol entry will
> > +    not be emitted into the symbol table.  This can prevent `objtool`
> > +    from generating correct unwind info.
> > +
> > +    Symbols with STB_LOCAL binding may still be used, and `.L` prefixed
> > +    local symbol names are still generally usable within a function,
> > +    but `.L` prefixed local symbol names should not be used to denote
> > +    the beginning or end of code regions via
> > +    `SYM_CODE_START_LOCAL`/`SYM_CODE_END`
> > +
> > +  **BIT_MACRO**
> > +    Defines like: 1 << <digit> could be BIT(digit).
> > +    The BIT() macro is defined in include/linux/bitops.h::
> > +
> > +      #define BIT(nr)         (1UL << (nr))
> > +
> > +  **CONST_READ_MOSTLY**
> > +    When a variable is tagged with the __read_mostly annotation, it is a
> > +    signal to the compiler that accesses to the variable will be mostly
> > +    reads and rarely(but NOT never) a write.
> > +
> > +    const __read_mostly does not make any sense as const data is already
> > +    read-only.  The __read_mostly annotation thus should be removed.
> > +
> > +  **DATE_TIME**
> > +    It is generally desirable that building the same source code with
> > +    the same set of tools is reproducible, i.e. the output is always
> > +    exactly the same.
> > +
> > +    The kernel does *not* use the ``__DATE__`` and ``__TIME__`` macros,
> > +    and enables warnings if they are used as they can lead to
> > +    non-deterministic builds.
> > +    See: https://www.kernel.org/doc/html/latest/kbuild/reproducible-builds.html#timestamps
> > +
> > +  **DEFINE_ARCH_HAS**
> > +    The ARCH_HAS_xyz or ARCH_HAVE_xyz patterns are totally wrong.
> > +
> > +    For big conceptual features use Kconfig symbols instead.  And for
> > +    smaller things where we have compatibility fallback functions but
> > +    want architectures able to override them with optimized ones one
> > +    function at a time, we should either use weak functions (appropriate
> > +    for some cases), or the symbol that protects them should the
> > +    SAME SYMBOL WE USE.
>
> Capital letter means SHOUTING. I do not see the need to shout here...
>
> Maybe that was needed in the original mail... in the documentation, we
> are happy that the reader reads it at all...
>

Yes that makes sense. We don't wanna scare our readers too. So i will
correct this.

Thanks!
Dwaipayan.