IPC: ABI misaligned with FW

The IPC ABI has been recently changed in FW to provide information for kernel/driver about the GDB support - thesofproject/sof@acce742

Now the ABI on kernel side shall be updated accordingly.

NAK. You don't change the ABI on the firmware side, and then file an issue with the kernel. That's a broken process and a sure recipe for breaking things.
If you have an ABI change, then you file it both on the kernel and firmware sides with clearly documented PRs, and we make sure it's applied at the same time.
I am not going to do this work for you.

@plbossart

@plbossart, I did not say you should do this on my behalf. I assigned you to this issue since I am not sure what is the proper process for such a change and I was seeking advice. In my original PR i signaled that corresponding update on kernel/driver side is necessary.

So, should I open PR in kernel repository with updated ABI of IPC, or there is some sort of code owner for this module which should update this?

@lgirdwood

I am not sure what is the proper process for such a change

yeah, what's the process? should we put down something in the project readme file? ->_-> @lgirdwood

should I open PR in kernel repository with updated ABI of IPC, or there is some sort of code owner for this module which should update this?

I think so. If you can , you are much welcomed to shoot a kernel patch yourself. code owner is mostly for code review purpose.

#433 fixes the issue.

But I agree that we need to follow a process next time. This will unblock PV for now.

@ranj063

@ranj063
#433 Looks good

Here is another talk here I have mentioned in thesofproject/sof#701 about ABI change guideline

@plbossart @lgirdwood

What is the basic guideline of change ABI?
let takes a change like GDB: Added IPC trigger for GDB debug. sof#657 as example here.
I did not care about the fix about this issue now, I backwards-compatible. only want to have a discussion about how we use the ABI version to control the IPC Strucutre change to makeThis will be very important after we got upstream. People should be able to use some version FW to work some some version Kernel with ABI here.

If a original structure is here that we have 4 reserved uint32_t and ABI version 3.0.0

        /* reserved for future use */
        uint32_t reserved[4];

If we want to add a new uint32_t flag gdb_enabled into this structure.

Option A

	uint32_t gdb_enabled;
 	/* reserved for future use */
	uint32_t reserved[3];

Option B

	uint32_t gdb_enabled;
 	/* reserved for future use */
	uint32_t reserved[4];

If a Option A or B happened, should ABI version change to 3.0.1, 3.1.0 or 4.0.0 ?
Or even we have an Option C and how to change the ABI verison?

How to make FW and Kernel backwards-compatible?
If Linux kernel have a ABI version 3.0.0
Should it accpet FW with ABI version 3.0.* or 3.*.* or even 4.*.*
How to make sure broken ABI changes applied to Linux and FW at same time?
This is important that we make changes at same time, or we may face some regression all the time for daily test.
A topic ABI change branch? or any other method to keep this work?
How to make sure ABI changes is enough tested?
Normal per-merge PR test will not take effect with ABI change, so we may need human test involve into this kind of PRs.

Any ideas?
Please join to discuss.
@singalsu @RanderWang @keyonjie @bardliao @libinyang @zhuyingjiang @mengdonglin @jocelyn-li @keqiaozhang @tlauda @jajanusz

@ranj063

@ranj063

How to make FW and Kernel backwards-compatible?

How about a simple rule: accept everything but the change to major version. And it is expected to be backward compatible?

How to make sure broken ABI changes applied to Linux and FW at same time

It should not be required to make them at the same time since adding new extensions/fields to the structures which specify the header with the size correctly is a backward compatible change. Making changes to existing fields, removing existing fields/whole structures from the protocol would be another story but it is not the case here. It is also not required to keep any reserved for future use fields in the structures with headers.

The driver may safely read and process the part that it understands and simply skip any new extensions that it does not know yet moving to the next serialized object by the size read from the header. The size check against expected/understood structure size is still required for any implementation to make sure that object may be deserialized safely.

All the communication/synchronization complexity would be reduced with this approach. Please note that some protocols go further and use TLV series as a layer to make the parties scan and process the payload in a more flexible way but I believe it is not needed for the structures already designed with headers, ones we are talking about.

@mmaka1

