linux-kernel - Re: Revert "dmaengine: pl330: add DMA

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: <53b13d76-16a1-0e0a-09e1-c917e5d49326@samsung.com>
Date:   Tue, 8 May 2018 11:04:06 +0200
From:   Marek Szyprowski <m.szyprowski@...sung.com>
To:     Frank Mori Hess <fmh6jj@...il.com>, Vinod Koul <vkoul@...nel.org>,
        dmaengine@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org,
        Dan Williams <dan.j.williams@...el.com>,
        r.baldyga@...kerion.com, Krzysztof Kozlowski <krzk@...nel.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Subject: Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

Hi Frank and Vinod,

On 2018-04-28 23:50, Frank Mori Hess wrote:
> This reverts commit 88987d2c7534a0269f567fb101e6d71a08f0f01d.
>
> The pl330.c pause implementation violates the dmaengine requirement
> for no data loss, since it relies on the DMAKILL
> instruction.  However, DMAKILL discards in-flight data from the
> dma controller's fifo.  This is documented in the dma-330 manual
> and I have observed it with hardware doing device-to-memory burst
> transfers.  The discarded data may or may not show up in the
> residue count, depending on timing (resulting in data corruption
> effectively).
>
> Signed-off-by: Frank Mori Hess <fmh6jj@...il.com>

This revert completely breaks serial driver operation on almost all Exynos
SoCs, because serial driver relies on having PAUSE feature and proper
residue reporting from dma engine. Please drop it if possible.

> ---
>   drivers/dma/pl330.c | 28 ----------------------------
>   1 file changed, 28 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 6237069001c4..f802bd3b0481 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2290,33 +2290,6 @@ static int pl330_terminate_all(struct dma_chan *chan)
>   	return 0;
>   }
>   
> -/*
> - * We don't support DMA_RESUME command because of hardware
> - * limitations, so after pausing the channel we cannot restore
> - * it to active state. We have to terminate channel and setup
> - * DMA transfer again. This pause feature was implemented to
> - * allow safely read residue before channel termination.
> - */
> -static int pl330_pause(struct dma_chan *chan)
> -{
> -	struct dma_pl330_chan *pch = to_pchan(chan);
> -	struct pl330_dmac *pl330 = pch->dmac;
> -	unsigned long flags;
> -
> -	pm_runtime_get_sync(pl330->ddma.dev);
> -	spin_lock_irqsave(&pch->lock, flags);
> -
> -	spin_lock(&pl330->lock);
> -	_stop(pch->thread);
> -	spin_unlock(&pl330->lock);
> -
> -	spin_unlock_irqrestore(&pch->lock, flags);
> -	pm_runtime_mark_last_busy(pl330->ddma.dev);
> -	pm_runtime_put_autosuspend(pl330->ddma.dev);
> -
> -	return 0;
> -}
> -
>   static void pl330_free_chan_resources(struct dma_chan *chan)
>   {
>   	struct dma_pl330_chan *pch = to_pchan(chan);
> @@ -3027,7 +3000,6 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>   	pd->device_tx_status = pl330_tx_status;
>   	pd->device_prep_slave_sg = pl330_prep_slave_sg;
>   	pd->device_config = pl330_config;
> -	pd->device_pause = pl330_pause;
>   	pd->device_terminate_all = pl330_terminate_all;
>   	pd->device_issue_pending = pl330_issue_pending;
>   	pd->src_addr_widths = PL330_DMA_BUSWIDTHS;
>
>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland