ASoC: SOF: add spin_lock to protect stream allocation function

[RanderWang]

According to hda framework, stream should be protected by reg_lock

Signed-off-by: Rander Wang rander.wang@linux.intel.com

[keyonjie]

good catch, only minor comment, shall we use spin_lock/unlock_irq() which can disable the preempted and changed by irq handler?

[RanderWang]

yes, it should be spin_lock_irq. I was misleaded by some code.

[RanderWang]

yes, I search the code, spin_lock_irq is used except one case which is copied by me......

[plbossart]

In addition reg_lock is not needed when SOF_HDA is not needed...so this patch is getting in the way of the work done to have a better split between HDA and non-HDA platforms.

[RanderWang]

Actually it is needed for all case no matter of HD or non-HD. For I2S codec, stream is used for host dma
@keyonjie could you confirm it?

[keyonjie]

yes, as Rander stated, for non-HD case, we still need manage host-DMA and host streams, so better to initialize bus->reg_lock for all cases(put it out of SOF_HDA braces).

[plbossart]

ok, so it's needed, but why are we not using spin_lock_irqsave() in hda_dsp_ipc_irq_handler() and hda_dsp_stream_interrupt()? All other platforms do so in interrupt context, so is this a miss or something intentional?

[keyonjie]

Hi Pierre, IMHO, spin_lock_irqsave() is useful when nested lock is hold with local irq disabled in interrupt context, in our case, both hda_dsp_ipc_irq_handler() and hda_dsp_stream_interrupt() are holding only one lock(and different lock), so spin_lock() there should be fine.

Switching to use spin_lock_irqsave() pairs may lead to a little more time cost at irq and preempt store/restore, but I am fine with the change if that make us more released from concerning about dead lock possibility.

[plbossart]

@keyonjie I am utterly confused by your answer. The first part says there is no problem but the second part says we might have deadlock cases, which hints at scarier and fundamental issues with the design? Can you please clarify your point?

[keyonjie]

@plbossart my point is that spin_lock() now in hda_dsp_ipc_irq_handler() and hda_dsp_stream_interrupt() are fine.

[libinyang]

@plbossart my point is that spin_lock() now in hda_dsp_ipc_irq_handler() and hda_dsp_stream_interrupt() are fine.

I can't find where we are calling hda_dsp_stream_get() and hda_dsp_stream_put(). I believe the spin_lock is trying to protect bus->stream_list. However, there are still other functions changing it, like hda_dsp_stream_get_pstream() and etc. Any reason we don't need the spin_lock in those functions?
Besides, I think we should use spin_lock_irq() very carefully. You must know the interrupt context each time you are calling these functions. Otherwise, some potential bugs may happen.

[keyonjie]

@libinyang I believe you are looking into old code base, xx_get_pstream() is not existed anymore.

Per my understanding, using spin_lock_irq() in thread context is fine, but in interrupt context, we need to be careful when using it(spin_lock_irq), spin_unlock_irq() there will enable irq which may confuse embedded spin_lock_irq() within the same irq handler. To avoid this kind of deadlock, we can use spin_lock_irqsave().

[libinyang]

@libinyang I believe you are looking into old code base, xx_get_pstream() is not existed anymore.

Per my understanding, using spin_lock_irq() in thread context is fine, but in interrupt context, we need to be careful when using it(spin_lock_irq), spin_unlock_irq() there will enable irq which may confuse embedded spin_lock_irq() within the same irq handler. To avoid this kind of deadlock, we can use spin_lock_irqsave().

It seems my code is not sync with yours. Thanks for point it out.
Actually, even in thread context, it is still unsafe to call spin_lock_irq(), except you know the interrupt status (Somehow, the interrupt enabling status may be manually changed by code.) everytime it is called. If you really know it is safe to call spin_lock_irq(), it's OK and please go ahead :). Otherwise, please use spin_lock_irqsave().

[keyonjie]

@libinyang thanks for education, so looks spin_lock() in hda_dsp_ipc_irq_handler() and hda_dsp_stream_interrupt() are fine, but in hda_dsp_stream_get/put() what @RanderWang's PR listed, better to switch to spin_lock_irqsave().

[plbossart]

@keyonjie you will need to educate me further on why we can use spin_lock() in hda_dsp_ipc_irq_handler() when all other interrupt handling uses spin_lock_irq_save() in the SOF code base. If I understand well, the default is to use spin_lock_irq_save() unless you absolutely now that spin_lock() is risk-free, and I am not getting a good explanation as to why it is?
Not trying to be painful, just to make sure we are all on the same page wrt. the interrupt handling flow.

[keyonjie]

@plbossart taking hda_dsp_ipc_irq_handler() as example, the resource we are locking here is sdev->hw_lock, we are holding this lock in these places of SOF code:

