pipeline: rollback pipeline state upon trigger error

[kv2019i]

If pipeline trigger fails, the pipeline state may be left in intermediate state like COMP_TRIGGER_PRE_START.

This is a problem in case a new trigger is sent to the same pipeline. The pipeline state may indicate the pipeline task is running, while in practise this is not the case. This can lead to IPC timeout as the trigger is incorrectly sent to delayed processing in the pipeline task (which is not running).

One case is the echo reference capture pipeline. This pipeline will return -ENODATA in case the linked playback pipeline is not running. Host may send a TRIG_STOP for the pipeline, and this will lead to IPC timeout:

[ 100212.287685] ( 20.468750) c0 pipe 9.47 ..../pipeline-schedule.c:64 ERROR pipeline_task_cmd(): failed to trigger components: -61
[ 100233.121017] ( 20.833332) c0 ll-schedule ./schedule/ll_schedule.c:142 INFO task complete 0xbe1d00c0 pipe-task
[ 100251.766850] ( 18.645832) c0 ll-schedule ./schedule/ll_schedule.c:145 INFO num_tasks 2 total_num_tasks 2
[ 163209.316431] ( 62957.550781) c0 ipc src/ipc/ipc3/handler.c:1576 INFO ipc: new cmd 0x60050000
[ 163237.441430] ( 28.124998) c0 pipe 9.47 ....../pipeline-stream.c:261 INFO pipe trigger cmd 0

Firmware will be stuck here as the command is never handled.

Fix the issue by rolling back pipeline state in case trigger error is hit.

Link: thesofproject/linux#4083
Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com

[pblaszko]

SetPipelineState IPC calls all modules and buffers in pipeline. When it finish with error, much more internal stuff may be left in invalid state, not only pipeline state. Every IPC fail should be treated in system as a fatal error and driver should not try to continue any operations. Hence I think we should rather detect such invalid state (like PRE_START, PRE_RELEASE) when driver tries to send next IPC's after fail and again respond with error code, than artificially fix pipeline state in error handler.

[kv2019i]

@pblaszko wrote:

invalid state, not only pipeline state. Every IPC fail should be treated in system as a fatal error and driver should not try to
continue any operations. Hence I think we should rather detect such invalid state (like PRE_START, PRE_RELEASE) when driver > tries to send next IPC's after fail and again respond with error code, than artificially fix pipeline state in error handler.

The -ENODATA error is not a fatal system error. It's a specified behaviour in IPC3 to communicate to host that a pipeline is dependent on another PCM.

So with current code, the FW will not fail gracefully if host sends a new IPC to the same pipelien. It should return a proper error (which happens with this PR), but currently FW gets stucks and we hit a IPC timeout.

[ranj063]

no need for the extra newline

[ranj063]

why is it PAUSED in all cases of failure @kv2019i ? What if the state was previously PREPARE before the trigger was run?

[kv2019i]

@ranj063 If the state is not paused, pipeline_trigger() will assume pipeline task is running and send the IPC to be handled in the task. If start fails, there's no task, and IPC will never be responded to, leading to IPC timeout. But yup, maybe we need a another method to discover that the task is not running.

[lgirdwood]

@ranj063 If the state is not paused, pipeline_trigger() will assume pipeline task is running and send the IPC to be handled in the task. If start fails, there's no task, and IPC will never be responded to, leading to IPC timeout. But yup, maybe we need a another method to discover that the task is not running.

Reporting non running task would have to be at the IPC level, as this sounds quite IPC specific.

[pblaszko]

The -ENODATA error is not a fatal system error. It's a specified behaviour in IPC3 to communicate to host that a pipeline is dependent on another PCM.

So with current code, the FW will not fail gracefully if host sends a new IPC to the same pipelien. It should return a proper error (which happens with this PR), but currently FW gets stucks and we hit a IPC timeout.

I still think proper way to fix graceful fail is to add checks on pipeline state machine which will catch invalid state on next IPC (and return error instead of entering timeout path).
To revert pipeline state when operation is not successful, we should walk back on all pipeline components and revert their state too (release all resources that were already allocated until fail etc.). But I don't think we want to implement such fail recovery.

[kv2019i]

@pblaszko wrote:

I still think proper way to fix graceful fail is to add checks on pipeline state machine which will catch invalid state on next IPC
(and return error instead of entering timeout path). To revert pipeline state when operation is not successful, we should walk
back on all pipeline components and revert their state too (release all resources that were already allocated until fail etc.). But I

Fair enough. It's just that I couldn't figure out how to signal this "invalid state" with other means in current code. But let me take another look. Let's see how much work it is to decouple the assumption that pipeline task is running if the pipeline state is active.

[lgirdwood]

LGTM, but I assume we need subsequent reporting PR ?

[kv2019i]

I'll move as draft to check whether there's another way to signal "pipeline-task-not-running" state in this case.

[gkbldcig]

Can one of the admins verify this patch?

[lgirdwood]

@kv2019i still needed ?

[kv2019i]

Let's close this, the original problem was solved on kernel side.