@mmaka1 The addition of fields is not going to be a symmetrical task on the firmware and driver sides. The firmware folks can add stuff in backwards compatible ways and test them with custom scripts and pretend drivers. However if the Linux driver team adds fields then they will be ignored by the firmware and thus we can't really test the driver...

Also I really think we should discourage people from adding fields just because they are backwards compatible. that's a sure recipe for feature creep and uncontrolled changes. All respectable companies have some sort of "API/ABI council" where proposals are reviews. It's even more important in an open-source setting so that all contributors can chime in.

@mmaka1

@mmaka1

How to make FW and Kernel backwards-compatible?

How about a simple rule: accept everything but the change to major version. And it is expected to be backward compatible?

Then we should first come up our first simple rule about which how IPC structure will change the ABI version.

But here is an agreement now:

ABI minor or version change should be backwards-compatible. Otherwise we should have more discuss about.

@plbossart so here is a simple but first agreement now.

@xiulipan

@xiulipan As I said, it's not enough. I do want to have information even on minor changes so that there are plans to add the support at the kernel level ahead of time. It's too easy to add a field at the firmware level without anyone knowing then ask for Linux support without advance notice. I am old enough to smell fishy processes. The ABI is a contract between two sides, and doing things unilaterally isn't quite right.

However if the Linux driver team adds fields then they will be ignored by the firmware and thus we can't really test the driver...

True. On the other hand, the driver should prepare itself for this case, too, i.e. a new driver can work with the old firmware by not mandating new features that old firmware cannot do. Being ignored is fine, driver should elegantly deal with it. Otherwise, it's a nightmare for distributors and end users.

However if the Linux driver team adds fields then they will be ignored by the firmware and thus we can't really test the driver...

True. On the other hand, the driver should prepare itself for this case, too, i.e. a new driver can work with the old firmware by not mandating new features that old firmware cannot do. Being ignored is fine, driver should elegantly deal with it. Otherwise, it's a nightmare for distributors and end users.

That's a different case. When there is an ABI change we want to
a) test that the kernel changes work with older firmware, obviously without any new functionality added. In other words is a 'do no harm' check but doesn't prove anything related to the fieds.
b) test that the kernel changes work with new firmware and expose a new functionality.

Case a) should be handled with the backwards-compatible stuff. Case b) is where we need collaboration between teams to actually test the new capability or feature.

The ABI is a contract between two sides, and doing things unilaterally isn't quite right.

It's probably a "N vs 1" contract among N+1 sides, when N is more than Linux Kernel...

@mmaka1

@mmaka1 It is important to revered some fields for future use. Without the revered fields, the header size will be changed if we add any field. Note that user may not update their kernel and firmware at the same time.

@plbossart

@plbossart
Agreed, all change need to be notified and discussed.

I'm not sure whether we really do need to change ABI so frequently, especially after SOF is upstreamed. I think we should have a plan that when we will upgrade the ABI with which features.

@plbossart

Please separate two different things:

ABI change approval process
Technical solution to make the implementation of changes much easier in terms of backward compatibility and reduced complexity of the implementation of changes.

Please note I simply talked about (2) only. I fully agree we need a good (1) and everybody should obey.
I found many responses related to (1), rather than to my proposal. Perhaps this is a good idea to have a separate discussion on:

(a) keeping reserved fields everywhere to keep the entire payload cast-able in the driver vs. (b) deserializing the objects using information provided in the headers and skip unknown extensions, not handled yet.

@plbossart

Also I really think we should discourage people from adding fields just because they are backwards compatible.

I think we should do this by implementing a good (1), but it should not stop us from implementing the (2) in a way that makes transitions much less complex. Protocol is going to run out of reserved fields sooner or later. I think they do not help long term.

However if the Linux driver team adds fields then they will be ignored by the firmware and thus we can't really test the driver...

Untested changes should not be committed, I agree. The FW is in a bit better position as it can be tested with the slim driver and committed, still not breaking backward compatibility with existing driver if (2) is improved.

@bardliao

It is important to revered some fields for future use.

I explained in the first post and above why they should not be required.

Note that user may not update their kernel and firmware at the same time.

This is exactly what I assumed in the first post.

@libinyang

