linux-kernel - Re: [RFC PATCH] dma: pl330: Change pl330_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: <52A5B0FA.901@metafoo.de>
Date:	Mon, 09 Dec 2013 13:00:58 +0100
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	Lukasz Czerwinski <l.czerwinski@...sung.com>
CC:	djbw@...com, linux-kernel@...r.kernel.org, s.nawrocki@...sung.com,
	m.szyprowski@...sung.com
Subject: Re: [RFC PATCH] dma: pl330: Change pl330_tx_status()

On 12/09/2013 12:19 PM, Lukasz Czerwinski wrote:
> This patch adds possibility to read status of unfinished transfers
> before termination.
> 
> Signed-off-by: Lukasz Czerwinski <l.czerwinski@...sung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@...sung.com>
> ---
> 
> I tested patch with Exynos4 board. This is only proof of concept.
> Any comments are welcome.

The patch does not look as if it is going to work correctly. And I think
that this can't be implemented correctly with the current structure of the
driver (at least not without jumping through a couple of hoops). I do have a
couple of WIP patches that restructure the driver and then implement residue
reporting on top of it.

> 
> Thanks
> Lukasz
> 
>  drivers/dma/pl330.c |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 24f8ae3..c88c36e 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -566,6 +566,7 @@ struct dma_pl330_chan {
>  	/* For D-to-M and M-to-D channels */
>  	int burst_sz; /* the peripheral fifo width */
>  	int burst_len; /* the number of burst */
> +	int transfered;

This is always set to 0.

>  	dma_addr_t fifo_addr;
>  
>  	/* for cyclic capability */
> @@ -602,6 +603,9 @@ struct dma_pl330_desc {
>  
>  	enum desc_status status;
>  
> +	int bytes_requested;
> +	int direction;
> +
>  	/* The channel which currently holds this desc */
>  	struct dma_pl330_chan *pchan;
>  };
> @@ -2366,6 +2370,29 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan)
>  	return 1;
>  }
>  
> +int pl330_get_current_xferred_count(struct dma_pl330_chan *pch,
> +		struct dma_pl330_desc *desc)
> +{
> +	u32 val, addr;
> +	struct pl330_thread *thrd = pch->pl330_chid;
> +	void __iomem *regs = thrd->dmac->pinfo->base;
> +
> +	val = addr = 0;
> +	switch (desc->direction) {
> +	case DMA_MEM_TO_DEV:
> +		val = readl(regs + SA(thrd->id));
> +		addr = desc->px.src_addr;
> +		break;
> +	case DMA_DEV_TO_MEM:
> +		val = readl(regs + DA(thrd->id));
> +		addr = desc->px.dst_addr;
> +		break;
> +	default:
> +		break;
> +	}
> +	return val - addr;
> +}
> +
>  static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned long arg)
>  {
>  	struct dma_pl330_chan *pch = to_pchan(chan);
> @@ -2446,7 +2473,33 @@ static enum dma_status
>  pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
>  		 struct dma_tx_state *txstate)
>  {
> -	return dma_cookie_status(chan, cookie, txstate);
> +	enum dma_status ret;
> +	unsigned long flags;
> +	struct dma_pl330_desc *desc;
> +	struct dma_pl330_chan *pch = to_pchan(chan);
> +	unsigned int bytes_transferred;
> +	unsigned int residual;
> +
> +	/* Check in pending list */
> +	spin_lock_irqsave(&pch->lock, flags);
> +	list_for_each_entry(desc, &pch->work_list, node) {
> +		if (desc->txd.cookie == cookie) {
> +			bytes_transferred =
> +				pl330_get_current_xferred_count(pch, desc);

pl330_get_current_xferred_count() will only return the correct result if it
is called for the currently active transfer.

And then there is also the issue that the driver creates multiple internal
descriptors for each segment in the transfer. Each of these descriptors gets
different cookie. pl330_tx_submit() returns the cookie to the descriptor to
the first segment in the transfer. This means if you pass that cookie to
pl330_tx_status() you'll only get the residue for the first segment and not
for the whole transfer, as it is supposed to be.

> +			residual =  desc->bytes_requested -
> +				bytes_transferred % desc->bytes_requested;

If bytes_transferred % desc->bytes_requested != bytes_transferred there is a
bug somewhere.

> +			dma_set_residue(txstate, residual);
> +			ret = desc->status;
> +			spin_unlock_irqrestore(&pch->lock, flags);
> +			return ret;
> +		}
> +	}
> +	spin_unlock_irqrestore(&pch->lock, flags);
> +
> +	ret = dma_cookie_status(chan, cookie, txstate);
> +	dma_set_residue(txstate,  pch->transfered);
> +
> +	return ret;
>  }
>  
[...]

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/