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