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

[Ard Biesheuvel]

On Fri, 1 Mar 2019 at 10:56, Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> On Fri, 22 Feb 2019 at 13:22, Kamil Konieczny
> <k.konieczny@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > Add AES crypto HW acceleration for Exynos5433, with the help of SlimSSS IP.
> >
> > Reviewed-by: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> > Signed-off-by: Kamil Konieczny <k.konieczny@xxxxxxxxxxxxxxxxxxx>
> > ---
> >  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[];
> >  };
> >
> >  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", },
> >  };
> >
> >  static const struct samsung_aes_variant exynos_aes_data = {
> >         .aes_offset     = 0x200,
> >         .hash_offset    = 0x400,
> > +       .clk_names      = { "secss", },
> > +};
> > +
> > +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]) {
> > +               pdata->pclk = devm_clk_get(dev, variant->clk_names[1]);
>
> I see on today's linux next:
> [ 6.391256] Unable to handle kernel paging request at virtual address 00004000
> on all my Exynos boards (U3, XU, XU3, HC1).
> I did not do a bisect yet but it might be connected with this commit.
>
> The call trace is:
> (strcmp) from [<c0774014>] (of_property_match_string+0x58/0xd0)
> (of_property_match_string) from [<c04d3d2c>] (__of_clk_get_by_name+0x5c/0x130)
> (__of_clk_get_by_name) from [<c04d3fa0>] (clk_get+0x34/0x70)
> (clk_get) from [<c04d3680>] (devm_clk_get+0x38/0x74)
> (devm_clk_get) from [<c075c248>] (s5p_aes_probe+0xd4/0x4a0)
> (s5p_aes_probe) from [<c059dbc4>] (platform_drv_probe+0x6c/0xa4)
> (platform_drv_probe) from [<c059b4c8>] (really_probe+0x280/0x414)
> (really_probe) from [<c059b810>] (driver_probe_device+0x78/0x1c0)
> (driver_probe_device) from [<c059bbb8>] (device_driver_attach+0x58/0x60)
> (device_driver_attach) from [<c059bcbc>] (__driver_attach+0xfc/0x160)
> (__driver_attach) from [<c059928c>] (bus_for_each_dev+0x68/0xb4)
> (bus_for_each_dev) from [<c059a5c4>] (bus_add_driver+0x158/0x214)
> (bus_add_driver) from [<c059cb94>] (driver_register+0x78/0x110)
> (driver_register) from [<c01031f8>] (do_one_initcall+0x8c/0x404)
>
> Full log:
> https://krzk.eu/#/builders/21/builds/1153/steps/12/logs/serial0
>
> The s5p_aes_probe address points here.
>

It seems that dereferencing clk_names[1] of struct samsung_aes_variant
exynos_aes_data

> > +       .clk_names      = { "secss", },

returns the first member of

> >  static const struct samsung_aes_variant s5p_aes_data = {
> >         .aes_offset     = 0x4000,

since the array has only one entry in this case, and so the second,
non-existing entry overlaps with the next data item in the object
file.

So the fix would be to use

.clk_names      = { "secss", NULL },

instead (or define the array to have 2 members)


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