Re: [PATCH 2/3] crypto: sahara: Add driver for SAHARA2 accelerator.

Javier Martin [off-list ref] writes:

Hi,

SAHARA2 HW module is included in the i.MX27 SoC from
Freescale. It is capable of performing cipher algorithms
such as AES, 3DES..., hashing and RNG too.

This driver provides support for AES-CBC and AES-ECB
by now.

[...]

+	int irq;
+	int err;
+	int i;
+
+	dev = devm_kzalloc(&pdev->dev, sizeof(struct sahara_dev), GFP_KERNEL);
+	if (dev == NULL) {
+		dev_err(&pdev->dev, "unable to alloc data struct.\n");
+		return -ENOMEM;
+	}
+
+	dev->device = &pdev->dev;
+	platform_set_drvdata(pdev, dev);
+
+	/* Get the base address */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "failed to get memory region resource\n");
+		return -ENODEV;
+	}
+
+	if (devm_request_mem_region(&pdev->dev, res->start,
+			resource_size(res), SAHARA_NAME) == NULL) {
+		dev_err(&pdev->dev, "failed to request memory region\n");
+		return -ENOENT;
+	}
+	dev->regs_base = devm_ioremap(&pdev->dev, res->start,
+				      resource_size(res));
+	if (!dev->regs_base) {
+		dev_err(&pdev->dev, "failed to ioremap address region\n");
+		return -ENOENT;
+	}
+
+	/* Get the IRQ */
+	irq = platform_get_irq(pdev,  0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "failed to get irq resource\n");
+		return irq;
+	}
+
+	if (devm_request_irq(&pdev->dev, irq, sahara_irq_handler,
+		0, SAHARA_NAME, dev) < 0) {
+		dev_err(&pdev->dev, "failed to request irq\n");
+		return -ENOENT;
+	}
+
+	/* clocks */
+	dev->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
+	if (IS_ERR(dev->clk_ipg)) {
+		dev_err(&pdev->dev, "Could not get ipg clock\n");
+		return PTR_ERR(dev->clk_ipg);
+	}
+
+	dev->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
+	if (IS_ERR(dev->clk_ahb)) {
+		dev_err(&pdev->dev, "Could not get ahb clock\n");
+		return PTR_ERR(dev->clk_ahb);
+	}
+
+	/* Allocate HW descriptors */
+	dev->hw_desc[0] = dma_alloc_coherent(&pdev->dev,
+			SAHARA_MAX_HW_DESC * sizeof(struct sahara_hw_desc),
+			&dev->hw_phys_desc[0], GFP_KERNEL);
+	if (!dev->hw_desc[0]) {
+		dev_err(&pdev->dev, "Could not allocate hw descriptors\n");
+		return -ENOMEM;
+	}
+	dev->hw_desc[1] = dev->hw_desc[0] + 1;
+	dev->hw_phys_desc[1] = dev->hw_phys_desc[0] +
+				sizeof(struct sahara_hw_desc);
+
+	/* Allocate space for iv and key */
+	dev->key_base = dma_alloc_coherent(&pdev->dev, 2 * AES_KEYSIZE_128,
+				&dev->key_phys_base, GFP_KERNEL);
+	if (!dev->key_base) {
+		dev_err(&pdev->dev, "Could not allocate memory for key\n");
+		err = -ENOMEM;
+		goto err_key;
+	}
+	dev->iv_base = dev->key_base + AES_KEYSIZE_128;
+	dev->iv_phys_base = dev->key_phys_base + AES_KEYSIZE_128;
+
+	/* Allocate space for HW links */
+	dev->hw_link[0] = dma_alloc_coherent(&pdev->dev,
+			SAHARA_MAX_HW_LINK * sizeof(struct sahara_hw_link),
+			&dev->hw_phys_link[0], GFP_KERNEL);
+	if (!dev->hw_link) {
+		dev_err(&pdev->dev, "Could not allocate hw links\n");
+		err = -ENOMEM;
+		goto err_link;
+	}
+	for (i = 1; i < SAHARA_MAX_HW_LINK; i++) {
+		dev->hw_phys_link[i] = dev->hw_phys_link[i - 1] +
+					sizeof(struct sahara_hw_link);
+		dev->hw_link[i] = dev->hw_link[i - 1] + 1;
+	}
+
+	crypto_init_queue(&dev->queue, SAHARA_QUEUE_LENGTH);
+
+	dev_ptr = dev;
+
+	tasklet_init(&dev->queue_task, sahara_aes_queue_task,
+		     (unsigned long)dev);
+	tasklet_init(&dev->done_task, sahara_aes_done_task,
+		     (unsigned long)dev);
+
+	init_timer(&dev->watchdog);
+	dev->watchdog.function = &sahara_watchdog;
+	dev->watchdog.data = (unsigned long)dev;
+
+	clk_prepare_enable(dev->clk_ipg);
+	clk_prepare_enable(dev->clk_ahb);
+
+	version = sahara_read(dev, SAHARA_REG_VERSION);
+	if (version != SAHARA_VERSION_3) {

According to fsl kernel tree on linaro.org, on imx5 (sahara 4), the
register layout is different so a new check should be added:
(version >> 8) & 0xff == 4

+		dev_err(&pdev->dev, "SAHARA version %d not supported\n",
+			version);
+		err = -ENODEV;
+		goto err_algs;
+	}
+
+	sahara_write(dev, SAHARA_CMD_RESET | SAHARA_CMD_MODE_BATCH,
+		     SAHARA_REG_CMD);
+	sahara_write(dev, SAHARA_CONTROL_SET_THROTTLE(0) |
+			SAHARA_CONTROL_SET_MAXBURST(8) |
+			SAHARA_CONTROL_RNG_AUTORSD |
+			SAHARA_CONTROL_ENABLE_INT,
+			SAHARA_REG_CONTROL);
+
+	err = sahara_register_algs(dev);
+	if (err)
+		goto err_algs;
+
+	dev_info(&pdev->dev, "SAHARA version %d initialized\n", version);
+
+	return 0;
+
+err_algs:
+	dev_ptr = NULL;
+	kfree(dev->key_base);
+	clk_disable_unprepare(dev->clk_ipg);
+	clk_disable_unprepare(dev->clk_ahb);
+err_link:
+	kfree(dev->key_base);

