1Code Review Guidelines 2====================== 3 4Why do we do code reviews? 5-------------------------- 6 7The main goal of code reviews is to improve the code quality. By reviewing each 8other's code, we can help catch issues that were missed by the author 9before they are integrated in the source tree. Different people bring different 10perspectives, depending on their past work, experiences and their current use 11cases of TF-A in their products. 12 13Code reviews also play a key role in sharing knowledge within the 14community. People with more expertise in one area of the code base can 15help those that are less familiar with it. 16 17Code reviews are meant to benefit everyone through team work. It is not about 18unfairly criticizing or belittling the work of any contributor. 19 20 21Overview of the code review process 22----------------------------------- 23 24All contributions to Trusted Firmware-A project are reviewed by the community to 25ensure they meet the project's expectations before they get merged, according to 26the `Project Maintenance Process`_ defined for all `Trusted Firmware` projects. 27 28Technical ownership of most parts of the codebase falls on the :ref:`code 29owners`. All patches are ultimately merged by the :ref:`maintainers`. 30 31Approval of a patch is tracked using Gerrit `labels`. For a patch to be merged, 32it must get all of the following votes: 33 34- At least one ``Code-Owner-Review+1`` up-vote, and no ``Code-Owner-Review-1`` 35 down-vote. 36 37- At least one ``Maintainer-Review+1`` up-vote, and no ``Maintainer-Review-1`` 38 down-vote. 39 40- ``Verified+1`` vote applied by the automated Continuous Integration (CI) 41 system. 42 43Note that, in some instances, the maintainers might give a waiver for some of 44the CI failures and manually override the ``Verified+1`` score. 45 46 47Good practices for all reviewers 48-------------------------------- 49 50To ensure the code review gives the greatest possible benefit, participants in 51the project should: 52 53- Be considerate of other people and their needs. Participants may be working 54 to different timescales, and have different priorities. Keep this in 55 mind - be gracious while waiting for action from others, and timely in your 56 actions when others are waiting for you. 57 58- Review other people's patches where possible. The more active reviewers there 59 are, the more quickly new patches can be reviewed and merged. Contributing to 60 code review helps everyone in the long run, as it creates a culture of 61 participation which serves everyone's interests. 62 63 64Guidelines for patch contributors 65--------------------------------- 66 67In addition to the rules outlined in the :ref:`Contributor's Guide`, as a patch 68contributor you are expected to: 69 70- Answer all comments from people who took the time to review your 71 patches. 72 73- Be patient and resilient. It is quite common for patches to go through 74 several rounds of reviews and rework before they get approved, especially 75 for larger features. 76 77 In the event that a code review takes longer than you would hope for, you 78 may try the following actions to speed it up: 79 80 - Ping the reviewers on Gerrit or on the mailing list. If it is urgent, 81 explain why. Please remain courteous and do not abuse this. 82 83 - If one code owner has become unresponsive, ask the other code owners for 84 help progressing the patch. 85 86 - If there is only one code owner and they have become unresponsive, ask one 87 of the project maintainers for help. 88 89- Do the right thing for the project, not the fastest thing to get code merged. 90 91 For example, if some existing piece of code - say a driver - does not quite 92 meet your exact needs, go the extra mile and extend the code with the missing 93 functionality you require - as opposed to copying the code into some other 94 directory to have the freedom to change it in any way. This way, your changes 95 benefit everyone and will be maintained over time. 96 97 98Guidelines for all reviewers 99---------------------------- 100 101There are no good or bad review comments. If you have any doubt about a patch or 102need some clarifications, it's better to ask rather than letting a potential 103issue slip. Examples of review comments could be: 104 105- Questions ("Why do you need to do this?", "What if X happens?") 106- Bugs ("I think you need a logical \|\| rather than a bitwise \|.") 107- Design issues ("This won't scale well when we introduce feature X.") 108- Improvements ("Would it be better if we did Y instead?") 109 110 111Guidelines for code owners 112-------------------------- 113 114Code owners are listed on the :ref:`Project Maintenance<code owners>` page, 115along with the module(s) they look after. 116 117When reviewing a patch, code owners are expected to check the following: 118 119- The patch looks good from a technical point of view. For example: 120 121 - The structure of the code is clear. 122 123 - It complies with the relevant standards or technical documentation (where 124 applicable). 125 126 - It leverages existing interfaces rather than introducing new ones 127 unnecessarily. 128 129 - It fits well in the design of the module. 130 131 - It adheres to the security model of the project. In particular, it does not 132 increase the attack surface (e.g. new SMCs) without justification. 133 134- The patch adheres to the TF-A :ref:`Coding Style`. The CI system should help 135 catch coding style violations. 136 137- (Only applicable to generic code) The code is MISRA-compliant (see 138 :ref:`misra-compliance`). The CI system should help catch violations. 139 140- Documentation is provided/updated (where applicable). 141 142- The patch has had an appropriate level of testing. Testing details are 143 expected to be provided by the patch author. If they are not, do not hesitate 144 to request this information. 145 146- All CI automated tests pass. 147 148If a code owner is happy with a patch, they should give their approval 149through the ``Code-Owner-Review+1`` label in Gerrit. If instead, they have 150concerns, questions, or any other type of blocking comment, they should set 151``Code-Owner-Review-1``. 152 153Code owners are expected to behave professionally and responsibly. Here are some 154guidelines for them: 155 156- Once you are engaged in a review, make sure you stay involved until the patch 157 is merged. Rejecting a patch and going away is not very helpful. You are 158 expected to monitor the patch author's answers to your review comments, 159 answer back if needed and review new revisions of their patch. 160 161- Provide constructive feedback. Just saying, "This is wrong, you should do X 162 instead." is usually not very helpful. The patch author is unlikely to 163 understand why you are requesting this change and might feel personally 164 attacked. 165 166- Be mindful when reviewing a patch. As a code owner, you are viewed as 167 the expert for the relevant module. By approving a patch, you are partially 168 responsible for its quality and the effects it has for all TF-A users. Make 169 sure you fully understand what the implications of a patch might be. 170 171 172Guidelines for maintainers 173-------------------------- 174 175Maintainers are listed on the :ref:`Project Maintenance<maintainers>` page. 176 177When reviewing a patch, maintainers are expected to check the following: 178 179- The general structure of the patch looks good. This covers things like: 180 181 - Code organization. 182 183 - Files and directories, names and locations. 184 185 For example, platform code should be added under the ``plat/`` directory. 186 187 - Naming conventions. 188 189 For example, platform identifiers should be properly namespaced to avoid 190 name clashes with generic code. 191 192 - API design. 193 194- Interaction of the patch with other modules in the code base. 195 196- The patch aims at complying with any standard or technical documentation 197 that applies. 198 199- New files must have the correct license and copyright headers. See :ref:`this 200 paragraph<copyright-license-guidance>` for more information. The CI system 201 should help catch files with incorrect or no copyright/license headers. 202 203- There is no third party code or binary blobs with potential IP concerns. 204 Maintainers should look for copyright or license notices in code, and use 205 their best judgement. If they are unsure about a patch, they should ask 206 other maintainers for help. 207 208- Generally speaking, new driver code should be placed in the generic 209 layer. There are cases where a driver has to stay into the platform layer but 210 this should be the exception, rather than the rule. 211 212- Existing common drivers (in particular for Arm IPs like the GIC driver) should 213 not be copied into the platform layer to cater for platform quirks. This 214 type of code duplication hurts the maintainability of the project. The 215 duplicate driver is less likely to benefit from bug fixes and future 216 enhancements. In most cases, it is possible to rework a generic driver to 217 make it more flexible and fit slightly different use cases. That way, these 218 enhancements benefit everyone. 219 220- When a platform specific driver really is required, the burden lies with the 221 patch author to prove the need for it. A detailed justification should be 222 posted via the commit message or on the mailing list. 223 224- Before merging a patch, verify that all review comments have been addressed. 225 If this is not the case, encourage the patch author and the relevant 226 reviewers to resolve these together. 227 228If a maintainer is happy with a patch, they should give their approval 229through the ``Maintainer-Review+1`` label in Gerrit. If instead, they have 230concerns, questions, or any other type of blocking comment, they should set 231``Maintainer-Review-1``. 232 233-------------- 234 235*Copyright (c) 2020-2023, Arm Limited. All rights reserved.* 236 237.. _Project Maintenance Process: https://developer.trustedfirmware.org/w/collaboration/project-maintenance-process/ 238