Re: [PATCH v1 1/4] Crypto: Crypto driver support aes/des/des3 for rk3288

[Heiko Stuebner <heiko@sntech.de>]

Hi Zain,

Am Dienstag, 3. November 2015, 13:52:05 schrieb Zain Wang:
> Crypto driver support cbc/ecb two chainmode, and aes/des/des3 three cipher
> mode.
> The names registered are:
>     ecb(aes) cbc(aes) ecb(des) cbc(des) ecb(des3_ede) cbc(des3_ede)
> You can alloc tags above in your case.
> 
> And other algorithms and platforms will be added later on.
> 
> Signed-off-by: Zain Wang <zain.wang@rock-chips.com>
> ---
>  drivers/crypto/Kconfig                             |  11 +
>  drivers/crypto/Makefile                            |   1 +
>  drivers/crypto/rockchip/Makefile                   |   3 +
>  drivers/crypto/rockchip/rk3288_crypto.c            | 383 ++++++++++++++++
>  drivers/crypto/rockchip/rk3288_crypto.h            | 290 ++++++++++++
>  drivers/crypto/rockchip/rk3288_crypto_ablkcipher.c | 501 +++++++++++++++++++++
>  6 files changed, 1189 insertions(+)
>  create mode 100644 drivers/crypto/rockchip/Makefile
>  create mode 100644 drivers/crypto/rockchip/rk3288_crypto.c
>  create mode 100644 drivers/crypto/rockchip/rk3288_crypto.h
>  create mode 100644 drivers/crypto/rockchip/rk3288_crypto_ablkcipher.c
> 
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 2569e04..d1e42cf 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -498,4 +498,15 @@ config CRYPTO_DEV_SUN4I_SS
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called sun4i-ss.
>  
> +config CRYPTO_DEV_ROCKCHIP
> +	tristate "Rockchip's Cryptographic Engine driver"
> +
> +	select CRYPTO_AES
> +	select CRYPTO_DES
> +	select CRYPTO_BLKCIPHER
> +
> +	help
> +	  This driver interfaces with the hardware crypto accelerator.
> +	  Supporting cbc/ecb chainmode, and aes/des/des3_ede cipher mode.
> +
>  endif # CRYPTO_HW
> diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> index c3ced6f..713de9d 100644
> --- a/drivers/crypto/Makefile
> +++ b/drivers/crypto/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_CRYPTO_DEV_QAT) += qat/
>  obj-$(CONFIG_CRYPTO_DEV_QCE) += qce/
>  obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
>  obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/
> +obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP) += rockchip/
> diff --git a/drivers/crypto/rockchip/Makefile b/drivers/crypto/rockchip/Makefile
> new file mode 100644
> index 0000000..7051c6c
> --- /dev/null
> +++ b/drivers/crypto/rockchip/Makefile
> @@ -0,0 +1,3 @@
> +obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP) += rk_crypto.o
> +rk_crypto-objs := rk3288_crypto.o \
> +		  rk3288_crypto_ablkcipher.o \
> diff --git a/drivers/crypto/rockchip/rk3288_crypto.c b/drivers/crypto/rockchip/rk3288_crypto.c
> new file mode 100644
> index 0000000..02830f2
> --- /dev/null
> +++ b/drivers/crypto/rockchip/rk3288_crypto.c
> @@ -0,0 +1,383 @@

[...]


> +static int rk_crypto_probe(struct platform_device *pdev)
> +{
> +	int err = 0;
> +	struct resource *res;
> +	struct device *dev = &pdev->dev;
> +	struct crypto_info_t *crypto_info;
> +

rk3288 chromebooks use the crypto-engine to validate the boot images and
seem to leave it in a half-on state. This results in an irq pending
during probe and thus a null-pointer dereference in the irq-handler, as
it runs before the crypto-device is fully initialized.

resetting the crypto block, successfull fixed that issue, so I did the
following change:

-------------------
diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 121b6d5..e978fb2 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -182,6 +182,8 @@
 			      "hclk",
 			      "sclk",
 			      "apb_pclk";
+		resets = <&cru SRST_CRYPTO>;
+		reset-names = "crypto";
 		status = "okay";
 	};
 
diff --git a/drivers/crypto/rockchip/rk3288_crypto.c b/drivers/crypto/rockchip/rk3288_crypto.c
index 02830f2..2245d3d 100644
--- a/drivers/crypto/rockchip/rk3288_crypto.c
+++ b/drivers/crypto/rockchip/rk3288_crypto.c
@@ -18,6 +18,7 @@
 #include <linux/of.h>
 #include <linux/clk.h>
 #include <linux/crypto.h>
+#include <linux/reset.h>
 
 struct crypto_info_t *crypto_p;
 
@@ -266,6 +267,15 @@ static int rk_crypto_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct device *dev = &pdev->dev;
 	struct crypto_info_t *crypto_info;
+	struct reset_control *rst;
+
+	/* reset the block to remove any pending actions */
+	rst = devm_reset_control_get(dev, "crypto");
+	if (!IS_ERR(rst)) {
+		reset_control_assert(rst);
+		usleep_range(10, 20);
+		reset_control_deassert(rst);
+	}
 
 	crypto_info = devm_kzalloc(&pdev->dev,
 			sizeof(*crypto_info), GFP_KERNEL);
