It was <2024-06-21 pią 14:40>, when Sam Protsenko wrote: > On Fri, Jun 21, 2024 at 2:00 PM Lukasz Stelmach <l.stelmach@xxxxxxxxxxx> 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@xxxxxxxxxx> >> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> >> > --- >> > 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? I guess Olivia or Herbert will be applying it. Let me try… How about: "Check if your bootloader loads the firmware (SMC) part of the driver." >> 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 > -- Łukasz Stelmach Samsung R&D Institute Poland Samsung Electronics
Attachment:
signature.asc
Description: PGP signature