I'm not sure whether we really do need to change ABI so frequently. I think we should have a plan that when we will upgrade the ABI with which features.

This relates to (1) and I fully agree. The existing feature plan could be flagged with 'ABI changes required' for each item. They we have a schedule ready of ABI changes ready.

Perhaps this is a good idea to have a separate discussion on:

(a) keeping reserved fields everywhere to keep the entire payload cast-able in the driver vs. (b) deserializing the objects using information provided in the headers and skip unknown extensions, not handled yet.

I am confused now. Isn't what you described precisely what Liam added few days ago? Or are you suggesting that what Liam provided is not enough and another set of changes are needed to deal with ABI changes in the future?

@plbossart

@plbossart
The reserved fields, recently added to the structures work fine as a short/mid term solution.
But I was a bit concerned about this requirement:

NAK. You don't change the ABI on the firmware side, and then file an issue with the kernel. That's a broken process and a sure recipe for breaking things.
If you have an ABI change, then you file it both on the kernel and firmware sides with clearly documented PRs, and we make sure it's applied at the same time.
I am not going to do this work for you.

Since another set of changes will be needed sooner or later once the payload grows beyond the number of reserved fields allocated now, I suggested to consider another approach in the driver to avoid another round(s) of adding rsv'ed fields in the way proposed above.
I believe that we could keep the process of protocol development free from sync'ed stops as long as the new implementation does not change the syntax nor semantics of the existing parts and does not require to understand the new ones in order to keep the existing functionality working, which is imo the case here, so just used it as an opportunity to discuss a potential alternative technical solution that could be applied in future.

On 12/14/18 11:28 AM, Marcin Maka wrote: @plbossart <https://github.com/plbossart> The reserved fields, recently added to the structures work fine as a short/mid term solution. But I was a bit concerned about this requirement: NAK. You don't change the ABI on the firmware side, and then file an issue with the kernel. *That's a broken process* and a sure recipe for breaking things. If you have an ABI change, then you file it both on the kernel and firmware sides with clearly documented PRs, and we make sure *it's applied at the same time*. I am not going to do this work for you. Since another set of changes will be needed sooner or later once the payload grows beyond the number of reserved fields allocated now, I suggested to consider another approach in the driver to avoid another round(s) of adding rsv'ed fields in the way proposed above. I believe that we could keep the process of protocol development free from sync'ed stops as long as the new implementation does not change the syntax nor semantics of the existing parts and does not require to understand the new ones in order to keep the existing functionality working, which is imo the case here, so just used it as an opportunity to discuss a potential alternative technical solution that could be applied in future.

The current solution has been up for reviews for weeks, I am somewhat surprised that there's even a mention that we need to do something different in the future...

…

@mmaka1

ABI change approval process

Technical solution to make the implementation of changes much easier in terms of backward compatibility and reduced complexity of the implementation of changes.

Great summary, @mmaka1. Would you and @lgirdwood work out the details of the "technical solutions" that both driver and fw follows, and post it here?

Although process details for ABI update will continue, closing this issue since the ABI misalignment is fixed by thesofproject/sof#713

Navigation Menu

Repository navigation

IPC: ABI misaligned with FW #414

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

IPC: ABI misaligned with FW #414

Description

Activity

plbossart commented on Dec 12, 2018

mrajwa commented on Dec 12, 2018

wenqingfu commented on Dec 12, 2018

ranj063 commented on Dec 13, 2018

xiulipan commented on Dec 13, 2018

mmaka1 commented on Dec 13, 2018

plbossart commented on Dec 13, 2018

xiulipan commented on Dec 13, 2018

plbossart commented on Dec 13, 2018

wenqingfu commented on Dec 13, 2018

plbossart commented on Dec 13, 2018

wenqingfu commented on Dec 13, 2018

bardliao commented on Dec 14, 2018

xiulipan commented on Dec 14, 2018

libinyang commented on Dec 14, 2018

mmaka1 commented on Dec 14, 2018

plbossart commented on Dec 14, 2018

mmaka1 commented on Dec 14, 2018

plbossart commented on Dec 14, 2018

wenqingfu commented on Dec 14, 2018

mengdonglin commented on Dec 29, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions