contributing: Add section on process for SOF FW ABI changes #244
Conversation
juimonen
left a comment
juimonen
left a comment
There was a problem hiding this comment.
LGTM. I can't really imagine in my head, how everything will go in practice, but these are good rules to start with. Somehow the full path from topology to firmware needs to be addressed with ABI changes.
|
Minor update to diagram, there was a bug in the state diagram, now fixed. I also added a note that for MAJOR version bump, kernel implementation is mandatory. Updated generated picture at: |
|
|
||
| FWImplementation --> ABIClassification: Submit PR for classification | ||
|
|
||
| ABIClassification -> DriverImplementation: Start working on driver changes |
There was a problem hiding this comment.
Why are there two paths marked "Start working on driver changes", leading to "DriverImplementation?" Can you start the same work twice? :-) It also indicates a route from fast path to driver implementation, which shouldn't be there, since the fast path means no driver changes? Also in one note the fast path is described as "no impact to driver" but in another it's "minor changes" which are two different criteria?
There was a problem hiding this comment.
@lyakh Good question. I This was on purpose. I wanted to highlight that if you go via the "normal path", we can sooner figure out whether a driver change is needed to merge the ABI change. Currenly we have many cases where this realization only happens at the ABI board meeting, which is very late. If you take the "fast path", you have the risk that you will be sent back to the drawing board. Makes sense?
There was a problem hiding this comment.
In what context is a change to the driver no required? If the ABI is being updated won't it need it in both places? Or is this referring to a backwards compatible change. (e.g. adding to an enum)
There was a problem hiding this comment.
@cujomalainey wrote:
In what context is a change to the driver no required? If the ABI is being updated won't it
need it in both places? Or is this referring to a backwards compatible change. (e.g. adding
Indeed these would be MINOR changes, which are backwards compatible. If we bump MAJOR version, then driver change is mandatory.
What has been muddying the picture are MINOR ABI changes, but where part of the FW change takes new features into use in the SOF topologies used in CI. This was now taken out from the ABI process description as it's not really ABI management anymore, but a question of how the CI(s) is/are managed. In many cases driver change is still optional, but e.g. if you now add a new component type (pre-UUID system), kernel must be modified to understand the new component. With the UUID based component creation in place, we will have less and less cases like this.
|
|
||
| ABIClassification -> DriverImplementation: Start working on driver changes | ||
|
|
||
| note right of DriverImplementation: If 1) FW change is MAJOR or\n2) MINOR but modifies\nupstream topologies, then\nkernel change must\nbe merged at the same time |
There was a problem hiding this comment.
what does "merged at the same time" mean and how does it help? You can click the two "merge" buttons within the same millisecond but it won't prevent users or distributions from mixing different versions?
"Upstream topologies" are topologies for platforms / configurations available in the kernel mainline (as opposed to only supported in sof-dev) or only those, whose names are used in one of machine drivers? E.g. is only sof-apl-pcm512x.tplg an upstream topology or all other sof-apl-*-pcm512x.tplg too?
There was a problem hiding this comment.
@lyakh I think we need to keep this a bit vague as we don't have infra to do cross-repo merges. This is not strictly related to the ABI, but rather our CI. Typically kernel side would have to be merged first, or otherwise CI will get broken (new topology files deployed to CI that the kernel does not know how to parse). As it's related to our CI, this consideration applies to all topologies we test in our CI.
|
v2-to-3 changes:
Generated process diagram looks like this: |
|
@lgirdwood Maybe we add a "RFC" column to the ABI change board to gather all PRs current in RFC or RFCApproved stages. |
|
I like workflow, I am just worried with the speed at which team that process would be followed simply because not everyone is aware. I wonder if something like https://github.com/palantir/policy-bot might come in handy. |
|
@kv2019i I would also show where the FW is merged alongside the driver merge. |
|
@cujomalainey wrote:
Thanks for the pointers! Palantir seems very interesting indeed. FYI for @marc-hb and @xiulipan . Based on review of ABI changes during last half year, the biggest slowdowns in this time period were due to unclarity on the criteria. So let's see how much we can smoothen the process by just clarifying the process (and i.e. having a diagram for the process). |
|
@kv2019i I think we should make it really obvious that the FW + driver PR owner(s) are wholly responsible for "driving" the process. i.e. it's their responsibility to bring in the correct people at each stage and get approvals from kernel/FW folks. i.e. I would want to see kernel approval on the FW PR and vice versa, this will save maintainers having to check with each other. |
|
Maybe part of the PR process would be beneficial to explicitly name the people who are handling ABI review (i.e. add it to the template) |
Yes, especially with both driver and FW maintainers (of which there are many) involved in the process, it doesn't scale if the maintainers need to chase after reviews. I now added text to this effect to the section outlining the process. Let me know if you have ideas how to emphasize this even more. |
Ack. I now added separate sections that point to the list of TSC members and list of driver maintainers. Interesting point though. I found from TSC minutes that ABI changes should be approved by TSC, but hmm, is that really so (@lrgirdwo )? I now put wording such that "approval from a TSC member is required". |
The process diagram is getting somewhat unwieldy already. How about this (picture generated from the commit I pushed today):
|
|
@kv2019i one more thing, I've now created an ABI milestones to work alongside the classifier. This makes the ABI classifier output visible on the PR and shows any ABI change history. i.e. I've had to move a lot from 3.16 -> 3.17 today. You should see this on kernel PRs now too. |
cujomalainey
left a comment
cujomalainey
left a comment
There was a problem hiding this comment.
Looks good, two minor questions, even if they are no then I still approve
- do we want to add in the UML that PRs for firmware and driver must land at the same time? Just to keep ABIs relatively in sync across trees.
- Given the ABI minor tracker is already quite full, how would we go about flushing it? Would we remove items from the tracker once they are part of a release?
I think so, we dont want too much time between kernel and FW merge of PRs.
I leave them in the tracker for history. I manually have to move things around but we could probably improve this my having a column for merged items. |
|
@lgirdwood wrote:
Hmm. How about we simply enforce a rule that if driver PR is required (not mandatory in the new process for MINOR update), then driver PR must be merged first. Driver side will have capability to support multiple MAJOR versions. And if we some CI-reason to update the kernel for a MINOR update, it makes sense to merge the driver first as well. If ok, I can update this to the diagram? |
Sounds reasonable to me |
|
@lgirdwood @cujomalainey Is this PR ready to be merged? Thanks. |
|
I think we are waiting for @lgirdwood to respond to the possible change in #244 (comment) |
|
@cujomalainey wrote:
I'll make an update later today with the proposed additional wording, so @lgirdwood can then go ahead and approve if it looks good. |
Document revised process for ABI changes including a new step for Request-for-Comments phase. This is intended to ensure early engagement of interface changes across driver and firmware teams, to avoid wasting effort when concerns are raised at the very end of the process. Add cross-links between kernel driver and firmware sections. Also add link to ABI versioning definition in kernel/abi.h and pointers to TSC and driver maintainer team and who are the members. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
|
Added explicit sequence for ordering of driver and FW change merges. Updated diagram now:
|
|
@cujomalainey @kv2019i @deb-intel all good by me. Please merge. |
|
Thanks all! @deb-intel @lgirdwood who does the publish step (I see old version still online)? |
|
@kv2019i I am currently making a few grammatical edits to some of the recently merged PRs. Once I merge those PRs, I will publish the SOF site. I am aiming for the end of today (U.S). Do you need to see it published sooner? If so, I can try to accommodate that. |
|
@deb-intel wrote:
Thanks a lot, no rush with my change! I just wanted to check whether I need to take more actions myself to get the change published. |
6 participants
This is documenting the proposed new, refined ABI process as discussed here:
https://github.com/orgs/thesofproject/teams/sof-developers/discussions/35
Document revised process for ABI changes including a new step
for Request-for-Comments phase. This is intended to ensure
early engagement of interface changes across driver and firmware
teams, to avoid wasting effort when concerns are raised at
the very end of the process.
Add cross-links between kernel driver and firmware sections.
Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com