Hi Courtney,
Thanks for the comments!
On 04/04/2014 02:15 AM, Courtney Cavin wrote:
> 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 <svarba...@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'.
I think the idea was to have only one exit point from the function,
never mind I will return from here.
>
>> + }
>> +
>> + 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().
>
The only reason to have "memsize" variable is to avoid carry over the
new line. The compiler is smart enough to delete this variable. But this
is a minor issue and I tend to agree with you.
> Additionally, is there some particular reason why we need to zero this
> memory?
IMO, it makes sense when debugging some issue related to dma to 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.
No, there is no such requirement, the error is fatal and driver will
refuse to .probe.
>
>> + 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.
Sure, will do.
>
>> +}
> [...]
>> +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.
Sure, will fix it.
>
>> +}
>> +
>> +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);
>
Good catch, we should care. Will check the error. Thanks.
>> + 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.
I decided to not include any files in driver private headers. Thus I
include the private header files in relevant c files in order.
--
regards,
Stan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
