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