Hi Jon,
ok, let's start slow and with something that should not need a whole lot
of bikeshedding. We have been repeating that a lot of times in the past
which means that, especially new submitters, do not know it. So here it
goes.
Btw, pls let me know when you start queueing the patches, on which tree
you do that so that I can send you the following ones ontop.
Also, pls have a look here and let me know what else you want moved to
generic docs:
https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=rc0%2b0-tip-handbook
Otherwise I'll go by my judgement, hoping that no tip-specific stuff
leaks out. :-)
Thx.
---
From: Borislav Petkov <[email protected]>
Subject: [PATCH] Documentation/submitting-patches: Document the SoB chain
Document what a chain of Signed-off-by's in a patch commit message
should mean, explicitly.
This has been carved out from a tip subsystem handbook patchset by
Thomas Gleixner:
https://lkml.kernel.org/r/[email protected]
and incorporates follow-on comments.
Signed-off-by: Borislav Petkov <[email protected]>
---
Documentation/process/submitting-patches.rst | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index fb8261a4be30..54d761319875 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -411,6 +411,12 @@ 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
point out some special detail about the sign-off.
+Any further SoBs (Signed-off-by:'s) following the author's SoB are from
+people handling and transporting the patch, but were not involved in its
+development. SoB chains should reflect the **real** route a patch took
+as it was propagated to the maintainers and ultimately to Linus, with
+the first SoB entry signalling primary authorship of a single author.
+
When to use Acked-by:, Cc:, and Co-developed-by:
------------------------------------------------
--
2.29.2
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, 17 Dec 2020 19:37:56 +0100
Borislav Petkov <[email protected]> wrote:
> ok, let's start slow and with something that should not need a whole lot
> of bikeshedding. We have been repeating that a lot of times in the past
> which means that, especially new submitters, do not know it. So here it
> goes.
>
> Btw, pls let me know when you start queueing the patches, on which tree
> you do that so that I can send you the following ones ontop.
Given that there was indeed a lack of bikeshedding, I just went ahead and
applied this to docs-next; will ship it Linusward in the near future.
Thanks,
jon
On Mon, Dec 21, 2020 at 09:54:25AM -0700, Jonathan Corbet wrote:
> Given that there was indeed a lack of bikeshedding, I just went ahead
> and applied this to docs-next; will ship it Linusward in the near
> future.
Cool.
Although I betcha that is because of the impending holiday season. I
think I should just use that moment to sneak in more stuff stealthily
and when everyone awakes from their drunken and food stupor, it'll all
be there! Tadaa!
:-)))
Ok, here's the next one which I think, is also, not really controversial.
Thx.
---
From: Borislav Petkov <[email protected]>
Date: Tue, 22 Dec 2020 13:58:22 +0100
Document that backtraces in commit messages should be trimmed down to
the useful information only.
This has been carved out from a tip subsystem handbook patchset by
Thomas Gleixner:
https://lkml.kernel.org/r/[email protected]
and incorporates follow-on comments.
Signed-off-by: Borislav Petkov <[email protected]>
---
Documentation/process/submitting-patches.rst | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index 5ba54120bef7..0ffb21366381 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -679,6 +679,26 @@ generates appropriate diffstats by default.)
See more details on the proper patch format in the following
references.
+Backtraces in commit mesages
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Backtraces help document the call chain leading to a problem. However,
+not all backtraces are helpful. For example, early boot call chains are
+unique and obvious. Copying the full dmesg output verbatim, however,
+adds distracting information like timestamps, module lists, register and
+stack dumps.
+
+Therefore, the most useful backtraces should distill the relevant
+information from the dump, which makes it easier to focus on the real
+issue. Here is an example of a well-trimmed backtrace::
+
+ unchecked MSR access error: WRMSR to 0xd51 (tried to write 0x0000000000000064)
+ at rIP: 0xffffffffae059994 (native_write_msr+0x4/0x20)
+ Call Trace:
+ mba_wrmsr
+ update_domains
+ rdtgroup_mkdir
+
.. _explicit_in_reply_to:
Explicit In-Reply-To headers
--
2.29.2
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Dec 22, 2020, Borislav Petkov wrote:
> Ok, here's the next one which I think, is also, not really controversial.
Heh, are you trying to jinx yourself?
> diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
> index 5ba54120bef7..0ffb21366381 100644
> --- a/Documentation/process/submitting-patches.rst
> +++ b/Documentation/process/submitting-patches.rst
> @@ -679,6 +679,26 @@ generates appropriate diffstats by default.)
> See more details on the proper patch format in the following
> references.
>
> +Backtraces in commit mesages
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Backtraces help document the call chain leading to a problem. However,
> +not all backtraces are helpful. For example, early boot call chains are
> +unique and obvious.
I'd argue that there is still value in the backtrace though, e.g. I find them
very helpful when doing git archaeology. A backtrace is an easily recognizable
signature (don't have to read a bunch of text to understand there was a splat of
some kind), and the call stack is often helpful even if it is unique, e.g. for
unfamiliar code (including early boot chains) and/or code that is substantially
different from the current upstream.
> Copying the full dmesg output verbatim, however,
> +adds distracting information like timestamps, module lists, register and
> +stack dumps.
> +
> +Therefore, the most useful backtraces should distill the relevant
> +information from the dump, which makes it easier to focus on the real
> +issue. Here is an example of a well-trimmed backtrace::
> +
> + unchecked MSR access error: WRMSR to 0xd51 (tried to write 0x0000000000000064)
> + at rIP: 0xffffffffae059994 (native_write_msr+0x4/0x20)
> + Call Trace:
> + mba_wrmsr
> + update_domains
> + rdtgroup_mkdir
> +
> .. _explicit_in_reply_to:
I'd prefer not to encourage people to strip the info after the function name,
though I do agree it's somewhat distracting (especially the offset/size). The
module, call site in the function, exact file/line if available, etc... provides
context that I find helpful for building a mental model of what went wrong.
E.g. which modules are in play, which short wrapper functions can likely be
glossed over, etc...
On Tue, Dec 22, 2020 at 10:59:22AM -0800, Sean Christopherson wrote:
> On Tue, Dec 22, 2020, Borislav Petkov wrote:
> > Ok, here's the next one which I think, is also, not really controversial.
>
> Heh, are you trying to jinx yourself?
I was trying to conjure up some bikeshedding... and there it is! :-)
> > +Backtraces help document the call chain leading to a problem. However,
> > +not all backtraces are helpful. For example, early boot call chains are
> > +unique and obvious.
>
> I'd argue that there is still value in the backtrace though, e.g. I find them
> very helpful when doing git archaeology. A backtrace is an easily recognizable
> signature (don't have to read a bunch of text to understand there was a splat of
> some kind), and the call stack is often helpful even if it is unique, e.g. for
> unfamiliar code (including early boot chains) and/or code that is substantially
> different from the current upstream.
I think the intent of the text is to say not to include callchains which
are *really* obvious. As in, there's no ambiguity as to how one has
landed here.
Also, sometimes people paste backtraces from a WARN* which are almost
always superfluous - only the warn's address is important. This is at
least how I go about debugging those.
Maybe the text should be made more precise.
> I'd prefer not to encourage people to strip the info after the function name,
> though I do agree it's somewhat distracting (especially the offset/size).
Yes. Especially since they don't make any sense on another system or
even on the same system but with a different .config.
> The module, call site in the function, exact file/line if available,
> etc... provides context that I find helpful for building a mental
> model of what went wrong.
File/line is more useful, yes, but only for the current code snapshot.
When time passes and stuff gets changed, those file/line things are not
correct anymore so one would have to checkout the tree on which the
splat happened.
I guess I need to make that aspect more precise too.
> E.g. which modules are in play, which short wrapper functions can
> likely be glossed over, etc...
That example doesn't have modules. I guess I'll generate a new one.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Dec 22, 2020, Borislav Petkov wrote:
> On Tue, Dec 22, 2020 at 10:59:22AM -0800, Sean Christopherson wrote:
> > On Tue, Dec 22, 2020, Borislav Petkov wrote:
> > > +Backtraces help document the call chain leading to a problem. However,
> > > +not all backtraces are helpful. For example, early boot call chains are
> > > +unique and obvious.
> >
> > I'd argue that there is still value in the backtrace though, e.g. I find them
> > very helpful when doing git archaeology. A backtrace is an easily recognizable
> > signature (don't have to read a bunch of text to understand there was a splat of
> > some kind), and the call stack is often helpful even if it is unique, e.g. for
> > unfamiliar code (including early boot chains) and/or code that is substantially
> > different from the current upstream.
>
> I think the intent of the text is to say not to include callchains which
> are *really* obvious. As in, there's no ambiguity as to how one has
> landed here.
>
> Also, sometimes people paste backtraces from a WARN* which are almost
> always superfluous - only the warn's address is important. This is at
> least how I go about debugging those.
Obvious and superfluous for people that are intimately familiar with the code,
but explicit call stacks are extremely helpful when (re)learning code. Using
the boot code as an example, until one fully understands all the function
pointer shenanigans for the real mode trampoline, initial_code, CPU HP, etc...,
even 100% unambiguous call chains may not be immediately obvious. IMO, a few
"unnecessary" lines in the changelog is a worthwhile tradeoff if it's helpful to
even one person, now or in the future.
> Maybe the text should be made more precise.
>
> > I'd prefer not to encourage people to strip the info after the function name,
> > though I do agree it's somewhat distracting (especially the offset/size).
>
> Yes. Especially since they don't make any sense on another system or
> even on the same system but with a different .config.
>
> > The module, call site in the function, exact file/line if available,
> > etc... provides context that I find helpful for building a mental
> > model of what went wrong.
>
> File/line is more useful, yes, but only for the current code snapshot.
> When time passes and stuff gets changed, those file/line things are not
> correct anymore so one would have to checkout the tree on which the
> splat happened.
I don't think that's a reason to encourage stripping that info though, e.g. I'll
often checkout the relevant tree when I'm deep down the blame rabbithole.
Similar to my above argument regarding call stacks, the cost of including the
may-be-stale file/line info is outweighed by the potential benefits for readers
and/or future archaeologists.
> I guess I need to make that aspect more precise too.
On Mon, Dec 28, 2020 at 09:59:48AM -0800, Sean Christopherson wrote:
> Obvious and superfluous for people that are intimately familiar with the code,
> but explicit call stacks are extremely helpful when (re)learning code.
Here's an example:
[ 2.649874] x86/mm: Checked W+X mappings: passed, no W+X pages found.
[ 2.651774] ------------[ cut here ]------------
[ 2.652726] WARNING: CPU: 2 PID: 1 at init/main.c:1436 kernel_init+0x58/0x107
[ 2.653643] Modules linked in:
[ 2.654260] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.11.0-rc1+ #2
[ 2.654396] usb 1-1: new full-speed USB device number 2 using uhci_hcd
[ 2.655125] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1 04/01/2014
[ 2.657687] RIP: 0010:kernel_init+0x58/0x107
[ 2.658391] Code: 5e fc ff eb 0c 48 c7 c7 18 bc 09 82 e8 8a a4 fc ff c7 05 a2 5f b0 07 02 00 00 00 e8 a9 1c 9b ff e8 b4 dc 89 ff e8 2f d3 a6 ff <0f> 0b 48 8b 3d 76 87 9d 00 48 85 ff 74 22 e8 ef 19 fc ff 85 c0 89
[ 2.660707] RSP: 0018:ffffc90000013f50 EFLAGS: 00010292
[ 2.661499] RAX: ffff88800ebcdc01 RBX: ffffffff81837b59 RCX: 0000000000000802
[ 2.662469] RDX: 00000000000007f2 RSI: 05b5d427c1a6d18d RDI: 000000000002d100
[ 2.663411] RBP: 0000000000000000 R08: 000000000000000d R09: 000000000000000c
[ 2.664318] R10: 000000000000000d R11: 0000000000000020 R12: 0000000000000000
[ 2.666458] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[ 2.667462] FS: 0000000000000000(0000) GS:ffff88807da80000(0000) knlGS:0000000000000000
[ 2.668472] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2.669257] CR2: 0000000000000000 CR3: 000000000220a000 CR4: 00000000003506e0
[ 2.670183] Call Trace:
[ 2.670723] ret_from_fork+0x22/0x30
[ 2.671340] ---[ end trace 385e10f347736cf9 ]---
Yes, useless. The callstack is not even there.
This is the only line that matters:
[ 2.652726] WARNING: CPU: 2 PID: 1 at init/main.c:1436 kernel_init+0x58/0x107
because what caused it is:
diff --git a/init/main.c b/init/main.c
index 6feee7f11eaf..aa52355d98ae 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1433,6 +1433,8 @@ static int __ref kernel_init(void *unused)
do_sysctl_args();
+ WARN_ON_ONCE(1);
+
if (ramdisk_execute_command) {
ret = run_init_process(ramdisk_execute_command);
if (!ret)
even if you had a stack trace here, it would be totally useless because
you *know* where it came from.
But ok, I can change the formulation that it is up to the committer's
judgement to decide whether to remove that blurb. Because I definitely
will be removing a useless noise like that before committing.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Dec 28, 2020, Borislav Petkov wrote:
> On Mon, Dec 28, 2020 at 09:59:48AM -0800, Sean Christopherson wrote:
> > Obvious and superfluous for people that are intimately familiar with the code,
> > but explicit call stacks are extremely helpful when (re)learning code.
>
> Here's an example:
>
> [ 2.649874] x86/mm: Checked W+X mappings: passed, no W+X pages found.
> [ 2.651774] ------------[ cut here ]------------
> [ 2.652726] WARNING: CPU: 2 PID: 1 at init/main.c:1436 kernel_init+0x58/0x107
> [ 2.653643] Modules linked in:
> [ 2.654260] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.11.0-rc1+ #2
> [ 2.654396] usb 1-1: new full-speed USB device number 2 using uhci_hcd
> [ 2.655125] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1 04/01/2014
> [ 2.657687] RIP: 0010:kernel_init+0x58/0x107
> [ 2.658391] Code: 5e fc ff eb 0c 48 c7 c7 18 bc 09 82 e8 8a a4 fc ff c7 05 a2 5f b0 07 02 00 00 00 e8 a9 1c 9b ff e8 b4 dc 89 ff e8 2f d3 a6 ff <0f> 0b 48 8b 3d 76 87 9d 00 48 85 ff 74 22 e8 ef 19 fc ff 85 c0 89
> [ 2.660707] RSP: 0018:ffffc90000013f50 EFLAGS: 00010292
> [ 2.661499] RAX: ffff88800ebcdc01 RBX: ffffffff81837b59 RCX: 0000000000000802
> [ 2.662469] RDX: 00000000000007f2 RSI: 05b5d427c1a6d18d RDI: 000000000002d100
> [ 2.663411] RBP: 0000000000000000 R08: 000000000000000d R09: 000000000000000c
> [ 2.664318] R10: 000000000000000d R11: 0000000000000020 R12: 0000000000000000
> [ 2.666458] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> [ 2.667462] FS: 0000000000000000(0000) GS:ffff88807da80000(0000) knlGS:0000000000000000
> [ 2.668472] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 2.669257] CR2: 0000000000000000 CR3: 000000000220a000 CR4: 00000000003506e0
> [ 2.670183] Call Trace:
> [ 2.670723] ret_from_fork+0x22/0x30
> [ 2.671340] ---[ end trace 385e10f347736cf9 ]---
>
> Yes, useless. The callstack is not even there.
>
> This is the only line that matters:
>
> [ 2.652726] WARNING: CPU: 2 PID: 1 at init/main.c:1436 kernel_init+0x58/0x107
>
> because what caused it is:
>
> diff --git a/init/main.c b/init/main.c
> index 6feee7f11eaf..aa52355d98ae 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1433,6 +1433,8 @@ static int __ref kernel_init(void *unused)
>
> do_sysctl_args();
>
> + WARN_ON_ONCE(1);
> +
> if (ramdisk_execute_command) {
> ret = run_init_process(ramdisk_execute_command);
> if (!ret)
>
>
> even if you had a stack trace here, it would be totally useless because
> you *know* where it came from.
>
> But ok, I can change the formulation that it is up to the committer's
> judgement to decide whether to remove that blurb. Because I definitely
> will be removing a useless noise like that before committing.
I 100% agree in this specific case. What I'm arguing is that cases like this
where the call stack is completely uninteresting are few and far between. The
documentation should focus on the common case where, IMO, the call stack should
be included, even if it might be obvious for some/many people. E.g. similar to
how many kconfig options have something like "If unsure, say Y", I think the
blurb should encourage including the call stack unless the author is 100% sure
that it's useless.
[Digging out from under the pile of mail...]
> From: Borislav Petkov <[email protected]>
> Date: Tue, 22 Dec 2020 13:58:22 +0100
>
> Document that backtraces in commit messages should be trimmed down to
> the useful information only.
>
> This has been carved out from a tip subsystem handbook patchset by
> Thomas Gleixner:
>
> https://lkml.kernel.org/r/[email protected]
>
> and incorporates follow-on comments.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> Documentation/process/submitting-patches.rst | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
> index 5ba54120bef7..0ffb21366381 100644
> --- a/Documentation/process/submitting-patches.rst
> +++ b/Documentation/process/submitting-patches.rst
> @@ -679,6 +679,26 @@ generates appropriate diffstats by default.)
> See more details on the proper patch format in the following
> references.
>
> +Backtraces in commit mesages
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Backtraces help document the call chain leading to a problem. However,
> +not all backtraces are helpful. For example, early boot call chains are
> +unique and obvious. Copying the full dmesg output verbatim, however,
> +adds distracting information like timestamps, module lists, register and
> +stack dumps.
> +
> +Therefore, the most useful backtraces should distill the relevant
> +information from the dump, which makes it easier to focus on the real
> +issue. Here is an example of a well-trimmed backtrace::
> +
> + unchecked MSR access error: WRMSR to 0xd51 (tried to write 0x0000000000000064)
> + at rIP: 0xffffffffae059994 (native_write_msr+0x4/0x20)
> + Call Trace:
> + mba_wrmsr
> + update_domains
> + rdtgroup_mkdir
> +
So I have some questions, I guess... How often is a backtrace *in a commit
message* really helpful at all? The value in problem reports is clear, but
I'm not sure how often having a backtrace in a commit message will really
help the reader understand why the patch was written. But perhaps I'm
wrong?
If we do want this advice in our already-too-long submitting-patches
document, we should perhaps give some advice as to what is "relevant
information" and what is not?
Thanks,
jon
On Mon, Jan 04, 2021 at 04:19:11PM -0700, Jonathan Corbet wrote:
> So I have some questions, I guess... How often is a backtrace *in a commit
> message* really helpful at all? The value in problem reports is clear, but
> I'm not sure how often having a backtrace in a commit message will really
> help the reader understand why the patch was written. But perhaps I'm
> wrong?
Does the subthread here with Sean shed some light on the matter or... ?
> If we do want this advice in our already-too-long submitting-patches
> document,
Thought the same thing when looking at that doc - it is a *lot* and I
guess we should put only very globally relevant info in there...
> we should perhaps give some advice as to what is "relevant
> information" and what is not?
Right, in that subthread, the gist of what we wanna say is to almost
always put the splat in the commit message - except for the example I
gave there and other early boot cases - but leave it to the committer to
do the final decision whether to keep or ditch the splat.
Something like that. Yah, I know, it is fuzzy :-\
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Jan 05, 2021 at 11:48:05AM +0100, Borislav Petkov wrote:
> On Mon, Jan 04, 2021 at 04:19:11PM -0700, Jonathan Corbet wrote:
> > So I have some questions, I guess... How often is a backtrace *in a commit
> > message* really helpful at all? The value in problem reports is clear, but
> > I'm not sure how often having a backtrace in a commit message will really
> > help the reader understand why the patch was written. But perhaps I'm
> > wrong?
>
> Does the subthread here with Sean shed some light on the matter or... ?
>
> > If we do want this advice in our already-too-long submitting-patches
> > document,
>
> Thought the same thing when looking at that doc - it is a *lot* and I
> guess we should put only very globally relevant info in there...
>
> > we should perhaps give some advice as to what is "relevant
> > information" and what is not?
>
> Right, in that subthread, the gist of what we wanna say is to almost
> always put the splat in the commit message - except for the example I
> gave there and other early boot cases - but leave it to the committer to
> do the final decision whether to keep or ditch the splat.
>
> Something like that. Yah, I know, it is fuzzy :-\
Lemme ping here quick - my TODO list still has it. :-)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Borislav Petkov <[email protected]> writes:
> On Tue, Jan 05, 2021 at 11:48:05AM +0100, Borislav Petkov wrote:
> Lemme ping here quick - my TODO list still has it. :-)
Yeah, it's been languishing on mine as well. Nobody seems to have any
objections, so I applied it, sorry for sitting on it for so long.
Thanks,
jon
From: Borislav Petkov <[email protected]>
Subject: [PATCH] Documentation/submitting-patches: Extend commit message layout description
Add more blurb about the level of detail that should be contained in a
patch's commit message. Extend and make more explicit what text should
be added under the --- line. Extend examples and split into more easily
palatable paragraphs.
This has been partially carved out from a tip subsystem handbook
patchset by Thomas Gleixner:
https://lkml.kernel.org/r/[email protected]
and incorporates follow-on comments.
Signed-off-by: Borislav Petkov <[email protected]>
---
/me sends the next generic topic blurb.
Documentation/process/submitting-patches.rst | 89 ++++++++++++--------
1 file changed, 56 insertions(+), 33 deletions(-)
diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index 5ca072f9ecb7..403784aca1f2 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -623,16 +623,19 @@ not considered part of the summary phrase, but describe how the patch
should be treated. Common tags might include a version descriptor if
the multiple versions of the patch have been sent out in response to
comments (i.e., "v1, v2, v3"), or "RFC" to indicate a request for
-comments. If there are four patches in a patch series the individual
-patches may be numbered like this: 1/4, 2/4, 3/4, 4/4. This assures
-that developers understand the order in which the patches should be
-applied and that they have reviewed or applied all of the patches in
-the patch series.
+comments.
+
+If there are four patches in a patch series the individual patches may
+be numbered like this: 1/4, 2/4, 3/4, 4/4. This assures that developers
+understand the order in which the patches should be applied and that
+they have reviewed or applied all of the patches in the patch series.
A couple of example Subjects::
Subject: [PATCH 2/5] ext2: improve scalability of bitmap searching
Subject: [PATCH v2 01/27] x86: fix eflags tracking
+ Subject: [PATCH v2] sub/sys: Condensed patch summary
+ Subject: [PATCH v2 M/N] sub/sys: Condensed patch summary
The ``from`` line must be the very first line in the message body,
and has the form:
@@ -645,34 +648,54 @@ then the ``From:`` line from the email header will be used to determine
the patch author in the changelog.
The explanation body will be committed to the permanent source
-changelog, so should make sense to a competent reader who has long
-since forgotten the immediate details of the discussion that might
-have led to this patch. Including symptoms of the failure which the
-patch addresses (kernel log messages, oops messages, etc.) is
-especially useful for people who might be searching the commit logs
-looking for the applicable patch. If a patch fixes a compile failure,
-it may not be necessary to include _all_ of the compile failures; just
-enough that it is likely that someone searching for the patch can find
-it. As in the ``summary phrase``, it is important to be both succinct as
-well as descriptive.
-
-The ``---`` marker line serves the essential purpose of marking for patch
-handling tools where the changelog message ends.
-
-One good use for the additional comments after the ``---`` marker is for
-a ``diffstat``, to show what files have changed, and the number of
-inserted and deleted lines per file. A ``diffstat`` is especially useful
-on bigger patches. Other comments relevant only to the moment or the
-maintainer, not suitable for the permanent changelog, should also go
-here. A good example of such comments might be ``patch changelogs``
-which describe what has changed between the v1 and v2 version of the
-patch.
-
-If you are going to include a ``diffstat`` after the ``---`` marker, please
-use ``diffstat`` options ``-p 1 -w 70`` so that filenames are listed from
-the top of the kernel source tree and don't use too much horizontal
-space (easily fit in 80 columns, maybe with some indentation). (``git``
-generates appropriate diffstats by default.)
+changelog, so should make sense to a competent reader who has long since
+forgotten the immediate details of the discussion that might have led to
+this patch. Including symptoms of the failure which the patch addresses
+(kernel log messages, oops messages, etc.) are especially useful for
+people who might be searching the commit logs looking for the applicable
+patch. The text should be written in such detail so that when read
+weeks, months or even years later, it can give the reader the needed
+details to grasp the reasoning for **why** the patch was created.
+
+If a patch fixes a compile failure, it may not be necessary to include
+_all_ of the compile failures; just enough that it is likely that
+someone searching for the patch can find it. As in the ``summary
+phrase``, it is important to be both succinct as well as descriptive.
+
+The ``---`` marker line serves the essential purpose of marking for
+patch handling tools where the changelog message ends.
+
+One good use for the additional comments after the ``---`` marker is
+for a ``diffstat``, to show what files have changed, and the number of
+inserted and deleted lines per file. A ``diffstat`` is especially useful
+on bigger patches. If you are going to include a ``diffstat`` after the
+``---`` marker, please use ``diffstat`` options ``-p 1 -w 70`` so that
+filenames are listed from the top of the kernel source tree and don't
+use too much horizontal space (easily fit in 80 columns, maybe with some
+indentation). (``git`` generates appropriate diffstats by default.)
+
+Other comments relevant only to the moment or the maintainer, not
+suitable for the permanent changelog, should also go here. A good
+example of such comments might be ``patch changelogs`` which describe
+what has changed between the v1 and v2 version of the patch.
+
+Please put this information **after** the ``---`` line which separates
+the changelog from the rest of the patch. The version information is
+not part of the changelog which gets committed to the git tree. It is
+additional information for the reviewers. If it's placed above the
+commit tags, it needs manual interaction to remove it. If it is below
+the separator line, it gets automatically stripped off when applying the
+patch::
+
+ <commit message>
+ ...
+ Signed-off-by: Author <author@mail>
+ ---
+ V2 -> V3: Removed redundant helper function
+ V1 -> V2: Cleaned up coding style and addressed review comments
+
+ path/to/file | 5+++--
+ ...
See more details on the proper patch format in the following
references.
--
2.29.2
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 15.02.21 15:19:49, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
> Subject: [PATCH] Documentation/submitting-patches: Extend commit message layout description
>
> Add more blurb about the level of detail that should be contained in a
> patch's commit message. Extend and make more explicit what text should
> be added under the --- line. Extend examples and split into more easily
> palatable paragraphs.
>
> This has been partially carved out from a tip subsystem handbook
> patchset by Thomas Gleixner:
>
> https://lkml.kernel.org/r/[email protected]
>
> and incorporates follow-on comments.
>
> Signed-off-by: Borislav Petkov <[email protected]>
Reviewed-by: Robert Richter <[email protected]>
Borislav Petkov <[email protected]> writes:
> From: Borislav Petkov <[email protected]>
> Subject: [PATCH] Documentation/submitting-patches: Extend commit message layout description
>
> Add more blurb about the level of detail that should be contained in a
> patch's commit message. Extend and make more explicit what text should
> be added under the --- line. Extend examples and split into more easily
> palatable paragraphs.
>
> This has been partially carved out from a tip subsystem handbook
> patchset by Thomas Gleixner:
>
> https://lkml.kernel.org/r/[email protected]
>
> and incorporates follow-on comments.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
>
> /me sends the next generic topic blurb.
>
> Documentation/process/submitting-patches.rst | 89 ++++++++++++--------
> 1 file changed, 56 insertions(+), 33 deletions(-)
Applied, with one tweak:
> +If there are four patches in a patch series the individual patches may
> +be numbered like this: 1/4, 2/4, 3/4, 4/4. This assures that developers
> +understand the order in which the patches should be applied and that
> +they have reviewed or applied all of the patches in the patch series.
>
> A couple of example Subjects::
>
> Subject: [PATCH 2/5] ext2: improve scalability of bitmap searching
> Subject: [PATCH v2 01/27] x86: fix eflags tracking
> + Subject: [PATCH v2] sub/sys: Condensed patch summary
> + Subject: [PATCH v2 M/N] sub/sys: Condensed patch summary
It's no longer "a couple" so I made this "Here are some good example
Subjects".
Thanks,
jon
Hi Jon,
here's the next piece of documentation which should be generic enough.
Thx.
---
From: Borislav Petkov <[email protected]>
Date: Tue, 13 Apr 2021 13:26:29 +0200
Explain when a submitter should tag a patch or a patch series with the
"RESEND" tag.
This has been partially carved out from a tip subsystem handbook
patchset by Thomas Gleixner:
https://lkml.kernel.org/r/[email protected]
and incorporates follow-on comments.
Signed-off-by: Borislav Petkov <[email protected]>
---
Documentation/process/submitting-patches.rst | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index ab92d9ccd39a..9284735e0b34 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -346,6 +346,16 @@ that you have sent your patches to the right place. Wait for a minimum of
one week before resubmitting or pinging reviewers - possibly longer during
busy times like merge windows.
+It's also ok to resend the patch or the patch series after a couple of
+weeks with the word "RESEND" added to the subject line::
+
+ [PATCH Vx RESEND] sub/sys: Condensed patch summary
+
+Don't add "RESEND" when you are submitting a modified version of your
+patch or patch series - "RESEND" only applies to resubmission of a
+patch or patch series which have not been modified in any way from the
+previous submission.
+
Include PATCH in the subject
-----------------------------
--
2.29.2
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Borislav Petkov <[email protected]> writes:
> Hi Jon,
>
> here's the next piece of documentation which should be generic enough.
>
> Thx.
>
> ---
> From: Borislav Petkov <[email protected]>
> Date: Tue, 13 Apr 2021 13:26:29 +0200
>
> Explain when a submitter should tag a patch or a patch series with the
> "RESEND" tag.
>
> This has been partially carved out from a tip subsystem handbook
> patchset by Thomas Gleixner:
>
> https://lkml.kernel.org/r/[email protected]
>
> and incorporates follow-on comments.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> Documentation/process/submitting-patches.rst | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
> index ab92d9ccd39a..9284735e0b34 100644
> --- a/Documentation/process/submitting-patches.rst
> +++ b/Documentation/process/submitting-patches.rst
> @@ -346,6 +346,16 @@ that you have sent your patches to the right place. Wait for a minimum of
> one week before resubmitting or pinging reviewers - possibly longer during
> busy times like merge windows.
>
> +It's also ok to resend the patch or the patch series after a couple of
> +weeks with the word "RESEND" added to the subject line::
> +
> + [PATCH Vx RESEND] sub/sys: Condensed patch summary
> +
> +Don't add "RESEND" when you are submitting a modified version of your
> +patch or patch series - "RESEND" only applies to resubmission of a
> +patch or patch series which have not been modified in any way from the
> +previous submission.
> +
Makes sense, applied.
For future installments, could you send them in their own thread as an
ordinary patch so I don't need to edit in the changelog after applying
them?
Thanks,
jon
On Tue, Apr 13, 2021 at 03:02:21PM -0600, Jonathan Corbet wrote:
> For future installments, could you send them in their own thread as an
> ordinary patch so I don't need to edit in the changelog after applying
> them?
Ok, sure but I might not need to anymore because, AFAICT, what is left
is really tip-tree specific and that can finally be the tip-tree doc
file.
I'm pasting the whole thing below, if you think something's still
generic enough, lemme know and I'll carve it out.
Thx.
---
.. SPDX-License-Identifier: GPL-2.0
The tip tree handbook
=====================
What is the tip tree?
---------------------
The tip tree is a collection of several subsystems and areas of
development. The tip tree is both a direct development tree and a
aggregation tree for several sub-maintainer trees. The tip tree gitweb URL
is: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
The tip tree contains the following subsystems:
- **x86 architecture**
The x86 architecture development takes place in the tip tree except
for the x86 KVM and XEN specific parts which are maintained in the
corresponding subsystems and routed directly to mainline from
there. It's still good practice to Cc the x86 maintainers on
x86-specific KVM and XEN patches.
Some x86 subsystems have their own maintainers in addition to the
overall x86 maintainers. Please Cc the overall x86 maintainers on
patches touching files in arch/x86 even when they are not called out
by the MAINTAINER file.
Note, that ``[email protected]`` is not a mailing list. It is merely a
mail alias which distributes mails to the x86 top-level maintainer
team. Please always Cc the Linux Kernel mailing list (LKML)
``[email protected]``, otherwise your mail ends up only in
the private inboxes of the maintainers.
- **Scheduler**
Scheduler development takes place in the -tip tree, in the
sched/core branch - with occasional sub-topic trees for
work-in-progress patch-sets.
- **Locking and atomics**
Locking development (including atomics and other synchronization
primitives that are connected to locking) takes place in the -tip
tree, in the locking/core branch - with occasional sub-topic trees
for work-in-progress patch-sets.
- **Generic interrupt subsystem and interrupt chip drivers**:
- interrupt core development happens in the irq/core branch
- interrupt chip driver development also happens in the irq/core
branch, but the patches are usually applied in a separate maintainer
tree and then aggregated into irq/core
- **Time, timers, timekeeping, NOHZ and related chip drivers**:
- timekeeping, clocksource core, NTP and alarmtimer development
happens in the timers/core branch, but patches are usually applied in
a separate maintainer tree and then aggregated into timers/core
- clocksource/event driver development happens in the timers/core
branch, but patches are mostly applied in a separate maintainer tree
and then aggregated into timers/core
- **Performance counters core, architecture support and tooling**:
- perf core and architecture support development happens in the
perf/core branch
- perf tooling development happens in the perf tools maintainer
tree and is aggregated into the tip tree.
- **CPU hotplug core**
- **RAS core**
- **EFI core**
EFI development in the efi git tree. The collected patches are
aggregated in the tip efi/core branch.
- **RCU**
RCU development happens in the linux-rcu tree. The resulting changes
are aggregated into the tip core/rcu branch.
- **Various core code components**:
- debugobjects
- objtool
- random bits and pieces
Patch submission notes
----------------------
Selecting the tree/branch
^^^^^^^^^^^^^^^^^^^^^^^^^
In general, development against the head of the tip tree master branch is
fine, but for the subsystems which are maintained separately, have their
own git tree and are only aggregated into the tip tree, development should
take place against the relevant subsystem tree or branch.
Bug fixes which target mainline should always be applicable against the
mainline kernel tree. Potential conflicts against changes which are already
queued in the tip tree are handled by the maintainers.
Patch subject
^^^^^^^^^^^^^
The tip tree preferred format for patch subject prefixes is
'subsys/component:', e.g. 'x86/apic:', 'x86/mm/fault:', 'sched/fair:',
'genirq/core:'. Please do not use file names or complete file paths as
prefix. 'git log path/to/file' should give you a reasonable hint in most
cases.
The condensed patch description in the subject line should start with a
uppercase letter and should be written in imperative tone.
Changelog
^^^^^^^^^
The general rules about changelogs in the process documentation, see
:ref:`Documentation/process/ <submittingpatches>`, apply.
The tip tree maintainers set value on following these rules, especially on
the request to write changelogs in imperative mood and not impersonating
code or the execution of it. This is not just a whim of the
maintainers. Changelogs written in abstract words are more precise and
tend to be less confusing than those written in the form of novels.
It's also useful to structure the changelog into several paragraphs and not
lump everything together into a single one. A good structure is to explain
the context, the problem and the solution in separate paragraphs and this
order.
Examples for illustration:
Example 1::
x86/intel_rdt/mbm: Fix MBM overflow handler during hot cpu
When a CPU is dying, we cancel the worker and schedule a new worker on a
different CPU on the same domain. But if the timer is already about to
expire (say 0.99s) then we essentially double the interval.
We modify the hot cpu handling to cancel the delayed work on the dying
cpu and run the worker immediately on a different cpu in same domain. We
donot flush the worker because the MBM overflow worker reschedules the
worker on same CPU and scans the domain->cpu_mask to get the domain
pointer.
Improved version::
x86/intel_rdt/mbm: Fix MBM overflow handler during CPU hotplug
When a CPU is dying, the overflow worker is canceled and rescheduled on a
different CPU in the same domain. But if the timer is already about to
expire this essentially doubles the interval which might result in a non
detected overflow.
Cancel the overflow worker and reschedule it immediately on a different CPU
in the same domain. The work could be flushed as well, but that would
reschedule it on the same CPU.
Example 2::
time: POSIX CPU timers: Ensure that variable is initialized
If cpu_timer_sample_group returns -EINVAL, it will not have written into
*sample. Checking for cpu_timer_sample_group's return value precludes the
potential use of an uninitialized value of now in the following block.
Given an invalid clock_idx, the previous code could otherwise overwrite
*oldval in an undefined manner. This is now prevented. We also exploit
short-circuiting of && to sample the timer only if the result will
actually be used to update *oldval.
Improved version::
posix-cpu-timers: Make set_process_cpu_timer() more robust
Because the return value of cpu_timer_sample_group() is not checked,
compilers and static checkers can legitimately warn about a potential use
of the uninitialized variable 'now'. This is not a runtime issue as all
call sites hand in valid clock ids.
Also cpu_timer_sample_group() is invoked unconditionally even when the
result is not used because *oldval is NULL.
Make the invocation conditional and check the return value.
Example 3::
The entity can also be used for other purposes.
Let's rename it to be more generic.
Improved version::
The entity can also be used for other purposes.
Rename it to be more generic.
For complex scenarios, especially race conditions and memory ordering
issues, it is valuable to depict the scenario with a table which shows
the parallelism and the temporal order of events. Here is an example::
CPU0 CPU1
free_irq(X) interrupt X
spin_lock(desc->lock)
wake irq thread()
spin_unlock(desc->lock)
spin_lock(desc->lock)
remove action()
shutdown_irq()
release_resources() thread_handler()
spin_unlock(desc->lock) access released resources.
^^^^^^^^^^^^^^^^^^^^^^^^^
synchronize_irq()
Lockdep provides similar useful output to depict a possible deadlock
scenario::
CPU0 CPU1
rtmutex_lock(&rcu->rt_mutex)
spin_lock(&rcu->rt_mutex.wait_lock)
local_irq_disable()
spin_lock(&timer->it_lock)
spin_lock(&rcu->mutex.wait_lock)
--> Interrupt
spin_lock(&timer->it_lock)
Function references in changelogs
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
When a function is mentioned in the changelog, either the text body or the
subject line, please use the format 'function_name()'. Omitting the
brackets after the function name can be ambiguous::
Subject: subsys/component: Make reservation_count static
reservation_count is only used in reservation_stats. Make it static.
The variant with brackets is more precise::
Subject: subsys/component: Make reservation_count() static
reservation_count() is only called from reservation_stats(). Make it
static.
Backtraces in changelogs
^^^^^^^^^^^^^^^^^^^^^^^^
(XXX: Add link to Documentation/process/submitting-patches.rst's section)
Ordering of commit tags
^^^^^^^^^^^^^^^^^^^^^^^
To have a uniform view of the commit tags, the tip maintainers use the
following tag ordering scheme:
- Fixes: 12char-SHA1 ("sub/sys: Original subject line")
A Fixes tag should be added even for changes which do not need to be
backported to stable kernels, i.e. when addressing a recently introduced
issue which only affects tip or the current head of mainline. These tags
are helpful to identify the original commit and are much more valuable
than prominently mentioning the commit which introduced a problem in the
text of the changelog itself because they can be automatically
extracted.
The following example illustrates the difference::
Commit
abcdef012345678 ("x86/xxx: Replace foo with bar")
left an unused instance of variable foo around. Remove it.
Signed-off-by: J.Dev <j.dev@mail>
Please say instead::
The recent replacement of foo with bar left an unused instance of
variable foo around. Remove it.
Fixes: abcdef012345678 ("x86/xxx: Replace foo with bar")
Signed-off-by: J.Dev <j.dev@mail>
The latter puts the information about the patch into the focus and
amends it with the reference to the commit which introduced the issue
rather than putting the focus on the original commit in the first place.
- Reported-by: ``Reporter <reporter@mail>``
- Originally-by: ``Original author <original-author@mail>``
- Suggested-by: ``Suggester <suggester@mail>``
- Co-developed-by: ``Co-author <co-author@mail>``
Signed-off: ``Co-author <co-author@mail>``
Note, that Co-developed-by and Signed-off-by of the co-author(s) must
come in pairs.
- Signed-off-by: ``Author <author@mail>``
The first Signed-off-by (SOB) after the last Co-developed-by/SOB pair is the
author SOB, i.e. the person flagged as author by git.
- Signed-off-by: ``Patch handler <handler@mail>``
SOBs after the author SOB are from people handling and transporting
the patch, but were not involved in development. SOB chains should
reflect the **real** route a patch took as it was propagated to us,
with the first SOB entry signalling primary authorship of a single
author. Acks should be given as Acked-by lines and review approvals
as Reviewed-by lines.
If the handler made modifications to the patch or the changelog, then
this should be mentioned **after** the changelog text and **above**
all commit tags in the following format::
... changelog text ends.
[ handler: Replaced foo by bar and updated changelog ]
First-tag: .....
Note the two empty new lines which separate the changelog text and the
commit tags from that notice.
If a patch is sent to the mailing list by a handler then the author has
to be noted in the first line of the changelog with::
From: Author <author@mail>
Changelog text starts here....
so the authorship is preserved. The 'From:' line has to be followed
by a empty newline. If that 'From:' line is missing, then the patch
would be attributed to the person who sent (transported, handled) it.
The 'From:' line is automatically removed when the patch is applied
and does not show up in the final git changelog. It merely affects
the authorship information of the resulting Git commit.
- Tested-by: ``Tester <tester@mail>``
- Reviewed-by: ``Reviewer <reviewer@mail>``
- Acked-by: ``Acker <acker@mail>``
- Cc: ``cc-ed-person <person@mail>``
If the patch should be backported to stable, then please add a '``Cc:
[email protected]``' tag, but do not Cc stable when sending your
mail.
- Link: ``https://link/to/information``
For referring to an email on LKML or other kernel mailing lists,
please use the lkml.kernel.org redirector URL::
https://lkml.kernel.org/r/email-message@id
The kernel.org redirector is considered a stable URL, unlike other email
archives.
Maintainers will add a Link tag referencing the email of the patch
submission when they apply a patch to the tip tree. This tag is useful
for later reference and is also used for commit notifications.
Please do not use combined tags, e.g. ``Reported-and-tested-by``, as
they just complicate automated extraction of tags.
Links to documentation
^^^^^^^^^^^^^^^^^^^^^^
Providing links to documentation in the changelog is a great help to later
debugging and analysis. Unfortunately, URLs often break very quickly
because companies restructure their websites frequently. Non-'volatile'
exceptions include the Intel SDM and the AMD APM.
Therefore, for 'volatile' documents, please create an entry in the kernel
bugzilla https://bugzilla.kernel.org and attach a copy of these documents
to the bugzilla entry. Finally, provide the URL of the bugzilla entry in
the changelog.
Patch version information
^^^^^^^^^^^^^^^^^^^^^^^^^
(XXX: Add link to Documentation/process/submitting-patches.rst's section)
Patch resend or reminders
^^^^^^^^^^^^^^^^^^^^^^^^^
(XXX: point this to "Don't get discouraged - or impatient" section in
Documentation/process/submitting-patches.rst)
Merge window
^^^^^^^^^^^^
Please do not expect large patch series to be handled during the merge
window or even during the week before. Such patches should be submitted in
mergeable state *at* *least* a week before the merge window opens.
Exceptions are made for bug fixes and *sometimes* for small standalone
drivers for new hardware or minimally invasive patches for hardware
enablement.
During the merge window, the maintainers instead focus on following the
upstream changes, fixing merge window fallout, collecting bug fixes, and
allowing themselves a breath. Please respect that.
The release candidate -rc1 is the starting point for new patches to be
applied which are targeted for the next merge window.
Git
^^^
The tip maintainers accept git pull requests from maintainers who provide
subsystem changes for aggregation in the tip tree.
Pull requests for new patch submissions are usually not accepted and do not
replace proper patch submission to the mailing list. The main reason for
this is that the review workflow is email based.
If you submit a larger patch series it is helpful to provide a git branch
in a private repository which allows interested people to easily pull the
series for testing. The usual way to offer this is a git URL in the cover
letter of the patch series.
Coding style notes
------------------
Comment style
^^^^^^^^^^^^^
Sentences in comments start with an uppercase letter.
Single line comments::
/* This is a single line comment */
Multi-line comments::
/*
* This is a properly formatted
* multi-line comment.
*
* Larger multi-line comments should be split into paragraphs.
*/
No tail comments:
Please refrain from using tail comments. Tail comments disturb the
reading flow in almost all contexts, but especially in code::
if (somecondition_is_true) /* Don't put a comment here */
dostuff(); /* Neither here */
seed = MAGIC_CONSTANT; /* Nor here */
Use freestanding comments instead::
/* This condition is not obvious without a comment */
if (somecondition_is_true) {
/* This really needs to be documented */
dostuff();
}
/* This magic initialization needs a comment. Maybe not? */
seed = MAGIC_CONSTANT;
Comment the important things:
Comments should be added where the operation is not obvious. Documenting
the obvious is just a distraction::
/* Decrement refcount and check for zero */
if (refcount_dec_and_test(&p->refcnt)) {
do;
lots;
of;
magic;
things;
}
Instead, comments should explain the non-obvious details and document
constraints::
if (refcount_dec_and_test(&p->refcnt)) {
/*
* Really good explanation why the magic things below
* need to be done, ordering and locking constraints,
* etc..
*/
do;
lots;
of;
magic;
/* Needs to be the last operation because ... */
things;
}
Function documentation comments:
To document functions and their arguments please use kernel-doc format
and not free form comments::
/**
* magic_function - Do lots of magic stuff
* @magic: Pointer to the magic data to operate on
* @offset: Offset in the data array of @magic
*
* Deep explanation of mysterious things done with @magic along
* with documentation of the return values.
*
* Note, that the argument descriptors above are arranged
* in a tabular fashion.
*/
This applies especially to globally visible functions and inline
functions in public header files. It might be overkill to use kernel-doc
format for every (static) function which needs a tiny explanation. The
usage of descriptive function names often replaces these tiny comments.
Apply common sense as always.
Documenting locking requirements
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Documenting locking requirements is a good thing, but comments are not
necessarily the best choice. Instead of writing::
/* Caller must hold foo->lock */
void func(struct foo *foo)
{
...
}
Please use::
void func(struct foo *foo)
{
lockdep_assert_held(&foo->lock);
...
}
In PROVE_LOCKING kernels, lockdep_assert_held() emits a warning
if the caller doesn't hold the lock. Comments can't do that.
Bracket rules
^^^^^^^^^^^^^
Brackets should be omitted only if the statement which follows 'if', 'for',
'while' etc. is truly a single line::
if (foo)
do_something();
The following is not considered to be a single line statement even
though C does not require brackets::
for (i = 0; i < end; i++)
if (foo[i])
do_something(foo[i]);
Adding brackets around the outer loop enhances the reading flow::
for (i = 0; i < end; i++) {
if (foo[i])
do_something(foo[i]);
}
Variable declarations
^^^^^^^^^^^^^^^^^^^^^
The preferred ordering of variable declarations at the beginning of a
function is reverse fir tree order::
struct long_struct_name *descriptive_name;
unsigned long foo, bar;
unsigned int tmp;
int ret;
The above is faster to parse than the reverse ordering::
int ret;
unsigned int tmp;
unsigned long foo, bar;
struct long_struct_name *descriptive_name;
And even more so than random ordering::
unsigned long foo, bar;
int ret;
struct long_struct_name *descriptive_name;
unsigned int tmp;
Also please try to aggregate variables of the same type into a single
line. There is no point in wasting screen space::
unsigned long a;
unsigned long b;
unsigned long c;
unsigned long d;
It's really sufficient to do::
unsigned long a, b, c, d;
Please also refrain from introducing line splits in variable declarations::
struct long_struct_name *descriptive_name = container_of(bar,
struct long_struct_name,
member);
struct foobar foo;
It's way better to move the initialization to a separate line after the
declarations::
struct long_struct_name *descriptive_name;
struct foobar foo;
descriptive_name = container_of(bar, struct long_struct_name, member);
Variable types
^^^^^^^^^^^^^^
Please use the proper u8, u16, u32, u64 types for variables which are meant
to describe hardware or are used as arguments for functions which access
hardware. These types are clearly defining the bit width and avoid
truncation, expansion and 32/64-bit confusion.
u64 is also recommended in code which would become ambiguous for 32-bit
kernels when 'unsigned long' would be used instead. While in such
situations 'unsigned long long' could be used as well, u64 is shorter
and also clearly shows that the operation is required to be 64 bits wide
independent of the target CPU.
Please use 'unsigned int' instead of 'unsigned'.
Constants
^^^^^^^^^
Please do not use literal (hexa)decimal numbers in code or initializers.
Either use proper defines which have descriptive names or consider using
an enum.
Struct declarations and initializers
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Struct declarations should align the struct member names in a tabular
fashion::
struct bar_order {
unsigned int guest_id;
int ordered_item;
struct menu *menu;
};
Please avoid documenting struct members within the declaration, because
this often results in strangely formatted comments and the struct members
become obfuscated::
struct bar_order {
unsigned int guest_id; /* Unique guest id */
int ordered_item;
/* Pointer to a menu instance which contains all the drinks */
struct menu *menu;
};
Instead, please consider using the kernel-doc format in a comment preceding
the struct declaration, which is easier to read and has the added advantage
of including the information in the kernel documentation, for example, as
follows::
/**
* struct bar_order - Description of a bar order
* @guest_id: Unique guest id
* @ordered_item: The item number from the menu
* @menu: Pointer to the menu from which the item
* was ordered
*
* Supplementary information for using the struct.
*
* Note, that the struct member descriptors above are arranged
* in a tabular fashion.
*/
struct bar_order {
unsigned int guest_id;
int ordered_item;
struct menu *menu;
};
Static struct initializers must use C99 initializers and should also be
aligned in a tabular fashion::
static struct foo statfoo = {
.a = 0,
.plain_integer = CONSTANT_DEFINE_OR_ENUM,
.bar = &statbar,
};
Note that while C99 syntax allows the omission of the final comma,
we recommend the use of a comma on the last line because it makes
reordering and addition of new lines easier, and makes such future
patches slightly easier to read as well.
Line breaks
^^^^^^^^^^^
Restricting line length to 80 characters makes deeply indented code hard to
read. Consider breaking out code into helper functions to avoid excessive
line breaking.
The 80 character rule is not a strict rule, so please use common sense when
breaking lines. Especially format strings should never be broken up.
When splitting function declarations or function calls, then please align
the first argument in the second line with the first argument in the first
line::
static int long_function_name(struct foobar *barfoo, unsigned int id,
unsigned int offset)
{
if (!id) {
ret = longer_function_name(barfoo, DEFAULT_BARFOO_ID,
offset);
...
Namespaces
^^^^^^^^^^
Function/variable namespaces improve readability and allow easy
grepping. These namespaces are string prefixes for globally visible
function and variable names, including inlines. These prefixes should
combine the subsystem and the component name such as 'x86_comp\_',
'sched\_', 'irq\_', and 'mutex\_'.
This also includes static file scope functions that are immediately put
into globally visible driver templates - it's useful for those symbols
to carry a good prefix as well, for backtrace readability.
Namespace prefixes may be omitted for local static functions and
variables. Truly local functions, only called by other local functions,
can have shorter descriptive names - our primary concern is greppability
and backtrace readability.
Please note that 'xxx_vendor\_' and 'vendor_xxx_` prefixes are not
helpful for static functions in vendor-specific files. After all, it
is already clear that the code is vendor-specific. In addition, vendor
names should only be for truly vendor-specific functionality.
As always apply common sense and aim for consistency and readability.
Commit notifications
--------------------
The tip tree is monitored by a bot for new commits. The bot sends an email
for each new commit to a dedicated mailing list
(``[email protected]``) and Cc's all people who are
mentioned in one of the commit tags. It uses the email message ID from the
Link tag at the end of the tag list to set the In-Reply-To email header so
the message is properly threaded with the patch submission email.
The tip maintainers and submaintainers try to reply to the submitter
when merging a patch, but they sometimes forget or it does not fit the
workflow of the moment. While the bot message is purely mechanical, it
also implies a 'Thank you! Applied.'.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Wow, time flies. :-\
A month and a half later, Jon, how about it?
Thx.
On Thu, Apr 15, 2021 at 08:05:05AM +0200, Borislav Petkov wrote:
> On Tue, Apr 13, 2021 at 03:02:21PM -0600, Jonathan Corbet wrote:
> > For future installments, could you send them in their own thread as an
> > ordinary patch so I don't need to edit in the changelog after applying
> > them?
>
> Ok, sure but I might not need to anymore because, AFAICT, what is left
> is really tip-tree specific and that can finally be the tip-tree doc
> file.
>
> I'm pasting the whole thing below, if you think something's still
> generic enough, lemme know and I'll carve it out.
>
> Thx.
>
> ---
> .. SPDX-License-Identifier: GPL-2.0
>
> The tip tree handbook
> =====================
>
> What is the tip tree?
> ---------------------
>
> The tip tree is a collection of several subsystems and areas of
> development. The tip tree is both a direct development tree and a
> aggregation tree for several sub-maintainer trees. The tip tree gitweb URL
> is: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
>
> The tip tree contains the following subsystems:
>
> - **x86 architecture**
>
> The x86 architecture development takes place in the tip tree except
> for the x86 KVM and XEN specific parts which are maintained in the
> corresponding subsystems and routed directly to mainline from
> there. It's still good practice to Cc the x86 maintainers on
> x86-specific KVM and XEN patches.
>
> Some x86 subsystems have their own maintainers in addition to the
> overall x86 maintainers. Please Cc the overall x86 maintainers on
> patches touching files in arch/x86 even when they are not called out
> by the MAINTAINER file.
>
> Note, that ``[email protected]`` is not a mailing list. It is merely a
> mail alias which distributes mails to the x86 top-level maintainer
> team. Please always Cc the Linux Kernel mailing list (LKML)
> ``[email protected]``, otherwise your mail ends up only in
> the private inboxes of the maintainers.
>
> - **Scheduler**
>
> Scheduler development takes place in the -tip tree, in the
> sched/core branch - with occasional sub-topic trees for
> work-in-progress patch-sets.
>
> - **Locking and atomics**
>
> Locking development (including atomics and other synchronization
> primitives that are connected to locking) takes place in the -tip
> tree, in the locking/core branch - with occasional sub-topic trees
> for work-in-progress patch-sets.
>
> - **Generic interrupt subsystem and interrupt chip drivers**:
>
> - interrupt core development happens in the irq/core branch
>
> - interrupt chip driver development also happens in the irq/core
> branch, but the patches are usually applied in a separate maintainer
> tree and then aggregated into irq/core
>
> - **Time, timers, timekeeping, NOHZ and related chip drivers**:
>
> - timekeeping, clocksource core, NTP and alarmtimer development
> happens in the timers/core branch, but patches are usually applied in
> a separate maintainer tree and then aggregated into timers/core
>
> - clocksource/event driver development happens in the timers/core
> branch, but patches are mostly applied in a separate maintainer tree
> and then aggregated into timers/core
>
> - **Performance counters core, architecture support and tooling**:
>
> - perf core and architecture support development happens in the
> perf/core branch
>
> - perf tooling development happens in the perf tools maintainer
> tree and is aggregated into the tip tree.
>
> - **CPU hotplug core**
>
> - **RAS core**
>
> - **EFI core**
>
> EFI development in the efi git tree. The collected patches are
> aggregated in the tip efi/core branch.
>
> - **RCU**
>
> RCU development happens in the linux-rcu tree. The resulting changes
> are aggregated into the tip core/rcu branch.
>
> - **Various core code components**:
>
> - debugobjects
>
> - objtool
>
> - random bits and pieces
>
>
> Patch submission notes
> ----------------------
>
> Selecting the tree/branch
> ^^^^^^^^^^^^^^^^^^^^^^^^^
>
> In general, development against the head of the tip tree master branch is
> fine, but for the subsystems which are maintained separately, have their
> own git tree and are only aggregated into the tip tree, development should
> take place against the relevant subsystem tree or branch.
>
> Bug fixes which target mainline should always be applicable against the
> mainline kernel tree. Potential conflicts against changes which are already
> queued in the tip tree are handled by the maintainers.
>
> Patch subject
> ^^^^^^^^^^^^^
>
> The tip tree preferred format for patch subject prefixes is
> 'subsys/component:', e.g. 'x86/apic:', 'x86/mm/fault:', 'sched/fair:',
> 'genirq/core:'. Please do not use file names or complete file paths as
> prefix. 'git log path/to/file' should give you a reasonable hint in most
> cases.
>
> The condensed patch description in the subject line should start with a
> uppercase letter and should be written in imperative tone.
>
>
> Changelog
> ^^^^^^^^^
>
> The general rules about changelogs in the process documentation, see
> :ref:`Documentation/process/ <submittingpatches>`, apply.
>
> The tip tree maintainers set value on following these rules, especially on
> the request to write changelogs in imperative mood and not impersonating
> code or the execution of it. This is not just a whim of the
> maintainers. Changelogs written in abstract words are more precise and
> tend to be less confusing than those written in the form of novels.
>
> It's also useful to structure the changelog into several paragraphs and not
> lump everything together into a single one. A good structure is to explain
> the context, the problem and the solution in separate paragraphs and this
> order.
>
> Examples for illustration:
>
> Example 1::
>
> x86/intel_rdt/mbm: Fix MBM overflow handler during hot cpu
>
> When a CPU is dying, we cancel the worker and schedule a new worker on a
> different CPU on the same domain. But if the timer is already about to
> expire (say 0.99s) then we essentially double the interval.
>
> We modify the hot cpu handling to cancel the delayed work on the dying
> cpu and run the worker immediately on a different cpu in same domain. We
> donot flush the worker because the MBM overflow worker reschedules the
> worker on same CPU and scans the domain->cpu_mask to get the domain
> pointer.
>
> Improved version::
>
> x86/intel_rdt/mbm: Fix MBM overflow handler during CPU hotplug
>
> When a CPU is dying, the overflow worker is canceled and rescheduled on a
> different CPU in the same domain. But if the timer is already about to
> expire this essentially doubles the interval which might result in a non
> detected overflow.
>
> Cancel the overflow worker and reschedule it immediately on a different CPU
> in the same domain. The work could be flushed as well, but that would
> reschedule it on the same CPU.
>
> Example 2::
>
> time: POSIX CPU timers: Ensure that variable is initialized
>
> If cpu_timer_sample_group returns -EINVAL, it will not have written into
> *sample. Checking for cpu_timer_sample_group's return value precludes the
> potential use of an uninitialized value of now in the following block.
> Given an invalid clock_idx, the previous code could otherwise overwrite
> *oldval in an undefined manner. This is now prevented. We also exploit
> short-circuiting of && to sample the timer only if the result will
> actually be used to update *oldval.
>
> Improved version::
>
> posix-cpu-timers: Make set_process_cpu_timer() more robust
>
> Because the return value of cpu_timer_sample_group() is not checked,
> compilers and static checkers can legitimately warn about a potential use
> of the uninitialized variable 'now'. This is not a runtime issue as all
> call sites hand in valid clock ids.
>
> Also cpu_timer_sample_group() is invoked unconditionally even when the
> result is not used because *oldval is NULL.
>
> Make the invocation conditional and check the return value.
>
> Example 3::
>
> The entity can also be used for other purposes.
>
> Let's rename it to be more generic.
>
> Improved version::
>
> The entity can also be used for other purposes.
>
> Rename it to be more generic.
>
>
> For complex scenarios, especially race conditions and memory ordering
> issues, it is valuable to depict the scenario with a table which shows
> the parallelism and the temporal order of events. Here is an example::
>
> CPU0 CPU1
> free_irq(X) interrupt X
> spin_lock(desc->lock)
> wake irq thread()
> spin_unlock(desc->lock)
> spin_lock(desc->lock)
> remove action()
> shutdown_irq()
> release_resources() thread_handler()
> spin_unlock(desc->lock) access released resources.
> ^^^^^^^^^^^^^^^^^^^^^^^^^
> synchronize_irq()
>
> Lockdep provides similar useful output to depict a possible deadlock
> scenario::
>
> CPU0 CPU1
> rtmutex_lock(&rcu->rt_mutex)
> spin_lock(&rcu->rt_mutex.wait_lock)
> local_irq_disable()
> spin_lock(&timer->it_lock)
> spin_lock(&rcu->mutex.wait_lock)
> --> Interrupt
> spin_lock(&timer->it_lock)
>
>
> Function references in changelogs
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> When a function is mentioned in the changelog, either the text body or the
> subject line, please use the format 'function_name()'. Omitting the
> brackets after the function name can be ambiguous::
>
> Subject: subsys/component: Make reservation_count static
>
> reservation_count is only used in reservation_stats. Make it static.
>
> The variant with brackets is more precise::
>
> Subject: subsys/component: Make reservation_count() static
>
> reservation_count() is only called from reservation_stats(). Make it
> static.
>
>
> Backtraces in changelogs
> ^^^^^^^^^^^^^^^^^^^^^^^^
>
> (XXX: Add link to Documentation/process/submitting-patches.rst's section)
>
> Ordering of commit tags
> ^^^^^^^^^^^^^^^^^^^^^^^
>
> To have a uniform view of the commit tags, the tip maintainers use the
> following tag ordering scheme:
>
> - Fixes: 12char-SHA1 ("sub/sys: Original subject line")
>
> A Fixes tag should be added even for changes which do not need to be
> backported to stable kernels, i.e. when addressing a recently introduced
> issue which only affects tip or the current head of mainline. These tags
> are helpful to identify the original commit and are much more valuable
> than prominently mentioning the commit which introduced a problem in the
> text of the changelog itself because they can be automatically
> extracted.
>
> The following example illustrates the difference::
>
> Commit
>
> abcdef012345678 ("x86/xxx: Replace foo with bar")
>
> left an unused instance of variable foo around. Remove it.
>
> Signed-off-by: J.Dev <j.dev@mail>
>
> Please say instead::
>
> The recent replacement of foo with bar left an unused instance of
> variable foo around. Remove it.
>
> Fixes: abcdef012345678 ("x86/xxx: Replace foo with bar")
> Signed-off-by: J.Dev <j.dev@mail>
>
> The latter puts the information about the patch into the focus and
> amends it with the reference to the commit which introduced the issue
> rather than putting the focus on the original commit in the first place.
>
> - Reported-by: ``Reporter <reporter@mail>``
>
> - Originally-by: ``Original author <original-author@mail>``
>
> - Suggested-by: ``Suggester <suggester@mail>``
>
> - Co-developed-by: ``Co-author <co-author@mail>``
>
> Signed-off: ``Co-author <co-author@mail>``
>
> Note, that Co-developed-by and Signed-off-by of the co-author(s) must
> come in pairs.
>
> - Signed-off-by: ``Author <author@mail>``
>
> The first Signed-off-by (SOB) after the last Co-developed-by/SOB pair is the
> author SOB, i.e. the person flagged as author by git.
>
> - Signed-off-by: ``Patch handler <handler@mail>``
>
> SOBs after the author SOB are from people handling and transporting
> the patch, but were not involved in development. SOB chains should
> reflect the **real** route a patch took as it was propagated to us,
> with the first SOB entry signalling primary authorship of a single
> author. Acks should be given as Acked-by lines and review approvals
> as Reviewed-by lines.
>
> If the handler made modifications to the patch or the changelog, then
> this should be mentioned **after** the changelog text and **above**
> all commit tags in the following format::
>
> ... changelog text ends.
>
> [ handler: Replaced foo by bar and updated changelog ]
>
> First-tag: .....
>
> Note the two empty new lines which separate the changelog text and the
> commit tags from that notice.
>
> If a patch is sent to the mailing list by a handler then the author has
> to be noted in the first line of the changelog with::
>
> From: Author <author@mail>
>
> Changelog text starts here....
>
> so the authorship is preserved. The 'From:' line has to be followed
> by a empty newline. If that 'From:' line is missing, then the patch
> would be attributed to the person who sent (transported, handled) it.
> The 'From:' line is automatically removed when the patch is applied
> and does not show up in the final git changelog. It merely affects
> the authorship information of the resulting Git commit.
>
> - Tested-by: ``Tester <tester@mail>``
>
> - Reviewed-by: ``Reviewer <reviewer@mail>``
>
> - Acked-by: ``Acker <acker@mail>``
>
> - Cc: ``cc-ed-person <person@mail>``
>
> If the patch should be backported to stable, then please add a '``Cc:
> [email protected]``' tag, but do not Cc stable when sending your
> mail.
>
> - Link: ``https://link/to/information``
>
> For referring to an email on LKML or other kernel mailing lists,
> please use the lkml.kernel.org redirector URL::
>
> https://lkml.kernel.org/r/email-message@id
>
> The kernel.org redirector is considered a stable URL, unlike other email
> archives.
>
> Maintainers will add a Link tag referencing the email of the patch
> submission when they apply a patch to the tip tree. This tag is useful
> for later reference and is also used for commit notifications.
>
> Please do not use combined tags, e.g. ``Reported-and-tested-by``, as
> they just complicate automated extraction of tags.
>
>
> Links to documentation
> ^^^^^^^^^^^^^^^^^^^^^^
>
> Providing links to documentation in the changelog is a great help to later
> debugging and analysis. Unfortunately, URLs often break very quickly
> because companies restructure their websites frequently. Non-'volatile'
> exceptions include the Intel SDM and the AMD APM.
>
> Therefore, for 'volatile' documents, please create an entry in the kernel
> bugzilla https://bugzilla.kernel.org and attach a copy of these documents
> to the bugzilla entry. Finally, provide the URL of the bugzilla entry in
> the changelog.
>
>
> Patch version information
> ^^^^^^^^^^^^^^^^^^^^^^^^^
>
> (XXX: Add link to Documentation/process/submitting-patches.rst's section)
>
> Patch resend or reminders
> ^^^^^^^^^^^^^^^^^^^^^^^^^
>
> (XXX: point this to "Don't get discouraged - or impatient" section in
> Documentation/process/submitting-patches.rst)
>
>
> Merge window
> ^^^^^^^^^^^^
>
> Please do not expect large patch series to be handled during the merge
> window or even during the week before. Such patches should be submitted in
> mergeable state *at* *least* a week before the merge window opens.
> Exceptions are made for bug fixes and *sometimes* for small standalone
> drivers for new hardware or minimally invasive patches for hardware
> enablement.
>
> During the merge window, the maintainers instead focus on following the
> upstream changes, fixing merge window fallout, collecting bug fixes, and
> allowing themselves a breath. Please respect that.
>
> The release candidate -rc1 is the starting point for new patches to be
> applied which are targeted for the next merge window.
>
>
> Git
> ^^^
>
> The tip maintainers accept git pull requests from maintainers who provide
> subsystem changes for aggregation in the tip tree.
>
> Pull requests for new patch submissions are usually not accepted and do not
> replace proper patch submission to the mailing list. The main reason for
> this is that the review workflow is email based.
>
> If you submit a larger patch series it is helpful to provide a git branch
> in a private repository which allows interested people to easily pull the
> series for testing. The usual way to offer this is a git URL in the cover
> letter of the patch series.
>
>
> Coding style notes
> ------------------
>
> Comment style
> ^^^^^^^^^^^^^
>
> Sentences in comments start with an uppercase letter.
>
> Single line comments::
>
> /* This is a single line comment */
>
> Multi-line comments::
>
> /*
> * This is a properly formatted
> * multi-line comment.
> *
> * Larger multi-line comments should be split into paragraphs.
> */
>
> No tail comments:
>
> Please refrain from using tail comments. Tail comments disturb the
> reading flow in almost all contexts, but especially in code::
>
> if (somecondition_is_true) /* Don't put a comment here */
> dostuff(); /* Neither here */
>
> seed = MAGIC_CONSTANT; /* Nor here */
>
> Use freestanding comments instead::
>
> /* This condition is not obvious without a comment */
> if (somecondition_is_true) {
> /* This really needs to be documented */
> dostuff();
> }
>
> /* This magic initialization needs a comment. Maybe not? */
> seed = MAGIC_CONSTANT;
>
> Comment the important things:
>
> Comments should be added where the operation is not obvious. Documenting
> the obvious is just a distraction::
>
> /* Decrement refcount and check for zero */
> if (refcount_dec_and_test(&p->refcnt)) {
> do;
> lots;
> of;
> magic;
> things;
> }
>
> Instead, comments should explain the non-obvious details and document
> constraints::
>
> if (refcount_dec_and_test(&p->refcnt)) {
> /*
> * Really good explanation why the magic things below
> * need to be done, ordering and locking constraints,
> * etc..
> */
> do;
> lots;
> of;
> magic;
> /* Needs to be the last operation because ... */
> things;
> }
>
> Function documentation comments:
>
> To document functions and their arguments please use kernel-doc format
> and not free form comments::
>
> /**
> * magic_function - Do lots of magic stuff
> * @magic: Pointer to the magic data to operate on
> * @offset: Offset in the data array of @magic
> *
> * Deep explanation of mysterious things done with @magic along
> * with documentation of the return values.
> *
> * Note, that the argument descriptors above are arranged
> * in a tabular fashion.
> */
>
> This applies especially to globally visible functions and inline
> functions in public header files. It might be overkill to use kernel-doc
> format for every (static) function which needs a tiny explanation. The
> usage of descriptive function names often replaces these tiny comments.
> Apply common sense as always.
>
>
> Documenting locking requirements
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Documenting locking requirements is a good thing, but comments are not
> necessarily the best choice. Instead of writing::
>
> /* Caller must hold foo->lock */
> void func(struct foo *foo)
> {
> ...
> }
>
> Please use::
>
> void func(struct foo *foo)
> {
> lockdep_assert_held(&foo->lock);
> ...
> }
>
> In PROVE_LOCKING kernels, lockdep_assert_held() emits a warning
> if the caller doesn't hold the lock. Comments can't do that.
>
> Bracket rules
> ^^^^^^^^^^^^^
>
> Brackets should be omitted only if the statement which follows 'if', 'for',
> 'while' etc. is truly a single line::
>
> if (foo)
> do_something();
>
> The following is not considered to be a single line statement even
> though C does not require brackets::
>
> for (i = 0; i < end; i++)
> if (foo[i])
> do_something(foo[i]);
>
> Adding brackets around the outer loop enhances the reading flow::
>
> for (i = 0; i < end; i++) {
> if (foo[i])
> do_something(foo[i]);
> }
>
>
> Variable declarations
> ^^^^^^^^^^^^^^^^^^^^^
>
> The preferred ordering of variable declarations at the beginning of a
> function is reverse fir tree order::
>
> struct long_struct_name *descriptive_name;
> unsigned long foo, bar;
> unsigned int tmp;
> int ret;
>
> The above is faster to parse than the reverse ordering::
>
> int ret;
> unsigned int tmp;
> unsigned long foo, bar;
> struct long_struct_name *descriptive_name;
>
> And even more so than random ordering::
>
> unsigned long foo, bar;
> int ret;
> struct long_struct_name *descriptive_name;
> unsigned int tmp;
>
> Also please try to aggregate variables of the same type into a single
> line. There is no point in wasting screen space::
>
> unsigned long a;
> unsigned long b;
> unsigned long c;
> unsigned long d;
>
> It's really sufficient to do::
>
> unsigned long a, b, c, d;
>
> Please also refrain from introducing line splits in variable declarations::
>
> struct long_struct_name *descriptive_name = container_of(bar,
> struct long_struct_name,
> member);
> struct foobar foo;
>
> It's way better to move the initialization to a separate line after the
> declarations::
>
> struct long_struct_name *descriptive_name;
> struct foobar foo;
>
> descriptive_name = container_of(bar, struct long_struct_name, member);
>
>
> Variable types
> ^^^^^^^^^^^^^^
>
> Please use the proper u8, u16, u32, u64 types for variables which are meant
> to describe hardware or are used as arguments for functions which access
> hardware. These types are clearly defining the bit width and avoid
> truncation, expansion and 32/64-bit confusion.
>
> u64 is also recommended in code which would become ambiguous for 32-bit
> kernels when 'unsigned long' would be used instead. While in such
> situations 'unsigned long long' could be used as well, u64 is shorter
> and also clearly shows that the operation is required to be 64 bits wide
> independent of the target CPU.
>
> Please use 'unsigned int' instead of 'unsigned'.
>
>
> Constants
> ^^^^^^^^^
>
> Please do not use literal (hexa)decimal numbers in code or initializers.
> Either use proper defines which have descriptive names or consider using
> an enum.
>
>
> Struct declarations and initializers
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Struct declarations should align the struct member names in a tabular
> fashion::
>
> struct bar_order {
> unsigned int guest_id;
> int ordered_item;
> struct menu *menu;
> };
>
> Please avoid documenting struct members within the declaration, because
> this often results in strangely formatted comments and the struct members
> become obfuscated::
>
> struct bar_order {
> unsigned int guest_id; /* Unique guest id */
> int ordered_item;
> /* Pointer to a menu instance which contains all the drinks */
> struct menu *menu;
> };
>
> Instead, please consider using the kernel-doc format in a comment preceding
> the struct declaration, which is easier to read and has the added advantage
> of including the information in the kernel documentation, for example, as
> follows::
>
>
> /**
> * struct bar_order - Description of a bar order
> * @guest_id: Unique guest id
> * @ordered_item: The item number from the menu
> * @menu: Pointer to the menu from which the item
> * was ordered
> *
> * Supplementary information for using the struct.
> *
> * Note, that the struct member descriptors above are arranged
> * in a tabular fashion.
> */
> struct bar_order {
> unsigned int guest_id;
> int ordered_item;
> struct menu *menu;
> };
>
> Static struct initializers must use C99 initializers and should also be
> aligned in a tabular fashion::
>
> static struct foo statfoo = {
> .a = 0,
> .plain_integer = CONSTANT_DEFINE_OR_ENUM,
> .bar = &statbar,
> };
>
> Note that while C99 syntax allows the omission of the final comma,
> we recommend the use of a comma on the last line because it makes
> reordering and addition of new lines easier, and makes such future
> patches slightly easier to read as well.
>
> Line breaks
> ^^^^^^^^^^^
>
> Restricting line length to 80 characters makes deeply indented code hard to
> read. Consider breaking out code into helper functions to avoid excessive
> line breaking.
>
> The 80 character rule is not a strict rule, so please use common sense when
> breaking lines. Especially format strings should never be broken up.
>
> When splitting function declarations or function calls, then please align
> the first argument in the second line with the first argument in the first
> line::
>
> static int long_function_name(struct foobar *barfoo, unsigned int id,
> unsigned int offset)
> {
>
> if (!id) {
> ret = longer_function_name(barfoo, DEFAULT_BARFOO_ID,
> offset);
> ...
>
> Namespaces
> ^^^^^^^^^^
>
> Function/variable namespaces improve readability and allow easy
> grepping. These namespaces are string prefixes for globally visible
> function and variable names, including inlines. These prefixes should
> combine the subsystem and the component name such as 'x86_comp\_',
> 'sched\_', 'irq\_', and 'mutex\_'.
>
> This also includes static file scope functions that are immediately put
> into globally visible driver templates - it's useful for those symbols
> to carry a good prefix as well, for backtrace readability.
>
> Namespace prefixes may be omitted for local static functions and
> variables. Truly local functions, only called by other local functions,
> can have shorter descriptive names - our primary concern is greppability
> and backtrace readability.
>
> Please note that 'xxx_vendor\_' and 'vendor_xxx_` prefixes are not
> helpful for static functions in vendor-specific files. After all, it
> is already clear that the code is vendor-specific. In addition, vendor
> names should only be for truly vendor-specific functionality.
>
> As always apply common sense and aim for consistency and readability.
>
>
> Commit notifications
> --------------------
>
> The tip tree is monitored by a bot for new commits. The bot sends an email
> for each new commit to a dedicated mailing list
> (``[email protected]``) and Cc's all people who are
> mentioned in one of the commit tags. It uses the email message ID from the
> Link tag at the end of the tag list to set the In-Reply-To email header so
> the message is properly threaded with the patch submission email.
>
> The tip maintainers and submaintainers try to reply to the submitter
> when merging a patch, but they sometimes forget or it does not fit the
> workflow of the moment. While the bot message is purely mechanical, it
> also implies a 'Thank you! Applied.'.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Borislav Petkov <[email protected]> writes:
> Wow, time flies. :-\
>
> A month and a half later, Jon, how about it?
Oops...somehow I missed the fact that there was something there for me
to look at and respond to, sorry.
I've just read it through... if it were me, I would find a way to
reduce its bulk in the hope that people would actually read it; much of
what's there is in coding-style.rst (or should be). But it's not me,
and if you want to keep it I won't whine (much). Except about reverse
fir tree, perhaps, but nobody listens to me on that...:) I'd say
package it up as a maintainer-guide entry and go for it.
Thanks,
jon
On 6/1/21 3:27 PM, Jonathan Corbet wrote:
> Borislav Petkov <[email protected]> writes:
>
>> Wow, time flies. :-\
>>
>> A month and a half later, Jon, how about it?
>
> Oops...somehow I missed the fact that there was something there for me
> to look at and respond to, sorry.
>
> I've just read it through... if it were me, I would find a way to
> reduce its bulk in the hope that people would actually read it; much of
> what's there is in coding-style.rst (or should be). But it's not me,
> and if you want to keep it I won't whine (much). Except about reverse
> fir tree, perhaps, but nobody listens to me on that...:) I'd say
> package it up as a maintainer-guide entry and go for it.
Yes, as a maintainer-profile/guide/whatever, please,
for X86/tip.
Same for several other feedback comments on patches that
are destined for the tip tree.
thanks.
--
~Randy
On Tue, Jun 01, 2021 at 04:27:13PM -0600, Jonathan Corbet wrote:
> Oops...somehow I missed the fact that there was something there for me
> to look at and respond to, sorry.
No worries, thanks for taking the time!
> I've just read it through... if it were me, I would find a way to
> reduce its bulk in the hope that people would actually read it; much of
> what's there is in coding-style.rst (or should be).
Right, the idea is to point to sections in it during review where the
topic in question is dealt with a greater detail so that we don't have
to type it each time.
I'll take a look at coding-style.rst and see if I can cross-link
sections so that there's no repetition.
> But it's not me, and if you want to keep it I won't whine (much).
> Except about reverse fir tree, perhaps, but nobody listens to me on
> that...:)
Yeah, that is the only correct way! :-)
> I'd say package it up as a maintainer-guide entry and go for it.
Thanks, will do so!
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette