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.hartley@xxxxxxxxxx>
> ---
[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.
> +#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.
> +
> + 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()
> + 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?
> + 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);
> + }
> + 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.
> + }
> +
> + 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.
> +hash_io_err:
> + clk_put(hdev->iclk);
Same as above.
> +clk_err:
> +res_err:
> + tasklet_kill(&hdev->done_task);
What is about killing &hdev->dma_task?
> +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.
> +
> +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.
> + 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.
--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
