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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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