Re: [PATCH 3/9] crypto: qce: Add dma and sg helpers

[Courtney Cavin <courtney.cavin@sonymobile.com>]

On Thu, Apr 03, 2014 at 06:18:00PM +0200, Stanimir Varbanov wrote:
> This adds dmaengine and sg-list helper functions used by
> other parts of the crypto driver.
> 
> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> ---
>  drivers/crypto/qce/dma.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/crypto/qce/dma.h |  57 ++++++++++++++
>  2 files changed, 258 insertions(+)
>  create mode 100644 drivers/crypto/qce/dma.c
>  create mode 100644 drivers/crypto/qce/dma.h

More nitpicking, because everyone loves that....

> 
> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> new file mode 100644
> index 000000000000..1bad958db2f9
> --- /dev/null
> +++ b/drivers/crypto/qce/dma.c
> @@ -0,0 +1,201 @@
[...]
> +int qce_dma_request(struct device *dev, struct qce_dma_data *dma)
> +{
> +	unsigned int memsize;
> +	void *va;
> +	int ret;
> +
> +	dma->txchan = dma_request_slave_channel_reason(dev, "tx");
> +	if (IS_ERR(dma->txchan)) {
> +		ret = PTR_ERR(dma->txchan);
> +		return ret;

You can just return PTR_ERR(dma->txchan) here, no need to set 'ret'.

> +	}
> +
> +	dma->rxchan = dma_request_slave_channel_reason(dev, "rx");
> +	if (IS_ERR(dma->rxchan)) {
> +		ret = PTR_ERR(dma->rxchan);
> +		goto error_rx;
> +	}
> +
> +	memsize = QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ;
> +	va = kzalloc(memsize, GFP_KERNEL);

'memsize' is only used here.  Just pass 'QCE_RESULT_BUF_SZ +
QCE_IGNORE_BUF_SZ' directly to kzalloc().

Additionally, is there some particular reason why we need to zero this
memory?

> +	if (!va) {
> +		ret = -ENOMEM;
> +		goto error_nomem;
> +	}
> +
> +	dma->result_buf = va;

Is there some requirement that we don't set dma->result_buf on error?
If not, we can discard the 'va' variable as well.

> +	dma->ignore_buf = dma->result_buf + QCE_RESULT_BUF_SZ;
> +
> +	return 0;
> +error_nomem:
> +	if (!IS_ERR(dma->rxchan))
> +		dma_release_channel(dma->rxchan);
> +error_rx:
> +	if (!IS_ERR(dma->txchan))
> +		dma_release_channel(dma->txchan);
> +	return ret;
> +}
> +
> +void qce_dma_release(struct qce_dma_data *dma)
> +{
> +	dma_release_channel(dma->txchan);
> +	dma_release_channel(dma->rxchan);
> +	kfree(dma->result_buf);
> +}
> +
> +int qce_mapsg(struct device *dev, struct scatterlist *sg, unsigned int nents,
> +	      enum dma_data_direction dir, bool chained)
> +{
> +	int err;
> +
> +	if (chained) {
> +		while (sg) {
> +			err = dma_map_sg(dev, sg, 1, dir);
> +			if (!err)
> +				goto error;
> +			sg = scatterwalk_sg_next(sg);
> +		}
> +	} else {
> +		err = dma_map_sg(dev, sg, nents, dir);
> +		if (!err)
> +			goto error;
> +	}
> +
> +	return nents;
> +error:
> +	return -EFAULT;

No need for this label, as there's no cleanup.  Just return
-EFAULT directly on error.

> +}
[...]
> +struct scatterlist *
> +qce_sgtable_add(struct sg_table *sgt, struct scatterlist *new_sgl)
> +{
> +	struct scatterlist *sg = sgt->sgl, *sg_last = NULL;
> +
> +	while (sg) {
> +		if (!sg_page(sg))
> +			break;
> +		sg = sg_next(sg);
> +	}
> +
> +	if (!sg)
> +		goto error;
> +
> +	while (new_sgl && sg) {
> +		sg_set_page(sg, sg_page(new_sgl), new_sgl->length,
> +			    new_sgl->offset);
> +		sg_last = sg;
> +		sg = sg_next(sg);
> +		new_sgl = sg_next(new_sgl);
> +	}
> +
> +	if (new_sgl)
> +		goto error;
> +
> +	return sg_last;
> +error:
> +	return ERR_PTR(-EINVAL);

No need for this label, as there's no cleanup.  Just return
ERR_PTR(-EINVAL) directly on error.

> +}
> +
> +static int qce_dma_prep_sg(struct dma_chan *chan, struct scatterlist *sg,
> +			   int nents, unsigned long flags,
> +			   enum dma_transfer_direction dir,
> +			   dma_async_tx_callback cb, void *cb_param)
> +{
> +	struct dma_async_tx_descriptor *desc;
> +
> +	if (!sg || !nents)
> +		return -EINVAL;
> +
> +	desc = dmaengine_prep_slave_sg(chan, sg, nents, dir, flags);
> +	if (!desc)
> +		return -EINVAL;
> +
> +	desc->callback = cb;
> +	desc->callback_param = cb_param;
> +	dmaengine_submit(desc);

Do we not care if there is an error here?

dma_cookie_t cookie;
...
cookie = dmaengine_submit(desc);
return dma_submit_error(cookie);

> +	return 0;
> +}
[...]
> diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h
> new file mode 100644
> index 000000000000..932b02fd8f25
> --- /dev/null
> +++ b/drivers/crypto/qce/dma.h
> @@ -0,0 +1,57 @@
> +/*
> + * Copyright (c) 2011-2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _DMA_H_
> +#define _DMA_H_
> +
> +#define QCE_AUTHIV_REGS_CNT		16
> +#define QCE_AUTH_BYTECOUNT_REGS_CNT	4
> +#define QCE_CNTRIV_REGS_CNT		4
> +
> +/* result dump format */
> +struct qce_result_dump {
> +	u32 auth_iv[QCE_AUTHIV_REGS_CNT];
> +	u32 auth_byte_count[QCE_AUTH_BYTECOUNT_REGS_CNT];
> +	u32 encr_cntr_iv[QCE_CNTRIV_REGS_CNT];
> +	u32 status;
> +	u32 status2;
> +};
> +
> +#define QCE_IGNORE_BUF_SZ	(2 * QCE_BAM_BURST_SIZE)

QCE_BAM_BURST_SIZE is defined in common.h in 6/9.  Either that file
needs to be included from this one, or the definition needs to be moved.