[PATCH 2/3] ARM: mxs: crypto: Add Freescale MXS DCP driver

On Thu, Sep 26, 2013 at 8:18 AM, Marek Vasut [off-list ref] wrote:

+config CRYPTO_DEV_MXS_DCP
+       tristate "Support for Freescale MXS DCP"
+       depends on ARCH_MXS
+       select CRYPTO_SHA1
+       select CRYPTO_SHA256
+       select CRYPTO_CBC
+       select CRYPTO_ECB
+       select CRYPTO_AES
+       select CRYPTO_BLKCIPHER
+       select CRYPTO_ALGAPI
+       help
+         The Freescale i.MX23/i.MX28 has SHA1/SHA256 and AES128 CBC/ECB
+         co-processor on the die.
+
+         To compile this driver as a module, choose M here: the module
+         will be called atmel-sha.

Actually it will be called 'mxs-dcp'.

+ * There can even be only one instance of the MXS DCP due to the
+ * design of Linux Crypto API.

Is this true? Usually we don't want to create a global struct.

+
+/* AES 128 ECB and AES 128 CBC */
+static struct crypto_alg dcp_aes_algs[] = {
+       [0] = {

No need for explicitely add this [0]

+               .cra_name               = "ecb(aes)",
+               .cra_driver_name        = "ecb-aes-dcp",
+               .cra_priority           = 400,
+               .cra_alignmask          = 15,
+               .cra_flags              = CRYPTO_ALG_TYPE_ABLKCIPHER |
+                                         CRYPTO_ALG_ASYNC |
+                                         CRYPTO_ALG_NEED_FALLBACK,
+               .cra_init               = mxs_dcp_aes_fallback_init,
+               .cra_exit               = mxs_dcp_aes_fallback_exit,
+               .cra_blocksize          = AES_BLOCK_SIZE,
+               .cra_ctxsize            = sizeof(struct dcp_async_ctx),
+               .cra_type               = &crypto_ablkcipher_type,
+               .cra_module             = THIS_MODULE,
+               .cra_u  = {
+                       .ablkcipher = {
+                               .min_keysize    = AES_MIN_KEY_SIZE,
+                               .max_keysize    = AES_MAX_KEY_SIZE,
+                               .setkey         = mxs_dcp_aes_setkey,
+                               .encrypt        = mxs_dcp_aes_ecb_encrypt,
+                               .decrypt        = mxs_dcp_aes_ecb_decrypt
+                       }
+               }
+       },
+       [1] = {

Same here.

+static int mxs_dcp_probe(struct platform_device *pdev)
+{
+       struct device *dev = &pdev->dev;
+       struct dcp *sdcp = NULL;
+       int i, ret;
+
+       struct resource *iores;
+       int dcp_vmi_irq, dcp_irq;
+
+       mutex_lock(&global_mutex);
+       if (global_sdcp) {
+               dev_err(dev, "Only one DCP instance allowed!\n");
+               ret = -ENODEV;
+               goto err_mutex;
+       }
+
+       iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       dcp_vmi_irq = platform_get_irq(pdev, 0);
+       dcp_irq = platform_get_irq(pdev, 1);
+       if (!iores || dcp_vmi_irq < 0 || dcp_irq < 0) {

No need to check for !iores here.

You use it inside devm_ioremap_resource, which already does this checking.

+       /*
+        * We do not enable context switching. Give the context buffer a
+        * pointer to an illegal address so if context switching is
+        * inadvertantly enabled, the DCP will return an error instead of
+        * trashing good memory. The DCP DMA cannot access ROM, so any ROM
+        * address will do.
+        */
+       writel(0xffff0000, sdcp->base + MXS_DCP_CONTEXT);

Can you use a define instead of this hardcoded number?

+static int mxs_dcp_remove(struct platform_device *pdev)
+{
+       struct dcp *sdcp;
+       int i;
+
+       sdcp = platform_get_drvdata(pdev);
+
+       kthread_stop(sdcp->thread[DCP_CHAN_HASH_SHA]);
+       kthread_stop(sdcp->thread[DCP_CHAN_CRYPTO]);
+
+       platform_set_drvdata(pdev, NULL);
+
+       dma_free_coherent(sdcp->dev, sizeof(struct dcp_coherent_block),
+                         sdcp->coh, sdcp->coh_phys);
+
+       if (sdcp->caps & MXS_DCP_CAPABILITY1_SHA256)
+               crypto_unregister_ahash(&dcp_sha256_alg);
+
+       if (sdcp->caps & MXS_DCP_CAPABILITY1_SHA1)
+               crypto_unregister_ahash(&dcp_sha1_alg);
+
+       if (sdcp->caps & MXS_DCP_CAPABILITY1_AES128) {
+               for (i = ARRAY_SIZE(dcp_aes_algs); i >= 0; i--)
+                       crypto_unregister_alg(&dcp_aes_algs[i]);
+       }
+
+       mutex_lock(&global_mutex);
+       global_sdcp = NULL;
+       mutex_unlock(&global_mutex);

The order of the resources removal does not look correct here.

It should match the order of the error path in probe.

+
+       return 0;
+}
+
+static const struct of_device_id mxs_dcp_dt_ids[] = {
+       {.compatible = "fsl,mxs-dcp", .data = NULL,},

In the other mxs/imx drivers we use:

.compatible = "fsl,<soc>-dcp"

You also need to provide a devicetree documentation for this binding.

+       { /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, mxs_dcp_dt_ids);
+
+static struct platform_driver mxs_dcp_driver = {
+       .probe  = mxs_dcp_probe,
+       .remove = mxs_dcp_remove,
+       .driver = {
+               .name           = "mxs-dcp",
+               .owner          = THIS_MODULE,
+               .of_match_table = mxs_dcp_dt_ids,
+       },
+};
+
+module_platform_driver(mxs_dcp_driver);
+
+MODULE_AUTHOR("Marek Vasut [off-list ref]");
+MODULE_DESCRIPTION("Freescale MXS DCP Driver");
+MODULE_LICENSE("GPL");

Could also add MODULE_ALIAS.