[alsa-devel] [PATCH 08/12] ASoC: SOF: ipc: fix a race, leading to IPC timeouts

[Takashi Iwai]

On Sat, 18 May 2019 22:27:00 +0200,
Pierre-Louis Bossart wrote:
> 
> From: Guennadi Liakhovetski <guennadi.liakhovetski at linux.intel.com>
> 
> 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 at linux.intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> ---
>  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