| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 4 Jun 2012 00:05:36 -0400
From: "Zhang, Sonic" <Sonic.Zhang@...log.com>
To: Mike Frysinger <vapier@...too.org>,
"uclinux-dist-devel@...ckfin.uclinux.org"
<uclinux-dist-devel@...ckfin.uclinux.org>
CC: Herbert Xu <herbert@...dor.hengli.com.au>,
Sonic Zhang <sonic.adi@...il.com>,
"David S. Miller" <davem@...emloft.net>,
LKML <linux-kernel@...r.kernel.org>,
"linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>
Subject: 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@...ckfin.uclinux.org [mailto:uclinux-dist-devel-
>bounces@...ckfin.uclinux.org] On Behalf Of Mike Frysinger
>Sent: Saturday, June 02, 2012 3:19 PM
>To: uclinux-dist-devel@...ckfin.uclinux.org
>Cc: Herbert Xu; Sonic Zhang; David S. Miller; LKML; linux-crypto@...r.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:
>> --- 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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists