Development Guidelines: Difference between revisions

From flashrom
Jump to navigation Jump to search
(Add info about reg_bits and decode_range for write-protect →‎Adding/reviewing a new flash chip)
(40 intermediate revisions by 5 users not shown)
Line 1: Line 1:
= Branches =
= Branches =


Till the release of flashrom 0.9.9 there was basically a single branch (trunk) where linear development happened.
=== Historical ===
Due to the lack of reviewing efforts proposed patches were not integrated in a timely manner which has not only led to shrinking participation over the years but also growing maintenance work to keep unintegrated patches from bitrotting.
This strategy was also responsible for numerous merge conflicts when said patches were rebased.


The upside was a very stable project in terms of bugs.
Till the release of flashrom 0.9.9 there was basically a single branch
While there were some mistakes that broke flashrom on some particular configurations, generally speaking one could just check out and use the newest development version of flashrom without problems.
(trunk) where linear development happened. After 0.9.9 it was decided
to switch to Git and a two branch model, a ''stable'' and a ''staging''
branch. This led to some confusion and as nobody who had access to the
''stable'' branch had the time to work on it, development continued
about one year after the 0.9.9 release on a ''staging'' branch at
coreboot.org. Despite its name, we strived to keep flashrom's quality
and hoped that everything would be merged to ''stable'' once work
continues there.


At the beginning of the 0.9.10 development cycle there were approximately 250 published unmerged patches.
=== ''master'' branch ===
To increase our pace of adopting changes we will merge patches faster for 0.9.10 with less resources spent on reviewing.
This will inevitably lead to some bugs crawling into the repository that might become a bad surprise for unprepared users.
To make this change more visible we will not merge patches to the previous trunk branch but a new development branch named ''staging''.
The old trunk branch will be renamed to ''stable''.


=== <tt>staging</tt> branch ===
The historical ''staging'' branch was finally renamed to ''master''.
Starting with 0.9.10 the main development of flashrom happens in the ''<tt>staging</tt>'' branch.
As usual there is no quality promise for the state of the code on the
Here the rough edges of flashrom will be softened over time.
''master'' branch. Even though we will try to keep the regression
Yet unmerged patches will be committed there as soon as anyone with commit rights deems them ready.
rate as low as possible, the main purpose of the branch is to merge
new commits and make them available to a broader audience for testing.


A patch is ready if it is believed to improve flashrom without any dramatic/foreseeable regressions in a maintainable way.
=== Release branches (e.g. ''1.0.x'') ===
In general a consensus should be reached among the community about this before committing a patch, ideally through the review process described below.
History has taught us that reaching consensus can be impossible if some parties do not participate in the required communication process (e.g. due to lack of time to do so).
For that reason there are no hard rules regarding the process of committing patches to ''staging'' apart from the ones stated in this chapter.
Like commits to ''stable'' every patch pushed to ''staging'' needs a <tt>Signed-off-by</tt> tag, however no <tt>Acked-by</tt> tag is needed there.


Fixes for patches already committed to <tt>staging</tt> should be written in a way that allows to apply them to the original patch independently from possible other intermediate changes.
Branching for a new release can happen at any point in time when a
That way they can be merged to <tt>stable</tt> in the proper state they should have been committed in the first place.
commit (branch point) on ''master'' seems to be in good shape and was
To support this the subject of the fixes should be kept equal to the original patch but prefixed with <tt>fixup!</tt> or <tt>squash!</tt> to facilitate git's <tt>rebase</tt> command as described in its [http://git-scm.com/docs/git-rebase manpage].
reasonably tested after previous invasive changes. Between the branch
The merging of changes from <tt>staging</tt> to <tt>stable</tt> should happen not earlier than 1 month after the last related commit to <tt>staging</tt> (i.e. a later fix will prolong this period).
point and the release, every fix pushed for ''master'' for a problem
To merge a patch an <tt>Acked-by</tt> tag by another person is required, optionally following a review process as described below.
that also persists on the release branch shall be backported. The same
also applies after the release for the latest release branch and,
optionally, for any earlier release branch that is still maintained
for other reasons (e.g. part of a long term distribution).