$ grep "hw_lock" sound/soc/sof/* -r
sound/soc/sof/core.c: spin_lock_init(&sdev->hw_lock);
sound/soc/sof/intel/hda-ipc.c: spin_lock(&sdev->hw_lock);
sound/soc/sof/intel/hda-ipc.c: spin_unlock(&sdev->hw_lock);
sound/soc/sof/intel/hsw.c: spin_lock(&sdev->hw_lock);
sound/soc/sof/intel/hsw.c: spin_unlock(&sdev->hw_lock);
sound/soc/sof/ops.c: spin_lock_irqsave(&sdev->hw_lock, flags);
sound/soc/sof/ops.c: spin_unlock_irqrestore(&sdev->hw_lock, flags);
sound/soc/sof/ops.c: spin_lock_irqsave(&sdev->hw_lock, flags);
sound/soc/sof/ops.c: spin_unlock_irqrestore(&sdev->hw_lock, flags);
sound/soc/sof/ops.c: spin_lock_irqsave(&sdev->hw_lock, flags);
sound/soc/sof/ops.c: spin_unlock_irqrestore(&sdev->hw_lock, flags);
sound/soc/sof/ops.c: spin_lock_irqsave(&sdev->hw_lock, flags);
sound/soc/sof/ops.c: spin_unlock_irqrestore(&sdev->hw_lock, flags);

a. interrupt context:
only one in ipc interrupt handler, e.g. hda_dsp_ipc_irq_handler() for APL.
b. thread context:
in ops.c: snd_sof_dsp_update_xxx() which will be called in normal thread context(the call in hda_dsp_ipc_irq_thread() is actually bottom half let's treat it as thread context also), and they are all using spin_lock_irqsave/restore() mode.

So, the scenarios I can imagine here are:

1. #b run firstly, irq is disabled before #b finished, no chance to run #a.
2. #a run firstly, irq is not disabled(we are using spin_lock()) in #a, but #b has no change to run before #a is finished.

For both #1 and #2, I can't imagine any possibility for dead lock happening, so that's why I think we can keep using spin_lock() in hda_dsp_ipc_irq_handler().

[RanderWang]

Yes, it is good case for us to make clear everything. I also study lock in Linux kernel, there is a good article https://kernel.readthedocs.io/en/sphinx-samples/kernel-locking.html

@plbossart @keyonjie
for spin_lock() in hda_dsp_ipc_irq_handler(), in above link, the explanation is

The irq handler does not to use spin_lock_irq(), because the softirq (or other low priority thread) cannot run while the irq handler is running: it can use spin_lock(), which is slightly faster. The only exception would be if a different hardware irq handler uses the same lock: spin_lock_irq() will stop that from interrupting us.

I study the code and my idea is why spin_lock of hw_lock is needed in hda_dsp_ipc_irq_handler ? Actually only HDA_DSP_REG_ADSPIC is accessed by irq_thread function which is called after irq_handler function. So no need to call spin_lock and it is not a good idea to call spin_lock in IRQ function

[RanderWang]

And I also check all the usage of bus->reg_lock. Two types of scenarios, to protect stream between two user threads in kernel or user threads with a softirq function: azx_irq_pending_work. It is never used in IRQ function, so spin_lock_irq is suitable, and of course spin_lock_irqsave is always right. Actually, in HDA framework, spin_lock_irqsave is never used on bus->reg_lock

[keyonjie]

Yes, it is good case for us to make clear everything. I also study lock in Linux kernel, there is a good article https://kernel.readthedocs.io/en/sphinx-samples/kernel-locking.html

@plbossart @keyonjie
for spin_lock() in hda_dsp_ipc_irq_handler(), in above link, the explanation is

The irq handler does not to use spin_lock_irq(), because the softirq (or other low priority thread) cannot run while the irq handler is running: it can use spin_lock(), which is slightly faster. The only exception would be if a different hardware irq handler uses the same lock: spin_lock_irq() will stop that from interrupting us.

I study the code and my idea is why spin_lock of hw_lock is needed in hda_dsp_ipc_irq_handler ? Actually only HDA_DSP_REG_ADSPIC is accessed by irq_thread function which is called after irq_handler function. So no need to call spin_lock and it is not a good idea to call spin_lock in IRQ function

Hi @RanderWang, I agree we actually can delete them, but with minor change as below at the same time:

	/* IPC message ? */
	if (sdev->irq_status & HDA_DSP_ADSPIS_IPC) {
		/* disable IPC interrupt */
--		snd_sof_dsp_update_bits_unlocked(sdev, HDA_DSP_BAR,
++		snd_sof_dsp_update_bits(sdev, HDA_DSP_BAR,
						 HDA_DSP_REG_ADSPIC,
						 HDA_DSP_ADSPIC_IPC, 0);
		ret = IRQ_WAKE_THREAD;
	}

[plbossart]

Sorry, @keyonjie @RanderWang your explanations are going in all sorts of directions without answering to my very simple question.

I was asking whether "reg_lock" needs to be protected by spin_lock() or or spin_lock_irq() or spin_lock_irqsave(). How "hw_lock" is managed is a different discussion, let's take things one step at a time, shall we?

Also your answers are sometimes quite difficult to follow, please re-read your write up to make sure there aren't missing verbs or that the sentence is easy to understand. Using 'it' or 'that' after 3 lines of explanations including multiple entities is a sure way to lose the reader. It may be perfectly clear to you, it's totally ambiguous to me.

[RanderWang]

I checked all the usage of bus->reg_lock in Linux kernel. Two types of scenarios are found, protecting stream between two user threads in kernel or protecting stream between user threads and a softirq function: azx_irq_pending_work. bus->reg_lock is never used in IRQ function, so spin_lock_irq is suitable. Actually, in HDA framework, spin_lock_irqsave is never used on bus->reg_lock.

[plbossart]

@RanderWang thanks for double-checking, merging now

[plbossart]

@xiulipan I have no idea what the CI issues are, will merge anyways.

[xiulipan]

@plbossart
Please ignore the on device test now.
Not sure what tplg should be used on up2 nocodec platform now.