Re: [PATCH v3 04/14] ASoC: SOF: Add support for IPC IO between DSP and Host

Mark Brown
2019-01-09T20:37:20Z

[-- Attachment #1.1: Type: text/plain, Size: 2220 bytes --]

On Tue, Dec 11, 2018 at 03:23:08PM -0600, Pierre-Louis Bossart wrote:

> +/* wait for IPC message reply */
> +static int tx_wait_done(struct snd_sof_ipc *ipc, struct snd_sof_ipc_msg *msg,
> + void *reply_data)
> +{

This has exactly one caller, why not inline it (or make both tx and rx
separate functions)?

> + spin_lock_irqsave(&sdev->ipc_lock, flags);
> +
> + /* get an empty message */
> + msg = msg_get_empty(ipc);
> + if (!msg) {
> + spin_unlock_irqrestore(&sdev->ipc_lock, flags);
> + return -EBUSY;
> + }
> +
> + msg->header = header;
> + msg->msg_size = msg_bytes;
> + msg->reply_size = reply_bytes;
> + msg->ipc_complete = false;
> +
> + /* attach any data */
> + if (msg_bytes)
> + memcpy(msg->msg_data, msg_data, msg_bytes);

How big do these messages get? Do we need to hold the lock while we
memcpy()?

> + /* schedule the message if not busy */
> + if (snd_sof_dsp_is_ready(sdev))
> + schedule_work(&ipc->tx_kwork);

If the DSP is idle is there a reason this has to happen in another
thread?

> +
> + spin_unlock_irqrestore(&sdev->ipc_lock, flags);

The thread is also going to take an irq spinlock after all.

> + /* send first message in TX list */
> + msg = list_first_entry(&ipc->tx_list, struct snd_sof_ipc_msg, list);
> + list_move(&msg->list, &ipc->reply_list);
> + snd_sof_dsp_send_msg(sdev, msg);

Should the move happen if the send fails (I see it can return an error
code, though we ignore it)?

> + int err = -EINVAL;

> + case SOF_IPC_FW_READY:
> + /* check for FW boot completion */
> + if (!sdev->boot_complete) {
> + if (sdev->ops->fw_ready)
> + err = sdev->ops->fw_ready(sdev, cmd);
> + if (err < 0) {
> + dev_err(sdev->dev, "error: DSP firmware boot timeout %d\n",
> + err);

err defaults to -EINVAL here which doesn't seem like the right thing if
fw_ready() is really optional. Perhaps just validate the ops on
registration and call this unconditionally?

> +void snd_sof_ipc_free(struct snd_sof_dev *sdev)
> +{
> + cancel_work_sync(&sdev->ipc->tx_kwork);
> + cancel_work_sync(&sdev->ipc->rx_kwork);
> +}
> +EXPORT_SYMBOL(snd_sof_ipc_free);

It might be better to have something that stops any new messages being
processed as well, to prevent races on removal.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]