On 2018/12/13 下午3:48, Takashi Iwai wrote:
On Thu, 13 Dec 2018 06:24:18 +0100,
Keyon Jie 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)
+{
+ 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).
Well, if we can use spin_lock() variant, mutex is often a better
alternative.
The most important point is to know which particular calls may be
called in spinlocked / interrupt context beforehand and which are not.
This reflects to the API design.
Thanks Takashi, we will refine this part and add comments for each
function about these context preconditions.
Thanks,
~Keyon
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
