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:
- 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