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 :doc:`Source Structure </technical_references/design_docs/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/ffm/psa_client.c */
82
83  /* R3.2 FILE: s/spm/cmsis_psa/tfm_secure_context.c */
84
85  /* R3.3 FILE: s/spm/cmsis_psa/main.c */
86
87  /* R3.4 FILE: s/spm/cmsis_psa/main.c, 'main' is a good entry name. */
88  void main(void);
89  /* R3.4 FILE: s/spm/ffm/spm.c, 'spm\_' as the namespace */
90  void spm_init(void);
91
92  /* R3.5 FILE: s/spm/ffm/main.c */
93  static void init_functions(void);
94
95  /* R3.6 Not permitted: */
96  /* static uint32_t __count; */
97
98Assembler code
99==============
100
101.. important::
102  - R4.1 Pure assembler sources or inline assembler code are required to be put
103    under the platform-independent or architecture-independent folders.
104    The logic folders should not contain any assembler code, referring to
105    external MACRO wrapped assembler code is allowed. Here is one example of the
106    logic folder:
107
108    - 'secure_fw/spm/ffm'.
109
110Examples:
111
112.. code-block:: c
113
114  /*
115   * R4.1 The following MACRO is allowed to be referenced under
116   * 'secure_fw/spm/ffm'
117   */
118  #define SVC(code) __asm volatile("svc  %0", ::"I"(code))
119
120Include
121=======
122This chapter describes the placement of the headers and including. There are
123two types of headers: The ``interface`` headers contain symbols to be shared
124between modules and the ``private`` headers contain symbols only for internal
125usage.
126
127.. important::
128  - R5.1 Put the ``interface header`` of one module in the ``include`` folder
129    under the root of this module. Deeper sub-folders can not have ``include``
130    folders, which means only one ``include`` is allowed for one module.
131
132  - R5.2 Creating sub-folders under ``include`` to represent the more granular
133    scope of the interfaces is allowed.
134
135  - R5.3 ``private header`` can be put at the same place with the implementation
136    sources for the private symbols contained in the header. It also can be put
137    at the place where the sources need it. The latter is useful when some
138    "private header" contains abstracted interfaces, but these interfaces are
139    not public interfaces so it won't be put under "include" folder.
140
141  - R5.4 Use <> when including public headers.
142
143  - R5.5 Use "" when including private headers.
144
145  - R5.6 The module's ``include`` folder needs to be added into referencing
146    module's header searching path.
147
148  - R5.7 The module's ``include`` folder and the root folder needs to be added
149    into its own header searching path and apply a hierarchy including with
150    folder name.
151
152  - R5.8 Path hierarchy including is allowed since there are sub-folders under
153    ``include`` folder and the module folder.
154
155  - R5.9 The including statement group order: the beginning group contains
156    toolchain headers, then follows the public headers group and finally the
157    private headers group.
158
159  - R5.10 The including statement order inside a group: Compare the include
160    statement as strings and sort by the string comparison result.
161
162  - R5.11 The header for the referenced symbol or definition must be included
163    even this header is included inside one of the existing included headers.
164    This improves portability in case the existing header implementation
165    changed.
166
167Examples:
168
169.. code-block:: c
170
171  /*
172   * The structure:
173   *   module_a/include/func1.h
174   *   module_a/include/func2/fmain.h
175   *   module_a/func1.c
176   *   module_a/func2/fmain.c
177   *   module_b/include/funcx.h
178   *   module_b/include/funcy/fmain.h
179   *   module_b/funcx.c
180   *   module_b/funcxi.h
181   *   module_b/funcy/fmain.c
182   *   module_b/funcy/fsub.c
183   *   module_b/funcy/fsub.h
184   * Here takes module_b/funcx.c as example:
185   */
186  #include <func1.h>
187  #include <func2/fmain.h>
188  #include <funcx.h>
189  #include "funcxi.h"
190  #include "funcy/fsub.h"
191
192Auto-doc
193========
194Auto document system such as doxygen is useful for describing interfaces. While
195it would be a development burden since the details are described in the design
196documents already. The guidelines for auto-doc:
197
198.. important::
199  - R6.1 Headers and sources under these folders need to apply auto-doc style
200    comments: ``*include``.
201  - R6.2 Developers decide the comment style for sources out of listed folders.
202
203Commit Message
204==============
205TF-M has the requirements on commit message:
206
207.. important::
208  - R7.1 Assign correct topic for a patch. Check the following table.
209
210============== ====================================================
211Topic          Justification
212============== ====================================================
213Boot           `bl2/*` or `bl1/*`
214Build          For build system related purpose.
215Docs           All \*.rst changes.
216Dualcpu        Dual-cpu related changes.
217HAL            Generic HAL interface/implementation changes.
218Pack           For packing purpose.
219Platform       Generic platform related changes under `platform/*`.
220Platform Name  Specific platform changes.
221Partition      Multiple partition related changes.
222Partition Name Specific partition related changes.
223Service        Multiple service related changes.
224Service Name   Specific service related changes.
225SPM            `secure_fw/spm/*`
226SPRTL          `secure-fw/partitions/lib/runtime/*`
227Tool           `tools` folder or `tf-m-tools` repo
228============== ====================================================
229
230.. note::
231  Ideally, one topic should cover one specific type of changes. For crossing
232  topic changes, check the main part of the change and use the main part
233  related topic as patch topic. If there is no suitable topics to cover the
234  change, contact the community for an update.
235
236--------------
237
238*Copyright (c) 2020-2022, Arm Limited. All rights reserved.*
239