On 2018/12/12 下午11:19, Pierre-Louis Bossart wrote:
diff --git a/include/sound/sof/control.h b/include/sound/sof/control.h
+/* generic channel mapped value data */ +struct sof_ipc_ctrl_value_chan { + uint32_t channel; /**< channel map - enum sof_ipc_chmap */ + uint32_t value;
Any reason to avoid s32 and u32? If this is supposed to be shared with user-space (or user-space may use this as a reference of data struct), we should consider placing in uapi directory, too.
it's intentional
The includes shared with userspace are in include/uapi/sound/sof.
All the files in include/sound/sof, and this one in particular, are more for host-dsp IPC.
In those two cases, uapi and IPC files, we don't use s32 and u32. we could move this directory under include/uapi/sound/sof-ipc if you prefer?
+/* wait for IPC message reply */ +static int tx_wait_done(struct snd_sof_ipc *ipc, struct snd_sof_ipc_msg *msg, + void *reply_data) +{ + struct snd_sof_dev *sdev = ipc->sdev; + struct sof_ipc_cmd_hdr *hdr = (struct sof_ipc_cmd_hdr *)msg->msg_data; + unsigned long flags; + int ret;
+ /* wait for DSP IPC completion */ + ret = wait_event_timeout(msg->waitq, msg->ipc_complete, + msecs_to_jiffies(IPC_TIMEOUT_MSECS));
+ spin_lock_irqsave(&sdev->ipc_lock, flags);
Since this must be a sleepable context, you can safely use spin_lock_irq() here.
+/* send IPC message from host to DSP */ +int sof_ipc_tx_message(struct snd_sof_ipc *ipc, u32 header, + void *msg_data, size_t msg_bytes, void *reply_data, + size_t reply_bytes) +{ + struct snd_sof_dev *sdev = ipc->sdev; + struct snd_sof_ipc_msg *msg; + unsigned long flags;
+ spin_lock_irqsave(&sdev->ipc_lock, flags);
Ditto. This one calls tx_wait_done() later.
It's better to define more strictly which one can be called from the spinlocked context and which not.
This one is for Keyon and team. I've asked that question multiple times and was told the irqsave was needed. Keyon, can you please chime in?
we basically have 3 parts where using this ipc_lock:
1. sof_ipc_tx_message(), get lock, update tx_list, schedule tx_work, put lock, then call tx_wait_done(); 2. ipc_tx_next_msg() (tx_work itself), get lock, send message, put lock; 2.5. ack/reply ipc interrupt arrived, mark ipc_complete in handler. 3. tx_wait_done(), wait until ipc_complete(or timeout), then get lock, handle the ack/reply, and put lock at last.
|1 -[--]-|-> 3------(done)-[--]-| | ^ V | |2-[--]-| | |2.5--|
those []s means holding locks.
So, all those 3 functions can't be called from the spin-locked context as they need to hold the lock inside them.
I admit that we are too conservative that using spin_lock_irqsave/restore() here, as Takashi mentioned, here all 3 functions are actually run in normal thread context, I think we can even run them with interrupt enabled(using spin_lock/unlock() directly).
Thanks, ~Keyon
+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);
Not specific to this function but a general question: why not EXPORT_SYMBOL_GPL() in general in the whole SOF codes?
We use a dual license (copied below)
// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) // // This file is provided under a dual BSD/GPLv2 license. When using or // redistributing this file, you may do so under either license.
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel