[Sound-open-firmware] [alsa-devel] [PATCH v3 04/14] ASoC: SOF: Add support for IPC IO between DSP and Host

[Keyon Jie]

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 at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>