Mauro Carvalho Chehab | f7c9fe4 | 2016-09-19 08:07:36 -0300 | [diff] [blame] | 1 | .. _development_followthrough: |
| 2 | |
| 3 | Followthrough |
| 4 | ============= |
Jonathan Corbet | 75b0214 | 2008-09-30 15:15:56 -0600 | [diff] [blame] | 5 | |
| 6 | At this point, you have followed the guidelines given so far and, with the |
| 7 | addition of your own engineering skills, have posted a perfect series of |
| 8 | patches. One of the biggest mistakes that even experienced kernel |
| 9 | developers can make is to conclude that their work is now done. In truth, |
| 10 | posting patches indicates a transition into the next stage of the process, |
| 11 | with, possibly, quite a bit of work yet to be done. |
| 12 | |
| 13 | It is a rare patch which is so good at its first posting that there is no |
| 14 | room for improvement. The kernel development process recognizes this fact, |
| 15 | and, as a result, is heavily oriented toward the improvement of posted |
| 16 | code. You, as the author of that code, will be expected to work with the |
| 17 | kernel community to ensure that your code is up to the kernel's quality |
| 18 | standards. A failure to participate in this process is quite likely to |
| 19 | prevent the inclusion of your patches into the mainline. |
| 20 | |
| 21 | |
Mauro Carvalho Chehab | f7c9fe4 | 2016-09-19 08:07:36 -0300 | [diff] [blame] | 22 | Working with reviewers |
| 23 | ---------------------- |
Jonathan Corbet | 75b0214 | 2008-09-30 15:15:56 -0600 | [diff] [blame] | 24 | |
| 25 | A patch of any significance will result in a number of comments from other |
| 26 | developers as they review the code. Working with reviewers can be, for |
| 27 | many developers, the most intimidating part of the kernel development |
| 28 | process. Life can be made much easier, though, if you keep a few things in |
| 29 | mind: |
| 30 | |
| 31 | - If you have explained your patch well, reviewers will understand its |
| 32 | value and why you went to the trouble of writing it. But that value |
| 33 | will not keep them from asking a fundamental question: what will it be |
| 34 | like to maintain a kernel with this code in it five or ten years later? |
| 35 | Many of the changes you may be asked to make - from coding style tweaks |
| 36 | to substantial rewrites - come from the understanding that Linux will |
| 37 | still be around and under development a decade from now. |
| 38 | |
| 39 | - Code review is hard work, and it is a relatively thankless occupation; |
| 40 | people remember who wrote kernel code, but there is little lasting fame |
| 41 | for those who reviewed it. So reviewers can get grumpy, especially when |
| 42 | they see the same mistakes being made over and over again. If you get a |
| 43 | review which seems angry, insulting, or outright offensive, resist the |
| 44 | impulse to respond in kind. Code review is about the code, not about |
| 45 | the people, and code reviewers are not attacking you personally. |
| 46 | |
| 47 | - Similarly, code reviewers are not trying to promote their employers' |
| 48 | agendas at the expense of your own. Kernel developers often expect to |
| 49 | be working on the kernel years from now, but they understand that their |
| 50 | employer could change. They truly are, almost without exception, |
| 51 | working toward the creation of the best kernel they can; they are not |
| 52 | trying to create discomfort for their employers' competitors. |
| 53 | |
| 54 | What all of this comes down to is that, when reviewers send you comments, |
| 55 | you need to pay attention to the technical observations that they are |
| 56 | making. Do not let their form of expression or your own pride keep that |
| 57 | from happening. When you get review comments on a patch, take the time to |
| 58 | understand what the reviewer is trying to say. If possible, fix the things |
| 59 | that the reviewer is asking you to fix. And respond back to the reviewer: |
| 60 | thank them, and describe how you will answer their questions. |
| 61 | |
| 62 | Note that you do not have to agree with every change suggested by |
| 63 | reviewers. If you believe that the reviewer has misunderstood your code, |
| 64 | explain what is really going on. If you have a technical objection to a |
| 65 | suggested change, describe it and justify your solution to the problem. If |
| 66 | your explanations make sense, the reviewer will accept them. Should your |
| 67 | explanation not prove persuasive, though, especially if others start to |
| 68 | agree with the reviewer, take some time to think things over again. It can |
| 69 | be easy to become blinded by your own solution to a problem to the point |
| 70 | that you don't realize that something is fundamentally wrong or, perhaps, |
| 71 | you're not even solving the right problem. |
| 72 | |
Jonathan Corbet | 5c050fb | 2011-03-25 12:17:53 -0600 | [diff] [blame] | 73 | Andrew Morton has suggested that every review comment which does not result |
| 74 | in a code change should result in an additional code comment instead; that |
| 75 | can help future reviewers avoid the questions which came up the first time |
| 76 | around. |
| 77 | |
Jonathan Corbet | 75b0214 | 2008-09-30 15:15:56 -0600 | [diff] [blame] | 78 | One fatal mistake is to ignore review comments in the hope that they will |
| 79 | go away. They will not go away. If you repost code without having |
| 80 | responded to the comments you got the time before, you're likely to find |
| 81 | that your patches go nowhere. |
| 82 | |
| 83 | Speaking of reposting code: please bear in mind that reviewers are not |
| 84 | going to remember all the details of the code you posted the last time |
| 85 | around. So it is always a good idea to remind reviewers of previously |
| 86 | raised issues and how you dealt with them; the patch changelog is a good |
| 87 | place for this kind of information. Reviewers should not have to search |
| 88 | through list archives to familiarize themselves with what was said last |
| 89 | time; if you help them get a running start, they will be in a better mood |
| 90 | when they revisit your code. |
| 91 | |
| 92 | What if you've tried to do everything right and things still aren't going |
| 93 | anywhere? Most technical disagreements can be resolved through discussion, |
| 94 | but there are times when somebody simply has to make a decision. If you |
| 95 | honestly believe that this decision is going against you wrongly, you can |
| 96 | always try appealing to a higher power. As of this writing, that higher |
| 97 | power tends to be Andrew Morton. Andrew has a great deal of respect in the |
| 98 | kernel development community; he can often unjam a situation which seems to |
| 99 | be hopelessly blocked. Appealing to Andrew should not be done lightly, |
| 100 | though, and not before all other alternatives have been explored. And bear |
| 101 | in mind, of course, that he may not agree with you either. |
| 102 | |
| 103 | |
Mauro Carvalho Chehab | f7c9fe4 | 2016-09-19 08:07:36 -0300 | [diff] [blame] | 104 | What happens next |
| 105 | ----------------- |
Jonathan Corbet | 75b0214 | 2008-09-30 15:15:56 -0600 | [diff] [blame] | 106 | |
| 107 | If a patch is considered to be a good thing to add to the kernel, and once |
| 108 | most of the review issues have been resolved, the next step is usually |
| 109 | entry into a subsystem maintainer's tree. How that works varies from one |
| 110 | subsystem to the next; each maintainer has his or her own way of doing |
| 111 | things. In particular, there may be more than one tree - one, perhaps, |
| 112 | dedicated to patches planned for the next merge window, and another for |
Jonathan Corbet | 5c050fb | 2011-03-25 12:17:53 -0600 | [diff] [blame] | 113 | longer-term work. |
Jonathan Corbet | 75b0214 | 2008-09-30 15:15:56 -0600 | [diff] [blame] | 114 | |
| 115 | For patches applying to areas for which there is no obvious subsystem tree |
| 116 | (memory management patches, for example), the default tree often ends up |
| 117 | being -mm. Patches which affect multiple subsystems can also end up going |
| 118 | through the -mm tree. |
| 119 | |
| 120 | Inclusion into a subsystem tree can bring a higher level of visibility to a |
| 121 | patch. Now other developers working with that tree will get the patch by |
Jonathan Corbet | 5c050fb | 2011-03-25 12:17:53 -0600 | [diff] [blame] | 122 | default. Subsystem trees typically feed linux-next as well, making their |
| 123 | contents visible to the development community as a whole. At this point, |
| 124 | there's a good chance that you will get more comments from a new set of |
| 125 | reviewers; these comments need to be answered as in the previous round. |
Jonathan Corbet | 75b0214 | 2008-09-30 15:15:56 -0600 | [diff] [blame] | 126 | |
| 127 | What may also happen at this point, depending on the nature of your patch, |
| 128 | is that conflicts with work being done by others turn up. In the worst |
| 129 | case, heavy patch conflicts can result in some work being put on the back |
| 130 | burner so that the remaining patches can be worked into shape and merged. |
| 131 | Other times, conflict resolution will involve working with the other |
| 132 | developers and, possibly, moving some patches between trees to ensure that |
| 133 | everything applies cleanly. This work can be a pain, but count your |
| 134 | blessings: before the advent of the linux-next tree, these conflicts often |
| 135 | only turned up during the merge window and had to be addressed in a hurry. |
| 136 | Now they can be resolved at leisure, before the merge window opens. |
| 137 | |
| 138 | Some day, if all goes well, you'll log on and see that your patch has been |
| 139 | merged into the mainline kernel. Congratulations! Once the celebration is |
| 140 | complete (and you have added yourself to the MAINTAINERS file), though, it |
| 141 | is worth remembering an important little fact: the job still is not done. |
| 142 | Merging into the mainline brings its own challenges. |
| 143 | |
| 144 | To begin with, the visibility of your patch has increased yet again. There |
| 145 | may be a new round of comments from developers who had not been aware of |
| 146 | the patch before. It may be tempting to ignore them, since there is no |
| 147 | longer any question of your code being merged. Resist that temptation, |
| 148 | though; you still need to be responsive to developers who have questions or |
| 149 | suggestions. |
| 150 | |
| 151 | More importantly, though: inclusion into the mainline puts your code into |
| 152 | the hands of a much larger group of testers. Even if you have contributed |
| 153 | a driver for hardware which is not yet available, you will be surprised by |
| 154 | how many people will build your code into their kernels. And, of course, |
| 155 | where there are testers, there will be bug reports. |
| 156 | |
| 157 | The worst sort of bug reports are regressions. If your patch causes a |
| 158 | regression, you'll find an uncomfortable number of eyes upon you; |
| 159 | regressions need to be fixed as soon as possible. If you are unwilling or |
| 160 | unable to fix the regression (and nobody else does it for you), your patch |
| 161 | will almost certainly be removed during the stabilization period. Beyond |
| 162 | negating all of the work you have done to get your patch into the mainline, |
| 163 | having a patch pulled as the result of a failure to fix a regression could |
| 164 | well make it harder for you to get work merged in the future. |
| 165 | |
| 166 | After any regressions have been dealt with, there may be other, ordinary |
| 167 | bugs to deal with. The stabilization period is your best opportunity to |
| 168 | fix these bugs and ensure that your code's debut in a mainline kernel |
| 169 | release is as solid as possible. So, please, answer bug reports, and fix |
| 170 | the problems if at all possible. That's what the stabilization period is |
| 171 | for; you can start creating cool new patches once any problems with the old |
| 172 | ones have been taken care of. |
| 173 | |
| 174 | And don't forget that there are other milestones which may also create bug |
| 175 | reports: the next mainline stable release, when prominent distributors pick |
| 176 | up a version of the kernel containing your patch, etc. Continuing to |
| 177 | respond to these reports is a matter of basic pride in your work. If that |
| 178 | is insufficient motivation, though, it's also worth considering that the |
| 179 | development community remembers developers who lose interest in their code |
| 180 | after it's merged. The next time you post a patch, they will be evaluating |
| 181 | it with the assumption that you will not be around to maintain it |
| 182 | afterward. |
| 183 | |
| 184 | |
Mauro Carvalho Chehab | f7c9fe4 | 2016-09-19 08:07:36 -0300 | [diff] [blame] | 185 | Other things that can happen |
| 186 | ----------------------------- |
Jonathan Corbet | 75b0214 | 2008-09-30 15:15:56 -0600 | [diff] [blame] | 187 | |
| 188 | One day, you may open your mail client and see that somebody has mailed you |
| 189 | a patch to your code. That is one of the advantages of having your code |
| 190 | out there in the open, after all. If you agree with the patch, you can |
| 191 | either forward it on to the subsystem maintainer (be sure to include a |
| 192 | proper From: line so that the attribution is correct, and add a signoff of |
| 193 | your own), or send an Acked-by: response back and let the original poster |
| 194 | send it upward. |
| 195 | |
| 196 | If you disagree with the patch, send a polite response explaining why. If |
| 197 | possible, tell the author what changes need to be made to make the patch |
| 198 | acceptable to you. There is a certain resistance to merging patches which |
| 199 | are opposed by the author and maintainer of the code, but it only goes so |
| 200 | far. If you are seen as needlessly blocking good work, those patches will |
| 201 | eventually flow around you and get into the mainline anyway. In the Linux |
| 202 | kernel, nobody has absolute veto power over any code. Except maybe Linus. |
| 203 | |
| 204 | On very rare occasion, you may see something completely different: another |
| 205 | developer posts a different solution to your problem. At that point, |
| 206 | chances are that one of the two patches will not be merged, and "mine was |
| 207 | here first" is not considered to be a compelling technical argument. If |
| 208 | somebody else's patch displaces yours and gets into the mainline, there is |
| 209 | really only one way to respond: be pleased that your problem got solved and |
| 210 | get on with your work. Having one's work shoved aside in this manner can |
| 211 | be hurtful and discouraging, but the community will remember your reaction |
| 212 | long after they have forgotten whose patch actually got merged. |