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

[Mark Brown <broonie@kernel.org>]

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