Differences between revisions 10 and 11
|Deletions are marked like this.||Additions are marked like this.|
|Line 143:||Line 143:|
= Patch description format for checkpatch fixes =
Please make sure 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.
Date: Sun, 14 Sep 2014 19:24:03 +0530
From: Vaishali Thakkar <email@example.com>
Subject: [OPW kernel] [PATCH] Staging: rtl8192e: Fix printk() warning style
This patch fixes the checkpatch.pl warning:
WARNING: Prefer [subsystem eg: netdev]_info([subsystem]dev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ...
Signed-off-by: Vaishali Thakkar<firstname.lastname@example.org>
- Philosophy of Linux kernel patches
- What is a patch?
- Patch subject formatting
- Subject format for checkpatch fixes
- Patch description format for checkpatch fixes
- What is a patchset?
- Patches are git commits
- How to break up changes
- Updating and resending patches
- Further reading
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 <email@example.com> To: firstname.lastname@example.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 <email@example.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
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.
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 <firstname.lastname@example.org>
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.
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.
somedriver: fix sleep while atomic 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.
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
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.
Patch description format for checkpatch fixes
Date: Sun, 14 Sep 2014 19:24:03 +0530 From: Vaishali Thakkar <email@example.com> To: firstname.lastname@example.org Subject: [OPW kernel] [PATCH] Staging: rtl8192e: Fix printk() warning style List-ID: <opw-kernel.googlegroups.com> This patch fixes the checkpatch.pl warning: WARNING: Prefer [subsystem eg: netdev]_info([subsystem]dev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ... Signed-off-by: Vaishali Thakkar<email@example.com> ---
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.
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`).
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 Sarah Sharp's blog post for a philsophical comparision of creating patchsets and creating four-course meals.
Updating and resending patches
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.)
v2: Fixed ..., noted by Some Person <firstname.lastname@example.org> Reworked to use ..., as suggested by ... v3: ...
Finally, to send your new patch series as a reply to the previous one, first look up the Message-Id of the cover letter (or the one-and-only patch) in your previous patch series, and then pass that to the --in-reply-to= option of either git format-patch or git send-email.
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.