[xiulipan]

Move the added GDB flag into the reserved filde. This will allow the fw
work with a long term linux kernel.

Fixed: #695
Signed-off-by: Pan Xiuli xiuli.pan@linux.intel.com

[RanderWang]

I don't like this change. why ? the structure in kernel and FW should be same.
So we need to add gdb_enabled in kernel, not hack the size of this structure

[RanderWang]

And gdb is required by google, so we need to add it in kernel

[mengdonglin]

@lgirdwood @plbossart The boot failure is not caused this PR but by other ABI updates. If this PR can be merged at first, QA will file new bugs to track opens coming with the ABI updates.

[xiulipan]

@RanderWang
Agreed.
But now we did not have any info about how to enable GDB with linux kernel.

@lgirdwood @plbossart
The final goal is to keep the UAPI header same, but as we are always develop new features we need some method to make linux and FW can work backwards-compatible.

In this case, if we have some reserved field that are not used. We should first use them to keep the total size of the structure same to let some old code work with each other.This can be a change to ABI_MINOR_VERSION. But if we reorder some IPC structure or change size of an IPC structure, no way to keep backwards-compatible, in this case I think an ABI_MAJOR_VERSION is needed.

Linux kernel should have a check for this kind of ABI version diff and show warning or error due to our ABI backwards-compatible policy.

@mrajwa
Chould you share how do you use this uint32_t gdb_enabled with linux kernl?

[mrajwa]

@xiulipan, I don't understand what this PR is trying to fix. Added '.gdb_enabled' flag into the IPC struct is aimed to provide information for the tester that gdb is enabled (so potentially they can make use of it).
In the very first comment you said "Move the added GDB flag into the reserved filde" but from the changes I only see you decreased the size of reserved array.
So, could you please tell, what problem are you trying to solve? It should all boil down to simple update of ABI, that is all.

[xiulipan]

@mrajwa
What I have done is to keep the size of the struct sof_ipc_fw_ready to make backwards-compatible.

If we have some reserved unused field that marked as /* reserved for future use */
Why not take one uint32_t from it and change it to uint32_t gdb_enabled

The whole propose to do that is because sizeof struct sof_ipc_fw_ready is very important because we have struct sof_ipc_window following it. If you only change the size of the struct sof_ipc_fw_ready in FW and do not make any change to Linux kernel. The linux driver with SOF FW will be broken!

So did you test this code with linux kernel and FW?

And could you share more info about how could we use with this uint32_t gdb_enabled from linux thus we can have the official patches to make alignment to linux kernel uapi and sof uapi.

[mrajwa]

@xiulipan, I see your intention now. It may fix local issues you are facing, however, in a long term use you will need to update ABI on kernel side too. Why? Because the whole point of adding gdb_enabled flag was to inform whatever peer is talking with FW that GDB is or is not supported and whether thay can or can not use it. Do you understand now why the whole ABI must be updated?

[mrajwa]

As pointed out in above comments - ABI on kernel side must be updated accordingly to PR #657

[jocelyn-li]

@mrajwa can you please file a github issue on kernel side?

[mrajwa]

@jocelyn-li, yes its opened here thesofproject/linux#414

[mengdonglin]

@mrajwa I think we're not aginst the new flag and kernel will support the new feature to enable GDB session, we only have concern on the way to add the new flag.

ABI update need be done in a backward compatible way and the reserved integers are defined for this purpose. You can use 1 of them as the db_enabled flag as this PR does. Thus the ABI header file ipc.h in kernel side doesn't need to update immediately and all existing functions are no affected. And when kernel update the ipc.h and add support for the new IPC, the new feature will be enabled in kernel side smoothly.

[plbossart]

I don't get what this patch is trying to fix. We don't have gdb_enabled on the kernel side, so this looks like a hack or a product-specific fix.
This also feels like the process for ABI changes is totally broken, we just had a change and the next day there is a report of a missing field. Seriously?

[mrajwa]

@mengdonglin, in original PR I signaled that corresponding ABI change of IPC is needed. I thought it will be updated on merge time. Anyway, since we all agree now that ABI needs to be updated on kernel side, should I update it myself or somebody else will take care of it?

[lgirdwood]

See #713 - better way to do this and exports to all platforms. Will merge as soon as CI completes.