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.