On 7/29/2022 4:19 PM, Takashi Iwai wrote: Thanks for your time.
[CAUTION: External Email] On Fri, 29 Jul 2022 12:34:51 +0200, Venkata Prasad Potturu wrote:On 7/28/22 18:19, Mark Brown wrote: Thanks for your time. On Thu, Jul 28, 2022 at 06:10:50PM +0530, Venkata Prasad Potturu wrote: @@ -104,14 +105,13 @@ static irqreturn_t i2s_irq_handler(int irq, void *data) ext_intr_stat = readl(ACP_EXTERNAL_INTR_STAT(adata, rsrc->irqp_used)); - for (i = 0; i < ACP_MAX_STREAM; i++) { - stream = adata->stream[i]; + spin_lock_irqsave(&adata->acp_lock, flags); + list_for_each_entry(stream, &adata->stream_list, list) { If we're already in an interrupt handler here (presumably not a threaded one) why are we using irqsave? Yes, your statement make sense, I have followed below statement in kernel document. so used irqsave in interrupt context as well. We will change it to spin_lock() and send it in the next version. statement:- spin_lock_irqsave() will turn off interrupts if they are on, otherwise does nothing (if we are already in an interrupt handler), hence these functions are safe to call from any context.Also the open and close callbacks are certainly non-irq context, hence you can use spin_lock_irq() instead of irqsave(), too.
One doubt, if we use *spin_lock_irq()* it will do *local_irq_disable() *even if interrupts are already disabled, and when we call *spin_unlock_irq() *after critical section it will forcibly re-enable interrupts in a potentially unwanted manner.
If we use *spin_lock_irqsave();* and *spin_unlock_irqrestore(); *will save and restore the interrupt state.
My understanding is *spin_lock_irqsave();* is better instead of improper interrupt enable and disable as like *spin_lock_irq();*
Could you please let me know is there any other thing to use *spin_lock_irq(); *particularly here.
Takashi
