This article describes the work on maintenance of kernel patches and process for inclusion.
Contents
The problem(s)
Work on maintenance of patches
We have lots of patches in our kernel. Ranging from simple bugfixes to added features. Maintaining them takes lots of work.
Documentation of patches
The patches in our kernel have little documentation. We don't have information regarding:
- Where the patch comes from
- Why it was added to our kernel
- Is it a feature or a bugfix?
- Why it is a separated patch and it isn't in the Linus tree yet?
This is also related to the problem on the previous section: we can't generate quickly a list of the patches we have to maintain separately (generally because we wanted some feature and decided to take the risk of maintaining the patch in our tree).
Less patches, the better
In order to minimize the time spent dealing with patches in our kernel, we SHOULD minimize the number of separated patches we have. The best place to maintain changes is upstream.
- 3rd party drivers: package them separately if possible(refer to DKMS policies)
- Additional features: only add them if it is worth the time spent in maintaining it
TODO: shouldn`t we state about features inclusion in upstream?
Proposal
Add information about the patches
We should have some information about ALL patches added to our package. Listed below.
Some information will be mandatory (preferably all fields will be mandatory).
Format of the patches
The patches should have the format described on section "The canonical patch format" on Documentation/SubmittingPatches.
The recommended tool to be used to generate the patches is git-format-patch.
Reason
Mandatory: yes
The Reason field explains why we have decided to include the patch on the package. It should be preferably a reference to a Mageia bugzilla ticket, but it is not mandatory. Every patch MUST have a Reason field.
Reason: It was requested by user XXX on bugzilla ticket #666
Origin
Mandatory: yes
The Origin field tells the reader where the patch came from, how it was found. Use common sense to tell how much information is needed to make sense of this. A good practice would be including a URL in this field.
If the upstream status field is backport, this field MUST tell from which kernel version the patch came from.
If the upstream status field is queued or own-maintenance, this field MUST be filled with information about where the patch was taken from. If it is a different kernel tree, it MUST give a pointer to it. If the patch was taken directly from the author (or the author itself is inserting the patch into the package), the field MAY be set just to "author".
Origin: Picked from upstream developer on ML.
Author and Signed-off fields
Mandatory: yes
The original author of the patch and Signed-off fields MUST be added to the patch, such as described on Documentation/SubmittingPatches.
The reason for that is the same described on Documentation/SubmittingPatches:
To improve tracking of who did what, especially with patches that can percolate to their final resting place in the kernel through several layers of maintainers, we've introduced a "sign-off" procedure on patches that are being emailed around.
Type
Mandatory: yes
The "type" field explains what kind of change the patch refers to. It is an enum value, which means that there will be a limited set of valid values for this field. The change may be:
- FEATURE - An additional feature;
- BUG_FIX - A bugfix
Type: BUG_FIX
Upstream status
Mandatory: yes
This field should explain why we are including this change as a patch, and why it is not on the upstream tarball (the Linus tree). It is an enum field, also. It will allow us to tell quickly how many patches we have to maintain ourselves, taking some work to maintain. The possible values for this field would be:
- backport: Backport of change already on Linus tree
- queued: Change will be added soon to Linus tree
- own-maintenance: Change is not on Linus tree and there are no plans to include the change on Linus tree. These patches take more work to maintain.
This field MUST be filled.
Upstream-status: queued on -mm
Summary phrase, Subject field
Mandatory: yes
Every patch MUST have a summary phrase. From Documentation/SubmittingPatches:
The "summary phrase" in the email's Subject should concisely describe the patch which that email contains. The "summary phrase" should not be a filename. Do not use the same "summary phrase" for every patch in a whole patch series.
Long description
Mandatory: yes
The long description is the "explanation body" as described on Documentation/SubmittingPatches. All patches MUST have an explanation body.
The explanation body will be committed to the permanent source changelog, so should make sense to a competent reader who has long since forgotten the immediate details of the discussion that might have led to this patch.
Example
A patch in the end should look like the following:
Reason: It was requested by user XXX on bugzilla ticket #666 Origin: Picked from upstream developer on ML. Author: Kumar Gala <galak@kernel.crashing.org> Type: BUG_FIX Upstream-status: queued on -mm Subject: powerpc/mm: Fix module instruction tlb fault handling on Book-E 64 We were seeing oops like the following when we did an rmmod on a module: Unable to handle kernel paging request for instruction fetch Faulting instruction address: 0x8000000000008010 Oops: Kernel access of bad area, sig: 11 [#1] SMP NR_CPUS=2 P5020 DS last sysfs file: /sys/devices/qman-portals.2/qman-pool.9/uevent Modules linked in: qman_tester(-) NIP: 8000000000008010 LR: c000000000074858 CTR: 8000000000008010 REGS: c00000002e29bab0 TRAP: 0400 Not tainted (2.6.34.6-00744-g2d21f14) MSR: 0000000080029000 <EE,ME,CE> CR: 24000448 XER: 00000000 TASK = c00000007a8be600[4987] 'rmmod' THREAD: c00000002e298000 CPU: 1 GPR00: 8000000000008010 c00000002e29bd30 8000000000012798 c00000000035fb28 GPR04: 0000000000000002 0000000000000002 0000000024022428 c000000000009108 GPR08: fffffffffffffffe 800000000000a618 c0000000003c13c8 0000000000000000 GPR12: 0000000022000444 c00000000fffed00 0000000000000000 0000000000000000 GPR16: 00000000100c0000 0000000000000000 00000000100dabc8 0000000010099688 GPR20: 0000000000000000 00000000100cfc28 0000000000000000 0000000010011a44 GPR24: 00000000100017b2 0000000000000000 0000000000000000 0000000000000880 GPR28: c00000000035fb28 800000000000a7b8 c000000000376d80 c0000000003cce50 NIP [8000000000008010] .test_exit+0x0/0x10 [qman_tester] LR [c000000000074858] .SyS_delete_module+0x1f8/0x2f0 Call Trace: [c00000002e29bd30] [c0000000000748b4] .SyS_delete_module+0x254/0x2f0 (unreliable) [c00000002e29be30] [c000000000000580] syscall_exit+0x0/0x2c Instruction dump: XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX 38600000 4e800020 60000000 60000000 <4e800020> 60000000 60000000 60000000 ---[ end trace 4f57124939a84dc8 ]--- This appears to be due to checking the wrong permission bits in the instruction_tlb_miss handling if the address that faulted was in vmalloc space. We need to look at the supervisor execute (_PAGE_BAP_SX) bit and not the user bit (_PAGE_BAP_UX/_PAGE_EXEC). Also removed a branch level since it did not appear to be use Signed-off-by: Kumar Gala <galak@kernel.crashing.org> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> [...]
Enforcing the policy
The buildsystem would enforce people to follow the approved documentation policy. Checking for policy conformance may be made on a commit-hook on the version control system, when patches are added to the package, thus unconforming changes may be rejected at commit-time.
TODO: Elaborate the procedure for the buildsystem
Benefits
Listing "What takes us work"
We will to be able to list quickly which changes we have added to our kernel that aren't on the Linus tree. If we keep changes that aren't on the Linus kernel tree, it will take us more work, and we need to have a good reason for the additional work of integration of the patches.
Easier maintenance
Easier maintenance/updating of patches due to good documentation. Sometimes regenerating a patch for a kernel update is hard because the code may have changed too much, and we don't know if there is a more recent version of the patch somewhere, who maintains it, etc. Including the origin, reason, author and signed-off information will make this task easier.
Taking decisions
We will be able to take better decisions about what we want to keep in our kernel. Listing "What takes us work" will allow the Community to decide if we really want to keep some features in the kernel, even having more work.
This page is available under the Creative Commons Attribution-ShareAlike 2.5
Authors
- Vincent Danen
- Leandro Dorileo
- Daniel Kreuter
- Rémy Clouard
