Hi Vladimir, thanks for the review! > -----Original Message----- > From: Vladimir Zapolskiy [mailto:vladimir_zapolskiy@xxxxxxxxxx] > Sent: 10 November 2014 15:10 > To: James Hartley; herbert@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; > grant.likely@xxxxxxxxxx; robh+dt@xxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx; > gregkh@xxxxxxxxxxxxxxxxxxx; joe@xxxxxxxxxxx; > mchehab@xxxxxxxxxxxxxxx; crope@xxxxxx; jg1.han@xxxxxxxxxxx; linux- > crypto@xxxxxxxxxxxxxxx > Cc: devicetree@xxxxxxxxxxxxxxx; pawel.moll@xxxxxxx; > mark.rutland@xxxxxxx; ijc+devicetree@xxxxxxxxxxxxxx; > galak@xxxxxxxxxxxxxx; abrestic@xxxxxxxxxxxx; 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.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. 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 ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
