Re: [PATCH v3 04/14] ASoC: SOF: Add support for IPC IO between DSP and Host

["rander.wang" <rander.wang@linux.intel.com>]

在 12/13/2018 4:06 PM, Keyon Jie 写道:
>
>
> On 2018/12/13 下午1:24, Keyon Jie wrote:
>>
>>
>> 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).
>
> Sorry, as #2 is a schedule work where should not sleep, so it is 
> better to use spin_lock/unlock_irq() here.
>
> Thanks,
> ~Keyon
>
I just discussed with keyon, got a conclusion that spin_lock_irq should 
be applied here.

Rander

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