=== <tt>stable</tt> branch ===
Whenever a release branch has no further unmerged commits in queue
The <tt>stable</tt> branch shall remain the branch we recommend to users unless it is clear that they require changes that are only present in <tt>staging</tt>.
and is not awaiting backported fixes, a release candidate (RC) can be
It is also the branch used to branch off and tag stable releases.
tagged on that branch. This can also be the original branch point. The
RC shall undergo extensive build tests and be publicly advertised as
ready for testing. Not less than three days after the last RC that
included code changes, a release can be tagged if no regressions
showed up.


=== Stable release branches (e.g. <tt>0.9.10</tt>) ===
Release-branch names follow the pattern '''''<major>.<minor>.x'''''
Release branches are used for backporting important patches to specific releases.
(e.g. ''1.0.x''). The first release of a branch is tagged
Point releases (e.g. <tt>0.9.10.1</tt>) are tagged on them.
'''''v<major>.<minor>''''', without a point-release number (e.g.
They should be the primary source of distribution packages but are largely ignored by maintainers that prefer more cutting edge versions at the cost of possible bugs.
''v1.0''). Every following release from the same branch will have
This is also an important reason for introducing the <tt>staging</tt> branch instead of shoving everything into <tt>stable</tt> directly.
a point-release number starting with '''''.1''''' (e.g. ''v1.0.1'')
and will only add backported fixes to the release.
 
Unless the branch point (i.e. last common commit of ''master''
and a release branch) is an RC, it should be tagged as
'''''p<major>.<minor>''''' (e.g. ''p1.0''), to keep track where
we are on the ''master'' branch.


= Patch submission =
= Patch submission =


Currently there are three ways to submit patches:
== Coding style ==


1. Via our [[Mailinglist|mailing list]]
Flashrom generally follows Linux kernel style: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst


