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:
signature.asc
Description: PGP signature
_______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
