| 1 | |
| 2 |
|
| 3 | The tip tree handbook
|
| 4 | =====================
|
| 5 |
|
| 6 | What is the tip tree?
|
| 7 | ---------------------
|
| 8 |
|
| 9 | The tip tree is a collection of several subsystems and areas of
|
| 10 | development. The tip tree is both a direct development tree and an
|
| 11 | aggregation tree for several sub-maintainer trees. The tip tree gitweb URL
|
| 12 | is: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
|
| 13 |
|
| 14 | The tip tree contains the following subsystems:
|
| 15 |
|
| 16 | - **x86 architecture**
|
| 17 |
|
| 18 | The x86 architecture development takes place in the tip tree except
|
| 19 | for the x86 KVM and XEN specific parts which are maintained in the
|
| 20 | corresponding subsystems and routed directly to mainline from
|
| 21 | there. It's still good practice to Cc the x86 maintainers on
|
| 22 | x86-specific KVM and XEN patches.
|
| 23 |
|
| 24 | Some x86 subsystems have their own maintainers in addition to the
|
| 25 | overall x86 maintainers. Please Cc the overall x86 maintainers on
|
| 26 | patches touching files in arch/x86 even when they are not called out
|
| 27 | by the MAINTAINER file.
|
| 28 |
|
| 29 | Note, that ``[email protected]`` is not a mailing list. It is merely a
|
| 30 | mail alias which distributes mails to the x86 top-level maintainer
|
| 31 | team. Please always Cc the Linux Kernel mailing list (LKML)
|
| 32 | ``[email protected]``, otherwise your mail ends up only in
|
| 33 | the private inboxes of the maintainers.
|
| 34 |
|
| 35 | - **Scheduler**
|
| 36 |
|
| 37 | Scheduler development takes place in the -tip tree, in the
|
| 38 | sched/core branch - with occasional sub-topic trees for
|
| 39 | work-in-progress patch-sets.
|
| 40 |
|
| 41 | - **Locking and atomics**
|
| 42 |
|
| 43 | Locking development (including atomics and other synchronization
|
| 44 | primitives that are connected to locking) takes place in the -tip
|
| 45 | tree, in the locking/core branch - with occasional sub-topic trees
|
| 46 | for work-in-progress patch-sets.
|
| 47 |
|
| 48 | - **Generic interrupt subsystem and interrupt chip drivers**:
|
| 49 |
|
| 50 | - interrupt core development happens in the irq/core branch
|
| 51 |
|
| 52 | - interrupt chip driver development also happens in the irq/core
|
| 53 | branch, but the patches are usually applied in a separate maintainer
|
| 54 | tree and then aggregated into irq/core
|
| 55 |
|
| 56 | - **Time, timers, timekeeping, NOHZ and related chip drivers**:
|
| 57 |
|
| 58 | - timekeeping, clocksource core, NTP and alarmtimer development
|
| 59 | happens in the timers/core branch, but patches are usually applied in
|
| 60 | a separate maintainer tree and then aggregated into timers/core
|
| 61 |
|
| 62 | - clocksource/event driver development happens in the timers/core
|
| 63 | branch, but patches are mostly applied in a separate maintainer tree
|
| 64 | and then aggregated into timers/core
|
| 65 |
|
| 66 | - **Performance counters core, architecture support and tooling**:
|
| 67 |
|
| 68 | - perf core and architecture support development happens in the
|
| 69 | perf/core branch
|
| 70 |
|
| 71 | - perf tooling development happens in the perf tools maintainer
|
| 72 | tree and is aggregated into the tip tree.
|
| 73 |
|
| 74 | - **CPU hotplug core**
|
| 75 |
|
| 76 | - **RAS core**
|
| 77 |
|
| 78 | Mostly x86-specific RAS patches are collected in the tip ras/core
|
| 79 | branch.
|
| 80 |
|
| 81 | - **EFI core**
|
| 82 |
|
| 83 | EFI development in the efi git tree. The collected patches are
|
| 84 | aggregated in the tip efi/core branch.
|
| 85 |
|
| 86 | - **RCU**
|
| 87 |
|
| 88 | RCU development happens in the linux-rcu tree. The resulting changes
|
| 89 | are aggregated into the tip core/rcu branch.
|
| 90 |
|
| 91 | - **Various core code components**:
|
| 92 |
|
| 93 | - debugobjects
|
| 94 |
|
| 95 | - objtool
|
| 96 |
|
| 97 | - random bits and pieces
|
| 98 |
|
| 99 |
|
| 100 | Patch submission notes
|
| 101 | ----------------------
|
| 102 |
|
| 103 | Selecting the tree/branch
|
| 104 | ^^^^^^^^^^^^^^^^^^^^^^^^^
|
| 105 |
|
| 106 | In general, development against the head of the tip tree master branch is
|
| 107 | fine, but for the subsystems which are maintained separately, have their
|
| 108 | own git tree and are only aggregated into the tip tree, development should
|
| 109 | take place against the relevant subsystem tree or branch.
|
| 110 |
|
| 111 | Bug fixes which target mainline should always be applicable against the
|
| 112 | mainline kernel tree. Potential conflicts against changes which are already
|
| 113 | queued in the tip tree are handled by the maintainers.
|
| 114 |
|
| 115 | Patch subject
|
| 116 | ^^^^^^^^^^^^^
|
| 117 |
|
| 118 | The tip tree preferred format for patch subject prefixes is
|
| 119 | 'subsys/component:', e.g. 'x86/apic:', 'x86/mm/fault:', 'sched/fair:',
|
| 120 | 'genirq/core:'. Please do not use file names or complete file paths as
|
| 121 | prefix. 'git log path/to/file' should give you a reasonable hint in most
|
| 122 | cases.
|
| 123 |
|
| 124 | The condensed patch description in the subject line should start with an
|
| 125 | uppercase letter and should be written in imperative tone.
|
| 126 |
|
| 127 |
|
| 128 | Changelog
|
| 129 | ^^^^^^^^^
|
| 130 |
|
| 131 | The general rules about changelogs in the :ref:`Submitting patches guide
|
| 132 | <describe_changes>`, apply.
|
| 133 |
|
| 134 | The tip tree maintainers set value on following these rules, especially on
|
| 135 | the request to write changelogs in imperative mood and not impersonating
|
| 136 | code or the execution of it. This is not just a whim of the
|
| 137 | maintainers. Changelogs written in abstract words are more precise and
|
| 138 | tend to be less confusing than those written in the form of novels.
|
| 139 |
|
| 140 | It's also useful to structure the changelog into several paragraphs and not
|
| 141 | lump everything together into a single one. A good structure is to explain
|
| 142 | the context, the problem and the solution in separate paragraphs and this
|
| 143 | order.
|
| 144 |
|
| 145 | Examples for illustration:
|
| 146 |
|
| 147 | Example 1::
|
| 148 |
|
| 149 | x86/intel_rdt/mbm: Fix MBM overflow handler during hot cpu
|
| 150 |
|
| 151 | When a CPU is dying, we cancel the worker and schedule a new worker on a
|
| 152 | different CPU on the same domain. But if the timer is already about to
|
| 153 | expire (say 0.99s) then we essentially double the interval.
|
| 154 |
|
| 155 | We modify the hot cpu handling to cancel the delayed work on the dying
|
| 156 | cpu and run the worker immediately on a different cpu in same domain. We
|
| 157 | do not flush the worker because the MBM overflow worker reschedules the
|
| 158 | worker on same CPU and scans the domain->cpu_mask to get the domain
|
| 159 | pointer.
|
| 160 |
|
| 161 | Improved version::
|
| 162 |
|
| 163 | x86/intel_rdt/mbm: Fix MBM overflow handler during CPU hotplug
|
| 164 |
|
| 165 | When a CPU is dying, the overflow worker is canceled and rescheduled on a
|
| 166 | different CPU in the same domain. But if the timer is already about to
|
| 167 | expire this essentially doubles the interval which might result in a non
|
| 168 | detected overflow.
|
| 169 |
|
| 170 | Cancel the overflow worker and reschedule it immediately on a different CPU
|
| 171 | in the same domain. The work could be flushed as well, but that would
|
| 172 | reschedule it on the same CPU.
|
| 173 |
|
| 174 | Example 2::
|
| 175 |
|
| 176 | time: POSIX CPU timers: Ensure that variable is initialized
|
| 177 |
|
| 178 | If cpu_timer_sample_group returns -EINVAL, it will not have written into
|
| 179 | *sample. Checking for cpu_timer_sample_group's return value precludes the
|
| 180 | potential use of an uninitialized value of now in the following block.
|
| 181 | Given an invalid clock_idx, the previous code could otherwise overwrite
|
| 182 | *oldval in an undefined manner. This is now prevented. We also exploit
|
| 183 | short-circuiting of && to sample the timer only if the result will
|
| 184 | actually be used to update *oldval.
|
| 185 |
|
| 186 | Improved version::
|
| 187 |
|
| 188 | posix-cpu-timers: Make set_process_cpu_timer() more robust
|
| 189 |
|
| 190 | Because the return value of cpu_timer_sample_group() is not checked,
|
| 191 | compilers and static checkers can legitimately warn about a potential use
|
| 192 | of the uninitialized variable 'now'. This is not a runtime issue as all
|
| 193 | call sites hand in valid clock ids.
|
| 194 |
|
| 195 | Also cpu_timer_sample_group() is invoked unconditionally even when the
|
| 196 | result is not used because *oldval is NULL.
|
| 197 |
|
| 198 | Make the invocation conditional and check the return value.
|
| 199 |
|
| 200 | Example 3::
|
| 201 |
|
| 202 | The entity can also be used for other purposes.
|
| 203 |
|
| 204 | Let's rename it to be more generic.
|
| 205 |
|
| 206 | Improved version::
|
| 207 |
|
| 208 | The entity can also be used for other purposes.
|
| 209 |
|
| 210 | Rename it to be more generic.
|
| 211 |
|
| 212 |
|
| 213 | For complex scenarios, especially race conditions and memory ordering
|
| 214 | issues, it is valuable to depict the scenario with a table which shows
|
| 215 | the parallelism and the temporal order of events. Here is an example::
|
| 216 |
|
| 217 | CPU0 CPU1
|
| 218 | free_irq(X) interrupt X
|
| 219 | spin_lock(desc->lock)
|
| 220 | wake irq thread()
|
| 221 | spin_unlock(desc->lock)
|
| 222 | spin_lock(desc->lock)
|
| 223 | remove action()
|
| 224 | shutdown_irq()
|
| 225 | release_resources() thread_handler()
|
| 226 | spin_unlock(desc->lock) access released resources.
|
| 227 | ^^^^^^^^^^^^^^^^^^^^^^^^^
|
| 228 | synchronize_irq()
|
| 229 |
|
| 230 | Lockdep provides similar useful output to depict a possible deadlock
|
| 231 | scenario::
|
| 232 |
|
| 233 | CPU0 CPU1
|
| 234 | rtmutex_lock(&rcu->rt_mutex)
|
| 235 | spin_lock(&rcu->rt_mutex.wait_lock)
|
| 236 | local_irq_disable()
|
| 237 | spin_lock(&timer->it_lock)
|
| 238 | spin_lock(&rcu->mutex.wait_lock)
|
| 239 | --> Interrupt
|
| 240 | spin_lock(&timer->it_lock)
|
| 241 |
|
| 242 |
|
| 243 | Function references in changelogs
|
| 244 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
| 245 |
|
| 246 | When a function is mentioned in the changelog, either the text body or the
|
| 247 | subject line, please use the format 'function_name()'. Omitting the
|
| 248 | brackets after the function name can be ambiguous::
|
| 249 |
|
| 250 | Subject: subsys/component: Make reservation_count static
|
| 251 |
|
| 252 | reservation_count is only used in reservation_stats. Make it static.
|
| 253 |
|
| 254 | The variant with brackets is more precise::
|
| 255 |
|
| 256 | Subject: subsys/component: Make reservation_count() static
|
| 257 |
|
| 258 | reservation_count() is only called from reservation_stats(). Make it
|
| 259 | static.
|
| 260 |
|
| 261 |
|
| 262 | Backtraces in changelogs
|
| 263 | ^^^^^^^^^^^^^^^^^^^^^^^^
|
| 264 |
|
| 265 | See :ref:`backtraces`.
|
| 266 |
|
| 267 | Ordering of commit tags
|
| 268 | ^^^^^^^^^^^^^^^^^^^^^^^
|
| 269 |
|
| 270 | To have a uniform view of the commit tags, the tip maintainers use the
|
| 271 | following tag ordering scheme:
|
| 272 |
|
| 273 | - Fixes: 12+char-SHA1 ("sub/sys: Original subject line")
|
| 274 |
|
| 275 | A Fixes tag should be added even for changes which do not need to be
|
| 276 | backported to stable kernels, i.e. when addressing a recently introduced
|
| 277 | issue which only affects tip or the current head of mainline. These tags
|
| 278 | are helpful to identify the original commit and are much more valuable
|
| 279 | than prominently mentioning the commit which introduced a problem in the
|
| 280 | text of the changelog itself because they can be automatically
|
| 281 | extracted.
|
| 282 |
|
| 283 | The following example illustrates the difference::
|
| 284 |
|
| 285 | Commit
|
| 286 |
|
| 287 | abcdef012345678 ("x86/xxx: Replace foo with bar")
|
| 288 |
|
| 289 | left an unused instance of variable foo around. Remove it.
|
| 290 |
|
| 291 | Signed-off-by: J.Dev <j.dev@mail>
|
| 292 |
|
| 293 | Please say instead::
|
| 294 |
|
| 295 | The recent replacement of foo with bar left an unused instance of
|
| 296 | variable foo around. Remove it.
|
| 297 |
|
| 298 | Fixes: abcdef012345678 ("x86/xxx: Replace foo with bar")
|
| 299 | Signed-off-by: J.Dev <j.dev@mail>
|
| 300 |
|
| 301 | The latter puts the information about the patch into the focus and
|
| 302 | amends it with the reference to the commit which introduced the issue
|
| 303 | rather than putting the focus on the original commit in the first place.
|
| 304 |
|
| 305 | - Reported-by: ``Reporter <reporter@mail>``
|
| 306 |
|
| 307 | - Closes: ``URL or Message-ID of the bug report this is fixing``
|
| 308 |
|
| 309 | - Originally-by: ``Original author <original-author@mail>``
|
| 310 |
|
| 311 | - Suggested-by: ``Suggester <suggester@mail>``
|
| 312 |
|
| 313 | - Co-developed-by: ``Co-author <co-author@mail>``
|
| 314 |
|
| 315 | Signed-off-by: ``Co-author <co-author@mail>``
|
| 316 |
|
| 317 | Note, that Co-developed-by and Signed-off-by of the co-author(s) must
|
| 318 | come in pairs.
|
| 319 |
|
| 320 | - Signed-off-by: ``Author <author@mail>``
|
| 321 |
|
| 322 | The first Signed-off-by (SOB) after the last Co-developed-by/SOB pair is the
|
| 323 | author SOB, i.e. the person flagged as author by git.
|
| 324 |
|
| 325 | - Signed-off-by: ``Patch handler <handler@mail>``
|
| 326 |
|
| 327 | SOBs after the author SOB are from people handling and transporting
|
| 328 | the patch, but were not involved in development. SOB chains should
|
| 329 | reflect the **real** route a patch took as it was propagated to us,
|
| 330 | with the first SOB entry signalling primary authorship of a single
|
| 331 | author. Acks should be given as Acked-by lines and review approvals
|
| 332 | as Reviewed-by lines.
|
| 333 |
|
| 334 | If the handler made modifications to the patch or the changelog, then
|
| 335 | this should be mentioned **after** the changelog text and **above**
|
| 336 | all commit tags in the following format::
|
| 337 |
|
| 338 | ... changelog text ends.
|
| 339 |
|
| 340 | [ handler: Replaced foo by bar and updated changelog ]
|
| 341 |
|
| 342 | First-tag: .....
|
| 343 |
|
| 344 | Note the two empty new lines which separate the changelog text and the
|
| 345 | commit tags from that notice.
|
| 346 |
|
| 347 | If a patch is sent to the mailing list by a handler then the author has
|
| 348 | to be noted in the first line of the changelog with::
|
| 349 |
|
| 350 | From: Author <author@mail>
|
| 351 |
|
| 352 | Changelog text starts here....
|
| 353 |
|
| 354 | so the authorship is preserved. The 'From:' line has to be followed
|
| 355 | by an empty newline. If that 'From:' line is missing, then the patch
|
| 356 | would be attributed to the person who sent (transported, handled) it.
|
| 357 | The 'From:' line is automatically removed when the patch is applied
|
| 358 | and does not show up in the final git changelog. It merely affects
|
| 359 | the authorship information of the resulting Git commit.
|
| 360 |
|
| 361 | - Tested-by: ``Tester <tester@mail>``
|
| 362 |
|
| 363 | - Reviewed-by: ``Reviewer <reviewer@mail>``
|
| 364 |
|
| 365 | - Acked-by: ``Acker <acker@mail>``
|
| 366 |
|
| 367 | - Cc: ``cc-ed-person <person@mail>``
|
| 368 |
|
| 369 | If the patch should be backported to stable, then please add a '``Cc:
|
| 370 | [email protected]``' tag, but do not Cc stable when sending your
|
| 371 | mail.
|
| 372 |
|
| 373 | - Link: ``https://link/to/information``
|
| 374 |
|
| 375 | For referring to an email posted to the kernel mailing lists, please
|
| 376 | use the lore.kernel.org redirector URL::
|
| 377 |
|
| 378 | Link: https://lore.kernel.org/email-message-id@here
|
| 379 |
|
| 380 | This URL should be used when referring to relevant mailing list
|
| 381 | topics, related patch sets, or other notable discussion threads.
|
| 382 | A convenient way to associate ``Link:`` trailers with the commit
|
| 383 | message is to use markdown-like bracketed notation, for example::
|
| 384 |
|
| 385 | A similar approach was attempted before as part of a different
|
| 386 | effort [1], but the initial implementation caused too many
|
| 387 | regressions [2], so it was backed out and reimplemented.
|
| 388 |
|
| 389 | Link: https://lore.kernel.org/some-msgid@here # [1]
|
| 390 | Link: https://bugzilla.example.org/bug/12345 # [2]
|
| 391 |
|
| 392 | You can also use ``Link:`` trailers to indicate the origin of the
|
| 393 | patch when applying it to your git tree. In that case, please use the
|
| 394 | dedicated ``patch.msgid.link`` domain instead of ``lore.kernel.org``.
|
| 395 | This practice makes it possible for automated tooling to identify
|
| 396 | which link to use to retrieve the original patch submission. For
|
| 397 | example::
|
| 398 |
|
| 399 | Link: https://patch.msgid.link/patch-source-message-id@here
|
| 400 |
|
| 401 | Please do not use combined tags, e.g. ``Reported-and-tested-by``, as
|
| 402 | they just complicate automated extraction of tags.
|
| 403 |
|
| 404 |
|
| 405 | Links to documentation
|
| 406 | ^^^^^^^^^^^^^^^^^^^^^^
|
| 407 |
|
| 408 | Providing links to documentation in the changelog is a great help to later
|
| 409 | debugging and analysis. Unfortunately, URLs often break very quickly
|
| 410 | because companies restructure their websites frequently. Non-'volatile'
|
| 411 | exceptions include the Intel SDM and the AMD APM.
|
| 412 |
|
| 413 | Therefore, for 'volatile' documents, please create an entry in the kernel
|
| 414 | bugzilla https://bugzilla.kernel.org and attach a copy of these documents
|
| 415 | to the bugzilla entry. Finally, provide the URL of the bugzilla entry in
|
| 416 | the changelog.
|
| 417 |
|
| 418 | Patch resend or reminders
|
| 419 | ^^^^^^^^^^^^^^^^^^^^^^^^^
|
| 420 |
|
| 421 | See :ref:`resend_reminders`.
|
| 422 |
|
| 423 | Merge window
|
| 424 | ^^^^^^^^^^^^
|
| 425 |
|
| 426 | Please do not expect patches to be reviewed or merged by tip
|
| 427 | maintainers around or during the merge window. The trees are closed
|
| 428 | to all but urgent fixes during this time. They reopen once the merge
|
| 429 | window closes and a new -rc1 kernel has been released.
|
| 430 |
|
| 431 | Large series should be submitted in mergeable state *at* *least* a week
|
| 432 | before the merge window opens. Exceptions are made for bug fixes and
|
| 433 | *sometimes* for small standalone drivers for new hardware or minimally
|
| 434 | invasive patches for hardware enablement.
|
| 435 |
|
| 436 | During the merge window, the maintainers instead focus on following the
|
| 437 | upstream changes, fixing merge window fallout, collecting bug fixes, and
|
| 438 | allowing themselves a breath. Please respect that.
|
| 439 |
|
| 440 | So called _urgent_ branches will be merged into mainline during the
|
| 441 | stabilization phase of each release.
|
| 442 |
|
| 443 |
|
| 444 | Git
|
| 445 | ^^^
|
| 446 |
|
| 447 | The tip maintainers accept git pull requests from maintainers who provide
|
| 448 | subsystem changes for aggregation in the tip tree.
|
| 449 |
|
| 450 | Pull requests for new patch submissions are usually not accepted and do not
|
| 451 | replace proper patch submission to the mailing list. The main reason for
|
| 452 | this is that the review workflow is email based.
|
| 453 |
|
| 454 | If you submit a larger patch series it is helpful to provide a git branch
|
| 455 | in a private repository which allows interested people to easily pull the
|
| 456 | series for testing. The usual way to offer this is a git URL in the cover
|
| 457 | letter of the patch series.
|
| 458 |
|
| 459 | Testing
|
| 460 | ^^^^^^^
|
| 461 |
|
| 462 | Code should be tested before submitting to the tip maintainers. Anything
|
| 463 | other than minor changes should be built, booted and tested with
|
| 464 | comprehensive (and heavyweight) kernel debugging options enabled.
|
| 465 |
|
| 466 | These debugging options can be found in kernel/configs/x86_debug.config
|
| 467 | and can be added to an existing kernel config by running:
|
| 468 |
|
| 469 | make x86_debug.config
|
| 470 |
|
| 471 | Some of these options are x86-specific and can be left out when testing
|
| 472 | on other architectures.
|
| 473 |
|
| 474 | .. _maintainer-tip-coding-style:
|
| 475 |
|
| 476 | Coding style notes
|
| 477 | ------------------
|
| 478 |
|
| 479 | Comment style
|
| 480 | ^^^^^^^^^^^^^
|
| 481 |
|
| 482 | Sentences in comments start with an uppercase letter.
|
| 483 |
|
| 484 | Single line comments::
|
| 485 |
|
| 486 | /* This is a single line comment */
|
| 487 |
|
| 488 | Multi-line comments::
|
| 489 |
|
| 490 | /*
|
| 491 | * This is a properly formatted
|
| 492 | * multi-line comment.
|
| 493 | *
|
| 494 | * Larger multi-line comments should be split into paragraphs.
|
| 495 | */
|
| 496 |
|
| 497 | No tail comments (see below):
|
| 498 |
|
| 499 | Please refrain from using tail comments. Tail comments disturb the
|
| 500 | reading flow in almost all contexts, but especially in code::
|
| 501 |
|
| 502 | if (somecondition_is_true) /* Don't put a comment here */
|
| 503 | dostuff(); /* Neither here */
|
| 504 |
|
| 505 | seed = MAGIC_CONSTANT; /* Nor here */
|
| 506 |
|
| 507 | Use freestanding comments instead::
|
| 508 |
|
| 509 | /* This condition is not obvious without a comment */
|
| 510 | if (somecondition_is_true) {
|
| 511 | /* This really needs to be documented */
|
| 512 | dostuff();
|
| 513 | }
|
| 514 |
|
| 515 | /* This magic initialization needs a comment. Maybe not? */
|
| 516 | seed = MAGIC_CONSTANT;
|
| 517 |
|
| 518 | Use C++ style, tail comments when documenting structs in headers to
|
| 519 | achieve a more compact layout and better readability::
|
| 520 |
|
| 521 | // eax
|
| 522 | u32 x2apic_shift : 5, // Number of bits to shift APIC ID right
|
| 523 | // for the topology ID at the next level
|
| 524 | : 27; // Reserved
|
| 525 | // ebx
|
| 526 | u32 num_processors : 16, // Number of processors at current level
|
| 527 | : 16; // Reserved
|
| 528 |
|
| 529 | versus::
|
| 530 |
|
| 531 | /* eax */
|
| 532 | /*
|
| 533 | * Number of bits to shift APIC ID right for the topology ID
|
| 534 | * at the next level
|
| 535 | */
|
| 536 | u32 x2apic_shift : 5,
|
| 537 | /* Reserved */
|
| 538 | : 27;
|
| 539 |
|
| 540 | /* ebx */
|
| 541 | /* Number of processors at current level */
|
| 542 | u32 num_processors : 16,
|
| 543 | /* Reserved */
|
| 544 | : 16;
|
| 545 |
|
| 546 | Comment the important things:
|
| 547 |
|
| 548 | Comments should be added where the operation is not obvious. Documenting
|
| 549 | the obvious is just a distraction::
|
| 550 |
|
| 551 | /* Decrement refcount and check for zero */
|
| 552 | if (refcount_dec_and_test(&p->refcnt)) {
|
| 553 | do;
|
| 554 | lots;
|
| 555 | of;
|
| 556 | magic;
|
| 557 | things;
|
| 558 | }
|
| 559 |
|
| 560 | Instead, comments should explain the non-obvious details and document
|
| 561 | constraints::
|
| 562 |
|
| 563 | if (refcount_dec_and_test(&p->refcnt)) {
|
| 564 | /*
|
| 565 | * Really good explanation why the magic things below
|
| 566 | * need to be done, ordering and locking constraints,
|
| 567 | * etc..
|
| 568 | */
|
| 569 | do;
|
| 570 | lots;
|
| 571 | of;
|
| 572 | magic;
|
| 573 | /* Needs to be the last operation because ... */
|
| 574 | things;
|
| 575 | }
|
| 576 |
|
| 577 | Function documentation comments:
|
| 578 |
|
| 579 | To document functions and their arguments please use kernel-doc format
|
| 580 | and not free form comments::
|
| 581 |
|
| 582 | /**
|
| 583 | * magic_function - Do lots of magic stuff
|
| 584 | * @magic: Pointer to the magic data to operate on
|
| 585 | * @offset: Offset in the data array of @magic
|
| 586 | *
|
| 587 | * Deep explanation of mysterious things done with @magic along
|
| 588 | * with documentation of the return values.
|
| 589 | *
|
| 590 | * Note, that the argument descriptors above are arranged
|
| 591 | * in a tabular fashion.
|
| 592 | */
|
| 593 |
|
| 594 | This applies especially to globally visible functions and inline
|
| 595 | functions in public header files. It might be overkill to use kernel-doc
|
| 596 | format for every (static) function which needs a tiny explanation. The
|
| 597 | usage of descriptive function names often replaces these tiny comments.
|
| 598 | Apply common sense as always.
|
| 599 |
|
| 600 |
|
| 601 | Documenting locking requirements
|
| 602 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
| 603 | Documenting locking requirements is a good thing, but comments are not
|
| 604 | necessarily the best choice. Instead of writing::
|
| 605 |
|
| 606 | /* Caller must hold foo->lock */
|
| 607 | void func(struct foo *foo)
|
| 608 | {
|
| 609 | ...
|
| 610 | }
|
| 611 |
|
| 612 | Please use::
|
| 613 |
|
| 614 | void func(struct foo *foo)
|
| 615 | {
|
| 616 | lockdep_assert_held(&foo->lock);
|
| 617 | ...
|
| 618 | }
|
| 619 |
|
| 620 | In PROVE_LOCKING kernels, lockdep_assert_held() emits a warning
|
| 621 | if the caller doesn't hold the lock. Comments can't do that.
|
| 622 |
|
| 623 | Bracket rules
|
| 624 | ^^^^^^^^^^^^^
|
| 625 |
|
| 626 | Brackets should be omitted only if the statement which follows 'if', 'for',
|
| 627 | 'while' etc. is truly a single line::
|
| 628 |
|
| 629 | if (foo)
|
| 630 | do_something();
|
| 631 |
|
| 632 | The following is not considered to be a single line statement even
|
| 633 | though C does not require brackets::
|
| 634 |
|
| 635 | for (i = 0; i < end; i++)
|
| 636 | if (foo[i])
|
| 637 | do_something(foo[i]);
|
| 638 |
|
| 639 | Adding brackets around the outer loop enhances the reading flow::
|
| 640 |
|
| 641 | for (i = 0; i < end; i++) {
|
| 642 | if (foo[i])
|
| 643 | do_something(foo[i]);
|
| 644 | }
|
| 645 |
|
| 646 |
|
| 647 | Variable declarations
|
| 648 | ^^^^^^^^^^^^^^^^^^^^^
|
| 649 |
|
| 650 | The preferred ordering of variable declarations at the beginning of a
|
| 651 | function is reverse fir tree order::
|
| 652 |
|
| 653 | struct long_struct_name *descriptive_name;
|
| 654 | unsigned long foo, bar;
|
| 655 | unsigned int tmp;
|
| 656 | int ret;
|
| 657 |
|
| 658 | The above is faster to parse than the reverse ordering::
|
| 659 |
|
| 660 | int ret;
|
| 661 | unsigned int tmp;
|
| 662 | unsigned long foo, bar;
|
| 663 | struct long_struct_name *descriptive_name;
|
| 664 |
|
| 665 | And even more so than random ordering::
|
| 666 |
|
| 667 | unsigned long foo, bar;
|
| 668 | int ret;
|
| 669 | struct long_struct_name *descriptive_name;
|
| 670 | unsigned int tmp;
|
| 671 |
|
| 672 | Also please try to aggregate variables of the same type into a single
|
| 673 | line. There is no point in wasting screen space::
|
| 674 |
|
| 675 | unsigned long a;
|
| 676 | unsigned long b;
|
| 677 | unsigned long c;
|
| 678 | unsigned long d;
|
| 679 |
|
| 680 | It's really sufficient to do::
|
| 681 |
|
| 682 | unsigned long a, b, c, d;
|
| 683 |
|
| 684 | Please also refrain from introducing line splits in variable declarations::
|
| 685 |
|
| 686 | struct long_struct_name *descriptive_name = container_of(bar,
|
| 687 | struct long_struct_name,
|
| 688 | member);
|
| 689 | struct foobar foo;
|
| 690 |
|
| 691 | It's way better to move the initialization to a separate line after the
|
| 692 | declarations::
|
| 693 |
|
| 694 | struct long_struct_name *descriptive_name;
|
| 695 | struct foobar foo;
|
| 696 |
|
| 697 | descriptive_name = container_of(bar, struct long_struct_name, member);
|
| 698 |
|
| 699 |
|
| 700 | Variable types
|
| 701 | ^^^^^^^^^^^^^^
|
| 702 |
|
| 703 | Please use the proper u8, u16, u32, u64 types for variables which are meant
|
| 704 | to describe hardware or are used as arguments for functions which access
|
| 705 | hardware. These types are clearly defining the bit width and avoid
|
| 706 | truncation, expansion and 32/64-bit confusion.
|
| 707 |
|
| 708 | u64 is also recommended in code which would become ambiguous for 32-bit
|
| 709 | kernels when 'unsigned long' would be used instead. While in such
|
| 710 | situations 'unsigned long long' could be used as well, u64 is shorter
|
| 711 | and also clearly shows that the operation is required to be 64 bits wide
|
| 712 | independent of the target CPU.
|
| 713 |
|
| 714 | Please use 'unsigned int' instead of 'unsigned'.
|
| 715 |
|
| 716 |
|
| 717 | Constants
|
| 718 | ^^^^^^^^^
|
| 719 |
|
| 720 | Please do not use literal (hexa)decimal numbers in code or initializers.
|
| 721 | Either use proper defines which have descriptive names or consider using
|
| 722 | an enum.
|
| 723 |
|
| 724 |
|
| 725 | Struct declarations and initializers
|
| 726 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
| 727 |
|
| 728 | Struct declarations should align the struct member names in a tabular
|
| 729 | fashion::
|
| 730 |
|
| 731 | struct bar_order {
|
| 732 | unsigned int guest_id;
|
| 733 | int ordered_item;
|
| 734 | struct menu *menu;
|
| 735 | };
|
| 736 |
|
| 737 | Please avoid documenting struct members within the declaration, because
|
| 738 | this often results in strangely formatted comments and the struct members
|
| 739 | become obfuscated::
|
| 740 |
|
| 741 | struct bar_order {
|
| 742 | unsigned int guest_id; /* Unique guest id */
|
| 743 | int ordered_item;
|
| 744 | /* Pointer to a menu instance which contains all the drinks */
|
| 745 | struct menu *menu;
|
| 746 | };
|
| 747 |
|
| 748 | Instead, please consider using the kernel-doc format in a comment preceding
|
| 749 | the struct declaration, which is easier to read and has the added advantage
|
| 750 | of including the information in the kernel documentation, for example, as
|
| 751 | follows::
|
| 752 |
|
| 753 |
|
| 754 | /**
|
| 755 | * struct bar_order - Description of a bar order
|
| 756 | * @guest_id: Unique guest id
|
| 757 | * @ordered_item: The item number from the menu
|
| 758 | * @menu: Pointer to the menu from which the item
|
| 759 | * was ordered
|
| 760 | *
|
| 761 | * Supplementary information for using the struct.
|
| 762 | *
|
| 763 | * Note, that the struct member descriptors above are arranged
|
| 764 | * in a tabular fashion.
|
| 765 | */
|
| 766 | struct bar_order {
|
| 767 | unsigned int guest_id;
|
| 768 | int ordered_item;
|
| 769 | struct menu *menu;
|
| 770 | };
|
| 771 |
|
| 772 | Static struct initializers must use C99 initializers and should also be
|
| 773 | aligned in a tabular fashion::
|
| 774 |
|
| 775 | static struct foo statfoo = {
|
| 776 | .a = 0,
|
| 777 | .plain_integer = CONSTANT_DEFINE_OR_ENUM,
|
| 778 | .bar = &statbar,
|
| 779 | };
|
| 780 |
|
| 781 | Note that while C99 syntax allows the omission of the final comma,
|
| 782 | we recommend the use of a comma on the last line because it makes
|
| 783 | reordering and addition of new lines easier, and makes such future
|
| 784 | patches slightly easier to read as well.
|
| 785 |
|
| 786 | Line breaks
|
| 787 | ^^^^^^^^^^^
|
| 788 |
|
| 789 | Restricting line length to 80 characters makes deeply indented code hard to
|
| 790 | read. Consider breaking out code into helper functions to avoid excessive
|
| 791 | line breaking.
|
| 792 |
|
| 793 | The 80 character rule is not a strict rule, so please use common sense when
|
| 794 | breaking lines. Especially format strings should never be broken up.
|
| 795 |
|
| 796 | When splitting function declarations or function calls, then please align
|
| 797 | the first argument in the second line with the first argument in the first
|
| 798 | line::
|
| 799 |
|
| 800 | static int long_function_name(struct foobar *barfoo, unsigned int id,
|
| 801 | unsigned int offset)
|
| 802 | {
|
| 803 |
|
| 804 | if (!id) {
|
| 805 | ret = longer_function_name(barfoo, DEFAULT_BARFOO_ID,
|
| 806 | offset);
|
| 807 | ...
|
| 808 |
|
| 809 | Namespaces
|
| 810 | ^^^^^^^^^^
|
| 811 |
|
| 812 | Function/variable namespaces improve readability and allow easy
|
| 813 | grepping. These namespaces are string prefixes for globally visible
|
| 814 | function and variable names, including inlines. These prefixes should
|
| 815 | combine the subsystem and the component name such as 'x86_comp\_',
|
| 816 | 'sched\_', 'irq\_', and 'mutex\_'.
|
| 817 |
|
| 818 | This also includes static file scope functions that are immediately put
|
| 819 | into globally visible driver templates - it's useful for those symbols
|
| 820 | to carry a good prefix as well, for backtrace readability.
|
| 821 |
|
| 822 | Namespace prefixes may be omitted for local static functions and
|
| 823 | variables. Truly local functions, only called by other local functions,
|
| 824 | can have shorter descriptive names - our primary concern is greppability
|
| 825 | and backtrace readability.
|
| 826 |
|
| 827 | Please note that 'xxx_vendor\_' and 'vendor_xxx_` prefixes are not
|
| 828 | helpful for static functions in vendor-specific files. After all, it
|
| 829 | is already clear that the code is vendor-specific. In addition, vendor
|
| 830 | names should only be for truly vendor-specific functionality.
|
| 831 |
|
| 832 | As always apply common sense and aim for consistency and readability.
|
| 833 |
|
| 834 |
|
| 835 | Commit notifications
|
| 836 | --------------------
|
| 837 |
|
| 838 | The tip tree is monitored by a bot for new commits. The bot sends an email
|
| 839 | for each new commit to a dedicated mailing list
|
| 840 | (``[email protected]``) and Cc's all people who are
|
| 841 | mentioned in one of the commit tags. It uses the email message ID from the
|
| 842 | Link tag at the end of the tag list to set the In-Reply-To email header so
|
| 843 | the message is properly threaded with the patch submission email.
|
| 844 |
|
| 845 | The tip maintainers and submaintainers try to reply to the submitter
|
| 846 | when merging a patch, but they sometimes forget or it does not fit the
|
| 847 | workflow of the moment. While the bot message is purely mechanical, it
|
| 848 | also implies a 'Thank you! Applied.'.
|