linux-kernel - Re: [PATCH] dmaengine: omap-dma: Do not call omap_dma_callback() from tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160226010658.GN5783@n2100.arm.linux.org.uk>
Date:	Fri, 26 Feb 2016 01:06:58 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Peter Ujfalusi <peter.ujfalusi@...com>
Cc:	vinod.koul@...el.com, dmaengine@...r.kernel.org,
	linux-omap@...r.kernel.org, nsekhar@...com,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] dmaengine: omap-dma: Do not call omap_dma_callback()
 from tx_status()

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

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

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.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.