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.