[PATCH v3 5/6] hwrng: exynos: Add SMC based TRNG operation

[Sam Protsenko]

On Fri, Jun 21, 2024 at 2:00 PM Lukasz Stelmach <l.stelmach at samsung.com> wrote:
>
> It was <2024-06-20 czw 18:13>, when Sam Protsenko wrote:
> > On some Exynos chips like Exynos850 the access to Security Sub System
> > (SSS) registers is protected with TrustZone, and therefore only possible
> > from EL3 monitor software. The Linux kernel is running in EL1, so the
> > only way for the driver to obtain TRNG data is via SMC calls to EL3
> > monitor. Implement such SMC operation and use it when EXYNOS_SMC flag is
> > set in the corresponding chip driver data.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko at linaro.org>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski at linaro.org>
> > ---
> > Changes in v3:
> >   - Added appropriate error messages for the case when init SMC command fails
> >
> > Changes in v2:
> >   - Used the "reversed Christmas tree" style in the variable declaration
> >     block in exynos_trng_do_read_smc()
> >   - Renamed .quirks to .flags in the driver structure
> >   - Added Krzysztof's R-b tag
> >
> >  drivers/char/hw_random/exynos-trng.c | 140 +++++++++++++++++++++++++--
> >  1 file changed, 130 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/char/hw_random/exynos-trng.c b/drivers/char/hw_random/exynos-trng.c
> > index 6ef2ee6c9804..9fa30583cc86 100644
> > --- a/drivers/char/hw_random/exynos-trng.c
> > +++ b/drivers/char/hw_random/exynos-trng.c
>
> [...]
>
>
> > @@ -103,6 +163,24 @@ static int exynos_trng_init(struct hwrng *rng)
> >       return 0;
> >  }
> >
> > +static int exynos_trng_init_smc(struct hwrng *rng)
> > +{
> > +     struct exynos_trng_dev *trng = (struct exynos_trng_dev *)rng->priv;
> > +     struct arm_smccc_res res;
> > +     int ret = 0;
> > +
> > +     arm_smccc_smc(SMC_CMD_RANDOM, HWRNG_INIT, 0, 0, 0, 0, 0, 0, &res);
> > +     if (res.a0 != HWRNG_RET_OK) {
> > +             dev_err(trng->dev, "SMC command for TRNG init failed (%d)\n",
> > +                     (int)res.a0);
> > +             ret = -EIO;
> > +     }
> > +     if ((int)res.a0 == -1)
> > +             dev_info(trng->dev, "Make sure LDFW is loaded by your BL\n");
>
> This is good, thank you for adding it. It can be even better though, if
> you don't skimp on message length (-; I mean, I know what BL is, I can
> fingure what LDFW is because you have explained to me and I can see the
> source code, but somewone who sees it for the first time will be only
> slightly less surprised than with v2 error message only. Come on, you
> can make this message twice as long and it will still fit in 80 characters (-;
>

Guess my OCD got in the way and I just didn't want to break the line
:) But yeah, LDFW = Loadable Firmware, and BL = bootloader. There is
an "ldfw" partition on eMMC, and I noticed Samsung usually uses LDFW
term, so I figured it was not a big deal to throw that abbreviation at
the user. But I totally agree on BL part, it might be confusing. I
don't have any strong opinion on this one. If you are going to apply
v3, can I kindly ask you to change that message the way you want it to
be?

> Don't change it if v3 is the last. If not, please, make it more verbose.
>
> > +
> > +     return ret;
> > +}
> > +
>
>
> [...]
>
>
> Kind regards,
> --
> Łukasz Stelmach
> Samsung R&D Institute Poland
> Samsung Electronics