[consolidated answers from Liam and me]

...

+/* 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)?

we'll look into it.

...

• 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()?

Messages can be as big as the mailbox, which is hardware dependent. It could be from a few bytes to a larger e.g. 4k page or more, and indeed we need to keep the lock.

...

• /* 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?

we will rename this as snd_sof_dsp_is_ipc_ready() to avoid any confusion with the DSP state. We only care about IPC registers/doorbells at this point, not the fact that the DSP is in its idle loop.

...

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

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

didn't get this point, sorry.

...

• /* 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)?

so far neither the HDaudio nor the BYT stuff return an error on send_msg, so we'll need to thing about what happens should this happen with other hardware, or demote the status to void.

We'll look into this.

...

• 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?

Good catch, err should default to 0, thanks for pointing this out.

...

+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.

ok, we will add something to stop the IPC, good suggestion indeed.