Re: [PATCH v4 3/3] crypto: s5p: add AES support for Exynos5433

[Guenter Roeck <linux@roeck-us.net>]

On Fri, Feb 22, 2019 at 01:21:44PM +0100, Kamil Konieczny wrote:
> Add AES crypto HW acceleration for Exynos5433, with the help of SlimSSS IP.
> 
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
>  drivers/crypto/s5p-sss.c | 50 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
> index 0064be0e3941..3f45cc5cb94a 100644
> --- a/drivers/crypto/s5p-sss.c
> +++ b/drivers/crypto/s5p-sss.c
> @@ -232,6 +232,7 @@
>   * struct samsung_aes_variant - platform specific SSS driver data
>   * @aes_offset: AES register offset from SSS module's base.
>   * @hash_offset: HASH register offset from SSS module's base.
> + * @clk_names: names of clocks needed to run SSS IP
>   *
>   * Specifies platform specific configuration of SSS module.
>   * Note: A structure for driver specific platform data is used for future
> @@ -240,6 +241,7 @@
>  struct samsung_aes_variant {
>  	unsigned int			aes_offset;
>  	unsigned int			hash_offset;
> +	const char			*clk_names[];

This array does not have a fixed size.

>  };
>  
>  struct s5p_aes_reqctx {
> @@ -296,6 +298,7 @@ struct s5p_aes_ctx {
>  struct s5p_aes_dev {
>  	struct device			*dev;
>  	struct clk			*clk;
> +	struct clk			*pclk;
>  	void __iomem			*ioaddr;
>  	void __iomem			*aes_ioaddr;
>  	int				irq_fc;
> @@ -384,11 +387,19 @@ struct s5p_hash_ctx {
>  static const struct samsung_aes_variant s5p_aes_data = {
>  	.aes_offset	= 0x4000,
>  	.hash_offset	= 0x6000,
> +	.clk_names	= { "secss", },

In this instantiation, there is no [1] element.

>  };
>  
>  static const struct samsung_aes_variant exynos_aes_data = {
>  	.aes_offset	= 0x200,
>  	.hash_offset	= 0x400,
> +	.clk_names	= { "secss", },

and neither is here.

> +};
> +
> +static const struct samsung_aes_variant exynos5433_slim_aes_data = {
> +	.aes_offset	= 0x400,
> +	.hash_offset	= 0x800,
> +	.clk_names	= { "pclk", "aclk", },
>  };
>  
>  static const struct of_device_id s5p_sss_dt_match[] = {
> @@ -400,6 +411,10 @@ static const struct of_device_id s5p_sss_dt_match[] = {
>  		.compatible = "samsung,exynos4210-secss",
>  		.data = &exynos_aes_data,
>  	},
> +	{
> +		.compatible = "samsung,exynos5433-slim-sss",
> +		.data = &exynos5433_slim_aes_data,
> +	},
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, s5p_sss_dt_match);
> @@ -2208,18 +2223,39 @@ static int s5p_aes_probe(struct platform_device *pdev)
>  			return PTR_ERR(pdata->ioaddr);
>  	}
>  
> -	pdata->clk = devm_clk_get(dev, "secss");
> +	pdata->clk = devm_clk_get(dev, variant->clk_names[0]);
>  	if (IS_ERR(pdata->clk)) {
> -		dev_err(dev, "failed to find secss clock source\n");
> +		dev_err(dev, "failed to find secss clock %s\n",
> +			variant->clk_names[0]);
>  		return -ENOENT;
>  	}
>  
>  	err = clk_prepare_enable(pdata->clk);
>  	if (err < 0) {
> -		dev_err(dev, "Enabling SSS clk failed, err %d\n", err);
> +		dev_err(dev, "Enabling clock %s failed, err %d\n",
> +			variant->clk_names[0], err);
>  		return err;
>  	}
>  
> +	if (variant->clk_names[1]) {

This results in

[   31.721963] Unhandled fault: page domain fault (0x01b) at 0x00004000
[   31.722417] pgd = (ptrval)
[   31.722611] [00004000] *pgd=00000000
[   31.723519] Internal error: : 1b [#1] PREEMPT SMP ARM
...
[   31.753264] [<c0be9978>] (strcmp) from [<c08cad50>] (of_property_match_string+0x58/0xd0)
[   31.754012] [<c08cad50>] (of_property_match_string) from [<c061eaac>] (of_clk_get_hw+0x5c/0x114)
[   31.754731] [<c061eaac>] (of_clk_get_hw) from [<c061656c>] (clk_get+0x34/0x74)
[   31.755397] [<c061656c>] (clk_get) from [<c0615dd8>] (devm_clk_get+0x3c/0x6c)
[   31.756018] [<c0615dd8>] (devm_clk_get) from [<c08b3298>] (s5p_aes_probe+0xcc/0x508)
[   31.756892] [<c08b3298>] (s5p_aes_probe) from [<c06e6740>] (platform_drv_probe+0x48/0x98)
[   31.757571] [<c06e6740>] (platform_drv_probe) from [<c06e40a4>] (really_probe+0x2bc/0x408)
[   31.758245] [<c06e40a4>] (really_probe) from [<c06e43a4>] (driver_probe_device+0x78/0x1cc)
[   31.758913] [<c06e43a4>] (driver_probe_device) from [<c06e4764>] (device_driver_attach+0x58/0x60)
[   31.759578] [<c06e4764>] (device_driver_attach) from [<c06e483c>] (__driver_attach+0xd0/0x168)
[   31.760010] [<c06e483c>] (__driver_attach) from [<c06e1e38>] (bus_for_each_dev+0x74/0xb4)
[   31.760354] [<c06e1e38>] (bus_for_each_dev) from [<c06e3174>] (bus_add_driver+0x174/0x210)
[   31.760697] [<c06e3174>] (bus_add_driver) from [<c06e572c>] (driver_register+0x74/0x108)
[   31.761039] [<c06e572c>] (driver_register) from [<c010325c>] (do_one_initcall+0x90/0x428)
[   31.761390] [<c010325c>] (do_one_initcall) from [<c1001318>] (kernel_init_freeable+0x42c/0x4cc)
[   31.761752] [<c1001318>] (kernel_init_freeable) from [<c0bf3ae8>] (kernel_init+0x8/0x118)
[   31.762093] [<c0bf3ae8>] (kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20)

when booting one of the "old" platforms where variant->clk_names[1]
points beyond the end of the array.

Guenter

> +		pdata->pclk = devm_clk_get(dev, variant->clk_names[1]);
> +		if (IS_ERR(pdata->pclk)) {
> +			dev_err(dev, "failed to find clock %s\n",
> +				variant->clk_names[1]);
> +			err = -ENOENT;
> +			goto err_clk;
> +		}
> +
> +		err = clk_prepare_enable(pdata->pclk);
> +		if (err < 0) {
> +			dev_err(dev, "Enabling clock %s failed, err %d\n",
> +				variant->clk_names[0], err);
> +			goto err_clk;
> +		}
> +	} else {
> +		pdata->pclk = NULL;
> +	}
> +
>  	spin_lock_init(&pdata->lock);
>  	spin_lock_init(&pdata->hash_lock);
>  
> @@ -2295,8 +2331,11 @@ static int s5p_aes_probe(struct platform_device *pdev)
>  	tasklet_kill(&pdata->tasklet);
>  
>  err_irq:
> -	clk_disable_unprepare(pdata->clk);
> +	if (pdata->pclk)
> +		clk_disable_unprepare(pdata->pclk);
>  
> +err_clk:
> +	clk_disable_unprepare(pdata->clk);
>  	s5p_dev = NULL;
>  
>  	return err;
> @@ -2323,6 +2362,9 @@ static int s5p_aes_remove(struct platform_device *pdev)
>  		pdata->use_hash = false;
>  	}
>  
> +	if (pdata->pclk)
> +		clk_disable_unprepare(pdata->pclk);
> +
>  	clk_disable_unprepare(pdata->clk);
>  	s5p_dev = NULL;
>