-------------------


> +	crypto_info = devm_kzalloc(&pdev->dev,
> +			sizeof(*crypto_info), GFP_KERNEL);
> +	if (!crypto_info)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&crypto_info->lock);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	crypto_info->reg = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(crypto_info->reg)) {
> +		err = PTR_ERR(crypto_info->reg);
> +		goto err_ioremap;
> +	}
> +
> +	crypto_info->aclk = devm_clk_get(&pdev->dev, "aclk");
> +	if (IS_ERR(crypto_info->aclk)) {
> +		err = PTR_ERR(crypto_info->aclk);
> +		goto err_ioremap;
> +	}
> +
> +	crypto_info->hclk = devm_clk_get(&pdev->dev, "hclk");
> +	if (IS_ERR(crypto_info->hclk)) {
> +		err = PTR_ERR(crypto_info->hclk);
> +		goto err_ioremap;
> +	}
> +
> +	crypto_info->sclk = devm_clk_get(&pdev->dev, "sclk");
> +	if (IS_ERR(crypto_info->sclk)) {
> +		err = PTR_ERR(crypto_info->sclk);
> +		goto err_ioremap;
> +	}
> +
> +	crypto_info->dmaclk = devm_clk_get(&pdev->dev, "apb_pclk");
> +	if (IS_ERR(crypto_info->dmaclk)) {
> +		err = PTR_ERR(crypto_info->dmaclk);
> +		goto err_ioremap;
> +	}
> +
> +	crypto_info->irq = platform_get_irq(pdev, 0);
> +	if (crypto_info->irq < 0) {
> +		dev_warn(crypto_info->dev,
> +				"control Interrupt is not available.\n");
> +		err = crypto_info->irq;
> +		goto err_ioremap;
> +	}
> +
> +	err = devm_request_irq(&pdev->dev, crypto_info->irq, crypto_irq_handle,
> +				IRQF_SHARED, "rk-crypto", pdev);
> +
> +	if (err) {
> +		dev_err(crypto_info->dev, "irq request failed.\n");
> +		goto err_ioremap;
> +	}
> +
> +	crypto_info->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, crypto_info);
> +	crypto_p = crypto_info;
> +
> +	tasklet_init(&crypto_info->crypto_tasklet,
> +			rk_crypto_tasklet_cb, (unsigned long)crypto_info);
> +	crypto_init_queue(&crypto_info->queue, 50);
> +
> +	crypto_info->enable_clk = rk_crypto_enable_clk;
> +	crypto_info->disable_clk = rk_crypto_disable_clk;
> +	crypto_info->load_data = rk_load_data;
> +	crypto_info->unload_data = rk_unload_data;
> +
> +	err = rk_crypto_register();
> +	if (err) {
> +		dev_err(dev, "err in register alg");
> +		goto err_reg_alg;
> +	}
> +
> +	return 0;
> +
> +err_reg_alg:
> +	free_irq(crypto_info->irq, crypto_info);
> +err_ioremap:
> +	crypto_p = NULL;
> +
> +	return err;
> +}
> +

[...]

> diff --git a/drivers/crypto/rockchip/rk3288_crypto.h b/drivers/crypto/rockchip/rk3288_crypto.h
> new file mode 100644
> index 0000000..153aafb
> --- /dev/null
> +++ b/drivers/crypto/rockchip/rk3288_crypto.h

[...]

> +
> +#define CRYPTO_READ(dev, offset)		  \
> +		__raw_readl(((dev)->reg + (offset)))
> +#define CRYPTO_WRITE(dev, offset, val)	  \
> +		__raw_writel((val), ((dev)->reg + (offset)))
> +/* get register virt address */
> +#define CRYPTO_GET_REG_VIRT(dev, offset)   ((dev)->reg + (offset))
> +
> +#define RK_ALIGN_MASK			(sizeof(u32)-1)
> +
> +struct crypto_info_t {

this is highly rockchip specific, so should probably be named
	rk_crypto_info
or similar instead of a generic sounding crypto_info_t

[...]

> diff --git a/drivers/crypto/rockchip/rk3288_crypto_ablkcipher.c b/drivers/crypto/rockchip/rk3288_crypto_ablkcipher.c
> new file mode 100644
> index 0000000..b3de229
> --- /dev/null
> +++ b/drivers/crypto/rockchip/rk3288_crypto_ablkcipher.c
> @@ -0,0 +1,501 @@

[...]

> +static int rk_ablk_cra_init(struct crypto_tfm *tfm)
> +{
> +	struct rk_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
> +
> +	ctx->dev = crypto_p;

as said above, please no static pointers for devices.

For example, sunxi_ss does the following to transport the core device-data
into the init function:

	struct sun4i_tfm_ctx *op = crypto_tfm_ctx(tfm);
	struct crypto_alg *alg = tfm->__crt_alg;
	struct sun4i_ss_alg_template *algt;

	algt = container_of(alg, struct sun4i_ss_alg_template, alg.crypto);
	op->ss = algt->ss;

so you could probably do something similar


Thanks
Heiko