Re: [PATCH v2 04/14] crypto: omap-aes: Simplify DMA usage by usingdirect SGs

[Lokesh Vutla]

Hi Joel,

On Sunday 18 August 2013 08:12 AM, Joel Fernandes wrote:
> In early version of this driver, assumptions were made such as DMA layer
> requires contiguous buffers etc. Due to this, new buffers were allocated,
> mapped and used for DMA. These assumptions are no longer true and DMAEngine
> scatter-gather DMA doesn't have such requirements. We simply the DMA operations
> by directly using the scatter-gather buffers provided by the crypto layer
> instead of creating our own.
> 
> Lot of logic that handled DMA'ing only X number of bytes of the total, or as
> much as fitted into a 3rd party buffer is removed and is no longer required.
> 
> Also, good performance improvement of atleast ~20% seen with encrypting a
> buffer size of 8K (1800 ops/sec vs 1400 ops/sec).  Improvement will be higher
> for much larger blocks though such benchmarking is left as an exercise for the
> reader.  Also DMA usage is much more simplified and coherent with rest of the
> code.
> 
> Signed-off-by: Joel Fernandes <joelf@xxxxxx>
> ---
>  drivers/crypto/omap-aes.c |  147 ++++++++-------------------------------------
>  1 file changed, 25 insertions(+), 122 deletions(-)
> 
> diff --git a/drivers/crypto/omap-aes.c b/drivers/crypto/omap-aes.c
> index e369e6e..64dd5c1 100644
> --- a/drivers/crypto/omap-aes.c
> +++ b/drivers/crypto/omap-aes.c
> @@ -480,22 +480,14 @@ static int sg_copy(struct scatterlist **sg, size_t *offset, void *buf,
>  }
>  
>  static int omap_aes_crypt_dma(struct crypto_tfm *tfm,
> -		struct scatterlist *in_sg, struct scatterlist *out_sg)
> +		struct scatterlist *in_sg, struct scatterlist *out_sg,
> +		int in_sg_len, int out_sg_len)
>  {
>  	struct omap_aes_ctx *ctx = crypto_tfm_ctx(tfm);
>  	struct omap_aes_dev *dd = ctx->dd;
>  	struct dma_async_tx_descriptor *tx_in, *tx_out;
>  	struct dma_slave_config cfg;
> -	dma_addr_t dma_addr_in = sg_dma_address(in_sg);
> -	int ret, length = sg_dma_len(in_sg);
> -
> -	pr_debug("len: %d\n", length);
> -
> -	dd->dma_size = length;
> -
> -	if (!(dd->flags & FLAGS_FAST))
> -		dma_sync_single_for_device(dd->dev, dma_addr_in, length,
> -					   DMA_TO_DEVICE);
> +	int ret;
By this change FLAGS_FAST is unsed, it can be cleaned right?
or Am I missing something?

Thanks and regards,
Lokesh
>  
>  	memset(&cfg, 0, sizeof(cfg));
>  
> @@ -514,7 +506,7 @@ static int omap_aes_crypt_dma(struct crypto_tfm *tfm,
>  		return ret;
>  	}
>  
> -	tx_in = dmaengine_prep_slave_sg(dd->dma_lch_in, in_sg, 1,
> +	tx_in = dmaengine_prep_slave_sg(dd->dma_lch_in, in_sg, in_sg_len,
>  					DMA_MEM_TO_DEV,
>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>  	if (!tx_in) {
> @@ -533,7 +525,7 @@ static int omap_aes_crypt_dma(struct crypto_tfm *tfm,
>  		return ret;
>  	}
>  
> -	tx_out = dmaengine_prep_slave_sg(dd->dma_lch_out, out_sg, 1,
> +	tx_out = dmaengine_prep_slave_sg(dd->dma_lch_out, out_sg, out_sg_len,
>  					DMA_DEV_TO_MEM,
>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>  	if (!tx_out) {
> @@ -551,7 +543,7 @@ static int omap_aes_crypt_dma(struct crypto_tfm *tfm,
>  	dma_async_issue_pending(dd->dma_lch_out);
>  
>  	/* start DMA */
> -	dd->pdata->trigger(dd, length);
> +	dd->pdata->trigger(dd, dd->total);
>  
>  	return 0;
>  }
> @@ -560,93 +552,28 @@ static int omap_aes_crypt_dma_start(struct omap_aes_dev *dd)
>  {
>  	struct crypto_tfm *tfm = crypto_ablkcipher_tfm(
>  					crypto_ablkcipher_reqtfm(dd->req));
> -	int err, fast = 0, in, out;
> -	size_t count;
> -	dma_addr_t addr_in, addr_out;
> -	struct scatterlist *in_sg, *out_sg;
> -	int len32;
> +	int err;
>  
>  	pr_debug("total: %d\n", dd->total);
>  
> -	if (sg_is_last(dd->in_sg) && sg_is_last(dd->out_sg)) {
> -		/* check for alignment */
> -		in = IS_ALIGNED((u32)dd->in_sg->offset, sizeof(u32));
> -		out = IS_ALIGNED((u32)dd->out_sg->offset, sizeof(u32));
> -
> -		fast = in && out;
> +	err = dma_map_sg(dd->dev, dd->in_sg, dd->in_sg_len, DMA_TO_DEVICE);
> +	if (!err) {
> +		dev_err(dd->dev, "dma_map_sg() error\n");
> +		return -EINVAL;
>  	}
>  
> -	if (fast)  {
> -		count = min(dd->total, sg_dma_len(dd->in_sg));
> -		count = min(count, sg_dma_len(dd->out_sg));
> -
> -		if (count != dd->total) {
> -			pr_err("request length != buffer length\n");
> -			return -EINVAL;
> -		}
> -
> -		pr_debug("fast\n");
> -
> -		err = dma_map_sg(dd->dev, dd->in_sg, 1, DMA_TO_DEVICE);
> -		if (!err) {
> -			dev_err(dd->dev, "dma_map_sg() error\n");
> -			return -EINVAL;
> -		}
> -
> -		err = dma_map_sg(dd->dev, dd->out_sg, 1, DMA_FROM_DEVICE);
> -		if (!err) {
> -			dev_err(dd->dev, "dma_map_sg() error\n");
> -			dma_unmap_sg(dd->dev, dd->in_sg, 1, DMA_TO_DEVICE);
> -			return -EINVAL;
> -		}
> -
> -		addr_in = sg_dma_address(dd->in_sg);
> -		addr_out = sg_dma_address(dd->out_sg);
> -
> -		in_sg = dd->in_sg;
> -		out_sg = dd->out_sg;
> -
> -		dd->flags |= FLAGS_FAST;
> -
> -	} else {
> -		/* use cache buffers */
> -		count = sg_copy(&dd->in_sg, &dd->in_offset, dd->buf_in,
> -				 dd->buflen, dd->total, 0);
> -
> -		len32 = DIV_ROUND_UP(count, DMA_MIN) * DMA_MIN;
> -
> -		/*
> -		 * The data going into the AES module has been copied
> -		 * to a local buffer and the data coming out will go
> -		 * into a local buffer so set up local SG entries for
> -		 * both.
> -		 */
> -		sg_init_table(&dd->in_sgl, 1);
> -		dd->in_sgl.offset = dd->in_offset;
> -		sg_dma_len(&dd->in_sgl) = len32;
> -		sg_dma_address(&dd->in_sgl) = dd->dma_addr_in;
> -
> -		sg_init_table(&dd->out_sgl, 1);
> -		dd->out_sgl.offset = dd->out_offset;
> -		sg_dma_len(&dd->out_sgl) = len32;
> -		sg_dma_address(&dd->out_sgl) = dd->dma_addr_out;
> -
> -		in_sg = &dd->in_sgl;
> -		out_sg = &dd->out_sgl;
> -
> -		addr_in = dd->dma_addr_in;
> -		addr_out = dd->dma_addr_out;
> -
> -		dd->flags &= ~FLAGS_FAST;
> -
> +	err = dma_map_sg(dd->dev, dd->out_sg, dd->out_sg_len, DMA_FROM_DEVICE);
> +	if (!err) {
> +		dev_err(dd->dev, "dma_map_sg() error\n");
> +		return -EINVAL;
>  	}
>  
> -	dd->total -= count;
> -
> -	err = omap_aes_crypt_dma(tfm, in_sg, out_sg);
> +	err = omap_aes_crypt_dma(tfm, dd->in_sg, dd->out_sg, dd->in_sg_len,
> +				 dd->out_sg_len);
>  	if (err) {
> -		dma_unmap_sg(dd->dev, dd->in_sg, 1, DMA_TO_DEVICE);
> -		dma_unmap_sg(dd->dev, dd->out_sg, 1, DMA_TO_DEVICE);
> +		dma_unmap_sg(dd->dev, dd->in_sg, dd->in_sg_len, DMA_TO_DEVICE);
> +		dma_unmap_sg(dd->dev, dd->out_sg, dd->out_sg_len,
> +			     DMA_FROM_DEVICE);
>  	}
>  
>  	return err;
> @@ -667,7 +594,6 @@ static void omap_aes_finish_req(struct omap_aes_dev *dd, int err)
>  static int omap_aes_crypt_dma_stop(struct omap_aes_dev *dd)
>  {
>  	int err = 0;
> -	size_t count;
>  
>  	pr_debug("total: %d\n", dd->total);
>  
> @@ -676,21 +602,8 @@ static int omap_aes_crypt_dma_stop(struct omap_aes_dev *dd)
>  	dmaengine_terminate_all(dd->dma_lch_in);
>  	dmaengine_terminate_all(dd->dma_lch_out);
>  
> -	if (dd->flags & FLAGS_FAST) {
> -		dma_unmap_sg(dd->dev, dd->out_sg, 1, DMA_FROM_DEVICE);
> -		dma_unmap_sg(dd->dev, dd->in_sg, 1, DMA_TO_DEVICE);
> -	} else {
> -		dma_sync_single_for_device(dd->dev, dd->dma_addr_out,
> -					   dd->dma_size, DMA_FROM_DEVICE);
> -
> -		/* copy data */
> -		count = sg_copy(&dd->out_sg, &dd->out_offset, dd->buf_out,
> -				 dd->buflen, dd->dma_size, 1);
> -		if (count != dd->dma_size) {
> -			err = -EINVAL;
> -			pr_err("not all data converted: %u\n", count);
> -		}
> -	}
> +	dma_unmap_sg(dd->dev, dd->in_sg, dd->in_sg_len, DMA_TO_DEVICE);
> +	dma_unmap_sg(dd->dev, dd->out_sg, dd->out_sg_len, DMA_FROM_DEVICE);
>  
>  	return err;
>  }
> @@ -760,21 +673,11 @@ static int omap_aes_handle_queue(struct omap_aes_dev *dd,
>  static void omap_aes_done_task(unsigned long data)
>  {
>  	struct omap_aes_dev *dd = (struct omap_aes_dev *)data;
> -	int err;
> -
> -	pr_debug("enter\n");
>  
> -	err = omap_aes_crypt_dma_stop(dd);
> -
> -	err = dd->err ? : err;
> -
> -	if (dd->total && !err) {
> -		err = omap_aes_crypt_dma_start(dd);
> -		if (!err)
> -			return; /* DMA started. Not fininishing. */
> -	}
> +	pr_debug("enter done_task\n");
>  
> -	omap_aes_finish_req(dd, err);
> +	omap_aes_crypt_dma_stop(dd);
> +	omap_aes_finish_req(dd, 0);
>  	omap_aes_handle_queue(dd, NULL);
>  
>  	pr_debug("exit\n");
> 

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