RE: [uclinux-dist-devel] [PATCH 2/2] crypto: bfin_crc: CRC hardware accelerator driver for BF60x family processors.

-----Original Message-----
From: uclinux-dist-devel-bounces@blackfin.uclinux.org [mailto:uclinux-dist-devel-
bounces@blackfin.uclinux.org] On Behalf Of Mike Frysinger
Sent: Saturday, June 02, 2012 3:19 PM
To: uclinux-dist-devel@blackfin.uclinux.org
Cc: Herbert Xu; Sonic Zhang; David S. Miller; LKML; linux-crypto@vger.kernel.org
Subject: Re: [uclinux-dist-devel] [PATCH 2/2] crypto: bfin_crc: CRC hardware
accelerator driver for BF60x family processors.

On Friday 25 May 2012 05:54:14 Sonic Zhang wrote:

quoted

--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig

+config CRYPTO_DEV_BFIN_CRC
+    tristate "Support for Blackfin CRC hareware accelerator"

hardware

quoted

+    depends on BF60x
+    help
+      Blackfin processors have CRC hardware accelerator.

Newer Blackfin processors have a CRC hardware accelerator.

quoted

--- /dev/null
+++ b/drivers/crypto/bfin_crc.c

+struct bfin_crypto_crc_list {
+    struct list_head        dev_list;
+    spinlock_t              lock;
+} crc_list;

static

quoted

+    if (sg_count(req->src) > CRC_MAX_DMA_DESC) {
+            dev_dbg(crc->dev, "init: requested sg list is too big > %d\n",
+                    CRC_MAX_DMA_DESC);
+            return -EINVAL;
+    }

do you need to do this ?  considering the crc is stream based, why can't you fill up
as many as possible, wait for it to finish, and then send more data ?

No, the async hash crypto API should not wait for the DMA to complete.

quoted

+    /* init crc results */
+    *(__le32 *)req->result =
+            cpu_to_le32p(&crc_ctx->key);

right handed casts are generally frowned upon if not banned.  plus, result is a u8*,
so it seems like a pretty bad idea to do this on a Blackfin (which doesn't allow
unaligned accesses).  isn't this what you want:
      put_unaligned_le32(crc_ctx->key, req->result);

The result element in structure ahash_request is always 4-byte aligned. So, put_unaligned_le32 is not necessary.
Of course, it is fine as well.

quoted

+            /* chop current sg dma len to multiply of 32 bits */

"multiply" -> "multiple"

quoted

+            dma_count = (dma_count >> 2) << 2;

dma_count &= ~0x3;

quoted

+    if (i == 0)
+            return ;

no space before the ;

quoted

+#define MIN(x,y) ((x) < (y) ? x : y)

linux/kernel.h already provides min() macros for you

quoted

+            /* Pack small data which is less than 32bit to buffer for next

update.*/

needs a space after the period

quoted

+            /* punch crc buffer size to multiply of 32 bit */

i think you mean "chop" rather than "punch", and it should be "multiple"
rather than "multiply"

quoted

+            ctx->sg_buflen = (ctx->sg_buflen >> 2) << 2;

ctx->sg_buflen &= ~0x3;

quoted

+                    memset(ctx->bufnext, 0, CHKSUM_DIGEST_SIZE);

use a space after the , not a tab

quoted

+                    nextlen = ctx->bufnext_len;
+                    for (i = nsg - 1; i >= 0; i--) {
+                            sg = sg_get(ctx->sg, nsg, i);

this is the only place you use sg_get(), and it looks like it's extremely
inefficient.  i guess it's not possible to re-order the pointer walking though.

quoted

+static int bfin_crypto_crc_setkey(struct crypto_ahash *tfm, const u8 *key,
+                  unsigned int keylen)
+{
+    struct bfin_crypto_crc_ctx *crc_ctx = crypto_ahash_ctx(tfm);
+
+    dev_dbg(crc_ctx->crc->dev, "crc_setkey\n");
+        if (keylen != CHKSUM_DIGEST_SIZE) {
+                crypto_ahash_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
+                return -EINVAL;
+        }

indentation seems to be off here.  suggest you run this through checkpatch.pl.

quoted

+    crc_ctx->key = le32_to_cpu(*(__le32 *)key);

shouldn't this be get_unaligned_le32 ?

quoted

+            /* prepare results */
+            *(__le32 *)crc->req->result =
+                    cpu_to_le32p((u32 *)&crc->regs->result);

put_unaligned_le32()

quoted

+static int bfin_crypto_crc_suspend(struct platform_device *pdev,
pm_message_t state) +{
+    struct bfin_crypto_crc *crc = platform_get_drvdata(pdev);
+    int i = 100000;
+
+    while ((crc->regs->control & BLKEN) && --i)
+            cpu_relax();
+
+    if (i == 0)
+            crc->regs->control &= ~BLKEN;
+
+    return 0;
+}

should this return -EBUSY instead of clearing BLKEN ?

quoted

+static int bfin_crypto_crc_resume(struct platform_device *pdev)
+{
+    return 0;
+}

if there's nothing to resume, do you need to provide your own func ?

quoted

+static int __devinit bfin_crypto_crc_probe(struct platform_device *pdev)
+{
+    struct device *dev = &pdev->dev;
+    struct resource *res;
+    struct bfin_crypto_crc *crc = NULL;

not much point in setting this to NULL considering the first thing you do is
allocate it

quoted

+    ret = request_irq(crc->irq, bfin_crypto_crc_handler, IRQF_SHARED,
DRIVER_NAME, crc);

you could use dev_name() rather than DRIVER_NAME

quoted

+    ret = request_dma(crc->dma_ch, DRIVER_NAME);

same here

quoted

+    /* need at most CRC_MAX_DMA_DESC sg + CRC_MAX_DMA_DESC

middle  +

quoted

+       1 last + 1 next dma descriptors
+     */

multiline comments look like:
      /*
       * foo
       * bar
       */

quoted

+    while (!(crc->regs->status & LUTDONE) && (--timeout) > 0)
+            cpu_relax();

no need for paren around the timeout decrement, and this should be an actual
timer based timeout rather than some big integer.  pretty easy to do with
wait_for_completion_timeout().

Wait_for_completion_timeout() depends on an LUTDONE interrupt to wake up the probing task. But, there is no such interrupt available in bfin CRC device.
May be call yield() other than cpu_relax() is what you expected?

quoted

+static int __devexit bfin_crypto_crc_remove(struct platform_device *pdev)
+{
+    struct bfin_crypto_crc *crc = platform_get_drvdata(pdev);
+
+    if (!crc)
+            return -ENODEV;

is this even possible ?

In case the platform drvdata is corrupted.

quoted

+static int __init bfin_crypto_crc_mod_init(void)
+{
+    int ret;
+
+    pr_info("Blackfin hardware CRC crypto driver\n");
+
+    INIT_LIST_HEAD(&crc_list.dev_list);
+    spin_lock_init(&crc_list.lock);
+
+    ret = platform_driver_register(&bfin_crypto_crc_driver);
+    if (ret) {
+            pr_info(KERN_ERR "unable to register driver\n");
+            return ret;
+    }
+
+    return 0;
+}

if you declare the list/spin_lock separately and not together in a structure,

It is clean and better to keep the spin lock in structure crc_list.


Sonic

you could statically initialize them (rather than needing to do it at
runtime), and then you could drop the init/exit functions here and replace
them with a single module_platform_driver(bfin_crypto_crc_driver).
-mike