module_adapter: return error if get_conf is not implemented

[johnylin76]

At present, if get_configuration is not implemented by internal module, module_adapter will return 0 directly on config read commands. Under the circumstances, the host will suppose the reading is succeeded, but in fact the ata in IPC buffer is unwanted.

This commit changes the return code to non-zero for such cases, which prevents the host getting the unwanted config data.

[LaurentiuM1234]

Change makes sense. Need to watch out for modules (such as cadence.c) which don't have get_configuration() and set_configuration() both implemented as this seems to cause errors such as:

[ 2834.159757] sof-audio-of-imx8 596e8000.dsp: ipc tx error for 0x50040000 (msg/reply size: 160/160): -5
[ 2834.169077] sof-audio-of-imx8 596e8000.dsp: kcontrol 1 read failed for widget CODEC_ADAPTER1.0

(extract is from a cplay run w/ cadence.c module and this patch)

[kv2019i]

Thanks, this is a good addition. The one warn-to-err log message change might potentially be good before merge, but no showstoppers, so leaving my +1 here.

[johnylin76]

I realized this leads to a special case that by calling this -EIO will be returned for CMD_SET_DATA if set_configuration is not implemented. It's beyond my purpose that returning -EIO for CMD_GET_DATA only.

Hi @LaurentiuM1234 , I just saw your comment saying that cadence.c doesn't implement both get_configuration and set_configuration, is that correct? If the non-implemented set_configuration is intentional, I think this line should be fixed to keep CMD_SET_DATA behavior as is.

[LaurentiuM1234]

I realized this leads to a special case that by calling this -EIO will be returned for CMD_SET_DATA if set_configuration is not implemented. It's beyond my purpose that returning -EIO for CMD_GET_DATA only.

ACK, so now get_configuration() becomes mandatory and set_configuration() remains optional. Might be out of scope for this, but did you consider adding all the interface->set/get_configuration() logic into some wrapper functions?

i.e: something like:

static inline int module_set_configuration(struct processing_module *mod, uint32_t config_id
                                                                   enum module_cfg_fragment_position pos, uint32_t data_offset_size,
                                                                   const uint8_t *fragment, size_t fragment_size, uint8_t *response,
                                                                   size_t response_size)
{
    const struct module_interface *const ops = mod->dev->drv->adapter_ops;

    if (ops->set_configuration) {
       // call set_configuration
    }

    // optional operation
    return 0;
}

static inline int module_get_configuration(struct processing_module *mod, uint32_t config_id
                                                                   enum module_cfg_fragment_position pos, uint32_t data_offset_size,
                                                                   const uint8_t *fragment, size_t fragment_size, uint8_t *response,
                                                                   size_t response_size)
{
    const struct module_interface *const ops = mod->dev->drv->adapter_ops;

    // this is not optional
    if (!ops->get_configuration)
        return -EIO;

    // some more code here
}

Since this is also used by the IPC4 stuff, having a centralized view showing which operation is optional and which isn't might be helpful?

Hi @LaurentiuM1234 , I just saw your comment saying that cadence.c doesn't implement both get_configuration and set_configuration, is that correct? If the non-implemented set_configuration is intentional, I think this line should be fixed to keep CMD_SET_DATA behavior as is.

The cadence module doesn't implement the get_configuration() stuff so if we make it mandatory the issue will still persist. Can't really tell you why it's not implemented but we can probably get around the issue by just implementing a dummy get_configuration() if we really don't need a proper one. Anyways, IMO not a blocker for this.

[johnylin76]

Thanks. Some contexts below for clearer statement:

The commit doesn't mean to make get_configuration mandatory (in a literal manner). Instead, the intention is to fix the use case of SOF build which integrates the instance without get_configuration in module_adapter.

Consider a host running the IPC command to read blob data from the module_adapter bytes control, if module_adapter doesn't return error while the instance doesn't implement get_configuration, the host will consider the blob data is read back successfully, leading to problems on wrong (or illegal) data blob access. As a consequence, it should be better to return error than zero, provided that the host has no clue whether get_configuration is implemented by instance or not.

[johnylin76]

Change makes sense. Need to watch out for modules (such as cadence.c) which don't have get_configuration() and set_configuration() both implemented as this seems to cause errors such as:

[ 2834.159757] sof-audio-of-imx8 596e8000.dsp: ipc tx error for 0x50040000 (msg/reply size: 160/160): -5
[ 2834.169077] sof-audio-of-imx8 596e8000.dsp: kcontrol 1 read failed for widget CODEC_ADAPTER1.0

(extract is from a cplay run w/ cadence.c module and this patch)

Thanks for catching this. I suppose the cause is as my last comment #9045 (comment)
I just updated the patch. PTAL, thanks.

[kv2019i]

@wszypelt Here's one more to check. I checked test job 13872687 and seems test terminates midway and does seem to be linked to this PR.