>-----Original Message----- >From: uclinux-dist-devel-bounces@xxxxxxxxxxxxxxxxxxxx [mailto:uclinux-dist-devel- >bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of Mike Frysinger >Sent: Saturday, June 02, 2012 3:19 PM >To: uclinux-dist-devel@xxxxxxxxxxxxxxxxxxxx >Cc: Herbert Xu; Sonic Zhang; David S. Miller; LKML; linux-crypto@xxxxxxxxxxxxxxx >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: >> --- a/drivers/crypto/Kconfig >> +++ b/drivers/crypto/Kconfig >> >> +config CRYPTO_DEV_BFIN_CRC >> + tristate "Support for Blackfin CRC hareware accelerator" > >hardware > >> + depends on BF60x >> + help >> + Blackfin processors have CRC hardware accelerator. > >Newer Blackfin processors have a CRC hardware accelerator. > >> --- /dev/null >> +++ b/drivers/crypto/bfin_crc.c >> >> +struct bfin_crypto_crc_list { >> + struct list_head dev_list; >> + spinlock_t lock; >> +} crc_list; > >static > >> + 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. >> + /* 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. > >> + /* chop current sg dma len to multiply of 32 bits */ > >"multiply" -> "multiple" > >> + dma_count = (dma_count >> 2) << 2; > >dma_count &= ~0x3; > >> + if (i == 0) >> + return ; > >no space before the ; > >> +#define MIN(x,y) ((x) < (y) ? x : y) > >linux/kernel.h already provides min() macros for you > >> + /* Pack small data which is less than 32bit to buffer for next >update.*/ > >needs a space after the period > >> + /* 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" > >> + ctx->sg_buflen = (ctx->sg_buflen >> 2) << 2; > >ctx->sg_buflen &= ~0x3; > >> + memset(ctx->bufnext, 0, CHKSUM_DIGEST_SIZE); > >use a space after the , not a tab > >> + 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. > >> +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. > >> + crc_ctx->key = le32_to_cpu(*(__le32 *)key); > >shouldn't this be get_unaligned_le32 ? > >> + /* prepare results */ >> + *(__le32 *)crc->req->result = >> + cpu_to_le32p((u32 *)&crc->regs->result); > >put_unaligned_le32() > >> +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 ? > >> +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 ? > >> +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 > >> + ret = request_irq(crc->irq, bfin_crypto_crc_handler, IRQF_SHARED, >> DRIVER_NAME, crc); > >you could use dev_name() rather than DRIVER_NAME > >> + ret = request_dma(crc->dma_ch, DRIVER_NAME); > >same here > >> + /* need at most CRC_MAX_DMA_DESC sg + CRC_MAX_DMA_DESC >middle + >> + 1 last + 1 next dma descriptors >> + */ > >multiline comments look like: > /* > * foo > * bar > */ > >> + 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? > >> +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. > >> +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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
