IPC4 error handling #5276

lyakh · 2022-01-27T11:27:34Z

switch IPC4 internal error codes to POSIX, also fix several apparent bugs

This is primarily needed to store IPC4 error codes to be returned in IPC replies. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

IPC4-specific error codes are only relevant for the IPC ABI, internally SOF is using POSIX error codes throughout its code base. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lgirdwood

Client API looks simple and will allow compatibility. Have you tried with Zephyr ?
@mwasko @marcinszkudlinski any comments ?

lgirdwood · 2022-01-27T14:33:15Z

src/include/sof/ipc/common.h

 	((ipc)->private)

+struct ipc_core_ctx {
+	int error;


this may not always be an error, but its more of a reply with context. maybe call it ipc4_reply.

lgirdwood · 2022-01-27T14:33:28Z

src/arch/xtensa/ipc.c

+{
+	struct core_context *ctx = (struct core_context *)cpu_read_threadptr();
+
+	return &ctx->ipc;


Why do we need for xtos ?

It is currently possible to build IPC4 with XTOS, right? And I think that configuration is or was also used for the initial development / testing? So I thought I shouldn't break that functionality

Lets use Zephyr for this work

@lgirdwood well, sure, I wouldn't mind that, but what exactly does that mean? Does that mean that it should only be possible to build SOF with IPC4 with Zephyr? So we should disable building IPC4 with XTOS? If we don't do that and keep the possibility to build SOF with IPC4, then how would error handling work there?

@lyakh IPC4 with XTOS is not needed.

@lgirdwood I just learned from @ujfalusi that work is ongoing for IPC4 with XTOS

@lyakh, to clarify: testing is done with both xtos and zephyr, but I personally build SOF with XTOS for my local development needs. The parts that I'm working with are way above anything xtos/zephyr and makes no difference for the end result.

@ujfalusi ok, got it, but IIUC it might happen that IPC4 will not even build with XTOS beginning from some point. In fact maybe we should add a Kconfig dependency for that.

ranj063 · 2022-01-28T06:54:57Z

src/ipc/ipc4/helper.c

 	return icd->cd;
 }
-
-int ipc4_add_comp_dev(struct comp_dev *dev)


why is this removed? doesnt seem related to the change in this patch or I am not able to connect immediately

That function isn't called anywhere outside the file where it's defined, so I made it static

src/include/sof/ipc/common.h

mwasko · 2022-01-31T13:58:11Z

@lyakh can you explain what we gain in changing internal error codes into POSIX? I currently only see increased complexity and memory consumption.

The POSIX is generic but we can make a great use of audio FW specific error codes (aligned with IPC error codes) that with proper granularity can help us in issue debugging and identifying potential root cause of the problem using only single error code.

lgirdwood · 2022-01-31T14:25:37Z

@mwasko sorry, this is not about changing IPC4 codes to posix, but about preserving the IPC4 reply within a POSIX environment so it can be sent correctly to the host drivers for every IPC. The PR naming probably does not help.

mwasko · 2022-01-31T14:44:33Z

@mwasko sorry, this is not about changing IPC4 codes to posix, but about preserving the IPC4 reply within a POSIX environment so it can be sent correctly to the host drivers for every IPC. The PR naming probably does not help.

I understand the idea, but this PR is a great example how cumbersome can be aligning with POSIX environment.

Since we are moving to Zephyr and we are no longer going to maintain generic kernel code (where POSIX makes sense), but instead of that SOF is going to be Audio application layer, then maybe we should consider to switch to application specific error codes within SOF audio domain.

lyakh · 2022-02-01T08:09:26Z

@mwasko I agree in principle, that we don't have to use POSIX error codes or any other APIs. But what I do think we have to do is use consistent APIs throughout our code. Yes, we have to interface to other systems: to Zephyr, to the Linux kernel, etc. There we have to define interfaces well and - if needed - convert between them. That's what we do e.g. with IPC3 triggers - we have IPC3 trigger commands, that we exchange with the user, in this case with the Linux kernel. And internally SOF defines its own trigger codes, and it cleanly converts between the two sets at the interface.
The same has to be done with return codes. Yes, IPC4 defines its set of error codes and SOF has to use them to communicate with IPC4 counterparts. That's it. We don't need to modify parts of SOF to use those codes for any other purposes e.g. for returning them from functions. The whole of SOF uses POSIX error codes so that's what we should continue to use consistently. Anything else we only use to interface with respective other parties.
And in fact you can already see multiple examples of confusion, fixed by this PR. It's easy enough to find multiple cases where error codes are currently confused. Error codes from generic SOF functions aren't evaluated and converted to IPC4 codes but are instead propagated as is. Just for a single example look at the current version of set_pipeline_state() in ipc4/handler.c. In most places it returns IPC4 error codes but in one place it also returns the return code from ipc_process_on_core() directly, which is a generic SOF function and returns POSIX error codes, 0 or 1. That's just one example of the confusion that such API mixing is causing.

mwasko · 2022-02-01T11:24:18Z

@mwasko I agree in principle, that we don't have to use POSIX error codes or any other APIs. But what I do think we have to do is use consistent APIs throughout our code. Yes, we have to interface to other systems: to Zephyr, to the Linux kernel, etc. There we have to define interfaces well and - if needed - convert between them. That's what we do e.g. with IPC3 triggers - we have IPC3 trigger commands, that we exchange with the user, in this case with the Linux kernel. And internally SOF defines its own trigger codes, and it cleanly converts between the two sets at the interface.
The same has to be done with return codes. Yes, IPC4 defines its set of error codes and SOF has to use them to communicate with IPC4 counterparts. That's it. We don't need to modify parts of SOF to use those codes for any other purposes e.g. for returning them from functions. The whole of SOF uses POSIX error codes so that's what we should continue to use consistently.

I am okay with the error status conversion at IPC process level but to do that we need something better then POSIX error codes. The only reason you had to add context for ipc error is because POSIX is too generic and we are unable to translate it into IPC status code. Additionally the specific application error codes can significantly speed up the debug and issue root causing.

Anything else we only use to interface with respective other parties. And in fact you can already see multiple examples of confusion, fixed by this PR. It's easy enough to find multiple cases where error codes are currently confused. Error codes from generic SOF functions aren't evaluated and converted to IPC4 codes but are instead propagated as is. Just for a single example look at the current version of set_pipeline_state() in ipc4/handler.c. In most places it returns IPC4 error codes but in one place it also returns the return code from ipc_process_on_core() directly, which is a generic SOF function and returns POSIX error codes, 0 or 1. That's just one example of the confusion that such API mixing is causing.

I agree that IPC4 specific status as a return of generic functions like set_pipeline_state is not a way to go, but I would expect a meaningful set_pipeline_state error code that can be consumed both by IPC3 and IPC4 handlers (or any other caller).

My recommendation is to define of SOF application error code enum and apply it incrementally with new code implementation/refactors, starting with IPC handlers.

sys-pt1s · 2022-04-20T06:36:31Z

Can one of the admins verify this patch?

lgirdwood · 2022-04-20T18:27:49Z

@lyakh I think we can probably close now ? Can always reopen if things get difficult.

lyakh · 2022-04-21T07:45:41Z

@lyakh I think we can probably close now ? Can always reopen if things get difficult.

@lgirdwood This PR is fixing real bugs and as a result of the discussion herein it was agreed to try to fix those bugs differently. However, those bugs still haven't been fixed. We can close this PR and I can add a high priority bug report instead if you prefer.

lgirdwood · 2022-04-22T10:20:30Z

Lets close now - we can always reopen if circumstances change.

lyakh added 2 commits January 27, 2022 12:22

ipc: add per-core context

d5d926f

This is primarily needed to store IPC4 error codes to be returned in IPC replies. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

ipc4: use standard POSIX error codes for function calls

6f254fb

IPC4-specific error codes are only relevant for the IPC ABI, internally SOF is using POSIX error codes throughout its code base. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh requested review from bardliao, dbaluta, lbetlej, lgirdwood, marcinszkudlinski, mmaka1 and plbossart as code owners January 27, 2022 11:27

lgirdwood reviewed Jan 27, 2022

View reviewed changes

ranj063 reviewed Jan 28, 2022

View reviewed changes

mwasko reviewed Jan 28, 2022

View reviewed changes

src/include/sof/ipc/common.h Show resolved Hide resolved

lgirdwood closed this Apr 22, 2022

lyakh mentioned this pull request Apr 26, 2022

[BUG][IPC4] wrong error handling in IPC4, potentially leading to missed failures #5737

Closed

lyakh deleted the ipc4-err branch April 11, 2024 13:10

IPC4 error handling #5276

IPC4 error handling #5276

Uh oh!

Conversation

lyakh commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mwasko commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Jan 31, 2022

Uh oh!

mwasko commented Jan 31, 2022

Uh oh!

lyakh commented Feb 1, 2022

Uh oh!

mwasko commented Feb 1, 2022

Uh oh!

sys-pt1s commented Apr 20, 2022

Uh oh!

lgirdwood commented Apr 20, 2022

Uh oh!

lyakh commented Apr 21, 2022

Uh oh!

lgirdwood commented Apr 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lyakh commented Jan 27, 2022 •

edited

Loading

ujfalusi Apr 21, 2022 •

edited

Loading

mwasko commented Jan 31, 2022 •

edited

Loading