Hi Vladimir, thanks for the review!
> -----Original Message-----
> From: Vladimir Zapolskiy [mailto:vladimir_zapols...@mentor.com]
> Sent: 10 November 2014 15:10
> To: James Hartley; herb...@gondor.apana.org.au; da...@davemloft.net;
> grant.lik...@linaro.org; robh...@kernel.org; a...@linux-foundation.org;
> gre...@linuxfoundation.org; j...@perches.com;
> mche...@osg.samsung.com; cr...@iki.fi; jg1....@samsung.com; linux-
> cry...@vger.kernel.org
> Cc: devicet...@vger.kernel.org; pawel.m...@arm.com;
> mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk;
> ga...@codeaurora.org; abres...@chromium.org; Ezequiel Garcia
> Subject: Re: [PATCH 1/2] crypto: Add Imagination Technologies hw hash
> accelerator
>
> Hello James,
>
> On 10.11.2014 14:10, James Hartley wrote:
> > This adds support for the Imagination Technologies hash accelerator
> > that provides hardware acceleration for
> > SHA1 SHA224 SHA256 and MD5 Hashes.
> >
> > Signed-off-by: James Hartley <james.hart...@imgtec.com>
> > ---
>
> [snip]
>
> > diff --git a/drivers/crypto/img-hash.c b/drivers/crypto/img-hash.c new
> > file mode 100644 index 0000000..e58c81a
> > --- /dev/null
> > +++ b/drivers/crypto/img-hash.c
> > @@ -0,0 +1,1048 @@
> > +/*
> > +* Copyright (c) 2014 Imagination Technologies
> > +* Author: Will Thomas
> > +*
> > +* This program is free software; you can redistribute it and/or
> > +modify
> > +* it under the terms of the GNU General Public License version 2 as
> > +published
> > +* by the Free Software Foundation.
> > +*
> > +* Interface structure taken from omap-sham driver
> > +*/
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/clk.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +#include <linux/scatterlist.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/of_device.h>
> > +#include <crypto/sha.h>
> > +#include <crypto/md5.h>
> > +#include <crypto/internal/hash.h>
> > +
> > +#define MD5_BLOCK_SIZE 64
> > +
> > +#define CR_RESET 0
> > +#define CR_RESET_SET 1
> > +#define CR_RESET_UNSET 0
> > +
> > +#define CR_MESSAGE_LENGTH_H 0x4
> > +#define CR_MESSAGE_LENGTH_L 0x8
> > +
> > +#define CR_CONTROL 0xc
>
> Tab symbol instead of space after #define.
Ah ok - fixed.
>
> > +#define CR_CONTROL_BYTE_ORDER_3210 0
> > +#define CR_CONTROL_BYTE_ORDER_0123 1
> > +#define CR_CONTROL_BYTE_ORDER_2310 2
> > +#define CR_CONTROL_BYTE_ORDER_1032 3
> > +#define CR_CONTROL_ALGO_MD5 0
> > +#define CR_CONTROL_ALGO_SHA1 1
> > +#define CR_CONTROL_ALGO_SHA224 2
> > +#define CR_CONTROL_ALGO_SHA256 3
> > +
>
> [snip]
>
> > +static int img_hash_hw_init(struct img_hash_dev *hdev) {
> > + unsigned long long nbits;
> > + u32 u, l;
> > +
> > + clk_prepare_enable(hdev->iclk);
>
> This call may fail, please add a check.
Ok - check added
>
> > +
> > + img_hash_write(hdev, CR_RESET, CR_RESET_SET);
> > + img_hash_write(hdev, CR_RESET, CR_RESET_UNSET);
> > + img_hash_write(hdev, CR_INTENAB, CR_INT_NEW_RESULTS_SET);
> > +
> > + nbits = (hdev->req->nbytes << 3);
> > + u = nbits >> 32;
> > + l = nbits;
> > + img_hash_write(hdev, CR_MESSAGE_LENGTH_H, u);
> > + img_hash_write(hdev, CR_MESSAGE_LENGTH_L, l);
> > +
> > + if (!(DRIVER_FLAGS_INIT & hdev->flags)) {
> > + hdev->flags |= DRIVER_FLAGS_INIT;
> > + hdev->err = 0;
> > + }
> > + pr_debug("hw initialized, nbits: %llx\n", nbits);
> > + return 0;
> > +}
> > +
>
> [snip]
>
> > +static void img_hash_xmit_dma(struct img_hash_dev *hdev, struct
> > +scatterlist *sg) {
> > + struct dma_async_tx_descriptor *desc;
> > + struct img_hash_request_ctx *ctx = ahash_request_ctx(hdev->req);
> > +
> > + ctx->dma_ct = dma_map_sg(hdev->dev, sg, 1,
> DMA_MEM_TO_DEV);
> > + if (ctx->dma_ct == 0) {
> > + dev_err(hdev->dev, "Invalid DMA sg\n");
> > + hdev->err = -EINVAL;
> > + return;
> > + }
> > +
> > + desc = dmaengine_prep_slave_sg(hdev->dma_lch,
> > + sg,
> > + ctx->dma_ct,
> > + DMA_MEM_TO_DEV,
> > + DMA_PREP_INTERRUPT |
> DMA_CTRL_ACK);
> > + if (!desc) {
> > + dev_err(hdev->dev, "Null DMA descriptor\n");
> > + hdev->err = -EINVAL;
>
> Missing dma_unmap_sg()
Yes, good spot - now added.
>
> > + return;
> > + }
> > + desc->callback = img_hash_dma_callback;
> > + desc->callback_param = hdev;
> > + dmaengine_submit(desc);
> > + dma_async_issue_pending(hdev->dma_lch);
> > +}
> > +
> > +static void img_hash_dma_task(unsigned long d) {
> > + struct img_hash_dev *hdev = (struct img_hash_dev *) d;
> > + struct img_hash_request_ctx *ctx = ahash_request_ctx(hdev->req);
> > + char *addr;
> > + size_t nbytes, bleft, bsend, len, tbc;
> > + struct scatterlist tsg;
> > +
> > + if (!ctx->sg)
> > + return;
> > + if (!hdev)
> > + pr_err("invalid ptr for hash device");
> > +
> > + addr = sg_virt(ctx->sg);
> > + nbytes = ctx->sg->length - ctx->offset;
> > + bleft = nbytes % 4;
> > + bsend = (nbytes / 4);
> > +
> > +
> > + if (bsend) {
> > + sg_init_one(&tsg, addr + ctx->offset, bsend * 4);
> > + img_hash_xmit_dma(hdev, &tsg);
>
> What happens, if img_hash_xmit_dma() fails?
I don't think it fails very gracefully - I'm not sure at the moment what is the
best way to fix that - I'll have a think about what to do there.
>
> > + ctx->sent += bsend * 4;
> > + }
> > +
> > + if (bleft) {
> > + ctx->bufcnt = sg_pcopy_to_buffer(ctx->sgfirst, ctx->nents,
> > + ctx->buffer, bleft, ctx->sent);
> > + tbc = 0;
> > + ctx->sg = sg_next(ctx->sg);
> > + while (ctx->sg && (ctx->bufcnt < 4)) {
> > + len = ctx->sg->length;
> > + if (likely(len > (4 - ctx->bufcnt)))
> > + len = 4 - ctx->bufcnt;
> > + tbc = sg_pcopy_to_buffer(ctx->sgfirst, ctx->nents,
> > + ctx->buffer + ctx->bufcnt, len,
> > + ctx->sent + ctx->bufcnt);
> > + ctx->bufcnt += tbc;
> > + if (tbc >= ctx->sg->length) {
> > + ctx->sg = sg_next(ctx->sg);
> > + tbc = 0;
> > + }
> > + }
> > +
> > + ctx->sent += ctx->bufcnt;
> > + ctx->offset = tbc;
> > +
> > + if (!bsend)
> > + img_hash_dma_callback(hdev);
> > + } else {
> > + ctx->offset = 0;
> > + ctx->sg = sg_next(ctx->sg);
> > +
> > + }
> > +}
> > +
> > +static int img_hash_dma_init(struct img_hash_dev *hdev) {
> > + int err = -EINVAL;
> > +
> > + hdev->dma_lch = dma_request_slave_channel(hdev->dev, "tx");
> > + if (!hdev->dma_lch) {
> > + dev_err(hdev->dev, "Couldn't aquire a slave DMA
> channel.\n");
> > + return -EBUSY;
> > + }
> > + hdev->dma_conf.direction = DMA_MEM_TO_DEV;
> > + hdev->dma_conf.dst_addr = hdev->bus_addr;
> > + hdev->dma_conf.dst_addr_width =
> DMA_SLAVE_BUSWIDTH_4_BYTES;
> > + hdev->dma_conf.dst_maxburst = 16;
> > + hdev->dma_conf.device_fc = false;
> > +
> > + err = dmaengine_slave_config(hdev->dma_lch, &hdev->dma_conf);
> > + if (err) {
> > + dev_err(hdev->dev, "Couldn't configure DMA slave.\n");
> > + return err;
>
> Missing dma_release_channel(hdev->dma_lch);
Agreed - now added
>
> > + }
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id img_hash_match[] = {
> > + { .compatible = "img,img-hash-accelerator-rev1" },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, img_hash_match)
> > +
> > +static int img_hash_probe(struct platform_device *pdev) {
> > + struct img_hash_dev *hdev;
> > + struct device *dev = &pdev->dev;
> > + struct resource *hash_res;
> > + int err;
> > +
> > + hdev = devm_kzalloc(dev, sizeof(*hdev), GFP_KERNEL);
> > + if (hdev == NULL) {
> > + err = -ENOMEM;
> > + goto sha_dev_err;
> > + }
> > + spin_lock_init(&hdev->lock);
> > +
> > + hdev->dev = dev;
> > +
> > + platform_set_drvdata(pdev, hdev);
> > +
> > + INIT_LIST_HEAD(&hdev->list);
> > +
> > + tasklet_init(&hdev->done_task, img_hash_done_task,
> > + (unsigned long) hdev);
> > + tasklet_init(&hdev->dma_task, img_hash_dma_task,
> > + (unsigned long) hdev);
> > +
> > + crypto_init_queue(&hdev->queue, IMG_HASH_QUEUE_LENGTH);
> > +
> > + hash_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!hash_res) {
> > + dev_err(dev, "no MEM resource info\n");
> > + err = -ENODEV;
> > + goto res_err;
> > + }
> > +
> > + hdev->io_base = devm_ioremap_resource(dev, hash_res);
> > + if (!hdev->io_base) {
> > + dev_err(dev, "can't ioremap\n");
> > + err = -ENOMEM;
> > + goto hash_io_err;
> > + }
> > +
> > + hash_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + if (!hash_res) {
> > + dev_err(dev, "no MEM resource info\n");
> > + err = -ENODEV;
> > + goto res_err;
> > + }
> > + hdev->bus_addr = hash_res->start;
> > + hdev->cpu_addr = devm_ioremap_resource(dev, hash_res);
> > +
> > + hdev->irq = platform_get_irq(pdev, 0);
> > + if (hdev->irq < 0) {
> > + dev_err(dev, "no IRQ resource info\n");
> > + err = hdev->irq;
> > + goto res_err;
> > + }
> > +
> > + err = devm_request_irq(dev, hdev->irq, img_irq_handler, 0,
> > + "img-hash-accelerator-rev1", hdev);
> > + if (err) {
> > + dev_err(dev, "unable to request img-hash-accelerator
> irq\n");
> > + goto res_err;
> > + }
> > + dev_info(dev, "using IRQ channel %d\n", hdev->irq);
> > +
> > + hdev->iclk = devm_clk_get(&pdev->dev, "hash_clk");
> > + if (IS_ERR(hdev->iclk)) {
> > + dev_err(dev, "clock initialization failed.\n");
> > + err = PTR_ERR(hdev->iclk);
> > + goto clk_err;
>
> goto res_err is good enough, no need to introduce another label.
ok
>
> > + }
> > +
> > + err = img_hash_dma_init(hdev);
> > + if (err)
> > + goto err_dma;
> > +
> > + dev_info(dev, "using %s for DMA transfers\n",
> > + dma_chan_name(hdev->dma_lch));
> > +
> > + spin_lock(&img_hash.lock);
> > + list_add_tail(&hdev->list, &img_hash.dev_list);
> > + spin_unlock(&img_hash.lock);
> > +
> > + err = img_register_algs(hdev);
> > + if (err)
> > + goto err_algs;
> > + dev_info(dev, "data bus: 0x%x;\tsize:0x%x\n", hdev->bus_addr,
> 0x4);
> > + dev_info(dev, "Img MD5/SHA1/SHA224/SHA256 Hardware
> accelerator
> > +initialized\n");
> > +
> > + return 0;
> > +
> > +err_algs:
> > + spin_lock(&img_hash.lock);
> > + list_del(&hdev->list);
> > + spin_unlock(&img_hash.lock);
> > + dma_release_channel(hdev->dma_lch);
> > +err_dma:
> > + iounmap(hdev->io_base);
>
> Mixing of devm_* resource initialization and commodity resource release
> leads to double decrement of clock usage count reference.
Ok, changed to devm_iounmap
>
> > +hash_io_err:
> > + clk_put(hdev->iclk);
>
> Same as above.
Done
>
> > +clk_err:
> > +res_err:
> > + tasklet_kill(&hdev->done_task);
>
> What is about killing &hdev->dma_task?
Yes, that should have been done - added.
>
> > +sha_dev_err:
> > + dev_err(dev, "initialization failed.\n");
> > + return err;
> > +}
> > +
> > +static void img_hash_dma_cleanup(struct img_hash_dev *hdev) {
> > + dma_release_channel(hdev->dma_lch);
> > +}
>
> The function is used only once in img_hash_remove(), probably it can be
> removed.
Agreed - done.
>
> > +
> > +static int img_hash_remove(struct platform_device *pdev) {
> > + static struct img_hash_dev *hdev;
> > +
> > + hdev = platform_get_drvdata(pdev);
> > + if (!hdev)
> > + return -ENODEV;
> > + spin_lock(&img_hash.lock);
> > + list_del(&hdev->list);
> > + spin_unlock(&img_hash.lock);
> > +
> > + img_unregister_algs(hdev);
> > +
> > + tasklet_kill(&hdev->done_task);
> > + tasklet_kill(&hdev->dma_task);
> > + img_hash_dma_cleanup(hdev);
> > +
> > + iounmap(hdev->io_base);
>
> Same as above, devres iounmap() is good enough.
Done
>
> > + return 0;
> > +}
> > +
> > +static struct platform_driver img_hash_driver = {
> > + .probe = img_hash_probe,
> > + .remove = img_hash_remove,
> > + .driver = {
> > + .name = "img-hash-accelerator-rev1",
> > + .of_match_table = of_match_ptr(img_hash_match),
> > + }
> > +};
> > +module_platform_driver(img_hash_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Imgtec SHA1/224/256 & MD5 hw accelerator
> > +driver"); MODULE_AUTHOR("Will Thomas.");
> > +
> >
>
> Also please check the whole patch for DOS line endings.
Fixed.
>
> --
> With best wishes,
> Vladimir
Thanks,
James
