[PATCH 0/9] Add Qualcomm crypto driver

[PATCH 0/9] Add Qualcomm crypto driver

[Stanimir Varbanov]

Hi,

The following patch set implements support for Qualcomm
crypto hardware accelerator driver. It registers itself
to the crypto subsystem and adds the following operations
for encryption/decription - AES with ECB CBC CTR XTS modes,
DES with ECB CBC modes, and 3DES ECB CBC modes.
For hashing and MAC it adds support for sha1 sha256,
hmac(sha1) hmac(sha256).

The version of the driver is reworked by me and it is based
on Codeaurora's qcrypto driver. The proposed version has many
coding style fixes, re-factored files, separated by
functionality. I wanted to make the driver more readable and
easier for review, hope I done well. I'll appreciate any review
comments which will help me to make this code clear and ready
for mainline kernel.

regards,
Stan

Stanimir Varbanov (9):
  crypto: qce: Add core driver implementation
  crypto: qce: Add register defines
  crypto: qce: Add dma and sg helpers
  crypto: qce: Add ablkcipher algorithms
  crypto: qce: Adds sha and hmac transforms
  crypto: qce: Adds infrastructure to setup the crypto block
  crypto: qce: Adds Makefile to build the driver
  crypto: qce: Build Qualcomm qce driver
  ARM: DT: qcom: Add Qualcomm crypto driver binding document

 .../devicetree/bindings/crypto/qcom-qce.txt        |  25 +
 drivers/crypto/Kconfig                             |  10 +
 drivers/crypto/Makefile                            |   1 +
 drivers/crypto/qce/Makefile                        |   6 +
 drivers/crypto/qce/ablkcipher.c                    | 397 ++++++++++++++
 drivers/crypto/qce/cipher.h                        |  62 +++
 drivers/crypto/qce/common.c                        | 424 +++++++++++++++
 drivers/crypto/qce/common.h                        | 111 ++++
 drivers/crypto/qce/core.c                          | 333 ++++++++++++
 drivers/crypto/qce/core.h                          |  69 +++
 drivers/crypto/qce/dma.c                           | 201 +++++++
 drivers/crypto/qce/dma.h                           |  57 ++
 drivers/crypto/qce/regs-v5.h                       | 327 +++++++++++
 drivers/crypto/qce/sha.c                           | 595 +++++++++++++++++++++
 drivers/crypto/qce/sha.h                           |  74 +++
 15 files changed, 2692 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/crypto/qcom-qce.txt
 create mode 100644 drivers/crypto/qce/Makefile
 create mode 100644 drivers/crypto/qce/ablkcipher.c
 create mode 100644 drivers/crypto/qce/cipher.h
 create mode 100644 drivers/crypto/qce/common.c
 create mode 100644 drivers/crypto/qce/common.h
 create mode 100644 drivers/crypto/qce/core.c
 create mode 100644 drivers/crypto/qce/core.h
 create mode 100644 drivers/crypto/qce/dma.c
 create mode 100644 drivers/crypto/qce/dma.h
 create mode 100644 drivers/crypto/qce/regs-v5.h
 create mode 100644 drivers/crypto/qce/sha.c
 create mode 100644 drivers/crypto/qce/sha.h

-- 
1.8.4.4

[PATCH 1/9] crypto: qce: Add core driver implementation

[Stanimir Varbanov]

This adds core driver files. The core part is implementing a
platform driver probe and remove callbaks, the probe enables
clocks, checks crypto version, initialize and request dma
channels, create done tasklet and work queue and finally
register the algorithms into crypto subsystem.

Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
---
 drivers/crypto/qce/core.c | 333 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/crypto/qce/core.h |  69 ++++++++++
 2 files changed, 402 insertions(+)
 create mode 100644 drivers/crypto/qce/core.c
 create mode 100644 drivers/crypto/qce/core.h

diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
new file mode 100644
index 000000000000..240b9983e9b9
--- /dev/null
+++ b/drivers/crypto/qce/core.c
@@ -0,0 +1,333 @@
+/*
+ * Copyright (c) 2010-2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+#include <crypto/algapi.h>
+#include <crypto/internal/hash.h>
+#include <crypto/sha.h>
+
+#include "dma.h"
+#include "core.h"
+#include "common.h"
+#include "cipher.h"
+#include "sha.h"
+#include "regs-v5.h"
+
+#define QCE_MAJOR_VERSION5	0x05
+#define QCE_QUEUE_LENGTH	50
+
+static int qce_async_request_queue(struct qce_device *qce,
+				   struct crypto_async_request *req)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&qce->lock, flags);
+	ret = crypto_enqueue_request(&qce->queue, req);
+	spin_unlock_irqrestore(&qce->lock, flags);
+
+	queue_work(qce->queue_wq, &qce->queue_work);
+
+	return ret;
+}
+
+static void qce_async_request_done(struct qce_device *qce, int ret)
+{
+	qce->result = ret;
+	tasklet_schedule(&qce->done_tasklet);
+}
+
+static struct qce_algo_ops qce_ops[] = {
+	{
+		.type = CRYPTO_ALG_TYPE_ABLKCIPHER,
+		.register_alg = qce_ablkcipher_register,
+	},
+	{
+		.type = CRYPTO_ALG_TYPE_AHASH,
+		.register_alg = qce_ahash_register,
+	},
+};
+
+static void qce_unregister_algs(struct qce_device *qce)
+{
+	struct qce_alg_template *tmpl, *n;
+
+	list_for_each_entry_safe(tmpl, n, &qce->alg_list, entry) {
+		if (tmpl->crypto_alg_type == CRYPTO_ALG_TYPE_AHASH)
+			crypto_unregister_ahash(&tmpl->alg.ahash);
+		else
+			crypto_unregister_alg(&tmpl->alg.crypto);
+
+		list_del(&tmpl->entry);
+		kfree(tmpl);
+	}
+}
+
+static int qce_register_algs(struct qce_device *qce)
+{
+	struct qce_algo_ops *ops;
+	int i, rc = -ENODEV;
+
+	for (i = 0; i < ARRAY_SIZE(qce_ops); i++) {
+		ops = &qce_ops[i];
+		ops->async_req_queue = qce_async_request_queue;
+		ops->async_req_done = qce_async_request_done;
+		rc = ops->register_alg(qce, ops);
+		if (rc)
+			break;
+	}
+
+	if (rc)
+		qce_unregister_algs(qce);
+
+	return rc;
+}
+
+static int qce_handle_request(struct crypto_async_request *async_req)
+{
+	int ret = -EINVAL, i;
+	struct qce_algo_ops *ops;
+	u32 type = crypto_tfm_alg_type(async_req->tfm);
+
+	for (i = 0; i < ARRAY_SIZE(qce_ops); i++) {
+		ops = &qce_ops[i];
+		if (type != ops->type)
+			continue;
+		ret = ops->async_req_handle(async_req);
+		break;
+	}
+
+	return ret;
+}
+
+static void qce_reqqueue_handler(struct work_struct *work)
+{
+	struct qce_device *qce =
+			container_of(work, struct qce_device, queue_work);
+	struct crypto_async_request *async_req = NULL, *backlog = NULL;
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&qce->lock, flags);
+	if (!qce->req) {
+		backlog = crypto_get_backlog(&qce->queue);
+		async_req = crypto_dequeue_request(&qce->queue);
+		qce->req = async_req;
+	}
+	spin_unlock_irqrestore(&qce->lock, flags);
+
+	if (!async_req)
+		return;
+
+	if (backlog)
+		backlog->complete(backlog, -EINPROGRESS);
+
+	ret = qce_handle_request(async_req);
+	if (ret) {
+		spin_lock_irqsave(&qce->lock, flags);
+		qce->req = NULL;
+		spin_unlock_irqrestore(&qce->lock, flags);
+
+		async_req->complete(async_req, ret);
+	}
+}
+
+static void qce_tasklet_req_done(unsigned long data)
+{
+	struct qce_device *qce = (struct qce_device *)data;
+	struct crypto_async_request *areq;
+	unsigned long flags;
+
+	spin_lock_irqsave(&qce->lock, flags);
+	areq = qce->req;
+	qce->req = NULL;
+	spin_unlock_irqrestore(&qce->lock, flags);
+
+	if (areq)
+		areq->complete(areq, qce->result);
+
+	queue_work(qce->queue_wq, &qce->queue_work);
+}
+
+static int qce_get_version(struct qce_device *qce)
+{
+	u32 major, minor, step;
+	u32 val;
+
+	val = readl(qce->base + REG_VERSION);
+	major = (val & CORE_MAJOR_REV_MASK) >> CORE_MAJOR_REV;
+	minor = (val & CORE_MINOR_REV_MASK) >> CORE_MINOR_REV;
+	step = (val & CORE_STEP_REV_MASK) >> CORE_STEP_REV;
+
+	/*
+	 * the driver does not support v5 with minor 0 because it has special
+	 * alignment requirements.
+	 */
+	if (major < QCE_MAJOR_VERSION5 && minor == 0)
+		return -ENODEV;
+
+	qce->burst_size = QCE_BAM_BURST_SIZE;
+	qce->pipe_pair_index = 1;
+
+	dev_info(qce->dev, "Crypto device found, version %d.%d.%d\n",
+		 major, minor, step);
+
+	return 0;
+}
+
+static int qce_clks_get(struct qce_device *qce)
+{
+	struct clk *clk;
+	int rc = 0;
+	int i;
+
+	for (i = 0; i < QCE_CLKS_NUM; i++) {
+		clk = devm_clk_get(qce->dev, clk_names[i]);
+		if (IS_ERR(clk)) {
+			rc = PTR_ERR(clk);
+			break;
+		}
+		qce->clks[i] = clk;
+	}
+
+	return rc;
+}
+
+static int qce_clks_enable(struct qce_device *qce, int enable)
+{
+	int rc = 0;
+	int i;
+
+	for (i = 0; i < QCE_CLKS_NUM; i++) {
+		if (enable)
+			rc = clk_prepare_enable(qce->clks[i]);
+		else
+			clk_disable_unprepare(qce->clks[i]);
+
+		if (rc)
+			break;
+	}
+
+	if (rc)
+		do
+			clk_disable_unprepare(qce->clks[i]);
+		while (--i >= 0);
+
+	return rc;
+}
+
+static int qce_crypto_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct qce_device *qce;
+	struct resource *res;
+	int rc;
+
+	qce = devm_kzalloc(dev, sizeof(*qce), GFP_KERNEL);
+	if (!qce)
+		return -ENOMEM;
+
+	qce->dev = dev;
+	platform_set_drvdata(pdev, qce);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	qce->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(qce->base))
+		return PTR_ERR(qce->base);
+
+	rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
+	if (rc < 0)
+		return rc;
+
+	rc = qce_clks_get(qce);
+	if (rc)
+		return rc;
+
+	rc = qce_clks_enable(qce, 1);
+	if (rc)
+		return rc;
+
+	rc = qce_dma_request(qce->dev, &qce->dma);
+	if (rc)
+		goto error_clks;
+
+	rc = qce_get_version(qce);
+	if (rc)
+		goto error_clks;
+
+	INIT_LIST_HEAD(&qce->alg_list);
+	spin_lock_init(&qce->lock);
+	tasklet_init(&qce->done_tasklet, qce_tasklet_req_done,
+		     (unsigned long)qce);
+
+	qce->queue_wq = alloc_workqueue("qce_wq", WQ_HIGHPRI | WQ_UNBOUND, 1);
+	if (!qce->queue_wq) {
+		rc = -ENOMEM;
+		goto error_dma;
+	}
+
+	INIT_WORK(&qce->queue_work, qce_reqqueue_handler);
+
+	crypto_init_queue(&qce->queue, QCE_QUEUE_LENGTH);
+
+	rc = qce_register_algs(qce);
+	if (rc)
+		goto error_dma;
+
+	return 0;
+error_dma:
+	qce_dma_release(&qce->dma);
+error_clks:
+	qce_clks_enable(qce, 0);
+	return rc;
+}
+
+static int qce_crypto_remove(struct platform_device *pdev)
+{
+	struct qce_device *qce = platform_get_drvdata(pdev);
+
+	cancel_work_sync(&qce->queue_work);
+	destroy_workqueue(qce->queue_wq);
+	tasklet_kill(&qce->done_tasklet);
+	qce_unregister_algs(qce);
+	qce_dma_release(&qce->dma);
+	qce_clks_enable(qce, 0);
+
+	return 0;
+}
+
+static const struct of_device_id qce_crypto_of_match[] = {
+	{ .compatible = "qcom,crypto-v5.1", },
+	{}
+};
+
+static struct platform_driver qce_crypto_driver = {
+	.probe = qce_crypto_probe,
+	.remove = qce_crypto_remove,
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = KBUILD_MODNAME,
+		.of_match_table = qce_crypto_of_match,
+	},
+};
+module_platform_driver(qce_crypto_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Qualcomm crypto engine driver");
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
+MODULE_AUTHOR("The Linux Foundation");
diff --git a/drivers/crypto/qce/core.h b/drivers/crypto/qce/core.h
new file mode 100644
index 000000000000..bd6b648276b2
--- /dev/null
+++ b/drivers/crypto/qce/core.h
@@ -0,0 +1,69 @@
+/*
+ * Copyright (c) 2010-2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _CORE_H_
+#define _CORE_H_
+
+static const char * const clk_names[] = {
+	"core",		/* GCC_CE_CLK */
+	"iface",	/* GCC_CE_AHB_CLK */
+	"bus",		/* GCC_CE_AXI_CLK */
+};
+
+#define QCE_CLKS_NUM	ARRAY_SIZE(clk_names)
+
+/*
+ * Crypto engine device structure
+ *
+ * @alg_list: list of registered algorithms
+ * @queue: request queue
+ * @lock: the lock protects queue and req
+ * @done_tasklet: done tasklet object
+ * @queue_wq: queue workqueue
+ * @queue_work: queue work
+ * @req: current active request
+ * @result: result of transform
+ * @base: virtual IO base
+ * @dev: pointer to device
+ * @clks: array of device clocks
+ * @dma: pointer to dma data
+ * @burst_size: the crypto burst size
+ * @pipe_pair_index: which pipe pair the device using
+ */
+struct qce_device {
+	struct list_head alg_list;
+	struct crypto_queue queue;
+	spinlock_t lock;
+	struct tasklet_struct done_tasklet;
+	struct workqueue_struct *queue_wq;
+	struct work_struct queue_work;
+	struct crypto_async_request *req;
+	int result;
+	void __iomem *base;
+	struct device *dev;
+	struct clk *clks[QCE_CLKS_NUM];
+	struct qce_dma_data dma;
+	int burst_size;
+	unsigned int pipe_pair_index;
+};
+
+struct qce_algo_ops {
+	u32 type;
+	int (*register_alg)(struct qce_device *qce, struct qce_algo_ops *ops);
+	int (*async_req_queue)(struct qce_device *qce,
+			       struct crypto_async_request *req);
+	void (*async_req_done)(struct qce_device *qce, int ret);
+	int (*async_req_handle)(struct crypto_async_request *async_req);
+};
+
+#endif /* _CORE_H_ */
-- 
1.8.4.4

