Message-Id: <201206020319.14870.vapier@gentoo.org>
Date:	Sat, 2 Jun 2012 03:19:12 -0400
From:	Mike Frysinger <vapier@...too.org>
To:	uclinux-dist-devel@...ckfin.uclinux.org
Cc:	Sonic Zhang <sonic.adi@...il.com>,
	Herbert Xu <herbert@...dor.hengli.com.au>,
	"David S. Miller" <davem@...emloft.net>,
	linux-crypto@...r.kernel.org, LKML <linux-kernel@...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 ?

> +	/* 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);

> +		/* 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().

> +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 ?

> +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, 
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

Download attachment "signature.asc " of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists