On Fri, Nov 08, 2019 at 12:45:39AM +0100, Nico Huber wrote:
On 07.11.19 12:05, Nico Huber wrote:
- Few people seem to take the might of Git into account. We have a tool that can significantly increase the efficiency of development and can help to preempt bugs. But many people would accept a process that breaks Git benefits. Especially with the growth of the coreboot community, I believe this gains importance.
Patrick already commented on two competing approaches to put the changes for a new chip into commits. I'd like to look closer at both and explain how I work with Git and why it makes it much much harder (at least for me) to work with copy-first additions.
What I call the copy-first approach
- First commit copies existing code (with name changes only).
n. Some commits change parts of the copied code that were identified to differ between the old and new chips. (n+1. Not a practice yet, but it was suggested to have a last commit to document things that aren't covered by the n changing commits; e.g. what didn't change but was tested)
The alternative step-by-step approach
Each commit adds a chunk of code in a state that is supposed to be compatible with the new chip. Generally, smaller, cohesive commits are easier to review, of course. But there is no strict rule what the steps should be.
The major difference between the two approaches lies in what we know about the code chunks that stay the same for old and new chips. With the copy-first approach, we basically know nothing beside what we find in the history of an older chip. But we don't know if the code was left unchanged intentionally or if differences between the chips were overlooked. Also, nobody had an opportunity to review if it applies to the new chip. The n+1 commit might mitigate the documentation issue, but it's too late for review.
Sometimes people tell me that the copy-first approach makes it more visible what the author thinks changed between the old and new chips. I don't see yet, how we benefit from that (please help me).
With the step-by-step approach, we can easily document intentions in the commit messages, everything can be reviewed, and it's less likely to overlook changes between old and new chips.
That much about the initial addition of a new chip. But how does it look like later, when the first (non-reference) boards using the chip are added, or during maintenance? No matter what approach was applied, the code won't be perfect. And whenever somebody hits a bug or an incompatibility, or maybe just wonders why the code exists (e.g. when a datasheet doesn't mention anything related), people will have to investigate the reasons behind the code.
Personally, I make use a lot of the `git blame` command. It shows for each line of a file which commit touched that line last. And very often, the commit message provides some reasoning about the code. The step-by- step approach gives us the opportunity to document reasons, assumptions made and what was tested in the commit messages. With the copy-first approach, OTOH, this information is scattered. I'm not saying the reasons or test coverage would be different, just much harder to find, if documented at all. So for code that didn't change between old and new chips, in the best case, the copy-first approach with a documenting n+1 commit still increases costs maybe 5x to find the information. In the worst case much more.
I'd like to share my experience here with porting an intel/denverton board to coreboot.
During the first phase while I was discovering both the coreboot codebase and the board/chip I was often browsing through the different intel generation, comparing code that looked alike but with sometime small differences, or sometime totally different implementation for implementing similar features (I recall some ACPI code for instance).
So I can see why Nico thinks this approach cost more.
The first implementation was done based on a non public port by Intel (as the chip was not public anyway this is ... understandable ...) by the time the chip was released I had more knowledge and the denverton chip clearly had internals very similar to the Apollolake chip.
And Apollolake is basically the initial version for the common code in intel soc codebase, so when I contributed changes I pushed to reuse the common code and extend it where there where difference between denverton and apollolake.
I think that this approach while not being usable for all chip might be better than the plain copy-first as most code remain shared (and can be enabled progressively). And by remaining shared it means fix or improvement can benefit all the chip using the common code. There is obviously the risk of breaking old board while modifying the common code. Which probably mean we need more testing !
That was my though and small experience on the subject.
Julien VdG
Patrick asked to be aware of costs. Here is my view on this: The step- by-step approach is more expensive for the initial addition. But these are one-time costs. The copy-first approach is cheaper but more error- prone due to the lack of a thorough review. It does not only create further costs due to more bugs to fix, but also makes it harder to reason about the code, so bug-fixing costs increase further. Assuming the copy-first approach costs X and another X per year of maintenance, and we'd want to maintain a chip 3 years (IIRC, Intel sometimes sells chips for 5~7 years), we could spend 4*X instead on the step-by-step approach without losing anything.
Nico _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org