[PATCH] ARM OMAP: Fix race in OMAP2/3 DMA IRQ handling

[PATCH] ARM OMAP: Fix race in OMAP2/3 DMA IRQ handling

[Juha Yrjölä]

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.

Signed-off-by: Juha Yrjola <juha.yrjola@solidboot.com>
---
 arch/arm/plat-omap/dma.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
index 8e6475d..265351b 100755
--- a/arch/arm/plat-omap/dma.c
+++ b/arch/arm/plat-omap/dma.c
@@ -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;
 }
 
-- 
1.5.6.5

RE: [PATCH] ARM OMAP: Fix race in OMAP2/3 DMA IRQ handling

[Shilimkar, Santosh]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Juha Yrjölä
> Sent: Wednesday, January 21, 2009 10:54 PM
> To: linux-omap@vger.kernel.org
> Subject: [PATCH] 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.
> 
> Signed-off-by: Juha Yrjola <juha.yrjola@solidboot.com>
> ---
>  arch/arm/plat-omap/dma.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
> index 8e6475d..265351b 100755
> --- a/arch/arm/plat-omap/dma.c
> +++ b/arch/arm/plat-omap/dma.c
> @@ -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.
> +
>  	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.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM OMAP: Fix race in OMAP2/3 DMA IRQ handling

[Juha Yrjola]

Shilimkar, Santosh wrote:
>> --- a/arch/arm/plat-omap/dma.c
>> +++ b/arch/arm/plat-omap/dma.c
>> @@ -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.

> 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.

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.

> 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

Re: [PATCH] ARM OMAP: Fix race in OMAP2/3 DMA IRQ handling

[Tony Lindgren]

* Juha Yrjola <juha.yrjola@solidboot.com> [090122 00:34]:
> Shilimkar, Santosh wrote:
>>> --- a/arch/arm/plat-omap/dma.c
>>> +++ b/arch/arm/plat-omap/dma.c
>>> @@ -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.
>
>> 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.
>
> 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.
>
>> 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

RE: [PATCH] ARM OMAP: Fix race in OMAP2/3 DMA IRQ handling

[Shilimkar, Santosh]

Regards,
Santosh
 

> -----Original Message-----
> From: Juha Yrjola [mailto:juha.yrjola@solidboot.com] 
> Sent: Thursday, January 22, 2009 1:44 PM
> To: Shilimkar, Santosh
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] ARM OMAP: Fix race in OMAP2/3 DMA IRQ handling
> 
> Shilimkar, Santosh wrote:
> >> --- a/arch/arm/plat-omap/dma.c
> >> +++ b/arch/arm/plat-omap/dma.c
> >> @@ -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.
> 
> > 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.
> 
> 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.
> If you start a quick transfer from the callback function, the 
> FRAME bit 
> *will* get set before control returns from the callback function.
> 
> > 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.