On Sat, 18 May 2019 22:27:00 +0200,
Pierre-Louis Bossart wrote:
>
> From: Guennadi Liakhovetski <guennadi.liakhovetski@xxxxxxxxxxxxxxx>
>
> Currently on all supported platforms the IPC IRQ thread first signals
> the sender when an IPC response is received from the DSP, then
> unmasks the IPC interrupt. Those actions are performed without
> holding any locks, so the thread can be interrupted between them. IPC
> timeouts have been observed in such scenarios: if the sender is woken
> up and it proceeds with sending the next message without unmasking
> the IPC interrupt, it can miss the next response. This patch takes a
> spin-lock to prevent the IRQ thread from being preempted at that
> point. It also makes sure, that the next IPC transmission by the host
> cannot take place before the IRQ thread has finished updating all the
> required IPC registers.
>
> Fixes: 53e0c72d98b ("ASoC: SOF: Add support for IPC IO between DSP and Host")
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@xxxxxxxxxxxxxxx>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> ---
> sound/soc/sof/intel/bdw.c | 11 ++++++-----
> sound/soc/sof/intel/byt.c | 12 +++++++-----
> sound/soc/sof/intel/cnl.c | 6 ++++++
> sound/soc/sof/intel/hda-ipc.c | 19 ++++++++++++++++---
> sound/soc/sof/ipc.c | 13 -------------
> 5 files changed, 35 insertions(+), 26 deletions(-)
>
> diff --git a/sound/soc/sof/intel/bdw.c b/sound/soc/sof/intel/bdw.c
> index 065cb868bdfa..9dfdc02192b7 100644
> --- a/sound/soc/sof/intel/bdw.c
> +++ b/sound/soc/sof/intel/bdw.c
> @@ -278,11 +278,15 @@ static irqreturn_t bdw_irq_thread(int irq, void *context)
> /* reply message from DSP */
> if (ipcx & SHIM_IPCX_DONE &&
> !(imrx & SHIM_IMRX_DONE)) {
> + unsigned long flags;
> +
> /* Mask Done interrupt before return */
> snd_sof_dsp_update_bits_unlocked(sdev, BDW_DSP_BAR,
> SHIM_IMRX, SHIM_IMRX_DONE,
> SHIM_IMRX_DONE);
> + spin_lock_irqsave(&sdev->ipc_lock, flags);
Here is an threaded irq handler, so the irqflag is superfluous.
You can use spin_lock_irq() and spin_unlock_irq().
> diff --git a/sound/soc/sof/intel/byt.c b/sound/soc/sof/intel/byt.c
> index 7bf9143d3106..5a11a104110b 100644
> --- a/sound/soc/sof/intel/byt.c
> +++ b/sound/soc/sof/intel/byt.c
> @@ -324,11 +324,16 @@ static irqreturn_t byt_irq_thread(int irq, void *context)
> /* reply message from DSP */
> if (ipcx & SHIM_BYT_IPCX_DONE &&
> !(imrx & SHIM_IMRX_DONE)) {
> + unsigned long flags;
> +
> /* Mask Done interrupt before first */
> snd_sof_dsp_update_bits64_unlocked(sdev, BYT_DSP_BAR,
> SHIM_IMRX,
> SHIM_IMRX_DONE,
> SHIM_IMRX_DONE);
BTW, is this usage of _unlocked() version safe? The previous one also
contained that, and I wonder why _unlocked variant must be used here.
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
