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

From: Takashi Iwai <tiwai@suse.de>
To: Keyon Jie <yang.jie@linux.intel.com>
Cc: Daniel Baluta <daniel.baluta@gmail.com>,
	andriy.shevchenko@intel.com, alsa-devel@alsa-project.org,
	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	liam.r.girdwood@linux.intel.com, vkoul@kernel.org,
	broonie@kernel.org, sound-open-firmware@alsa-project.org,
	"rander.wang" <rander.wang@linux.intel.com>,
	Alan Cox <alan@linux.intel.com>
Subject: Re: [PATCH v3 04/14] ASoC: SOF: Add support for IPC IO between DSP and Host
Date: Thu, 13 Dec 2018 08:48:05 +0100	[thread overview]
Message-ID: <s5htvjhuatm.wl-tiwai@suse.de> (raw)
In-Reply-To: <2ce474f7-ac64-13dd-a5e0-365f76559a3d@linux.intel.com>

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