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