Documentation/dev-tools/checkpatch.rst | 6 ++++ scripts/checkpatch.pl | 40 ++++++++++++++++++++++++++ 2 files changed, 46 insertions(+)
Modified the checkpatch script to ensure that commit tags (e.g.,
Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the
correct order according to kernel conventions [1].
checkpatch.pl will now emit a BAD_TAG_ORDER warning when tags are out of
the expected sequence. Multiple tags of the same type are allowed, but
they must also follow the order. 'Link:' tags in the changelog are still
allowed before the tag sequence begins, but once the sequence has started,
any 'Link:' tags must follow the ordered commit tags.
Link: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#ordering-of-commit-tags # [1]
Signed-off-by: Hendrik Hamerlinck <hendrik.hamerlinck@hammernet.be>
---
Documentation/dev-tools/checkpatch.rst | 6 ++++
scripts/checkpatch.pl | 40 ++++++++++++++++++++++++++
2 files changed, 46 insertions(+)
diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index 76bd0ddb0041..696b42bf4ff5 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -599,6 +599,12 @@ Commit message
See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
+ **BAD_TAG_ORDER**
+ The tags in the commit message are not in the correct order according to
+ community conventions. Common tags like Signed-off-by, Reviewed-by,
+ Tested-by, Acked-by, Fixes, Cc, etc., should follow a standardized sequence.
+
+ See: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#ordering-of-commit-tags
Comparison style
----------------
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 664f7b7a622c..267ec02de9ec 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -661,6 +661,24 @@ foreach my $entry (@link_tags) {
}
$link_tags_search = "(?:${link_tags_search})";
+# Ordered commit tags
+our @commit_tags = (
+ "Fixes:",
+ "Reported-by:",
+ "Closes:",
+ "Originally-by:",
+ "Suggested-by:",
+ "Co-developed-by:",
+ "Signed-off-by:",
+ "Tested-by:",
+ "Reviewed-by",
+ "Acked-by:",
+ "Cc:",
+ "Link:"
+);
+our $commit_tag_pattern = join '|', map { quotemeta($_) } @commit_tags;
+our $commit_tags_regex = qr{(?xi: ^\s*($commit_tag_pattern))};
+
our $tracing_logging_tags = qr{(?xi:
[=-]*> |
<[=-]* |
@@ -2712,6 +2730,8 @@ sub process {
my $checklicenseline = 1;
+ my $last_matched_tag;
+
sanitise_line_reset();
my $line;
foreach my $rawline (@rawlines) {
@@ -3258,6 +3278,26 @@ sub process {
}
}
+# Check commit tags sorting
+ if (!$in_header_lines && $line =~ $commit_tags_regex) {
+ my $tag = $1;
+ my ($tag_index) = grep { lc($commit_tags[$_]) eq lc($tag) } 0..$#commit_tags;
+
+ if ($last_matched_tag &&
+ $last_matched_tag->{tag_index} > $tag_index) {
+ WARN("BAD_TAG_ORDER",
+ "Tag '$tag' is out of order. Should come before '$last_matched_tag->{tag}'\n" . $herecurr);
+ }
+
+ # Allow link tags to occur before the commit tags
+ if (lc($tag) ne "link:" || defined $last_matched_tag) {
+ $last_matched_tag = {
+ tag => $tag,
+ tag_index => $tag_index,
+ };
+ }
+ }
+
# Check email subject for common tools that don't need to be mentioned
if ($in_header_lines &&
$line =~ /^Subject:.*\b(?:checkpatch|sparse|smatch)\b[^:]/i) {
--
2.43.0On 7/24/25 09:20, Hendrik Hamerlinck wrote: > Modified the checkpatch script to ensure that commit tags (e.g., > Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the > correct order according to kernel conventions [1]. Hello all, Thank you for the feedback. I wasn’t aware that the tag ordering conventions used in the TIP tree are not universally followed across all kernel subsystems. My motivation for this change came from a recent mistake I made in a patch submission, where I incorrectly placed a Fixes: tag after the Signed-off-by: line. I realized that checkpatch.pl didn’t flag this, and I thought a warning might be helpful, especially for newer contributors like myself. I now realize that my approach is too strict by trying to enforce an order for all tags. However, I still believe that a targeted warning could be useful. Another mentee I work with recently made the same mistake, so it may be a common pitfall. Is there a general consensus on placing the first Fixes: tag at the start of the tag sequence? If so, a warning might be helpful for newer contributors? I was still using checkpatch as that was how I initially learned it. I'll definitely look into using b4 as well. Kind regards, Hendrik
On 24/07/2025 09:20, Hendrik Hamerlinck wrote: > Modified the checkpatch script to ensure that commit tags (e.g., > Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the > correct order according to kernel conventions [1]. These are not the conventions I use for my subsystems and ask others to follow, so imposing TIP rules to all maintainers needs broad consensus, not (yet) checkpatch. What's more, I think above TIP rules are contradicting with existing, widely used and approved toolset - b4. So no, if you want universal tool, please use b4 or whatever b4 defines. Best regards, Krzysztof
Hi Krzysztof,
On Fri, 25 Jul 2025 at 10:42, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 24/07/2025 09:20, Hendrik Hamerlinck wrote:
> > Modified the checkpatch script to ensure that commit tags (e.g.,
> > Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the
> > correct order according to kernel conventions [1].
>
> These are not the conventions I use for my subsystems and ask others to
> follow, so imposing TIP rules to all maintainers needs broad consensus,
> not (yet) checkpatch.
>
> What's more, I think above TIP rules are contradicting with existing,
> widely used and approved toolset - b4. So no, if you want universal
> tool, please use b4 or whatever b4 defines.
B4 does not follow the proper order:
1. Multiple Reviewed-by tags may be added in a different order
than given,
2. When applying my own patches, b4 adds the given tags before
instead of after my own SoB.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 31/07/2025 13:55, Geert Uytterhoeven wrote: > Hi Krzysztof, > > On Fri, 25 Jul 2025 at 10:42, Krzysztof Kozlowski <krzk@kernel.org> wrote: >> On 24/07/2025 09:20, Hendrik Hamerlinck wrote: >>> Modified the checkpatch script to ensure that commit tags (e.g., >>> Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the >>> correct order according to kernel conventions [1]. >> >> These are not the conventions I use for my subsystems and ask others to >> follow, so imposing TIP rules to all maintainers needs broad consensus, >> not (yet) checkpatch. >> >> What's more, I think above TIP rules are contradicting with existing, >> widely used and approved toolset - b4. So no, if you want universal >> tool, please use b4 or whatever b4 defines. > > B4 does not follow the proper order: There is no "proper order" in terms of absolute facts. > 1. Multiple Reviewed-by tags may be added in a different order > than given, Maybe this could be fixed. > 2. When applying my own patches, b4 adds the given tags before > instead of after my own SoB. This is working exactly as expected (intentional), explained few times by Konstantin. Some people of course have different opinion and prefer different order (I know few subsystems), but majority I think accepted Konstantin's explanation. Best regards, Krzysztof
On Thu, 31 Jul 2025, Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 31/07/2025 13:55, Geert Uytterhoeven wrote: >> B4 does not follow the proper order: > > There is no "proper order" in terms of absolute facts. Let's just decide whatever order b4 uses *is* the proper order, and save ourselves endless hours of debating! :p BR, Jani. -- Jani Nikula, Intel
Em Fri, 01 Aug 2025 10:55:55 +0300 Jani Nikula <jani.nikula@intel.com> escreveu: > On Thu, 31 Jul 2025, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 31/07/2025 13:55, Geert Uytterhoeven wrote: > >> B4 does not follow the proper order: > > > > There is no "proper order" in terms of absolute facts. > > Let's just decide whatever order b4 uses *is* the proper order, and save > ourselves endless hours of debating! :p I don't think it makes sense to have a "proper order" verified on checkpatch, as some tags may appear on different places. For instance, the custody chain was designed to have SoBs appearing in different places: - author(s) SoB together co-developed-by are usually the first ones; - then patches may have been reviewed, tested, acked or passed on some other trees, gaining tags like tested-by, R-B, A-B, SoB, Cc; - the subsystem maintainer will add his SoB in the end. non-custody chain tags, like fixes, closes, reported-by... usually comes first, but I don't think we need to enforce an specific order. Link, for instance, could be used on different places, with different purposes. At least for me, the only part that shall really follow a proper order is the custody chain: It has to follow how the patch was handled, from the authors at the top up to the maintainers at the bottom. Thanks, Mauro
On Sat, Aug 02, 2025 at 12:12:00PM +0200, Mauro Carvalho Chehab wrote: > > Let's just decide whatever order b4 uses *is* the proper order, and save > > ourselves endless hours of debating! :p > > I don't think it makes sense to have a "proper order" verified on > checkpatch, as some tags may appear on different places. > > For instance, the custody chain was designed to have SoBs appearing > in different places: > > - author(s) SoB together co-developed-by are usually the first ones; > - then patches may have been reviewed, tested, acked or passed on some > other trees, gaining tags like tested-by, R-B, A-B, SoB, Cc; > - the subsystem maintainer will add his SoB in the end. > > non-custody chain tags, like fixes, closes, reported-by... > usually comes first, but I don't think we need to enforce an specific > order. I wholeheartedly agree -- it really doesn't matter the order the trailers are in, as long as it's clear who is the person who pulled the trailer in, which is why I stick to the chain of custody. I'm pretty sure nobody has ever looked at a commit and went "I can't believe they put the Link trailer above the Suggested-by trailer," so enforcing it in checkpatch seems like wasted effort to me. -K
On Thu, Jul 24, 2025 at 09:20:32AM +0200, Hendrik Hamerlinck wrote: > Modified the checkpatch script to ensure that commit tags (e.g., > Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the > correct order according to kernel conventions [1]. As already indicated, this is the convention used by the TIP tree and is not universal. Moreover, there is a lot more nuance to how trailers are used, with many other subsystems strongly preferring chain-of-custody boundaries. For example, the following trailers indicate a history of how the patch was reviewed: | Suggested-by: Sug Gester <sug@example.com> | Signed-off-by: Alex Dev <alex@example.com> -- boundary 1 | Acked-by: Acker Mack <acker@example.com> | Tested-by: Test Rogen <test@example.com> | Signed-off-by: Sub Maintainer <sub@example.com> -- boundary 2 | Reviewed-by: Rev Yewer <rev@example.com> | Tested-by: Integration Bot <int@example.com> | Link: https://patch.msgid.link/foomsgid@exmple.com | Signed-off-by: Main Tainer <main@example.com> -- boundary 3 There are three chain of custody boundaries in the example above and in the chain-of-custody scenario the trailers should NOT be moved around between these boundaries, because each of the boundaries indicates what each signed-off-by person is claiming as their responsibility. Everything above boundary 1 is claimed by Alex Dev; all trailers above boundary 2 were collected and applied by Sub Maintainer; all trailers above boundary 3 were collected and applied by Main Tainer. Alex Dev has no responsibility for the tag provided by the Integration Bot, so moving their signed-off-by to the bottom of this series of trailer would imply that they are. I would leave the trailer order entirely alone and out of tools like checkpatch, so this is a gentle but firm NACK from me. -K
On 7/24/2025 12:20 AM, Hendrik Hamerlinck wrote:
> Modified the checkpatch script to ensure that commit tags (e.g.,
> Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the
> correct order according to kernel conventions [1].
>
> checkpatch.pl will now emit a BAD_TAG_ORDER warning when tags are out of
> the expected sequence. Multiple tags of the same type are allowed, but
> they must also follow the order. 'Link:' tags in the changelog are still
> allowed before the tag sequence begins, but once the sequence has started,
> any 'Link:' tags must follow the ordered commit tags.
>
> Link: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#ordering-of-commit-tags # [1]
>
> Signed-off-by: Hendrik Hamerlinck <hendrik.hamerlinck@hammernet.be>
> ---
> Documentation/dev-tools/checkpatch.rst | 6 ++++
> scripts/checkpatch.pl | 40 ++++++++++++++++++++++++++
> 2 files changed, 46 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index 76bd0ddb0041..696b42bf4ff5 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -599,6 +599,12 @@ Commit message
>
> See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
>
> + **BAD_TAG_ORDER**
> + The tags in the commit message are not in the correct order according to
> + community conventions. Common tags like Signed-off-by, Reviewed-by,
> + Tested-by, Acked-by, Fixes, Cc, etc., should follow a standardized sequence.
> +
> + See: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#ordering-of-commit-tags
>
> Comparison style
> ----------------
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 664f7b7a622c..267ec02de9ec 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -661,6 +661,24 @@ foreach my $entry (@link_tags) {
> }
> $link_tags_search = "(?:${link_tags_search})";
>
> +# Ordered commit tags
> +our @commit_tags = (
> + "Fixes:",
> + "Reported-by:",
> + "Closes:",
> + "Originally-by:",
> + "Suggested-by:",
> + "Co-developed-by:",
> + "Signed-off-by:",
> + "Tested-by:",
> + "Reviewed-by",
> + "Acked-by:",
> + "Cc:",
> + "Link:"
> +);
> +our $commit_tag_pattern = join '|', map { quotemeta($_) } @commit_tags;
> +our $commit_tags_regex = qr{(?xi: ^\s*($commit_tag_pattern))};
> +
> our $tracing_logging_tags = qr{(?xi:
> [=-]*> |
> <[=-]* |
> @@ -2712,6 +2730,8 @@ sub process {
>
> my $checklicenseline = 1;
>
> + my $last_matched_tag;
> +
> sanitise_line_reset();
> my $line;
> foreach my $rawline (@rawlines) {
> @@ -3258,6 +3278,26 @@ sub process {
> }
> }
>
> +# Check commit tags sorting
> + if (!$in_header_lines && $line =~ $commit_tags_regex) {
> + my $tag = $1;
> + my ($tag_index) = grep { lc($commit_tags[$_]) eq lc($tag) } 0..$#commit_tags;
> +
> + if ($last_matched_tag &&
> + $last_matched_tag->{tag_index} > $tag_index) {
> + WARN("BAD_TAG_ORDER",
> + "Tag '$tag' is out of order. Should come before '$last_matched_tag->{tag}'\n" . $herecurr);
> + }
> +
> + # Allow link tags to occur before the commit tags
> + if (lc($tag) ne "link:" || defined $last_matched_tag) {
> + $last_matched_tag = {
> + tag => $tag,
> + tag_index => $tag_index,
> + };
> + }
> + }
> +
> # Check email subject for common tools that don't need to be mentioned
> if ($in_header_lines &&
> $line =~ /^Subject:.*\b(?:checkpatch|sparse|smatch)\b[^:]/i) {
Seems this logic would fail when there are multiple reporters, such as in
commit 9a44b5e36cd699fdd2150a63fab225ac510c1971
Fixes: 49e47223ecc4 ("wifi: cfg80211: allocate memory for link_station info structure")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/81f30515-a83d-4b05-a9d1-e349969df9e9@sabinyo.mountain/
Reported-by: syzbot+4ba6272678aa468132c8@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/68655325.a70a0220.5d25f.0316.GAE@google.com
Hello, On Thu, 24 Jul 2025 09:20:32 +0200, Hendrik Hamerlinck wrote: > Modified the checkpatch script to ensure that commit tags (e.g., > Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the > correct order according to kernel conventions [1]. > > checkpatch.pl will now emit a BAD_TAG_ORDER warning when tags are out of > the expected sequence. Multiple tags of the same type are allowed, but > they must also follow the order. 'Link:' tags in the changelog are still > allowed before the tag sequence begins, but once the sequence has started, > any 'Link:' tags must follow the ordered commit tags. > > Link: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#ordering-of-commit-tags # [1] > So, you are citing ordering rules specific to the "tip" tree. I don't think there is any wider consensus with regard to such strict rules. I have to say that your change will make a lot of unneeded noise for most contributors. If such warnings are only shown when the user opted in to the tip rule, say, by using a "--tip" flag or something, they might be helpful. BR, Akira > Signed-off-by: Hendrik Hamerlinck <hendrik.hamerlinck@hammernet.be> > --- > Documentation/dev-tools/checkpatch.rst | 6 ++++ > scripts/checkpatch.pl | 40 ++++++++++++++++++++++++++ > 2 files changed, 46 insertions(+) [...]
© 2016 - 2025 Red Hat, Inc.