thesofproject / sof

Sound Open Firmware
Other
563 stars 319 forks source link

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

Open lyakh opened 2 years ago

lyakh commented 2 years ago

Describe the bug Files under src/ipc/ipc4/ mostly use error codes from enum ipc4_status which includes 0 as IPC4_SUCCESS and positive integer values as errors, but some code in that directory also uses negative POSIX return codes to indicate errors. Sometimes those codes are mixed. Sometimes checks like if (ret < 0) are performed where IPC4 errors are returned, so errors will be missed. An attempt has been made before in #5276 to fix those issues but it failed.

Code examples src/ipc/ipc4/helper.c:

static int ipc4_create_pipeline(struct ipc *ipc, uint32_t pipeline_id, uint32_t priority,
                uint32_t memory_size)
{
    ...
    if (ipc_pipe) {
        ...
        return IPC4_INVALID_RESOURCE_ID; // returning an IPC4_* error
    }
}

int ipc4_add_comp_dev(struct comp_dev *dev)
{
    ...
    if (!icd) {
        ...
        return IPC4_OUT_OF_MEMORY; // returning an IPC4_* error
    }
}

int ipc4_create_chain_dma(struct ipc *ipc, struct ipc4_chain_dma *cdma)
{
    ret = ipc4_create_pipeline(ipc, pipeline_id, 0, cdma->data.r.fifo_size);
    if (ret < 0) { // *BUG*: ipc4_create_pipeline() returns IPC4_* errors
    ...
    ret = ipc4_add_comp_dev(host);
    if (ret < 0)
        return IPC4_FAILURE;
    ...
}

static int ipc_pipeline_module_free(uint32_t pipeline_id)
{
    ...
    ret = ipc_comp_free(ipc, icd->id);
    if (ret)
        return ret; // *BUG*: ipc_comp_free() returns negative POSIX codes
    ...
}

int ipc_pipeline_free(struct ipc *ipc, uint32_t comp_id)
{
    ...
    if (!ipc_pipe)
        return -ENODEV;
    ...
    ret = pipeline_free(ipc_pipe->pipeline);
    if (ret < 0) {
        ...
        return IPC4_INVALID_RESOURCE_STATE; // *BUG* mixed error codes
    }
    ...
}
lgirdwood commented 2 years ago

@RanderWang @marcinszkudlinski @tmleman any comment or PR ?

RanderWang commented 2 years ago

I will check and give fix

lyakh commented 2 months ago

A further bad example of mixing up POSIX and IPC4 error codes is https://github.com/thesofproject/sof/blob/main/src/audio/base_fw.c It must be fixed.