by Heiko Stuebner

[permalink] [raw]

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

Hi Zain,

looks like my comment on v1 came later than your v2 submission,
so here it is again :-)

Am Freitag, 6. November 2015, 09:17:21 schrieb Zain Wang:
> 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 <[email protected]>
> ---
>

[...]

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

[...]

> +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..cf4cd18
> --- /dev/null
> +++ b/drivers/crypto/rockchip/rk3288_crypto.h
> @@ -0,0 +1,222 @@

[...]

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

[...]

> + struct device *dev;
> + struct clk *aclk;
> + struct clk *hclk;
> + struct clk *sclk;
> + struct clk *dmaclk;
> + void __iomem *reg;
> + int irq;
> + struct crypto_queue queue;
> + struct tasklet_struct crypto_tasklet;
> + struct ablkcipher_request *ablk_req;
> + /* device lock */
> + spinlock_t lock;
> +
> + /* the public variable */
> + struct scatterlist *sg_src;
> + struct scatterlist *sg_dst;
> + struct scatterlist sg_tmp;
> + struct scatterlist *first;
> + unsigned int left_bytes;
> + void *addr_vir;
> + int aligned;
> + int align_size;
> + size_t nents;
> + unsigned int total;
> + unsigned int count;
> + u32 mode;
> + dma_addr_t addr_in;
> + dma_addr_t addr_out;
> + int (*start)(struct crypto_info_t *dev);
> + int (*update)(struct crypto_info_t *dev);
> + void (*complete)(struct crypto_info_t *dev, int err);
> + int (*enable_clk)(struct crypto_info_t *dev);
> + void (*disable_clk)(struct crypto_info_t *dev);
> + int (*load_data)(struct crypto_info_t *dev,
> + struct scatterlist *sg_src,
> + struct scatterlist *sg_dst);
> + void (*unload_data)(struct crypto_info_t *dev);
> +};
> +
> +/* the private variable of cipher */
> +struct rk_cipher_ctx {
> + struct crypto_info_t *dev;
> + unsigned int keylen;
> +};
> +
> +extern struct crypto_info_t *crypto_p;
> +
> +extern struct crypto_alg rk_ecb_aes_alg;
> +extern struct crypto_alg rk_cbc_aes_alg;
> +extern struct crypto_alg rk_ecb_des_alg;
> +extern struct crypto_alg rk_cbc_des_alg;
> +extern struct crypto_alg rk_ecb_des3_ede_alg;
> +extern struct crypto_alg rk_cbc_des3_ede_alg;
> +
> +#endif
> diff --git a/drivers/crypto/rockchip/rk3288_crypto_ablkcipher.c b/drivers/crypto/rockchip/rk3288_crypto_ablkcipher.c
> new file mode 100644
> index 0000000..28b49c9
> --- /dev/null
> +++ b/drivers/crypto/rockchip/rk3288_crypto_ablkcipher.c
> @@ -0,0 +1,511 @@

[...]

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

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

Same of course for the other crypto_p users.

Thanks
Heiko

2015-11-09 03:47:13

by zain

[permalink] [raw]

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

Hi Heiko,
Thanks for your serious comments always.

On 2015年11月08日 07:19, Heiko Stuebner wrote:
> Hi Zain,
>
> looks like my comment on v1 came later than your v2 submission,
> so here it is again :-)
>
> Am Freitag, 6. November 2015, 09:17:21 schrieb Zain Wang:
>> 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 <[email protected]>
>> ---
>>
> [...]
>
>> diff --git a/drivers/crypto/rockchip/rk3288_crypto.c b/drivers/crypto/rockchip/rk3288_crypto.c
>> new file mode 100644
>> index 0000000..c2a419b
>> --- /dev/null
>> +++ b/drivers/crypto/rockchip/rk3288_crypto.c
> [...]
>
>> +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);
> -------------------
>
Ok! done!
>> + 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..cf4cd18
>> --- /dev/null
>> +++ b/drivers/crypto/rockchip/rk3288_crypto.h
>> @@ -0,0 +1,222 @@
> [...]
>
>> +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
Ok! Done!
>
> [...]
>
>> + struct device *dev;
>> + struct clk *aclk;
>> + struct clk *hclk;
>> + struct clk *sclk;
>> + struct clk *dmaclk;
>> + void __iomem *reg;
>> + int irq;
>> + struct crypto_queue queue;
>> + struct tasklet_struct crypto_tasklet;
>> + struct ablkcipher_request *ablk_req;
>> + /* device lock */
>> + spinlock_t lock;
>> +
>> + /* the public variable */
>> + struct scatterlist *sg_src;
>> + struct scatterlist *sg_dst;
>> + struct scatterlist sg_tmp;
>> + struct scatterlist *first;
>> + unsigned int left_bytes;
>> + void *addr_vir;
>> + int aligned;
>> + int align_size;
>> + size_t nents;
>> + unsigned int total;
>> + unsigned int count;
>> + u32 mode;
>> + dma_addr_t addr_in;
>> + dma_addr_t addr_out;
>> + int (*start)(struct crypto_info_t *dev);
>> + int (*update)(struct crypto_info_t *dev);
>> + void (*complete)(struct crypto_info_t *dev, int err);
>> + int (*enable_clk)(struct crypto_info_t *dev);
>> + void (*disable_clk)(struct crypto_info_t *dev);
>> + int (*load_data)(struct crypto_info_t *dev,
>> + struct scatterlist *sg_src,
>> + struct scatterlist *sg_dst);
>> + void (*unload_data)(struct crypto_info_t *dev);
>> +};
>> +
>> +/* the private variable of cipher */
>> +struct rk_cipher_ctx {
>> + struct crypto_info_t *dev;
>> + unsigned int keylen;
>> +};
>> +
>> +extern struct crypto_info_t *crypto_p;
>> +
>> +extern struct crypto_alg rk_ecb_aes_alg;
>> +extern struct crypto_alg rk_cbc_aes_alg;
>> +extern struct crypto_alg rk_ecb_des_alg;
>> +extern struct crypto_alg rk_cbc_des_alg;
>> +extern struct crypto_alg rk_ecb_des3_ede_alg;
>> +extern struct crypto_alg rk_cbc_des3_ede_alg;
>> +
>> +#endif
>> diff --git a/drivers/crypto/rockchip/rk3288_crypto_ablkcipher.c b/drivers/crypto/rockchip/rk3288_crypto_ablkcipher.c
>> new file mode 100644
>> index 0000000..28b49c9
>> --- /dev/null
>> +++ b/drivers/crypto/rockchip/rk3288_crypto_ablkcipher.c
>> @@ -0,0 +1,511 @@
> [...]
>
>> +static int rk_ablk_cra_init(struct crypto_tfm *tfm)
>> +{
>> + struct rk_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
>> +
>> + ctx->dev = crypto_p;
> 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
>
> Same of course for the other crypto_p users.
Ok! Done!
>
>
> Thanks
> Heiko
>
>
>
Thanks
Zain