[v2] dmaengine: imx-sdma: remove BD_INTR for channel0

Commit Message

Robin Gong June 21, 2019, 8:23 a.m. UTC

From: Robin Gong <[e-mail]>

It is possible for an irq triggered by channel0 to be received later
after clks are disabled once firmware loaded during sdma probe. If
that happens then clearing them by writing to SDMA_H_INTR won't work
and the kernel will hang processing infinite interrupts. Actually,
don't need interrupt triggered on channel0 since it's pollling
SDMA_H_STATSTOP to know channel0 done rather than interrupt in
current code, just clear BD_INTR to disable channel0 interrupt to
avoid the above case.
This issue was brought by commit 1d069bfa3c78 ("dmaengine: imx-sdma:
ack channel 0 IRQ in the interrupt handler") which didn't take care
the above case.

Fixes: 1d069bfa3c78 ("dmaengine: imx-sdma: ack channel 0 IRQ in the interrupt handler")
Cc: [e-mail] #5.0+
Signed-off-by: Robin Gong <[e-mail]>
Reported-by: Sven Van Asbroeck <[e-mail]>
Tested-by: Sven Van Asbroeck <[e-mail]>
---
 drivers/dma/imx-sdma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Michael Olbrich June 21, 2019, 12:53 p.m. UTC | #1

On Fri, Jun 21, 2019 at 04:23:06PM +0800, [e-mail] wrote:
> From: Robin Gong <[e-mail]>
> 
> It is possible for an irq triggered by channel0 to be received later
> after clks are disabled once firmware loaded during sdma probe. If
> that happens then clearing them by writing to SDMA_H_INTR won't work
> and the kernel will hang processing infinite interrupts. Actually,
> don't need interrupt triggered on channel0 since it's pollling
> SDMA_H_STATSTOP to know channel0 done rather than interrupt in
> current code, just clear BD_INTR to disable channel0 interrupt to
> avoid the above case.
> This issue was brought by commit 1d069bfa3c78 ("dmaengine: imx-sdma:
> ack channel 0 IRQ in the interrupt handler") which didn't take care
> the above case.
> 
> Fixes: 1d069bfa3c78 ("dmaengine: imx-sdma: ack channel 0 IRQ in the interrupt handler")
> Cc: [e-mail] #5.0+
> Signed-off-by: Robin Gong <[e-mail]>
> Reported-by: Sven Van Asbroeck <[e-mail]>
> Tested-by: Sven Van Asbroeck <[e-mail]>

Reviewed-by: Michael Olbrich <[e-mail]>

> ---
>  drivers/dma/imx-sdma.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index deea9aa..b5a1ee2 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -742,7 +742,7 @@ static int sdma_load_script(struct sdma_engine *sdma, void *buf, int size,
>  	spin_lock_irqsave(&sdma->channel_0_lock, flags);
>  
>  	bd0->mode.command = C0_SETPM;
> -	bd0->mode.status = BD_DONE | BD_INTR | BD_WRAP | BD_EXTD;
> +	bd0->mode.status = BD_DONE | BD_WRAP | BD_EXTD;
>  	bd0->mode.count = size / 2;
>  	bd0->buffer_addr = buf_phys;
>  	bd0->ext_buffer_addr = address;
> @@ -1064,7 +1064,7 @@ static int sdma_load_context(struct sdma_channel *sdmac)
>  	context->gReg[7] = sdmac->watermark_level;
>  
>  	bd0->mode.command = C0_SETDM;
> -	bd0->mode.status = BD_DONE | BD_INTR | BD_WRAP | BD_EXTD;
> +	bd0->mode.status = BD_DONE | BD_WRAP | BD_EXTD;
>  	bd0->mode.count = sizeof(*context) / 4;
>  	bd0->buffer_addr = sdma->context_phys;
>  	bd0->ext_buffer_addr = 2048 + (sizeof(*context) / 4) * channel;
> -- 
> 2.7.4
> 
>

Sven Van Asbroeck June 21, 2019, 1:12 p.m. UTC | #2

Looking great.

Thank you for taking the time for this, Robin !

Fabio Estevam June 24, 2019, 12:54 p.m. UTC | #3

Hi Robin,

On Fri, Jun 21, 2019 at 5:21 AM <[e-mail]> wrote:
>
> From: Robin Gong <[e-mail]>
>
> It is possible for an irq triggered by channel0 to be received later
> after clks are disabled once firmware loaded during sdma probe. If
> that happens then clearing them by writing to SDMA_H_INTR won't work
> and the kernel will hang processing infinite interrupts. Actually,
> don't need interrupt triggered on channel0 since it's pollling
> SDMA_H_STATSTOP to know channel0 done rather than interrupt in
> current code, just clear BD_INTR to disable channel0 interrupt to
> avoid the above case.
> This issue was brought by commit 1d069bfa3c78 ("dmaengine: imx-sdma:
> ack channel 0 IRQ in the interrupt handler") which didn't take care
> the above case.
>
> Fixes: 1d069bfa3c78 ("dmaengine: imx-sdma: ack channel 0 IRQ in the interrupt handler")
> Cc: [e-mail] #5.0+

This 5.0 notation does not look correct, as 1d069bfa3c78 was introduced in 4.10.

Robin Gong June 25, 2019, 7:36 a.m. UTC | #4

> -----Original Message-----
> From: Fabio Estevam <[e-mail]>
> Sent: 2019年6月24日 20:55
> 
> Hi Robin,
> 
> On Fri, Jun 21, 2019 at 5:21 AM <[e-mail]> wrote:
> >
> > From: Robin Gong <[e-mail]>
> >
> > It is possible for an irq triggered by channel0 to be received later
> > after clks are disabled once firmware loaded during sdma probe. If
> > that happens then clearing them by writing to SDMA_H_INTR won't work
> > and the kernel will hang processing infinite interrupts. Actually,
> > don't need interrupt triggered on channel0 since it's pollling
> > SDMA_H_STATSTOP to know channel0 done rather than interrupt in current
> > code, just clear BD_INTR to disable channel0 interrupt to avoid the
> > above case.
> > This issue was brought by commit 1d069bfa3c78 ("dmaengine: imx-sdma:
> > ack channel 0 IRQ in the interrupt handler") which didn't take care
> > the above case.
> >
> > Fixes: 1d069bfa3c78 ("dmaengine: imx-sdma: ack channel 0 IRQ in the
> > interrupt handler")
> > Cc: [e-mail] #5.0+
> 
> This 5.0 notation does not look correct, as 1d069bfa3c78 was introduced in
> 4.10.
Yes, although Sven only met this issue after v4.19, this potential issue should be there.
But 1d069bfa3c78 seems merged from v4.7 instead?

Fabio Estevam June 25, 2019, 3:45 p.m. UTC | #5

Hi Robin,

On Tue, Jun 25, 2019 at 4:36 AM Robin Gong <[e-mail]> wrote:

> Yes, although Sven only met this issue after v4.19, this potential issue should be there.
> But 1d069bfa3c78 seems merged from v4.7 instead?

Yes, you could simply do:

Fixes: 1d069bfa3c78 ("dmaengine: imx-sdma: ack channel 0 IRQ in the
interrupt handler")
Cc: [e-mail]

And it will get applied to all relevant stable trees.

Thanks

Vinod Koul July 5, 2019, 7:46 a.m. UTC | #6

On 21-06-19, 16:23, [e-mail] wrote:
> From: Robin Gong <[e-mail]>
> 
> It is possible for an irq triggered by channel0 to be received later
> after clks are disabled once firmware loaded during sdma probe. If
> that happens then clearing them by writing to SDMA_H_INTR won't work
> and the kernel will hang processing infinite interrupts. Actually,
> don't need interrupt triggered on channel0 since it's pollling
> SDMA_H_STATSTOP to know channel0 done rather than interrupt in
> current code, just clear BD_INTR to disable channel0 interrupt to
> avoid the above case.
> This issue was brought by commit 1d069bfa3c78 ("dmaengine: imx-sdma:
> ack channel 0 IRQ in the interrupt handler") which didn't take care
> the above case.

Applied, thanks

Patch

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index deea9aa..b5a1ee2 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -742,7 +742,7 @@  static int sdma_load_script(struct sdma_engine *sdma, void *buf, int size,
 	spin_lock_irqsave(&sdma->channel_0_lock, flags);
 
 	bd0->mode.command = C0_SETPM;
-	bd0->mode.status = BD_DONE | BD_INTR | BD_WRAP | BD_EXTD;
+	bd0->mode.status = BD_DONE | BD_WRAP | BD_EXTD;
 	bd0->mode.count = size / 2;
 	bd0->buffer_addr = buf_phys;
 	bd0->ext_buffer_addr = address;
@@ -1064,7 +1064,7 @@  static int sdma_load_context(struct sdma_channel *sdmac)
 	context->gReg[7] = sdmac->watermark_level;
 
 	bd0->mode.command = C0_SETDM;
-	bd0->mode.status = BD_DONE | BD_INTR | BD_WRAP | BD_EXTD;
+	bd0->mode.status = BD_DONE | BD_WRAP | BD_EXTD;
 	bd0->mode.count = sizeof(*context) / 4;
 	bd0->buffer_addr = sdma->context_phys;
 	bd0->ext_buffer_addr = 2048 + (sizeof(*context) / 4) * channel;