1#####################
2Code Review Guideline
3#####################
4The purpose of this document is to clarify design items to be reviewed during
5the code review process.
6
7Please contact :doc:`maintainers </contributing/maintainers>` or write an e-mail
8thread on the `TF-M mailing list <mailto:tf-m@lists.trustedfirmware.org>`_ for
9any questions.
10
11**********************
12List of the guidelines
13**********************
14The prerequisites before going to the review stage:
15
16- Read the :doc:`Contributing Process </contributing/contributing_process>`
17  to know basic concepts.
18- Read the :ref:`source_structure`
19  for structure related reference.
20
21The review guidelines consist of these items:
22
23- `Exceptions`_.
24- `Non-source items`_.
25- `Namespace`_.
26- `Assembler code`_.
27- `Include`_.
28- `Auto-doc`_.
29- `Commit Message`_.
30
31.. note::
32  Each guideline item is assigned with a unique symbol in the format ``Rx.y``,
33  while ``x`` and ``y`` are numeric symbols. These symbols are created for easy
34  referencing during the code review process.
35
36Exceptions
37==========
38Files under listed folders are fully or partially imported from 3rd party
39projects, these files follow the original project defined guidelines and
40styles:
41
42.. important::
43  - R1.1 ``ext`` and its subfolders.
44
45Non-source items
46================
47For all non-source files such as build system files or configuration files:
48
49.. important::
50  - R2.1 Follow the existing style while making changes.
51
52Namespace
53=========
54TF-M assign the namespace to files and symbols for an easier code reading. The
55symbol here includes functions, variables, types and MACROs. The prerequisites:
56
57.. important::
58  - R3.1 Follow the documents or specifications if they propose namespaces
59    ('psa\_' for PSA defined items E.g.,). Ask the contributor to tell the
60    documents or specifications if the reviewer is not sure about a namespace.
61  - R3.2 Assign TF-M specific namespace 'tfm\_' or 'TFM\_' for symbols
62    implementing TF-M specific features. Ask the contributor to clarify the
63    purpose of the patch if the reviewer is not sure about the namespace.
64
65For the sources out of the above prerequisites (R3.1 and R3.2):
66
67.. important::
68  - R3.3 Do not assign a namespace for source files.
69  - R3.4 Assigning a namespace for interface symbols is recommended. The
70    namespace needs to be expressed in one word to be followed by other words
71    into a phrase can represent the functionalities implemented. One word
72    naming is allowed if the namespace can represent the functionality
73    perfectly.
74  - R3.5 Assign a namespace for private symbols is NOT recommended.
75  - R3.6 Do not assign characters out of alphabets as the leading character.
76
77Examples:
78
79.. code-block:: c
80
81  /* R3.1 FILE: s/spm/core/psa_client.c */
82
83  /* R3.2 FILE: s/spm/core/tfm_secure_context.c */
84
85  /* R3.4 FILE: s/spm/core/spm.c, 'spm\_' as the namespace */
86  void spm_init(void);
87
88  /* R3.5 FILE: s/spm/core/main.c */
89  static void init_functions(void);
90
91  /* R3.6 Not permitted: */
92  /* static uint32_t __count; */
93
94Assembler code
95==============
96
97.. important::
98  - R4.1 Pure assembler sources or inline assembler code are required to be put
99    under the platform-independent or architecture-independent folders.
100    The logic folders should not contain any assembler code, referring to
101    external MACRO wrapped assembler code is allowed. Here is one example of the
102    logic folder:
103
104    - 'secure_fw/spm'.
105
106Examples:
107
108.. code-block:: c
109
110  /*
111   * R4.1 The following MACRO is allowed to be referenced under
112   * 'secure_fw/spm'
113   */
114  #define SVC(code) __asm volatile("svc  %0", ::"I"(code))
115
116Include
117=======
118This chapter describes the placement of the headers and including. There are
119two types of headers: The ``interface`` headers contain symbols to be shared
120between modules and the ``private`` headers contain symbols only for internal
121usage.
122
123.. important::
124  - R5.1 Put the ``interface header`` of one module in the ``include`` folder
125    under the root of this module. Deeper sub-folders can not have ``include``
126    folders, which means only one ``include`` is allowed for one module.
127
128  - R5.2 Creating sub-folders under ``include`` to represent the more granular
129    scope of the interfaces is allowed.
130
131  - R5.3 ``private header`` can be put at the same place with the implementation
132    sources for the private symbols contained in the header. It also can be put
133    at the place where the sources need it. The latter is useful when some
134    "private header" contains abstracted interfaces, but these interfaces are
135    not public interfaces so it won't be put under "include" folder.
136
137  - R5.4 Use <> when including public headers.
138
139  - R5.5 Use "" when including private headers.
140
141  - R5.6 The module's ``include`` folder needs to be added into referencing
142    module's header searching path.
143
144  - R5.7 The module's ``include`` folder and the root folder needs to be added
145    into its own header searching path and apply a hierarchy including with
146    folder name.
147
148  - R5.8 Path hierarchy including is allowed since there are sub-folders under
149    ``include`` folder and the module folder.
150
151  - R5.9 The including statement group order: the beginning group contains
152    toolchain headers, then follows the public headers group and finally the
153    private headers group.
154
155  - R5.10 The including statement order inside a group: Compare the include
156    statement as strings and sort by the string comparison result.
157
158  - R5.11 The header for the referenced symbol or definition must be included
159    even this header is included inside one of the existing included headers.
160    This improves portability in case the existing header implementation
161    changed.
162
163Examples:
164
165.. code-block:: c
166
167  /*
168   * The structure:
169   *   module_a/include/func1.h
170   *   module_a/include/func2/fmain.h
171   *   module_a/func1.c
172   *   module_a/func2/fmain.c
173   *   module_b/include/funcx.h
174   *   module_b/include/funcy/fmain.h
175   *   module_b/funcx.c
176   *   module_b/funcxi.h
177   *   module_b/funcy/fmain.c
178   *   module_b/funcy/fsub.c
179   *   module_b/funcy/fsub.h
180   * Here takes module_b/funcx.c as example:
181   */
182  #include <func1.h>
183  #include <func2/fmain.h>
184  #include <funcx.h>
185  #include "funcxi.h"
186  #include "funcy/fsub.h"
187
188Auto-doc
189========
190Auto document system such as doxygen is useful for describing interfaces. While
191it would be a development burden since the details are described in the design
192documents already. The guidelines for auto-doc:
193
194.. important::
195  - R6.1 Headers and sources under these folders need to apply auto-doc style
196    comments: ``*include``.
197  - R6.2 Developers decide the comment style for sources out of listed folders.
198
199Commit Message
200==============
201TF-M has the requirements on commit message:
202
203.. important::
204  - R7.1 Assign correct topic for a patch. Check the following table.
205
206============== ====================================================
207Topic          Justification
208============== ====================================================
209Boot           `bl2/*` or `bl1/*`
210Build          For build system related purpose.
211Docs           All \*.rst changes.
212Dualcpu        Dual-cpu related changes.
213HAL            Generic HAL interface/implementation changes.
214Interface      Interface changes, either Non-source and secure.
215Pack           For packing purpose.
216Platform       Generic platform related changes under `platform/*`.
217Platform Name  Specific platform changes.
218Partition      Multiple partition related changes.
219Partition Name Specific partition related changes.
220Service        Multiple service related changes.
221Service Name   Specific service related changes.
222SPM            `secure_fw/spm/*`
223SPRTL          `secure-fw/partitions/lib/runtime/*`
224Tool           `tools` folder or `tf-m-tools` repo
225============== ====================================================
226
227.. note::
228  Ideally, one topic should cover one specific type of changes. For crossing
229  topic changes, check the main part of the change and use the main part
230  related topic as patch topic. If there is no suitable topics to cover the
231  change, contact the community for an update.
232
233--------------
234
235*Copyright (c) 2020-2022, Arm Limited. All rights reserved.*
236