| Jonathan Corbet | 75b0214 | 2008-09-30 15:15:56 -0600 | [diff] [blame] | 1 | 5: POSTING PATCHES | 
|  | 2 |  | 
|  | 3 | Sooner or later, the time comes when your work is ready to be presented to | 
|  | 4 | the community for review and, eventually, inclusion into the mainline | 
|  | 5 | kernel.  Unsurprisingly, the kernel development community has evolved a set | 
|  | 6 | of conventions and procedures which are used in the posting of patches; | 
|  | 7 | following them will make life much easier for everybody involved.  This | 
|  | 8 | document will attempt to cover these expectations in reasonable detail; | 
|  | 9 | more information can also be found in the files SubmittingPatches, | 
|  | 10 | SubmittingDrivers, and SubmitChecklist in the kernel documentation | 
|  | 11 | directory. | 
|  | 12 |  | 
|  | 13 |  | 
|  | 14 | 5.1: WHEN TO POST | 
|  | 15 |  | 
|  | 16 | There is a constant temptation to avoid posting patches before they are | 
|  | 17 | completely "ready."  For simple patches, that is not a problem.  If the | 
|  | 18 | work being done is complex, though, there is a lot to be gained by getting | 
|  | 19 | feedback from the community before the work is complete.  So you should | 
|  | 20 | consider posting in-progress work, or even making a git tree available so | 
|  | 21 | that interested developers can catch up with your work at any time. | 
|  | 22 |  | 
|  | 23 | When posting code which is not yet considered ready for inclusion, it is a | 
|  | 24 | good idea to say so in the posting itself.  Also mention any major work | 
|  | 25 | which remains to be done and any known problems.  Fewer people will look at | 
|  | 26 | patches which are known to be half-baked, but those who do will come in | 
|  | 27 | with the idea that they can help you drive the work in the right direction. | 
|  | 28 |  | 
|  | 29 |  | 
|  | 30 | 5.2: BEFORE CREATING PATCHES | 
|  | 31 |  | 
|  | 32 | There are a number of things which should be done before you consider | 
|  | 33 | sending patches to the development community.  These include: | 
|  | 34 |  | 
|  | 35 | - Test the code to the extent that you can.  Make use of the kernel's | 
|  | 36 | debugging tools, ensure that the kernel will build with all reasonable | 
|  | 37 | combinations of configuration options, use cross-compilers to build for | 
|  | 38 | different architectures, etc. | 
|  | 39 |  | 
|  | 40 | - Make sure your code is compliant with the kernel coding style | 
|  | 41 | guidelines. | 
|  | 42 |  | 
|  | 43 | - Does your change have performance implications?  If so, you should run | 
|  | 44 | benchmarks showing what the impact (or benefit) of your change is; a | 
|  | 45 | summary of the results should be included with the patch. | 
|  | 46 |  | 
|  | 47 | - Be sure that you have the right to post the code.  If this work was done | 
|  | 48 | for an employer, the employer likely has a right to the work and must be | 
|  | 49 | agreeable with its release under the GPL. | 
|  | 50 |  | 
|  | 51 | As a general rule, putting in some extra thought before posting code almost | 
|  | 52 | always pays back the effort in short order. | 
|  | 53 |  | 
|  | 54 |  | 
|  | 55 | 5.3: PATCH PREPARATION | 
|  | 56 |  | 
|  | 57 | The preparation of patches for posting can be a surprising amount of work, | 
|  | 58 | but, once again, attempting to save time here is not generally advisable | 
|  | 59 | even in the short term. | 
|  | 60 |  | 
|  | 61 | Patches must be prepared against a specific version of the kernel.  As a | 
|  | 62 | general rule, a patch should be based on the current mainline as found in | 
|  | 63 | Linus's git tree.  It may become necessary to make versions against -mm, | 
|  | 64 | linux-next, or a subsystem tree, though, to facilitate wider testing and | 
|  | 65 | review.  Depending on the area of your patch and what is going on | 
|  | 66 | elsewhere, basing a patch against these other trees can require a | 
|  | 67 | significant amount of work resolving conflicts and dealing with API | 
|  | 68 | changes. | 
|  | 69 |  | 
|  | 70 | Only the most simple changes should be formatted as a single patch; | 
|  | 71 | everything else should be made as a logical series of changes.  Splitting | 
|  | 72 | up patches is a bit of an art; some developers spend a long time figuring | 
|  | 73 | out how to do it in the way that the community expects.  There are a few | 
|  | 74 | rules of thumb, however, which can help considerably: | 
|  | 75 |  | 
|  | 76 | - The patch series you post will almost certainly not be the series of | 
|  | 77 | changes found in your working revision control system.  Instead, the | 
|  | 78 | changes you have made need to be considered in their final form, then | 
|  | 79 | split apart in ways which make sense.  The developers are interested in | 
|  | 80 | discrete, self-contained changes, not the path you took to get to those | 
|  | 81 | changes. | 
|  | 82 |  | 
|  | 83 | - Each logically independent change should be formatted as a separate | 
|  | 84 | patch.  These changes can be small ("add a field to this structure") or | 
|  | 85 | large (adding a significant new driver, for example), but they should be | 
|  | 86 | conceptually small and amenable to a one-line description.  Each patch | 
|  | 87 | should make a specific change which can be reviewed on its own and | 
|  | 88 | verified to do what it says it does. | 
|  | 89 |  | 
|  | 90 | - As a way of restating the guideline above: do not mix different types of | 
|  | 91 | changes in the same patch.  If a single patch fixes a critical security | 
|  | 92 | bug, rearranges a few structures, and reformats the code, there is a | 
|  | 93 | good chance that it will be passed over and the important fix will be | 
|  | 94 | lost. | 
|  | 95 |  | 
|  | 96 | - Each patch should yield a kernel which builds and runs properly; if your | 
|  | 97 | patch series is interrupted in the middle, the result should still be a | 
|  | 98 | working kernel.  Partial application of a patch series is a common | 
|  | 99 | scenario when the "git bisect" tool is used to find regressions; if the | 
|  | 100 | result is a broken kernel, you will make life harder for developers and | 
|  | 101 | users who are engaging in the noble work of tracking down problems. | 
|  | 102 |  | 
|  | 103 | - Do not overdo it, though.  One developer recently posted a set of edits | 
|  | 104 | to a single file as 500 separate patches - an act which did not make him | 
|  | 105 | the most popular person on the kernel mailing list.  A single patch can | 
|  | 106 | be reasonably large as long as it still contains a single *logical* | 
|  | 107 | change. | 
|  | 108 |  | 
|  | 109 | - It can be tempting to add a whole new infrastructure with a series of | 
|  | 110 | patches, but to leave that infrastructure unused until the final patch | 
|  | 111 | in the series enables the whole thing.  This temptation should be | 
|  | 112 | avoided if possible; if that series adds regressions, bisection will | 
|  | 113 | finger the last patch as the one which caused the problem, even though | 
|  | 114 | the real bug is elsewhere.  Whenever possible, a patch which adds new | 
|  | 115 | code should make that code active immediately. | 
|  | 116 |  | 
|  | 117 | Working to create the perfect patch series can be a frustrating process | 
|  | 118 | which takes quite a bit of time and thought after the "real work" has been | 
|  | 119 | done.  When done properly, though, it is time well spent. | 
|  | 120 |  | 
|  | 121 |  | 
| Jonathan Corbet | 5d98932 | 2009-04-21 13:33:06 -0600 | [diff] [blame] | 122 | 5.4: PATCH FORMATTING AND CHANGELOGS | 
| Jonathan Corbet | 75b0214 | 2008-09-30 15:15:56 -0600 | [diff] [blame] | 123 |  | 
|  | 124 | So now you have a perfect series of patches for posting, but the work is | 
|  | 125 | not done quite yet.  Each patch needs to be formatted into a message which | 
|  | 126 | quickly and clearly communicates its purpose to the rest of the world.  To | 
|  | 127 | that end, each patch will be composed of the following: | 
|  | 128 |  | 
|  | 129 | - An optional "From" line naming the author of the patch.  This line is | 
|  | 130 | only necessary if you are passing on somebody else's patch via email, | 
|  | 131 | but it never hurts to add it when in doubt. | 
|  | 132 |  | 
|  | 133 | - A one-line description of what the patch does.  This message should be | 
|  | 134 | enough for a reader who sees it with no other context to figure out the | 
|  | 135 | scope of the patch; it is the line that will show up in the "short form" | 
|  | 136 | changelogs.  This message is usually formatted with the relevant | 
|  | 137 | subsystem name first, followed by the purpose of the patch.  For | 
|  | 138 | example: | 
|  | 139 |  | 
|  | 140 | gpio: fix build on CONFIG_GPIO_SYSFS=n | 
|  | 141 |  | 
|  | 142 | - A blank line followed by a detailed description of the contents of the | 
|  | 143 | patch.  This description can be as long as is required; it should say | 
|  | 144 | what the patch does and why it should be applied to the kernel. | 
|  | 145 |  | 
|  | 146 | - One or more tag lines, with, at a minimum, one Signed-off-by: line from | 
|  | 147 | the author of the patch.  Tags will be described in more detail below. | 
|  | 148 |  | 
| Jonathan Corbet | 5d98932 | 2009-04-21 13:33:06 -0600 | [diff] [blame] | 149 | The items above, together, form the changelog for the patch.  Writing good | 
|  | 150 | changelogs is a crucial but often-neglected art; it's worth spending | 
|  | 151 | another moment discussing this issue.  When writing a changelog, you should | 
|  | 152 | bear in mind that a number of different people will be reading your words. | 
|  | 153 | These include subsystem maintainers and reviewers who need to decide | 
|  | 154 | whether the patch should be included, distributors and other maintainers | 
|  | 155 | trying to decide whether a patch should be backported to other kernels, bug | 
|  | 156 | hunters wondering whether the patch is responsible for a problem they are | 
|  | 157 | chasing, users who want to know how the kernel has changed, and more.  A | 
|  | 158 | good changelog conveys the needed information to all of these people in the | 
|  | 159 | most direct and concise way possible. | 
|  | 160 |  | 
|  | 161 | To that end, the summary line should describe the effects of and motivation | 
|  | 162 | for the change as well as possible given the one-line constraint.  The | 
|  | 163 | detailed description can then amplify on those topics and provide any | 
|  | 164 | needed additional information.  If the patch fixes a bug, cite the commit | 
|  | 165 | which introduced the bug if possible.  If a problem is associated with | 
|  | 166 | specific log or compiler output, include that output to help others | 
|  | 167 | searching for a solution to the same problem.  If the change is meant to | 
|  | 168 | support other changes coming in later patch, say so.  If internal APIs are | 
|  | 169 | changed, detail those changes and how other developers should respond.  In | 
|  | 170 | general, the more you can put yourself into the shoes of everybody who will | 
|  | 171 | be reading your changelog, the better that changelog (and the kernel as a | 
|  | 172 | whole) will be. | 
|  | 173 |  | 
|  | 174 | Needless to say, the changelog should be the text used when committing the | 
|  | 175 | change to a revision control system.  It will be followed by: | 
| Jonathan Corbet | 75b0214 | 2008-09-30 15:15:56 -0600 | [diff] [blame] | 176 |  | 
|  | 177 | - The patch itself, in the unified ("-u") patch format.  Using the "-p" | 
|  | 178 | option to diff will associate function names with changes, making the | 
|  | 179 | resulting patch easier for others to read. | 
|  | 180 |  | 
|  | 181 | You should avoid including changes to irrelevant files (those generated by | 
|  | 182 | the build process, for example, or editor backup files) in the patch.  The | 
|  | 183 | file "dontdiff" in the Documentation directory can help in this regard; | 
|  | 184 | pass it to diff with the "-X" option. | 
|  | 185 |  | 
|  | 186 | The tags mentioned above are used to describe how various developers have | 
|  | 187 | been associated with the development of this patch.  They are described in | 
|  | 188 | detail in the SubmittingPatches document; what follows here is a brief | 
|  | 189 | summary.  Each of these lines has the format: | 
|  | 190 |  | 
|  | 191 | tag: Full Name <email address>  optional-other-stuff | 
|  | 192 |  | 
|  | 193 | The tags in common use are: | 
|  | 194 |  | 
|  | 195 | - Signed-off-by: this is a developer's certification that he or she has | 
|  | 196 | the right to submit the patch for inclusion into the kernel.  It is an | 
|  | 197 | agreement to the Developer's Certificate of Origin, the full text of | 
|  | 198 | which can be found in Documentation/SubmittingPatches.  Code without a | 
|  | 199 | proper signoff cannot be merged into the mainline. | 
|  | 200 |  | 
|  | 201 | - Acked-by: indicates an agreement by another developer (often a | 
|  | 202 | maintainer of the relevant code) that the patch is appropriate for | 
|  | 203 | inclusion into the kernel. | 
|  | 204 |  | 
|  | 205 | - Tested-by: states that the named person has tested the patch and found | 
|  | 206 | it to work. | 
|  | 207 |  | 
|  | 208 | - Reviewed-by: the named developer has reviewed the patch for correctness; | 
|  | 209 | see the reviewer's statement in Documentation/SubmittingPatches for more | 
|  | 210 | detail. | 
|  | 211 |  | 
|  | 212 | - Reported-by: names a user who reported a problem which is fixed by this | 
|  | 213 | patch; this tag is used to give credit to the (often underappreciated) | 
|  | 214 | people who test our code and let us know when things do not work | 
|  | 215 | correctly. | 
|  | 216 |  | 
|  | 217 | - Cc: the named person received a copy of the patch and had the | 
|  | 218 | opportunity to comment on it. | 
|  | 219 |  | 
|  | 220 | Be careful in the addition of tags to your patches: only Cc: is appropriate | 
|  | 221 | for addition without the explicit permission of the person named. | 
|  | 222 |  | 
|  | 223 |  | 
|  | 224 | 5.5: SENDING THE PATCH | 
|  | 225 |  | 
|  | 226 | Before you mail your patches, there are a couple of other things you should | 
|  | 227 | take care of: | 
|  | 228 |  | 
|  | 229 | - Are you sure that your mailer will not corrupt the patches?  Patches | 
|  | 230 | which have had gratuitous white-space changes or line wrapping performed | 
|  | 231 | by the mail client will not apply at the other end, and often will not | 
|  | 232 | be examined in any detail.  If there is any doubt at all, mail the patch | 
|  | 233 | to yourself and convince yourself that it shows up intact. | 
|  | 234 |  | 
|  | 235 | Documentation/email-clients.txt has some helpful hints on making | 
|  | 236 | specific mail clients work for sending patches. | 
|  | 237 |  | 
|  | 238 | - Are you sure your patch is free of silly mistakes?  You should always | 
|  | 239 | run patches through scripts/checkpatch.pl and address the complaints it | 
|  | 240 | comes up with.  Please bear in mind that checkpatch.pl, while being the | 
|  | 241 | embodiment of a fair amount of thought about what kernel patches should | 
|  | 242 | look like, is not smarter than you.  If fixing a checkpatch.pl complaint | 
|  | 243 | would make the code worse, don't do it. | 
|  | 244 |  | 
|  | 245 | Patches should always be sent as plain text.  Please do not send them as | 
|  | 246 | attachments; that makes it much harder for reviewers to quote sections of | 
|  | 247 | the patch in their replies.  Instead, just put the patch directly into your | 
|  | 248 | message. | 
|  | 249 |  | 
|  | 250 | When mailing patches, it is important to send copies to anybody who might | 
|  | 251 | be interested in it.  Unlike some other projects, the kernel encourages | 
|  | 252 | people to err on the side of sending too many copies; don't assume that the | 
|  | 253 | relevant people will see your posting on the mailing lists.  In particular, | 
|  | 254 | copies should go to: | 
|  | 255 |  | 
|  | 256 | - The maintainer(s) of the affected subsystem(s).  As described earlier, | 
|  | 257 | the MAINTAINERS file is the first place to look for these people. | 
|  | 258 |  | 
|  | 259 | - Other developers who have been working in the same area - especially | 
|  | 260 | those who might be working there now.  Using git to see who else has | 
|  | 261 | modified the files you are working on can be helpful. | 
|  | 262 |  | 
|  | 263 | - If you are responding to a bug report or a feature request, copy the | 
|  | 264 | original poster as well. | 
|  | 265 |  | 
|  | 266 | - Send a copy to the relevant mailing list, or, if nothing else applies, | 
|  | 267 | the linux-kernel list. | 
|  | 268 |  | 
|  | 269 | - If you are fixing a bug, think about whether the fix should go into the | 
|  | 270 | next stable update.  If so, stable@kernel.org should get a copy of the | 
|  | 271 | patch.  Also add a "Cc: stable@kernel.org" to the tags within the patch | 
|  | 272 | itself; that will cause the stable team to get a notification when your | 
|  | 273 | fix goes into the mainline. | 
|  | 274 |  | 
|  | 275 | When selecting recipients for a patch, it is good to have an idea of who | 
|  | 276 | you think will eventually accept the patch and get it merged.  While it | 
|  | 277 | is possible to send patches directly to Linus Torvalds and have him merge | 
|  | 278 | them, things are not normally done that way.  Linus is busy, and there are | 
|  | 279 | subsystem maintainers who watch over specific parts of the kernel.  Usually | 
|  | 280 | you will be wanting that maintainer to merge your patches.  If there is no | 
|  | 281 | obvious maintainer, Andrew Morton is often the patch target of last resort. | 
|  | 282 |  | 
|  | 283 | Patches need good subject lines.  The canonical format for a patch line is | 
|  | 284 | something like: | 
|  | 285 |  | 
|  | 286 | [PATCH nn/mm] subsys: one-line description of the patch | 
|  | 287 |  | 
|  | 288 | where "nn" is the ordinal number of the patch, "mm" is the total number of | 
|  | 289 | patches in the series, and "subsys" is the name of the affected subsystem. | 
|  | 290 | Clearly, nn/mm can be omitted for a single, standalone patch. | 
|  | 291 |  | 
|  | 292 | If you have a significant series of patches, it is customary to send an | 
|  | 293 | introductory description as part zero.  This convention is not universally | 
|  | 294 | followed though; if you use it, remember that information in the | 
|  | 295 | introduction does not make it into the kernel changelogs.  So please ensure | 
|  | 296 | that the patches, themselves, have complete changelog information. | 
|  | 297 |  | 
|  | 298 | In general, the second and following parts of a multi-part patch should be | 
|  | 299 | sent as a reply to the first part so that they all thread together at the | 
|  | 300 | receiving end.  Tools like git and quilt have commands to mail out a set of | 
|  | 301 | patches with the proper threading.  If you have a long series, though, and | 
|  | 302 | are using git, please provide the --no-chain-reply-to option to avoid | 
|  | 303 | creating exceptionally deep nesting. |