Contents
Philosophy of Linux kernel patches
What is a patch?
A patch is a small file that contains a short commit message (less than 50 characters), a description of the changes in paragraph form, and a diff of the code changes. Here's an example of a patch to a Linux kernel documentation file:
From 2c97a63f6fec91db91241981808d099ec60a4688 Mon Sep 17 00:00:00 2001 From: Sarah Sharp <sarah.a.sharp@linux.intel.com> To: linux-doc@vger.kernel.org Date: Sat, 13 Apr 2013 18:40:55 -0700 Subject: [PATCH] Docs: Add info on supported kernels to REPORTING-BUGS. One of the most common frustrations maintainers have with bug reporters is the email that starts with "I have a two year old kernel from an embedded vendor with some random drivers and fixes thrown in, and it's crashing". Be specific about what kernel versions the upstream maintainers will fix bugs in, and direct bug reporters to their Linux distribution or embedded vendor if the bug is in an unsupported kernel. Suggest that bug reporters should reproduce their bugs on the latest -rc kernel. Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com> --- REPORTING-BUGS | 22 ++++++++++++++++++++++ 1 files changed, 22 insertions(+), 0 deletions(-) diff --git a/REPORTING-BUGS b/REPORTING-BUGS index f86e500..c1f6e43 100644 --- a/REPORTING-BUGS +++ b/REPORTING-BUGS @@ -1,3 +1,25 @@ +Background +========== + +The upstream Linux kernel maintainers only fix bugs for specific kernel +versions. Those versions include the current "release candidate" (or -rc) +kernel, any "stable" kernel versions, and any "long term" kernels. + +Please see https://www.kernel.org/ for a list of supported kernels. Any +kernel marked with [EOL] is "end of life" and will not have any fixes +backported to it. + +If you've found a bug on a kernel version isn't listed on kernel.org, +contact your Linux distribution or embedded vendor for support. +Alternatively, you can attempt to run one of the supported stable or -rc +kernels, and see if you can reproduce the bug on that. It's preferable +to reproduce the bug on the latest -rc kernel. + + +How to report Linux kernel bugs +=============================== + + Identify the problematic subsystem ---------------------------------- -- 1.7.9
Take a look at the short description portion of the patch:
Subject: [PATCH] Docs: Add info on supported kernels to REPORTING-BUGS.
This short description should tell the reader, at a glance, what this patch is about. Usually there will be a prefix to the short description, to let the reader know which subsystem it applies to. In this case, it applies to the Documentation subsystem.
The next part of the patch is the body of the patch:
One of the most common frustrations maintainers have with bug reporters is the email that starts with "I have a two year old kernel from an embedded vendor with some random drivers and fixes thrown in, and it's crashing". Be specific about what kernel versions the upstream maintainers will fix bugs in, and direct bug reporters to their Linux distribution or embedded vendor if the bug is in an unsupported kernel. Suggest that bug reporters should reproduce their bugs on the latest -rc kernel. Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
This description tells why the change was made. The "why" of a patch is more important than "what" the patch is doing. If your commit is just a restatement of the diff of the code (e.g. "I changed the X function to not do Y"), you should rethink your patch description.
The body of the patch is a chance to tell a maintainer why they should take your patch. This is your chance to convince them, beyond a doubt, that they should take the time to apply your patch. If your patch is a bug fix, you should describe how to trigger the bug, and reference any bugzilla entries that describe the patch, and say how this patch fixes the bug. If your patch is a new feature, you should describe why the new feature is needed, and what other code will use it. New features and drivers are the hardest patches to get in, so your patch descriptions should be air tight.
All your patch descriptions must end with your "Signed-off-by" line. This means your code meets the Developer's certificate of origin. Please read that whole section to ensure you're submitting your own code to the Linux kernel.
In patch descriptions and in the subject, it is common and preferable to use present-tense, imperative language. Write as if you are telling git what to do with your patch. For example, instead of:
somedriver: fixed sleep while atomic in send_a_packet()
The send_a_packet() function was called in atomic context but took a mutex,
which caused a sleeping while atomic warning. Changed the skb_lock to
be a spin lock instead of a mutex to fix.Consider:
somedriver: replace mutex with spinlock in send_a_packet()
The send_a_packet() function is called in atomic context but takes a mutex,
causing a sleeping while atomic warning. Change the skb_lock to be a spin
lock instead of a mutex to fix.Similarly, avoid first-person singular pronouns ("I removed XYZ" => "Remove Xyz"). Although you would like your patch to tell a story, resist the temptation to list every step and right or wrong turn you took to arrive at the solution; instead summarize what was wrong, and how the fix corrects it. If you think the reviewer needs to know how you arrived at it, please add that information after the "---" line before the diffstat: such information does not make it into the git history.
Patch subject formatting
As the example above showed, each git commit has a short description that becomes the patch subject line. Each patch subject will need to have a driver prefix. In the example above, the prefix was "Docs: ". This driver prefix will vary based on which kernel driver you are changing. When figuring out which driver prefix to use, it is helpful to look at the git log for the particular file you're changing.
For example, if we changed a file in a staging driver, we can look at the git log to figure out what driver prefix to use:
sarah@xanatos:~/git/kernels/xhci$ git log --pretty=oneline --abbrev-commit drivers/staging/et131x/et131x.c 015851c34202 Staging: et131x: Fix warning of prefer ether_addr_copy() in et131x.c a9f488831cb4 staging: et131x: fix allocation failures 0b15862d46fd staging: et131x: remove spinlock adapter->lock a863a15bf24a staging: et131x: stop read when hit max delay in et131x_phy_mii_read
In this case, it looks like "Staging: et131x: " is the right prefix to use. Make sure to put spaces after the colons ":" in your short description.
Subject format for checkpatch fixes
When creating a patch that fixes a checkpatch.pl warning or error, it is important to include which checkpatch error or warning you are fixing in your subject line. This is especially important when you have several checkpatch.pl fixes in a patchset to a driver.
For example, this subject line is too generic for kernel developers to tell what error or warning you are fixing:
staging: rtl8712: Fix checkpatch warning
But this subject tells us at a glance what you're fixing:
staging: rtl8712: Add blank line after variable declarations
Patch description format for checkpatch fixes
There are several possible strategies for writing commit messages for checkpatch warnings. One strategy is to include the full checkpatch.pl warning or error output in your patch body. Make sure the warning or error doesn't linewrap. E.g.
From sevinj.aghayeva@gmail.com Tue Mar 29 20:52:31 2022
Date: Tue, 29 Mar 2022 20:52:31 -0400
From: Sevinj Aghayeva <sevinj.aghayeva@gmail.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
outreachy@lists.linux.dev
Subject: [PATCH v2] staging: rtl8723bs: remove redundant braces in if
statements
Adhere to Linux kernel coding style.
Reported by checkpatch:
WARNING: braces {} are not necessary for single statement blocks
Signed-off-by: Sevinj Aghayeva <sevinj.aghayeva@gmail.com>
---This strategy is reasonable if the checkpatch warning makes very obvious what change is made. However, in some cases, it may be necessary to make some choices about how to resolve the checkpatch warning. In such cases, it can be better to write a message that explains what you have done and why. It is still good to give credit to checkpatch for helping you find the problem. So the message could be something like:
Replace printk(KERN_INFO...) by netdev_info for more uniform error reporting. Issue found by checkpatch.
What is a patchset?
A patchset is a series of patches that are related, that describe changes to add new functionality, or fix bugs in a specific driver. The idea of a patchset is to break changes into a logical series. To understand how to arrange your patches, you have to first understand some background.
Patches are git commits
In the Linux kernel, all our history is stored in git commits. If you send a patch to a maintainer, they will apply it to their own git tree using `git am`, in order to create a git commit for it. They'll also add their Signed-off-by line to the patch, below your own, in order to certify that they have reviewed, and possibly tested, your patch.
You can see the commits by running this command in your kernel git tree:
git log
We keep the kernel under revision control in order to be able to know who made a change (by running `git blame <file>`), to be able to revert individual changes if they prove problematic (with `git revert`), and so that if there's a bug, we can bisect the commits to the specific commit that caused the issue (with `git bisect`).
It is very important that every commit in your patchset compiles properly, in order not to break git bisect.
How to break up changes
Each patch should group changes into a logical sequence. Bug fixes must come first in the patchset, then new features. This is because we need to be able to backport bug fixes to older kernels, and they should not depend on new features.
A logical change may span multiple files. For example, if you're making whitespace changes to a particular driver that is compiled from several files (like drivers/usb/host/xhci.c and drivers/usb/host/xhci-mem.c), then you would combine all those whitespace changes in one commit. If you also wanted to make other cleanups to the driver (like removing one camel-case variable name), then you would make a separate commit.
The idea here is that you should break changes up in such a way that it will be easy to review. Whitespace changes are easy to review in one patch, but may be difficult to review if other changes are thrown in. A change to one variable name is visually easy to track, but throwing in two variable name changes into one patch is hard to track. As you read more patches on the mailing list, you will get a sense of how to break up your changes in logical ways. Some kernel developers are better at this than others.
When creating a new feature patchset, you may need to break up your changes into multiple commits. For example, if you're trying to add feature A, but you find you need to refactor some code to allow new functions to call it, your first commit would be the refactoring, and the second commit would be the new feature. This makes it easier to track down breakage with `git bisect`, in case you made a mistake in your refactoring.
Clean up patches that are over 200 lines long are discouraged, because they are hard to review. Break those patches up into smaller patches. Clean up patchsets with more than 10 patches are also hard to review, so you may want to send a subset of those patches, wait for them to be accepted, and then send another patchset. These are not hard-and-fast rules, but rather ways to avoid making Linux kernel maintainers feel like they're drinking from a firehose.
See Sage Sharp's blog post for a philsophical comparision of creating patchsets and creating four-course meals.
Updating and resending patches
When you receive feedback on a patch, or notice an issue yourself, you'll want to update your patchset and resend it.
You can use git rebase -i to reorder, combine, split, or otherwise edit your patchset; you'll probably want to save a tag or branch pointing to the previous versions of your patchset first. If you need to dig up an older version of your patchset that you didn't save, use git reflog.
Once you've reorganized your patchset, you should resend it via email. You'll need to do three additional things for subsequent versions: use PATCHv2 (or PATCHv3 and so on) in the subject lines instead of PATCH, add a patch changelog to the patch (for single patches) or cover letter (for patchsets), and send the new patch series a reply to the previous one.
To update the subject lines, add the -v 2 (or -v 3, etc) options to git format-patch. (If you have an older version of git that doesn't understand the -v option, you may need to use --subject-prefix=PATCHv2 instead.)
To document what changed in the new patchset, put a patch changelog at the bottom of your cover letter, just above the diffstat. A patch changelog should consist of a series of entries like this:
v2: Fixed ..., noted by Some Person <some.person@example.org>
Reworked to use ..., as suggested by ...
v3: ...If you're just sending a single patch, put the patch changelog after your commit message, below the -- line, but above the diffstat.
Finally, send your new patch or patch series as a new thread rather than as a reply to the previous patch or patch series. I.e., a new version of a patch (series) should begin a new thread.
Further reading
The following files are included in the Linux kernel git tree, but you can also find them through the Linux Cross reference site:
https://www.kernel.org/doc/html/latest/process/coding-style.html
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
https://www.kernel.org/doc/html/latest/process/submit-checklist.html
This old but still highly relevant article from Andrew Morton describes the perfect patch:
http://www.ozlabs.org/~akpm/stuff/tpp.txt
Todo
Thoughts on improving this page (write up later):
Prefix your patch subject with the subsystem. In this case, staging and the driver. E.g. "Fix checkpatch.pl issues with quoted string split across lines in hfa384x_usb.c" becomes "staging: hfa384x: Fix split strings". Double check past patches to see which prefix was used by running `git log` on a driver file.