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

[Pierre-Louis Bossart]

>>> 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().
> 
> Oh, sure, thanks for catching this! That was a blind moving of the
> spin-lock from lower-level functions. I'll send an updated version to
> Pierre, unless you want to apply it immediately, in which case I can
> send it to you now.

I can send a v2 with Guennadi's update and Takashi's Reviewed-by tag.

> 
>>> 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.
> 
> Some of those uses seem rather fragile to me too, but it looks like they
> *should* be safe in normal cases. There seem to be a potential problem
> on Broadwell, where an ISR is first configured, which uses *_unlocked()
> to access the SHIM_IMRX register, and then bdw_set_dsp_D0() is called,
> which also touches that register. So, it's relying on the fact, that no
> IPC interrupts will occur until probing is completed. This should also
> normally be the case, but if for some reason such an interrupt does
> trigger at that time, access to that register can be messed up. This
> should be reviewed and possibly refined separately.

There will be additional IPC hardening fixes that are being tested at 
the moment, the introduction of HDaudio support seems to have exposed a 
number of issues we didn't see with I2S/TDM/DMIC interfaces. I included 
Guennadi's patch in the series since we have evidence it does improve 
things on the Up2. we can look at the _unlocked parts in the next update 
with more validation.