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.