On 2016-02-26 03:06, Russell King - ARM Linux wrote:
> On Thu, Feb 25, 2016 at 10:28:59AM +0200, Peter Ujfalusi wrote:
>> When based on the CCR_ENABLE bit the channel is stopped we should not call
>> omap_dma_callback(), only change the return value to DMA_COMPLETE. Client
>> drivers will do the right thing to clean up the channel after the transfer
>> has been completed.
>> Check the CCR_ENABLE only if the channel is not paused since pause in sDMA
>> means that the channel is stopped.
>> This will fix one hard to reproduce race condition when the channel is
>> terminated during transfer (affecting cyclic operation).
>>
>> Fixes: 1a7cf7b26f25 ("dmaengine: omap-dma: Handle cases when the channel is polled for completion")
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@xxxxxx>
>> ---
>> drivers/dma/omap-dma.c | 16 ++++++++++------
>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
>> index f6bef0d93998..a6b189fdbbe6 100644
>> --- a/drivers/dma/omap-dma.c
>> +++ b/drivers/dma/omap-dma.c
>> @@ -671,18 +671,22 @@ static enum dma_status omap_dma_tx_status(struct dma_chan *chan,
>> struct omap_chan *c = to_omap_dma_chan(chan);
>> struct virt_dma_desc *vd;
>> enum dma_status ret;
>> - uint32_t ccr;
>> unsigned long flags;
>>
>> - ccr = omap_dma_chan_read(c, CCR);
>> - /* The channel is no longer active, handle the completion right away */
>> - if (!(ccr & CCR_ENABLE))
>> - omap_dma_callback(c->dma_ch, 0, c);
>> -
>> ret = dma_cookie_status(chan, cookie, txstate);
>> if (ret == DMA_COMPLETE || !txstate)
>> return ret;
>>
>> + if (!c->paused) {
>> + uint32_t ccr = omap_dma_chan_read(c, CCR);
>> + /*
>> + * The channel is no longer active, set the return value
>> + * accordingly
>> + */
>> + if (!(ccr & CCR_ENABLE))
>> + ret = DMA_COMPLETE;
>> + }
>> +
>
> This looks very much like a hack, and surely opens a race condition
> up: what happens when a request submitted and pending but not yet
> started? If the channel is idle, requesting status will report
> that the request has completed.
>
> It's also wrong for another reason. If txstate is NULL...
True, I have fixed these up.
> Your original commit adding the original hack that you're now removing
> above says that this is to support polled operation: I'm not aware of
> DMA engine supporting such a mode. DMA_PREP_INTERRUPT is a mechanism
> where requests can be queued without an interrupt to allow batching.
Also it is used to suppress DMA interrupts during audio playback for example.
In this case we will run w/o interrupts and the position is polled.
> See the raid5/async_tx code, which queues a set of operations without
> DMA_PREP_INTERRUPT, with the final operation with DMA_PREP_INTERRUPT
> set.
We only allow the interrupts to be disabled in cyclic or memcpy mode. With
slave_sg we have interrupts as it is needed to move to the next SG.
> As the driver is reliant on interrupts to move to the next transfer,
> the patch which causes DMA_PREP_INTERRUPT to influence whether
> interrupts are sent is actually buggy, and will prevent several
> queued DMA operations to fail.
Yes, the omap-dma only allows the interrupts to be actually disabled when it
is save to do so. slave_sg can not work w/o interrupts so there we don't
disable them.
>
--
Péter
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
