ARM OMAP: Fix race in OMAP2/3 DMA IRQ handling
Subsystems (2)
PATCH 1/1 ARM OMAP: Fix race in OMAP2/3 DMA IRQ handling
CSR must be cleared before invoking the callback.
If the callback function starts a new, fast DMA transfer on the same channel, the completion status might lost if CSR is cleared after the callback invocation.
--- arch/arm/plat-omap/dma.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
arch/arm/plat-omap/dma.c +2 -2
@@ -1898,11 +1898,11 @@ static int omap2_dma_handle_ch(int ch) status =dma_read (CSR (ch)); } +dma_write (status,CSR (ch)); +if (likely (dma_chan[ch].callback !=NULL )) dma_chan[ch].callback (ch, status, dma_chan[ch].data); -dma_write (status,CSR (ch)); -return 0 ; }
Signature · 1 line
1.5.6.5
Quoted 23 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Juha Yrjölä
Sent: Wednesday, January 21, 2009 10:54 PM
To: [email protected]
Subject: [PATCH] ARM OMAP: Fix race in OMAP2/3 DMA IRQ handlingCSR must be cleared before invoking the callback.
If the callback function starts a new, fast DMA transfer on the same channel, the completion status might lost if CSR is cleared after the callback invocation.
Signed-off-by Juha Yrjola <[email protected]>--- arch/arm/plat-omap/dma.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)arch/arm/plat-omap/dma.c +0
-0
arch/arm/plat-omap/dma.c static int omap2_dma_handle_ch(int ch) · line 1901
Quoted 4 lines
arch/arm/plat-omap/dma.c +1
-0@@ -1898,11 +1898,11 @@ static int omap2_dma_handle_ch(int ch) status =dma_read (CSR (ch)); } +dma_write (status,CSR (ch));
This is not necessary. Refers line "dma_write(OMAP2_DMA_CSR_CLEAR_MASK, CSR(ch));" just above.
It will any way do the job of clearing. In a way, clear done after the callback has no effect since the status reg and global IRQ_enable for the particular channel is already disabled before that.
arch/arm/plat-omap/dma.c static int omap2_dma_handle_ch(int ch) · line 1908
Quoted 10 lines
arch/arm/plat-omap/dma.c +1
-2+if (likely (dma_chan[ch].callback !=NULL )) dma_chan[ch].callback (ch, status, dma_chan[ch].data); -dma_write (status,CSR (ch)); -return 0 ; }
So dma library is safe from the problem you have described.
Quoted 7 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 8 lines · Santosh
arch/arm/plat-omap/dma.c static int omap2_dma_handle_ch(int ch) · line 1901
Quoted 7 lines
arch/arm/plat-omap/dma.c +1
-0@@ -1898,11 +1898,11 @@ static int omap2_dma_handle_ch(int ch) status =dma_read (CSR (ch)); } +dma_write (status,CSR (ch));This is not necessary. Refers line "
dma_write(OMAP2_DMA_CSR_CLEAR_MASK, CSR(ch));" just above.
Yes, the current DMA code is full of inconsistencies and illogic. In general, clearing a hard-coded mask of bits in an IRQ status register is a nice way to enter a race with the machine. And that's a race you cannot win every time, so you'll miss IRQs that you haven't handled yet.
A major cleanup should be done to the DMA code, but that's no reason not to fix bugs now.
Quoted 3 lines
It will any way do the job of clearing. In a way, clear done after the callback has no effect since the status reg and global
IRQ_enablefor the particular channel is already disabled before that.
Yes, in a way that completely ignores the code and hardware behaviour.
If you write a 1 to, say, the FRAME bit of the CSR *after* a transfer has been completed, *before* handling the event, you lose the CSR value, so the channel handling function complains (correctly) about a spurious IRQ and refuses to do anything more productive.
If you start a quick transfer from the callback function, the FRAME bit *will* get set before control returns from the callback function.
Quoted 1 line
So dma library is safe from the problem you have described.
Sounds like you're in denial, man. I didn't just randomly send a patch, I actually ran into the problem, fixed it, verified it indeed is fixed, and only after that did I send it.
Cheers,
Juha
Tony Lindgren
AppliedQuoted 28 lines · Juha Yrjola
Quoted 7 lines · Santosh
arch/arm/plat-omap/dma.c static int omap2_dma_handle_ch(int ch) · line 1900
Quoted 6 lines
arch/arm/plat-omap/dma.c +0
-0@@ -1898,11 +1898,11 @@ static int omap2_dma_handle_ch(int ch) status =dma_read (CSR (ch)); } +dma_write (status,CSR (ch));This is not necessary. Refers line "
dma_write(OMAP2_DMA_CSR_CLEAR_MASK, CSR(ch));" just above.Yes, the current DMA code is full of inconsistencies and illogic. In general, clearing a hard-coded mask of bits in an IRQ status register is a nice way to enter a race with the machine. And that's a race you cannot win every time, so you'll miss IRQs that you haven't handled yet.
A major cleanup should be done to the DMA code, but that's no reason not to fix bugs now.
Quoted 3 lines
It will any way do the job of clearing. In a way, clear done after the callback has no effect since the status reg and global
IRQ_enablefor the particular channel is already disabled before that.Yes, in a way that completely ignores the code and hardware behaviour.
If you write a 1 to, say, the FRAME bit of the CSR *after* a transfer has been completed, *before* handling the event, you lose the CSR value, so the channel handling function complains (correctly) about a spurious IRQ and refuses to do anything more productive.
Quoted 8 lines
If you start a quick transfer from the callback function, the FRAME bit *will* get set before control returns from the callback function.
Quoted 1 line
So dma library is safe from the problem you have described.
Sounds like you're in denial, man. I didn't just randomly send a patch, I actually ran into the problem, fixed it, verified it indeed is fixed, and only after that did I send it.
Pushed to l-o master and omap-2.6.28 branches, and added to omap-fixes queue for the mainline.
Tony
Regards,
Santosh
Quoted 34 lines
-----Original Message-----
From: Juha Yrjola [mailto:[email protected]]
Sent: Thursday, January 22, 2009 1:44 PM
To: Shilimkar, Santosh
Cc: [email protected]
Subject: Re: [PATCH] ARM OMAP: Fix race in OMAP2/3 DMA IRQ handlingQuoted 8 lines · Santosh
arch/arm/plat-omap/dma.c static int omap2_dma_handle_ch(int ch) · line 1901
Quoted 7 lines
arch/arm/plat-omap/dma.c +1
-0@@ -1898,11 +1898,11 @@ static int omap2_dma_handle_ch(int ch) status =dma_read (CSR (ch)); } +dma_write (status,CSR (ch));This is not necessary. Refers line
"
dma_write(OMAP2_DMA_CSR_CLEAR_MASK, CSR(ch));" just above.Yes, the current DMA code is full of inconsistencies and illogic. In general, clearing a hard-coded mask of bits in an IRQ status register is
a nice way to enter a race with the machine. And that's a race you cannot win every time, so you'll miss IRQs that you haven't handled yet.A major cleanup should be done to the DMA code, but that's no reason not
to fix bugs now.Quoted 2 lines
It will any way do the job of clearing. In a way, clear done after the callback has no effect since the status reg and global
IRQ_enableforQuoted 1 line
the particular channel is already disabled before that.
Quoted 8 lines
Yes, in a way that completely ignores the code and hardware behaviour.
If you write a 1 to, say, the FRAME bit of the CSR *after* a transfer has been completed, *before* handling the event, you lose the CSR value,
so the channel handling function complains (correctly) about a spurious
IRQ and refuses to do anything more productive.
What you mean by *before* handling the event ? Do you mean the callback. Because status passed to the callback is read in the beginning of ISR. I agree with your point partly. Partly because I sill have a question in mind.
Quoted 11 lines
If you start a quick transfer from the callback function, the FRAME bit
*will* get set before control returns from the callback function.Quoted 1 line
So dma library is safe from the problem you have described.
Sounds like you're in denial, man. I didn't just randomly send a patch,
I actually ran into the problem, fixed it, verified it indeed is fixed,
and only after that did I send it.
Not really in denail mode.I just couldn't visualise the issue completely. For that matter any patch fixes the bug is welcome :)
Since DMA channel interrupt refelcts at two levels ( global DMA_IRQSTATUS and Cannel specific CSR), and global updates are independent of any mask, I was curious about the issue. We have already seen some spurious IRQ issue because of above when DMA hardware is being used by two independent software modules running together.