2. Via [https://review.coreboot.org/#/q/project:flashrom gerrit on coreboot.org], i.e. ''git push origin HEAD:refs/for/staging''
The notable exception is line length limit. Our guidelines are:
* 80-columns soft limit for most code and comments. This is to encourage simple design and concise naming.


3. Via pull request on [https://github.com/flashrom/flashrom/ flashrom's github mirror]
* 112-columns hard limit. Use this to reduce line breaks in cases where they harm grep-ability or overall readability, such as print statements and function signatures. Don't abuse this for long variable/function names or deep nesting.


Our guidelines borrow heavily from [http://www.coreboot.org/Development_Guidelines the coreboot development guidelines], and most of them apply to flashrom as well. The really important part is about the Signed-off-by procedure which is quoted [[#Sign-off Procedure|below]].
* Tables are the only exception to the hard limit and may be as long as needed for practical purposes.
 
== Sending a patch ==
 
'''Before submitting a patch for review, put your [[#Sign-off Procedure|Signed-off-by line]] in the commit message.'''
 
Currently there are two ways to submit patches:
 
1. '''Our strong preference, especially for large patches, is via [https://review.coreboot.org/#/q/project:flashrom Gerrit on review.coreboot.org]''', i.e. ''git push origin HEAD:refs/for/master''.
 
2. For small patches there is an option to send them via our [[Contact#Mailing_List|mailing list]]. When sending a patch via the mailing list, send it in-line instead of as an attachment so that reviewers can easily comment on specific parts of it. However, eventually the reviewer will need to push the patch to Gerrit anyway.
 
Our guidelines borrow heavily from coreboot [https://doc.coreboot.org/contributing/coding_style.html Coding style] and [https://doc.coreboot.org/contributing/gerrit_guidelines.html Gerrit guidelines], and most of them apply to flashrom as well. The really important part is about the [[#Sign-off Procedure]].


We try to '''reuse as much code as possible''' and create new files only if absolutely needed, so if you find a function somewhere in the tree which already does what you want (even if it is for a totally different chip), please use it. See also [[Random notes#Command_set_secrets|Command set secrets]].
We try to '''reuse as much code as possible''' and create new files only if absolutely needed, so if you find a function somewhere in the tree which already does what you want (even if it is for a totally different chip), please use it. See also [[Random notes#Command_set_secrets|Command set secrets]].
Line 57: Line 81:
The '''patch reviews''' may sound harsh, but please don't get discouraged. We try to merge simple patches after one or two iterations and complicated ones as soon as possible, but we have quite high standards regarding code quality.
The '''patch reviews''' may sound harsh, but please don't get discouraged. We try to merge simple patches after one or two iterations and complicated ones as soon as possible, but we have quite high standards regarding code quality.


If you introduce new features (not flash chips, but stuff like partial programming, support for new external programmers, voltage handling, etc) please '''discuss your plans''' on the [[Mailinglist|mailing list]] first. That way, we can avoid duplicated work and know about how flashrom internals need to be adjusted and you avoid frustration if there is some disagreement about the design.
If you introduce new features (not flash chips, but stuff like partial programming, support for new external programmers, voltage handling, etc) please '''discuss your plans''' on the [[Contact#Mailing_List|mailing list]] first. That way, we can avoid duplicated work and know about how flashrom internals need to be adjusted and you avoid frustration if there is some disagreement about the design.
 
For patches that modify convoluted tables like <tt>struct flashchip flashchips[]</tt> in flashchips.c it may make sense to increase the lines of '''context''' to include enough information directly in the patch for reviewers (for example to include the chip names when changing other parameters like .voltage). To do this with git use '''git format-patch -U5''' where 5 is an example for the number of lines of context you want.


For patches that modify convoluted tables like <tt>struct flashchip flashchips[]</tt> in flashchips.c it may make sense to increase the lines of '''context''' to include enough information directly in the patch for reviewers (for example to include the chip names when changing other parameters like .voltage). To do this with subversion use '''svn diff --diff-cmd diff -x -u5'''; or with git use '''git format-patch -U5''' where 5 is an example for the number of lines of context you want.
== GitHub ==


== Sign-off Procedure ==
The official Flashrom '''mirror''' on GitHub is: https://github.com/flashrom/flashrom
 
Importantly, '''GitHub repo is only a mirror''', so all changes must go through Gerrit on review.coreboot.org in order to be merged, even small patches such as adding support for a flash chip. For this reason, '''reviewers do not look at pull requests'''. Even if pull requests were automatically transferred to Gerrit, requirements such as the [[#Sign-off Procedure]] still present a problem.
 
The quickest and best way to get your patch reviewed and merged is by sending it to review.coreboot.org. Conveniently, you can use your GitHub, GitLab or Google account as an OAuth2 [https://review.coreboot.org/login login method]. Please see the [https://www.flashrom.org/Development_Guidelines#Set_up_your_Gerrit_account_on_review.coreboot.org instructions for setting up a Gerrit Account].
 
=== Set up your Gerrit account on review.coreboot.org ===
 
1. Go to https://review.coreboot.org/login and sign in using your GitHub credentials.
 
2. Edit your settings by clicking on the gear icon in the upper right corner.
 
3. Set your Gerrit username (this is different from your GitHub username).
 
4. Add an e-mail address so that Gerrit can send notifications to you about your patch.
 
5. Upload an SSH public key, or click the button to generate an HTTPS password
 
=== Push your patch ===
 
1. Install Change-Id hook: ''gitdir=$(git rev-parse --git-dir); scp -p -P 29418 <gerrit_username>@review.coreboot.org:hooks/commit-msg ${gitdir}/hooks/''
 
2. Add upstream as a remote:
 
* If using SSH: ''git remote add -f upstream ssh://'''<gerrit_username>'''@review.coreboot.org:29418/flashrom''
 
* If using HTTPS: ''git remote add -f upstream https://review.coreboot.org/flashrom''
 
3. Check out a new local branch that tracks upstream/master: ''git checkout -b <branch_name> upstream/master''
 
4. Cherry-pick to new local branch: ''git cherry-pick <your_commit_from_local_branch>''
 
5. Push to gerrit: ''git push upstream HEAD:refs/for/master%topic=my_wonderful_patch''.
 
* If using HTTPS you will be prompted for the username and password you set in the Gerrit UI.
 
* If successful, the Gerrit URL for your patch will be shown in the output.
 
== Commit message ==
 
Commit messages shall have the following format:
<component>: Short description (up to 72 characters)
This is a long description. Max width of each line in the description
is 72 characters. It is separated from the summary by a blank line. You
may skip the long description if the short description is sufficient,
for example "flashchips: Add FOO25Q128" to add FOO25Q128 chip support.
You may have multiple paragraphs in the long description, but please
do not write a novel here. For non-trivial changes you must explain
what your patch does, why, and how it was tested.
Finally, follow the [[#Sign-off Procedure]] to add your sign-off!
<span style="color:red">Signed-off-by: Your Name <your@domain></span>
 
 
=== Sign-off Procedure ===


We employ a similar sign-off procedure [http://web.archive.org/web/20070306195036/http://osdlab.org/newsroom/press_releases/2004/2004_05_24_dco.html as the Linux kernel developers] do.
We employ a similar sign-off procedure [http://web.archive.org/web/20070306195036/http://osdlab.org/newsroom/press_releases/2004/2004_05_24_dco.html as the Linux kernel developers] do.
Please add a note such as
Add a note such as
  Signed-off-by: Random J Developer <random@developer.example.org>
  Signed-off-by: Random J Developer <random@developer.example.org>
to your email/patch if you agree with the following Developer's Certificate of Origin 1.1.
to your email/patch if you agree with the Developer's Certificate of Origin 1.1 printed below. Read [https://lkml.org/lkml/2004/5/23/10 this post on the LKML] for rationale (spoiler: SCO).


<span style="color:red">You have to use your real name in the Signed-off-by line and in any copyright notices you add.</span> Patches without an associated real name cannot be committed!
<span style="color:red">You must use your real name in the Signed-off-by line and in any copyright notices you add.</span> Patches without an associated real name lack provenance and cannot be committed!


'''Developer's Certificate of Origin 1.1:'''
'''Developer's Certificate of Origin 1.1:'''
Line 92: Line 175:
= Reviews =
= Reviews =


Contributors without commit rights should receive at least a preliminary review within one week of submission by some committer.
All patches finally have to go through Gerrit. Though, if the author prefers, the actual reviewing process can also take place on the mailing list. GitHub pull requests might still be reviewed, but discouraged; see [[#GitHub]].
 
All contributions should receive at least a preliminary review within one week of submission by some flashrom developer (if that doesn't happen in time, please be patient).
At minimum this should include a broad indication of acceptance or rejection of...
At minimum this should include a broad indication of acceptance or rejection of...
* the idea/rationale/motivation,
* the idea/rationale/motivation,
Line 104: Line 189:
NB: New contributors may need more detailed advices and should be told about minor issues like formatting problems more precisely.
NB: New contributors may need more detailed advices and should be told about minor issues like formatting problems more precisely.
The result of a review should either be an accepted patch or a guideline how the existing code should be changed to be eventually accepted.
The result of a review should either be an accepted patch or a guideline how the existing code should be changed to be eventually accepted.
== Acked-by ==
* Trivial stuff like...
** compile fixes (that are tested on all or at least some affected and some ''unaffected'' platforms),
** marking boards/chips/programmers as tested, and
** typo and whitespace fixes etc.
:do not need an Acked-by line from someone else than the author.
* If the patch is something like a board enable, new chip(set) support etc. (i.e. no refactoring, no architectural impact), you can get an ack from someone who is not an experienced developer after he/she has tested the patch, or ack yourself if proof exists that it worked.
* However, for more complicated patches you should get a review from someone who knows the code well if possible.
** If no one replies to the patch submission on the mailing list within 2 weeks, you may assume consent by the community and ack yourself.
** If, however, one of the known community members comments on your submission and requests changes or further information/a rationale etc., you need to come to some consensus with that member about the points complained about (only).
*** If your answer (e.g. in form of a new iteration of the patch including the requested changes or a mail containing the requested information) does not get any replies for three weeks, you may ack yourself.
*** When consensus in all points have been reached, but no explicit ack has been sent, then the submission falls under the same timeout rule again; i.e. if no one replies to your last e-mail within 2 weeks, you may ack yourself.
* Contributions from people without commit rights should not be treated much differently.
: Maintainers should merge them if there is no objection from the community within the timeframes and according to the rule set described above.


== Adding/reviewing a new flash chip ==
== Adding/reviewing a new flash chip ==
Line 147: Line 216:
# <tt>.voltage</tt> defines the upper and lower bounds of the supply voltage of the chip. If there are multiple chip models with different allowed voltage ranges, the [http://en.wikipedia.org/wiki/Intersection_(set_theory) intersection] should be used and an appropriate comment added.
# <tt>.voltage</tt> defines the upper and lower bounds of the supply voltage of the chip. If there are multiple chip models with different allowed voltage ranges, the [http://en.wikipedia.org/wiki/Intersection_(set_theory) intersection] should be used and an appropriate comment added.
# The write [http://www.flashrom.org/pipermail/flashrom/2013-April/010817.html granularity] can be expressed by the <tt>.gran</tt> field. If you think you need something else than the default (<tt>write_gran_256bytes</tt>) then you should definitely ask one of the regular flashrom hackers first. Possible values can be found in <tt>flash.h</tt>.
# The write [http://www.flashrom.org/pipermail/flashrom/2013-April/010817.html granularity] can be expressed by the <tt>.gran</tt> field. If you think you need something else than the default (<tt>write_gran_256bytes</tt>) then you should definitely ask one of the regular flashrom hackers first. Possible values can be found in <tt>flash.h</tt>.
# <tt>.reg_bits</tt> stores information about what configuration bits the chip has and where they are found. For example, <tt>.cmp = {STATUS2, 6, RW}</tt> indicates that the chip has a complement bit (CMP) and it is the 6th bit of the 2nd status register. See <tt>struct reg_bit_info</tt> in flash.h for details on each of the structure's fields.
#* Note that some chips have configuration bits that function like TB/SEC/CMP but are called something else in the datasheet (e.g. BP3/BP4/...). These bits should be assigned to a field according their function and the datasheet name should be noted in a comment, for example:
#*:<tt>.sec    = {STATUS1, 6, RW}, /* Called BP4 in datasheet, acts like SEC */</tt>
# <tt>.decode_range</tt> points to a function that determines what protection range will be selected by particular configuration bit values. It is required for write-protect operations on the chip.


= Committing =
= Merging to branches =


Those with commit rights to the (currently subversion-based) source repository should follow the following rules.
Merging to branches is limited to the "flashrom developers" group on
Gerrit. This means every patch reviewed somewhere else (e.g. mailing
list or GitHub) must finally be pushed to Gerrit. The following rules
apply, some are already enforced by Gerrit:


* Reviews
* Every commit has to be reviewed and needs at least one +2 that was not given by the commit's author.
** Changes should generally be discussed and reviewed before committing, see [[Development_Guidelines#Reviews|above]]
* Except, if a commit is authored by more than one person, each author may +2 the other author's changes.
* The commit log
* Merging should not take place within less than 24 hours after the review started (i.e. the first message by a reviewer on Gerrit).
** Should start with a short sentence summing up the changes of the patch.
* Finally, before hitting ''Submit'', one is reponsible to check that all comments have been addressed, especially if there was a negative review (-1).
** Optionally this should be prefixed with a topic or file name if the changes are targeting one area only.
** End the sentence with a full stop followed by an empty line.
** The body of the commit log should be short, but descriptive: If anyone involved in flashrom reads your comment in a year, she/he shall still be able to understand what your commit is about, without analyzing the code (what was the motivation for the change? what are the differences to the previous behavior?).
** At the end it has to list Signed-off-by lines of all authors and at least one Acked-by line.
** Optionally one can add Tested-by lines to give credit to testers.
* Committing
** Who actually pushes/commits to the repository depends on who is actually allowed to:
*** If you ack something and the sender has commit rights, let him commit unless he asks you to commit. Ask the sender when there is no commit for a few days after the ack - sometimes things are forgotten.
*** If the sender does not have commit rights, note in the mail with the ack that you intend to commit the change to give him/her an opportunity to add final refinements or comments. Wait for at least 24 hours before you commit.
** After committing please reply to the mailing thread that contains the patch with at least the revision in the body to notify anyone involved of the commit and document it for references in the future and in patchwork or similar facilities. Also, mark the patch as accepted on patchwork if you can.
* Don't share your login(s)/password(s) (should be obvious, but sadly it is not for everyone :)

Revision as of 04:19, 21 March 2022

Branches

Historical

Till the release of flashrom 0.9.9 there was basically a single branch (trunk) where linear development happened. After 0.9.9 it was decided to switch to Git and a two branch model, a stable and a staging branch. This led to some confusion and as nobody who had access to the stable branch had the time to work on it, development continued about one year after the 0.9.9 release on a staging branch at coreboot.org. Despite its name, we strived to keep flashrom's quality and hoped that everything would be merged to stable once work continues there.

master branch

The historical staging branch was finally renamed to master. As usual there is no quality promise for the state of the code on the master branch. Even though we will try to keep the regression rate as low as possible, the main purpose of the branch is to merge new commits and make them available to a broader audience for testing.

Release branches (e.g. 1.0.x)

Branching for a new release can happen at any point in time when a commit (branch point) on master seems to be in good shape and was reasonably tested after previous invasive changes. Between the branch point and the release, every fix pushed for master for a problem that also persists on the release branch shall be backported. The same also applies after the release for the latest release branch and, optionally, for any earlier release branch that is still maintained for other reasons (e.g. part of a long term distribution).

Whenever a release branch has no further unmerged commits in queue and is not awaiting backported fixes, a release candidate (RC) can be tagged on that branch. This can also be the original branch point. The RC shall undergo extensive build tests and be publicly advertised as ready for testing. Not less than three days after the last RC that included code changes, a release can be tagged if no regressions showed up.

Release-branch names follow the pattern <major>.<minor>.x (e.g. 1.0.x). The first release of a branch is tagged v<major>.<minor>, without a point-release number (e.g. v1.0). Every following release from the same branch will have a point-release number starting with .1 (e.g. v1.0.1) and will only add backported fixes to the release.

Unless the branch point (i.e. last common commit of master and a release branch) is an RC, it should be tagged as p<major>.<minor> (e.g. p1.0), to keep track where we are on the master branch.

Patch submission

Coding style

Flashrom generally follows Linux kernel style: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst

The notable exception is line length limit. Our guidelines are:

  • 80-columns soft limit for most code and comments. This is to encourage simple design and concise naming.
  • 112-columns hard limit. Use this to reduce line breaks in cases where they harm grep-ability or overall readability, such as print statements and function signatures. Don't abuse this for long variable/function names or deep nesting.
  • Tables are the only exception to the hard limit and may be as long as needed for practical purposes.

Sending a patch

Before submitting a patch for review, put your Signed-off-by line in the commit message.

Currently there are two ways to submit patches:

1. Our strong preference, especially for large patches, is via Gerrit on review.coreboot.org, i.e. git push origin HEAD:refs/for/master.

2. For small patches there is an option to send them via our mailing list. When sending a patch via the mailing list, send it in-line instead of as an attachment so that reviewers can easily comment on specific parts of it. However, eventually the reviewer will need to push the patch to Gerrit anyway.

Our guidelines borrow heavily from coreboot Coding style and Gerrit guidelines, and most of them apply to flashrom as well. The really important part is about the #Sign-off Procedure.

We try to reuse as much code as possible and create new files only if absolutely needed, so if you find a function somewhere in the tree which already does what you want (even if it is for a totally different chip), please use it. See also Command set secrets.

The patch reviews may sound harsh, but please don't get discouraged. We try to merge simple patches after one or two iterations and complicated ones as soon as possible, but we have quite high standards regarding code quality.

If you introduce new features (not flash chips, but stuff like partial programming, support for new external programmers, voltage handling, etc) please discuss your plans on the mailing list first. That way, we can avoid duplicated work and know about how flashrom internals need to be adjusted and you avoid frustration if there is some disagreement about the design.

For patches that modify convoluted tables like struct flashchip flashchips[] in flashchips.c it may make sense to increase the lines of context to include enough information directly in the patch for reviewers (for example to include the chip names when changing other parameters like .voltage). To do this with git use git format-patch -U5 where 5 is an example for the number of lines of context you want.

GitHub

The official Flashrom mirror on GitHub is: https://github.com/flashrom/flashrom

Importantly, GitHub repo is only a mirror, so all changes must go through Gerrit on review.coreboot.org in order to be merged, even small patches such as adding support for a flash chip. For this reason, reviewers do not look at pull requests. Even if pull requests were automatically transferred to Gerrit, requirements such as the #Sign-off Procedure still present a problem.

The quickest and best way to get your patch reviewed and merged is by sending it to review.coreboot.org. Conveniently, you can use your GitHub, GitLab or Google account as an OAuth2 login method. Please see the instructions for setting up a Gerrit Account.

Set up your Gerrit account on review.coreboot.org

1. Go to https://review.coreboot.org/login and sign in using your GitHub credentials.

2. Edit your settings by clicking on the gear icon in the upper right corner.

3. Set your Gerrit username (this is different from your GitHub username).

4. Add an e-mail address so that Gerrit can send notifications to you about your patch.

5. Upload an SSH public key, or click the button to generate an HTTPS password

Push your patch

1. Install Change-Id hook: gitdir=$(git rev-parse --git-dir); scp -p -P 29418 <gerrit_username>@review.coreboot.org:hooks/commit-msg ${gitdir}/hooks/

2. Add upstream as a remote:

  • If using SSH: git remote add -f upstream ssh://<gerrit_username>@review.coreboot.org:29418/flashrom

3. Check out a new local branch that tracks upstream/master: git checkout -b <branch_name> upstream/master

4. Cherry-pick to new local branch: git cherry-pick <your_commit_from_local_branch>

5. Push to gerrit: git push upstream HEAD:refs/for/master%topic=my_wonderful_patch.

  • If using HTTPS you will be prompted for the username and password you set in the Gerrit UI.
  • If successful, the Gerrit URL for your patch will be shown in the output.

Commit message

Commit messages shall have the following format:

<component>: Short description (up to 72 characters)

This is a long description. Max width of each line in the description
is 72 characters. It is separated from the summary by a blank line. You
may skip the long description if the short description is sufficient,
for example "flashchips: Add FOO25Q128" to add FOO25Q128 chip support.

You may have multiple paragraphs in the long description, but please
do not write a novel here. For non-trivial changes you must explain
what your patch does, why, and how it was tested.

Finally, follow the #Sign-off Procedure to add your sign-off!

Signed-off-by: Your Name <your@domain>


Sign-off Procedure

We employ a similar sign-off procedure as the Linux kernel developers do. Add a note such as

Signed-off-by: Random J Developer <random@developer.example.org>

to your email/patch if you agree with the Developer's Certificate of Origin 1.1 printed below. Read this post on the LKML for rationale (spoiler: SCO).

You must use your real name in the Signed-off-by line and in any copyright notices you add. Patches without an associated real name lack provenance and cannot be committed!

Developer's Certificate of Origin 1.1:

By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or
(b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or
(c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it; and
(d) In the case of each of (a), (b), or (c), I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license indicated in the file.

Note: The Developer's Certificate of Origin 1.1 is licensed under the terms of the Creative Commons Attribution-ShareAlike 2.5 License.

Reviews

All patches finally have to go through Gerrit. Though, if the author prefers, the actual reviewing process can also take place on the mailing list. GitHub pull requests might still be reviewed, but discouraged; see #GitHub.

All contributions should receive at least a preliminary review within one week of submission by some flashrom developer (if that doesn't happen in time, please be patient). At minimum this should include a broad indication of acceptance or rejection of...

  • the idea/rationale/motivation,
  • the implementation

respectively.

In general, reviews should focus on the architectural changes and things that affect flashrom as a whole. This includes (but is by no means limited to) changes in APIs and types, safety, portability, extensibility, and maintainability. The purpose of reviews is not to create perfect patches, but to steer development in the right direction and produce consensus within the community. The goal of each patch should be to improve the state of the project - it does not need to fix all problems of the respective field perfectly. NB: New contributors may need more detailed advices and should be told about minor issues like formatting problems more precisely. The result of a review should either be an accepted patch or a guideline how the existing code should be changed to be eventually accepted.

Adding/reviewing a new flash chip

  1. Get the datasheet of the exact type of chip.
  2. Open flashchips.c and flashchips.h.
  3. First, find the best* IDs in the datasheet (*FIXME: this needs to be explained together with the probing somewhere else in detail) and check if the ID exists in flashchips.h already
    • If it does but is named after a different chip,
      then add a comment regarding the twin and continue by comparing the definition in flashchips.c with the datasheet of the twin/new chip as if you would add it but leave out the next step (see below). First you should change the .name to reflect the additional chip model (see other chips of naming examples). If you find significant* differences in the chips behavior you have found a so called evil twin (*judging the significance of a difference is quite hard and requires some understanding of flashrom behavior, examples of significant differences are: different sizes of blocks or different opcodes for operations). In that case copy the entry and continue to change that (don't forget to undo the previous changes before).
    • If it does and the name matches too,
      the chip is either already added or only the ID was added and you should use that define.
    • If it does not,
      then you should add it conforming to the standards/comments in the file.
    Usually the chip IDs follow a simple scheme: They are all uppercase; first the manufacturer name (like for the manufacturer IDs on top of each paragraph in flashchips.h) followed by an underscore and then the chipname. The latter should in general equal the .name, with dots (and other disallowed characters) replaced by underscores. Shared chip IDs typically use the macro name that happened to be added first to flashrom (which is also probably the first one manufactured) and which usually matches the other chips of that series in flashchips.h.
  4. If possible copy an existing, similar entry in the giant array in flashchips.c or start a new one at the right position (according to the comment on top of the array)
  5. Add .vendor, .name, IDs selected as explained above and .total_size.
  6. .page_size is really hard. Please read this long explanation, or ignore it for now and set it to 256.
  7. We encode various features of flash chips in a bitmask named .feature_bits. The various possibilities can be found in flash.h.
  8. .tested is used to indicate if the code was tested to work with real hardware, its possible values are defined in flash.h. Without any tests it should be set to TEST_UNTESTED.
  9. .probe indicates which function is called to fetch IDs from the chip and to compare them with the ones in .manufacture_id and .model_id. This requires some knowledge or source reading. For most SPI flash chips probe_spi_rdid is the right one if the datasheets mentions 0x9f as an identification/probing opcode.
  10. .probe_timing is only used for non-SPI chips. It indicates the delay after "enter/exit ID mode" commands in microseconds (see flash.h for special values).
  11. .block_erasers stores an array of pairs of erase functions (.block_erase) with their respective layout (.eraseblocks).
    1. .block_erase is similar to the probing function. You should at least check that the opcode named in the function name is matching the respective opcode in the datasheet.
    2. Two forms of .eraseblocks can be distinguished: symmetric and asymmetric layouts. Symmetric means that all blocks that can be erased by an opcode are sized equal. In that case a single range can define the whole layout (e.g. {4 * 1024, 256} means 256 blocks of 4 kB each). Asymmetric layouts on the other hand contain differently sized blocks, ordered by their base addresses (e.g. {{8 * 1024, 1}, {4 * 1024, 2}, {16 * 1024, 7}} describes a layout that starts with a single 8 kB block, followed by two 4 kB blocks and 7 16 kB blocks at the end).
  12. .printlock is a misnomer to some extent. It is misused not only to print (write) protected address ranges of the chip, but also to pretty print the values of the status register(s) - especially true for SPI chips. There are a lot of existing functions for that already and you should reuse one if possible. Comparing the description of the status register in the datasheet of an already supported chip with that of your chip can help to determine if you can reuse a printlock function.
  13. .unlock is called before flashrom wants to modify the chip's contents to disable possible write protections. It is tightly related to the .printlock function as it tries to change some of the bits displayed by .printlock.
  14. .write and .read are function pointers with the obvious meaning. Currently flashrom does only support a single function each. The one that is best supported by existing programmers should be used for now, but others should be noted in a comment if available.
  15. .voltage defines the upper and lower bounds of the supply voltage of the chip. If there are multiple chip models with different allowed voltage ranges, the intersection should be used and an appropriate comment added.
  16. The write granularity can be expressed by the .gran field. If you think you need something else than the default (write_gran_256bytes) then you should definitely ask one of the regular flashrom hackers first. Possible values can be found in flash.h.
  17. .reg_bits stores information about what configuration bits the chip has and where they are found. For example, .cmp = {STATUS2, 6, RW} indicates that the chip has a complement bit (CMP) and it is the 6th bit of the 2nd status register. See struct reg_bit_info in flash.h for details on each of the structure's fields.
    • Note that some chips have configuration bits that function like TB/SEC/CMP but are called something else in the datasheet (e.g. BP3/BP4/...). These bits should be assigned to a field according their function and the datasheet name should be noted in a comment, for example:
      .sec = {STATUS1, 6, RW}, /* Called BP4 in datasheet, acts like SEC */
  18. .decode_range points to a function that determines what protection range will be selected by particular configuration bit values. It is required for write-protect operations on the chip.

Merging to branches

Merging to branches is limited to the "flashrom developers" group on Gerrit. This means every patch reviewed somewhere else (e.g. mailing list or GitHub) must finally be pushed to Gerrit. The following rules apply, some are already enforced by Gerrit:

  • Every commit has to be reviewed and needs at least one +2 that was not given by the commit's author.
  • Except, if a commit is authored by more than one person, each author may +2 the other author's changes.
  • Merging should not take place within less than 24 hours after the review started (i.e. the first message by a reviewer on Gerrit).
  • Finally, before hitting Submit, one is reponsible to check that all comments have been addressed, especially if there was a negative review (-1).