Re: [PATCH 1/9] crypto: qce: Add core driver implementation

[Josh Cartwright]

Hey Stanimir-

Just a few comments/questions from a quick scan of your patchset:

On Thu, Apr 03, 2014 at 07:17:58PM +0300, Stanimir Varbanov wrote:
[..]
> +++ b/drivers/crypto/qce/core.c
[..]
> +
> +static struct qce_algo_ops qce_ops[] = {
> +	{
> +		.type = CRYPTO_ALG_TYPE_ABLKCIPHER,
> +		.register_alg = qce_ablkcipher_register,
> +	},
> +	{
> +		.type = CRYPTO_ALG_TYPE_AHASH,
> +		.register_alg = qce_ahash_register,
> +	},
> +};
> +
> +static void qce_unregister_algs(struct qce_device *qce)
> +{
> +	struct qce_alg_template *tmpl, *n;
> +
> +	list_for_each_entry_safe(tmpl, n, &qce->alg_list, entry) {
> +		if (tmpl->crypto_alg_type == CRYPTO_ALG_TYPE_AHASH)
> +			crypto_unregister_ahash(&tmpl->alg.ahash);
> +		else
> +			crypto_unregister_alg(&tmpl->alg.crypto);

Why no 'unregister_alg' member in qce_algo_ops?

> +
> +		list_del(&tmpl->entry);
> +		kfree(tmpl);
> +	}
> +}
> +
> +static int qce_register_algs(struct qce_device *qce)
> +{
> +	struct qce_algo_ops *ops;
> +	int i, rc = -ENODEV;
> +
> +	for (i = 0; i < ARRAY_SIZE(qce_ops); i++) {
> +		ops = &qce_ops[i];
> +		ops->async_req_queue = qce_async_request_queue;
> +		ops->async_req_done = qce_async_request_done;

Why not set these statically?

> +		rc = ops->register_alg(qce, ops);
> +		if (rc)
> +			break;
> +	}
> +
> +	if (rc)
> +		qce_unregister_algs(qce);
> +
> +	return rc;
> +}
[..]
> +static int qce_get_version(struct qce_device *qce)
> +{
> +	u32 major, minor, step;
> +	u32 val;
> +
> +	val = readl(qce->base + REG_VERSION);
> +	major = (val & CORE_MAJOR_REV_MASK) >> CORE_MAJOR_REV;
> +	minor = (val & CORE_MINOR_REV_MASK) >> CORE_MINOR_REV;
> +	step = (val & CORE_STEP_REV_MASK) >> CORE_STEP_REV;
> +
> +	/*
> +	 * the driver does not support v5 with minor 0 because it has special
> +	 * alignment requirements.
> +	 */
> +	if (major < QCE_MAJOR_VERSION5 && minor == 0)
> +		return -ENODEV;
> +
> +	qce->burst_size = QCE_BAM_BURST_SIZE;
> +	qce->pipe_pair_index = 1;
> +
> +	dev_info(qce->dev, "Crypto device found, version %d.%d.%d\n",
> +		 major, minor, step);

I'd suggest dev_dbg().  Kernel boot is chatty enough.

[..]
> +static int qce_clks_enable(struct qce_device *qce, int enable)
> +{
> +	int rc = 0;
> +	int i;
> +
> +	for (i = 0; i < QCE_CLKS_NUM; i++) {
> +		if (enable)
> +			rc = clk_prepare_enable(qce->clks[i]);
> +		else
> +			clk_disable_unprepare(qce->clks[i]);
> +
> +		if (rc)
> +			break;
> +	}
> +
> +	if (rc)
> +		do
> +			clk_disable_unprepare(qce->clks[i]);
> +		while (--i >= 0);
> +
> +	return rc;
> +}

See my below comment about lumping clocks together.

[..]
> +static int qce_crypto_remove(struct platform_device *pdev)
> +{
> +	struct qce_device *qce = platform_get_drvdata(pdev);
> +
> +	cancel_work_sync(&qce->queue_work);
> +	destroy_workqueue(qce->queue_wq);
> +	tasklet_kill(&qce->done_tasklet);
> +	qce_unregister_algs(qce);
> +	qce_dma_release(&qce->dma);
> +	qce_clks_enable(qce, 0);

qce_clks_enable(qce, 0) is really confusing....I'd suggest creating
separate qce_clks_enable() and qce_clks_disable() functions.

[..]
> +static const struct of_device_id qce_crypto_of_match[] = {
> +	{ .compatible = "qcom,crypto-v5.1", },
> +	{}
> +};

MODULE_DEVICE_TABLE()?

[..]
> +++ b/drivers/crypto/qce/core.h
> @@ -0,0 +1,69 @@
> +/*
> + * Copyright (c) 2010-2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _CORE_H_
> +#define _CORE_H_
> +
> +static const char * const clk_names[] = {
> +	"core",		/* GCC_CE_CLK */
> +	"iface",	/* GCC_CE_AHB_CLK */
> +	"bus",		/* GCC_CE_AXI_CLK */
> +};

You probably don't want this in a header file, as now each compilation
unit will have a copy :(.

Lumping all the clocks together assumes that you will only ever have all
clocks enabled, or all clocks disabled, are you sure that's what you
want?

[..]
> +struct qce_algo_ops {
> +	u32 type;
> +	int (*register_alg)(struct qce_device *qce, struct qce_algo_ops *ops);
> +	int (*async_req_queue)(struct qce_device *qce,
> +			       struct crypto_async_request *req);
> +	void (*async_req_done)(struct qce_device *qce, int ret);

What is the relationship between qce_algo_ops and the qce_alg_template
(which has these same two identically named callbacks)?

> +	int (*async_req_handle)(struct crypto_async_request *async_req);
> +};
> +
> +#endif /* _CORE_H_ */

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 1/9] crypto: qce: Add core driver implementation

[Stanimir Varbanov]

Hi Josh,

Thanks for the comments!

On 04/03/2014 09:19 PM, Josh Cartwright wrote:
> Hey Stanimir-
> 
> Just a few comments/questions from a quick scan of your patchset:
> 
> On Thu, Apr 03, 2014 at 07:17:58PM +0300, Stanimir Varbanov wrote:
> [..]
>> +++ b/drivers/crypto/qce/core.c
> [..]
>> +
>> +static struct qce_algo_ops qce_ops[] = {
>> +	{
>> +		.type = CRYPTO_ALG_TYPE_ABLKCIPHER,
>> +		.register_alg = qce_ablkcipher_register,
>> +	},
>> +	{
>> +		.type = CRYPTO_ALG_TYPE_AHASH,
>> +		.register_alg = qce_ahash_register,
>> +	},
>> +};
>> +
>> +static void qce_unregister_algs(struct qce_device *qce)
>> +{
>> +	struct qce_alg_template *tmpl, *n;
>> +
>> +	list_for_each_entry_safe(tmpl, n, &qce->alg_list, entry) {
>> +		if (tmpl->crypto_alg_type == CRYPTO_ALG_TYPE_AHASH)
>> +			crypto_unregister_ahash(&tmpl->alg.ahash);
>> +		else
>> +			crypto_unregister_alg(&tmpl->alg.crypto);
> 
> Why no 'unregister_alg' member in qce_algo_ops?

Because we have a common unregister function :). We have common
unregster function because in my opinion there is no need to copy the
same piece of code on every algorithm file (ablkcipher.c, sha.c and one
or two more could be added in the future). Something more, I'm not the
inventor of this asymmetric calling convention in driver internals, it
is widely spread in crypto drivers. So, I'm just have been inspired by
this idea, and giving that I personally do not like the monotonic and
linear  coding thus it was easy to me to borrow it.

> 
>> +
>> +		list_del(&tmpl->entry);
>> +		kfree(tmpl);
>> +	}
>> +}
>> +
>> +static int qce_register_algs(struct qce_device *qce)
>> +{
>> +	struct qce_algo_ops *ops;
>> +	int i, rc = -ENODEV;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(qce_ops); i++) {
>> +		ops = &qce_ops[i];
>> +		ops->async_req_queue = qce_async_request_queue;
>> +		ops->async_req_done = qce_async_request_done;
> 
> Why not set these statically?

To save few lines of code. If this breaks readability I could move those
function pointers to the qce_ops[].

> 
>> +		rc = ops->register_alg(qce, ops);
>> +		if (rc)
>> +			break;
>> +	}
>> +
>> +	if (rc)
>> +		qce_unregister_algs(qce);
>> +
>> +	return rc;
>> +}
> [..]
>> +static int qce_get_version(struct qce_device *qce)
>> +{
>> +	u32 major, minor, step;
>> +	u32 val;
>> +
>> +	val = readl(qce->base + REG_VERSION);
>> +	major = (val & CORE_MAJOR_REV_MASK) >> CORE_MAJOR_REV;
>> +	minor = (val & CORE_MINOR_REV_MASK) >> CORE_MINOR_REV;
>> +	step = (val & CORE_STEP_REV_MASK) >> CORE_STEP_REV;
>> +
>> +	/*
>> +	 * the driver does not support v5 with minor 0 because it has special
>> +	 * alignment requirements.
>> +	 */
>> +	if (major < QCE_MAJOR_VERSION5 && minor == 0)
>> +		return -ENODEV;
>> +
>> +	qce->burst_size = QCE_BAM_BURST_SIZE;
>> +	qce->pipe_pair_index = 1;
>> +
>> +	dev_info(qce->dev, "Crypto device found, version %d.%d.%d\n",
>> +		 major, minor, step);
> 
> I'd suggest dev_dbg().  Kernel boot is chatty enough.

OK.

> 
> [..]
>> +static int qce_clks_enable(struct qce_device *qce, int enable)
>> +{
>> +	int rc = 0;
>> +	int i;
>> +
>> +	for (i = 0; i < QCE_CLKS_NUM; i++) {
>> +		if (enable)
>> +			rc = clk_prepare_enable(qce->clks[i]);
>> +		else
>> +			clk_disable_unprepare(qce->clks[i]);
>> +
>> +		if (rc)
>> +			break;
>> +	}
>> +
>> +	if (rc)
>> +		do
>> +			clk_disable_unprepare(qce->clks[i]);
>> +		while (--i >= 0);
>> +
>> +	return rc;
>> +}
> 
> See my below comment about lumping clocks together.
> 
> [..]
>> +static int qce_crypto_remove(struct platform_device *pdev)
>> +{
>> +	struct qce_device *qce = platform_get_drvdata(pdev);
>> +
>> +	cancel_work_sync(&qce->queue_work);
>> +	destroy_workqueue(qce->queue_wq);
>> +	tasklet_kill(&qce->done_tasklet);
>> +	qce_unregister_algs(qce);
>> +	qce_dma_release(&qce->dma);
>> +	qce_clks_enable(qce, 0);
> 
> qce_clks_enable(qce, 0) is really confusing....I'd suggest creating
> separate qce_clks_enable() and qce_clks_disable() functions.
> 

OK will do.

> [..]
>> +static const struct of_device_id qce_crypto_of_match[] = {
>> +	{ .compatible = "qcom,crypto-v5.1", },
>> +	{}
>> +};
> 
> MODULE_DEVICE_TABLE()?

Good catch. Thanks.

> 
> [..]
>> +++ b/drivers/crypto/qce/core.h
>> @@ -0,0 +1,69 @@
>> +/*
>> + * Copyright (c) 2010-2014, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#ifndef _CORE_H_
>> +#define _CORE_H_
>> +
>> +static const char * const clk_names[] = {
>> +	"core",		/* GCC_CE_CLK */
>> +	"iface",	/* GCC_CE_AHB_CLK */
>> +	"bus",		/* GCC_CE_AXI_CLK */
>> +};
> 
> You probably don't want this in a header file, as now each compilation
> unit will have a copy :(.

It is my fault, I just forgot to move this array in the core.c.

> 
> Lumping all the clocks together assumes that you will only ever have all
> clocks enabled, or all clocks disabled, are you sure that's what you
> want?

At least until now all clocks are needed. When adding power management
this code should be revised.

> 
> [..]
>> +struct qce_algo_ops {
>> +	u32 type;
>> +	int (*register_alg)(struct qce_device *qce, struct qce_algo_ops *ops);
>> +	int (*async_req_queue)(struct qce_device *qce,
>> +			       struct crypto_async_request *req);
>> +	void (*async_req_done)(struct qce_device *qce, int ret);
> 
> What is the relationship between qce_algo_ops and the qce_alg_template
> (which has these same two identically named callbacks)?

This is a way of passing function pointers from core.c to the
ablkcipher.c and sha.c (where they are needed to queue up new crypto
async requests and finish the requests with done counterpart). This
avoids prototyping those core.c functions and calling them directly.

-- 
regards,
Stan

Re: [PATCH 1/9] crypto: qce: Add core driver implementation

[Courtney Cavin]

On Thu, Apr 03, 2014 at 06:17:58PM +0200, Stanimir Varbanov wrote:
> This adds core driver files. The core part is implementing a
> platform driver probe and remove callbaks, the probe enables
> clocks, checks crypto version, initialize and request dma
> channels, create done tasklet and work queue and finally
> register the algorithms into crypto subsystem.
> 
> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> ---
>  drivers/crypto/qce/core.c | 333 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/crypto/qce/core.h |  69 ++++++++++
>  2 files changed, 402 insertions(+)
>  create mode 100644 drivers/crypto/qce/core.c
>  create mode 100644 drivers/crypto/qce/core.h
> 
> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
[...]
> +static struct qce_algo_ops qce_ops[] = {
> +	{
> +		.type = CRYPTO_ALG_TYPE_ABLKCIPHER,
> +		.register_alg = qce_ablkcipher_register,
> +	},
> +	{
> +		.type = CRYPTO_ALG_TYPE_AHASH,
> +		.register_alg = qce_ahash_register,
> +	},
> +};
> +
> +static void qce_unregister_algs(struct qce_device *qce)
> +{
> +	struct qce_alg_template *tmpl, *n;
> +
> +	list_for_each_entry_safe(tmpl, n, &qce->alg_list, entry) {
> +		if (tmpl->crypto_alg_type == CRYPTO_ALG_TYPE_AHASH)
> +			crypto_unregister_ahash(&tmpl->alg.ahash);
> +		else
> +			crypto_unregister_alg(&tmpl->alg.crypto);
> +
> +		list_del(&tmpl->entry);
> +		kfree(tmpl);

I find this whole memory/list management to be very disorganised.
ops->register_alg() is supposed to allocate this item--more precisely,
multiple items--using something that must be able to be kfree'd
directly, register it with the crypto core, and put it on this list
manually.  Here we unregister/remove/free this in the core.  Josh's
recommendation of a unregister_alg callback might help, but it all
remains a bit unclear with register_alg/unregister_alg managing X
algorithms per call. 

Additionally, above you have qce_ops, which clearly defines the
operations for specific algorithms types/groups, which in later patches
are shown to be seperated out into independent implementations.

>From what I can tell, this seems to be a framework with built-in yet
independent crypto implementations which call the crypto API directly.

It would be more logical to me if this was seperated out into a
"library/core" API, with the individual implementations as platform
drivers of their own.  Then they can register with the core, managing
memory how they please.

What am I missing?

> +	}
> +}
> +
> +static int qce_register_algs(struct qce_device *qce)
> +{
> +	struct qce_algo_ops *ops;
> +	int i, rc = -ENODEV;
> +
> +	for (i = 0; i < ARRAY_SIZE(qce_ops); i++) {
> +		ops = &qce_ops[i];
> +		ops->async_req_queue = qce_async_request_queue;
> +		ops->async_req_done = qce_async_request_done;
> +		rc = ops->register_alg(qce, ops);
> +		if (rc)
> +			break;
> +	}
> +
> +	if (rc)
> +		qce_unregister_algs(qce);
> +
> +	return rc;
> +}

-Courtney

Re: [PATCH 1/9] crypto: qce: Add core driver implementation

[Stanimir Varbanov]

Hi Courtney,

Thanks for the review!

On 04/04/2014 02:38 AM, Courtney Cavin wrote:
> On Thu, Apr 03, 2014 at 06:17:58PM +0200, Stanimir Varbanov wrote:
>> This adds core driver files. The core part is implementing a
>> platform driver probe and remove callbaks, the probe enables
>> clocks, checks crypto version, initialize and request dma
>> channels, create done tasklet and work queue and finally
>> register the algorithms into crypto subsystem.
>>
>> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
>> ---
>>  drivers/crypto/qce/core.c | 333 ++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/crypto/qce/core.h |  69 ++++++++++
>>  2 files changed, 402 insertions(+)
>>  create mode 100644 drivers/crypto/qce/core.c
>>  create mode 100644 drivers/crypto/qce/core.h
>>
>> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> [...]
>> +static struct qce_algo_ops qce_ops[] = {
>> +	{
>> +		.type = CRYPTO_ALG_TYPE_ABLKCIPHER,
>> +		.register_alg = qce_ablkcipher_register,
>> +	},
>> +	{
>> +		.type = CRYPTO_ALG_TYPE_AHASH,
>> +		.register_alg = qce_ahash_register,
>> +	},
>> +};
>> +
>> +static void qce_unregister_algs(struct qce_device *qce)
>> +{
>> +	struct qce_alg_template *tmpl, *n;
>> +
>> +	list_for_each_entry_safe(tmpl, n, &qce->alg_list, entry) {
>> +		if (tmpl->crypto_alg_type == CRYPTO_ALG_TYPE_AHASH)
>> +			crypto_unregister_ahash(&tmpl->alg.ahash);
>> +		else
>> +			crypto_unregister_alg(&tmpl->alg.crypto);
>> +
>> +		list_del(&tmpl->entry);
>> +		kfree(tmpl);
> 
> I find this whole memory/list management to be very disorganised.
> ops->register_alg() is supposed to allocate this item--more precisely,
> multiple items--using something that must be able to be kfree'd
> directly, register it with the crypto core, and put it on this list
> manually.  Here we unregister/remove/free this in the core.  Josh's
> recommendation of a unregister_alg callback might help, but it all
> remains a bit unclear with register_alg/unregister_alg managing X
> algorithms per call. 
> 
> Additionally, above you have qce_ops, which clearly defines the
> operations for specific algorithms types/groups, which in later patches
> are shown to be seperated out into independent implementations.
> 
> From what I can tell, this seems to be a framework with built-in yet
> independent crypto implementations which call the crypto API directly.
> 
> It would be more logical to me if this was seperated out into a
> "library/core" API, with the individual implementations as platform
> drivers of their own.  Then they can register with the core, managing
> memory how they please.
> 
> What am I missing?
> 

No, you have not miss nothing.

OK I see your point. I made few changes in the core, killed the alg_list
and its manipulation function and added a .unregister_algs operation.
Now every type of algorithm will handle all core crypto api functions
itself. Also I'm using devm_kzalloc() in .register_algs when allocating
memory for qce_alg_template structures to avoid kfree(). The callbacks
async_req_queue/done are now embedded in qce_device structure and they
are invoked directly from algorithm implementations. Thus I have
separated the interfaces: functions implemented in core part of the
driver and struct qce_algo_ops having the function pointers implemented
by every type of algorithm.

If you don't have some objections I can send out a version 2.

-- 
regards,
Stan

Re: [PATCH 1/9] crypto: qce: Add core driver implementation

[Courtney Cavin]

On Tue, Apr 08, 2014 at 06:26:44PM +0200, Stanimir Varbanov wrote:
> On 04/04/2014 02:38 AM, Courtney Cavin wrote:
> > On Thu, Apr 03, 2014 at 06:17:58PM +0200, Stanimir Varbanov wrote:
> >> This adds core driver files. The core part is implementing a
> >> platform driver probe and remove callbaks, the probe enables
> >> clocks, checks crypto version, initialize and request dma
> >> channels, create done tasklet and work queue and finally
> >> register the algorithms into crypto subsystem.
> >>
> >> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> >> ---
> >>  drivers/crypto/qce/core.c | 333 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  drivers/crypto/qce/core.h |  69 ++++++++++
> >>  2 files changed, 402 insertions(+)
> >>  create mode 100644 drivers/crypto/qce/core.c
> >>  create mode 100644 drivers/crypto/qce/core.h
> >>
> >> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> > [...]
> >> +static struct qce_algo_ops qce_ops[] = {
> >> +	{
> >> +		.type = CRYPTO_ALG_TYPE_ABLKCIPHER,
> >> +		.register_alg = qce_ablkcipher_register,
> >> +	},
> >> +	{
> >> +		.type = CRYPTO_ALG_TYPE_AHASH,
> >> +		.register_alg = qce_ahash_register,
> >> +	},
> >> +};
> >> +
> >> +static void qce_unregister_algs(struct qce_device *qce)
> >> +{
> >> +	struct qce_alg_template *tmpl, *n;
> >> +
> >> +	list_for_each_entry_safe(tmpl, n, &qce->alg_list, entry) {
> >> +		if (tmpl->crypto_alg_type == CRYPTO_ALG_TYPE_AHASH)
> >> +			crypto_unregister_ahash(&tmpl->alg.ahash);
> >> +		else
> >> +			crypto_unregister_alg(&tmpl->alg.crypto);
> >> +
> >> +		list_del(&tmpl->entry);
> >> +		kfree(tmpl);
> > 
> > I find this whole memory/list management to be very disorganised.
> > ops->register_alg() is supposed to allocate this item--more precisely,
> > multiple items--using something that must be able to be kfree'd
> > directly, register it with the crypto core, and put it on this list
> > manually.  Here we unregister/remove/free this in the core.  Josh's
> > recommendation of a unregister_alg callback might help, but it all
> > remains a bit unclear with register_alg/unregister_alg managing X
> > algorithms per call. 
> > 
> > Additionally, above you have qce_ops, which clearly defines the
> > operations for specific algorithms types/groups, which in later patches
> > are shown to be seperated out into independent implementations.
> > 
> > From what I can tell, this seems to be a framework with built-in yet
> > independent crypto implementations which call the crypto API directly.
> > 
> > It would be more logical to me if this was seperated out into a
> > "library/core" API, with the individual implementations as platform
> > drivers of their own.  Then they can register with the core, managing
> > memory how they please.
> > 
> > What am I missing?
> > 
> 
> No, you have not miss nothing.
> 
> OK I see your point. I made few changes in the core, killed the alg_list
> and its manipulation function and added a .unregister_algs operation.
> Now every type of algorithm will handle all core crypto api functions
> itself. Also I'm using devm_kzalloc() in .register_algs when allocating
> memory for qce_alg_template structures to avoid kfree(). The callbacks
> async_req_queue/done are now embedded in qce_device structure and they
> are invoked directly from algorithm implementations. Thus I have
> separated the interfaces: functions implemented in core part of the
> driver and struct qce_algo_ops having the function pointers implemented
> by every type of algorithm.
> 
> If you don't have some objections I can send out a version 2.


Well, I'd have to see the code to understand clearly what you are
describing here, but the mention of devm_kzalloc() concerns me.  The
only device which I currently see to which this allocation could be
associated is the single platform_device in the core.  Associating the
memory with the core gets rid of the explicit call to kfree() by the
core, but doesn't rearrange the way the memory is actually managed.

If you have changed it so that each algorithm "block" has its own
device, then this would seem more reasonable, but I don't see that in
the explanation you provided.

-Courtney

Re: [PATCH 1/9] crypto: qce: Add core driver implementation

[Stanimir Varbanov]

Hi Courtney,

On 04/09/2014 01:00 AM, Courtney Cavin wrote:
> On Tue, Apr 08, 2014 at 06:26:44PM +0200, Stanimir Varbanov wrote:
>> On 04/04/2014 02:38 AM, Courtney Cavin wrote:
>>> On Thu, Apr 03, 2014 at 06:17:58PM +0200, Stanimir Varbanov wrote:
>>>> This adds core driver files. The core part is implementing a
>>>> platform driver probe and remove callbaks, the probe enables
>>>> clocks, checks crypto version, initialize and request dma
>>>> channels, create done tasklet and work queue and finally
>>>> register the algorithms into crypto subsystem.
>>>>
>>>> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
>>>> ---
>>>>  drivers/crypto/qce/core.c | 333 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>  drivers/crypto/qce/core.h |  69 ++++++++++
>>>>  2 files changed, 402 insertions(+)
>>>>  create mode 100644 drivers/crypto/qce/core.c
>>>>  create mode 100644 drivers/crypto/qce/core.h
>>>>
>>>> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
>>> [...]
>>>> +static struct qce_algo_ops qce_ops[] = {
>>>> +	{
>>>> +		.type = CRYPTO_ALG_TYPE_ABLKCIPHER,
>>>> +		.register_alg = qce_ablkcipher_register,
>>>> +	},
>>>> +	{
>>>> +		.type = CRYPTO_ALG_TYPE_AHASH,
>>>> +		.register_alg = qce_ahash_register,
>>>> +	},
>>>> +};
>>>> +
>>>> +static void qce_unregister_algs(struct qce_device *qce)
>>>> +{
>>>> +	struct qce_alg_template *tmpl, *n;
>>>> +
>>>> +	list_for_each_entry_safe(tmpl, n, &qce->alg_list, entry) {
>>>> +		if (tmpl->crypto_alg_type == CRYPTO_ALG_TYPE_AHASH)
>>>> +			crypto_unregister_ahash(&tmpl->alg.ahash);
>>>> +		else
>>>> +			crypto_unregister_alg(&tmpl->alg.crypto);
>>>> +
>>>> +		list_del(&tmpl->entry);
>>>> +		kfree(tmpl);
>>>
>>> I find this whole memory/list management to be very disorganised.
>>> ops->register_alg() is supposed to allocate this item--more precisely,
>>> multiple items--using something that must be able to be kfree'd
>>> directly, register it with the crypto core, and put it on this list
>>> manually.  Here we unregister/remove/free this in the core.  Josh's
>>> recommendation of a unregister_alg callback might help, but it all
>>> remains a bit unclear with register_alg/unregister_alg managing X
>>> algorithms per call. 
>>>
>>> Additionally, above you have qce_ops, which clearly defines the
>>> operations for specific algorithms types/groups, which in later patches
>>> are shown to be seperated out into independent implementations.
>>>
>>> From what I can tell, this seems to be a framework with built-in yet
>>> independent crypto implementations which call the crypto API directly.
>>>
>>> It would be more logical to me if this was seperated out into a
>>> "library/core" API, with the individual implementations as platform
>>> drivers of their own.  Then they can register with the core, managing
>>> memory how they please.
>>>
>>> What am I missing?
>>>
>>
>> No, you have not miss nothing.
>>
>> OK I see your point. I made few changes in the core, killed the alg_list
>> and its manipulation function and added a .unregister_algs operation.
>> Now every type of algorithm will handle all core crypto api functions
>> itself. Also I'm using devm_kzalloc() in .register_algs when allocating
>> memory for qce_alg_template structures to avoid kfree(). The callbacks
>> async_req_queue/done are now embedded in qce_device structure and they
>> are invoked directly from algorithm implementations. Thus I have
>> separated the interfaces: functions implemented in core part of the
>> driver and struct qce_algo_ops having the function pointers implemented
>> by every type of algorithm.
>>
>> If you don't have some objections I can send out a version 2.
> 
> 
> Well, I'd have to see the code to understand clearly what you are
> describing here, but the mention of devm_kzalloc() concerns me.  The
> only device which I currently see to which this allocation could be
> associated is the single platform_device in the core.  Associating the
> memory with the core gets rid of the explicit call to kfree() by the
> core, but doesn't rearrange the way the memory is actually managed.

OK, no worries. I have no strong opinion and will use kzalloc() then.

> 
> If you have changed it so that each algorithm "block" has its own
> device, then this would seem more reasonable, but I don't see that in
> the explanation you provided.

No, that is not possible. The platform driver must be one because the
register space is common. The hardware accesses must be serialised from
core part of the driver.

-- 
regards,
Stan

[PATCH 9/9] ARM: DT: qcom: Add Qualcomm crypto driver binding document

[Stanimir Varbanov]

Here is Qualcomm crypto driver device tree binding documentation
to used as a reference example.

Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
---
 .../devicetree/bindings/crypto/qcom-qce.txt        | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/crypto/qcom-qce.txt

diff --git a/Documentation/devicetree/bindings/crypto/qcom-qce.txt b/Documentation/devicetree/bindings/crypto/qcom-qce.txt
new file mode 100644
index 000000000000..7ec67c90d2d1
--- /dev/null
+++ b/Documentation/devicetree/bindings/crypto/qcom-qce.txt
@@ -0,0 +1,25 @@
+Qualcomm crypto engine driver
+
+Required properties:
+
+- compatible  : should be "qcom,crypto-v5.1"
+- reg         : specifies base physical address and size of the registers map
+- clocks      : phandle to clock-controller plus clock-specifier pair
+- clock-names : "iface" clocks register interface
+		"bus" clocks data transfer interface
+		"core" clocks reset of the crypto block
+- dmas        : DMA specifiers for tx and rx dma channels. For more see
+		Documentation/devicetree/bindings/dma/dma.txt
+- dma-names   : DMA request names should be "rx" and "tx"
+
+Example:
+	crypto@fd45a000 {
+		compatible = "qcom,crypto-v5.1";
+		reg = <0xfd45a000 0x6000>;
+		clocks = <&gcc GCC_CE2_AHB_CLK>,
+			 <&gcc GCC_CE2_AXI_CLK>,
+			 <&gcc GCC_CE2_CLK>;
+		clock-names = "iface", "bus", "core";
+		dmas = <&cryptobam 2>, <&cryptobam 3>;
+		dma-names = "rx", "tx";
+	};
-- 
1.8.4.4