you're freeing 2 time key_base but not hw_link

+err_key:
+	kfree(dev->hw_desc[0]);

This one makes my kernel oops. Can be also reproduced by unloading the
module.

+	return err;
+}
+
+static int __devexit sahara_remove(struct platform_device *pdev)

__devinit & __devexit are gone.

+{
+	struct sahara_dev *dev = platform_get_drvdata(pdev);
+
+	kfree(dev->key_base);
+	kfree(dev->hw_desc[0]);

missing kfree for hw_link too ?

Other than the remarks above, I'm hitting this while loading the kernel:

kernel: [   49.305657] sahara 83ff8000.sahara: nbytes: 16, enc: 1, cbc: 0
kernel: [   53.625599] BUG: spinlock lockup suspected on CPU#0, cryptomgr_test/2040
kernel: [   53.625772]  lock: 0xc27a1824, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0
kernel: [   53.625943] Backtrace: 
kernel: [   53.626061] [<c0010e34>] (dump_backtrace+0x0/0x10c) from [<c05173d0>] (dump_stack+0x18/0x1c)
kernel: [   53.626251]  r6:0fd84000 r5:c27a1824 r4:00000000 r3:c24f26c0
kernel: [   53.626444] [<c05173b8>] (dump_stack+0x0/0x1c) from [<c051ab18>] (spin_dump+0x80/0x94)
kernel: [   53.626621] [<c051aa98>] (spin_dump+0x0/0x94) from [<c01f680c>] (do_raw_spin_lock+0xe8/0x12c)
kernel: [   53.626792]  r5:00000000 r4:0fd84000
kernel: [   53.626913] [<c01f6724>] (do_raw_spin_lock+0x0/0x12c) from [<c051ed14>] (_raw_spin_lock_irqsave+0x64/0x70)
kernel: [   53.627133] [<c051ecb0>] (_raw_spin_lock_irqsave+0x0/0x70) from [<bf00d388>] (sahara_aes_crypt+0x7c/0x100 [sahara])
kernel: [   53.627363]  r6:00000001 r5:c20b8580 r4:c27a1810
kernel: [   53.627525] [<bf00d30c>] (sahara_aes_crypt+0x0/0x100 [sahara]) from [<bf00d538>] (sahara_aes_ecb_encrypt+0x48/0x4c [sahara])
kernel: [   53.627744]  r8:c2755d28 r7:df9f3000 r6:00000001 r5:c20b8b00 r4:c20b8580
kernel: [   53.627970] [<bf00d4f0>] (sahara_aes_ecb_encrypt+0x0/0x4c [sahara]) from [<c01cce08>] (__test_skcipher+0x22c/0x874)
kernel: [   53.628176]  r5:c0772794 r4:c0772794
kernel: [   53.628293] [<c01ccbdc>] (__test_skcipher+0x0/0x874) from [<c01ceb68>] (test_skcipher+0x2c/0x58)
kernel: [   53.628483] [<c01ceb3c>] (test_skcipher+0x0/0x58) from [<c01cec08>] (alg_test_skcipher+0x74/0xac)
kernel: [   53.628660]  r7:dfbfe9c0 r6:c0539aac r5:c20b8b00 r4:00000000
kernel: [   53.628840] [<c01ceb94>] (alg_test_skcipher+0x0/0xac) from [<c01ce54c>] (alg_test+0x154/0x1c8)
kernel: [   53.629014]  r7:ffffffff r6:00000185 r5:dfbfe9c0 r4:00000000
kernel: [   53.632986] [<c01ce3f8>] (alg_test+0x0/0x1c8) from [<c01cc280>] (cryptomgr_test+0x2c/0x50)

Despite this, the test seems to succeed and the sahara interrupt is
increasing so it may be working.


Arnaud