Messages in this thread | | | Date | Mon, 14 Nov 2022 15:20:56 +0100 | Subject | Re: [PATCH v2 2/3] nvmem: stm32: add OP-TEE support for STM32MP13x | From | Patrick DELAUNAY <> |
| |
Hi,
On 11/11/22 18:18, Srinivas Kandagatla wrote: > > > On 10/11/2022 15:45, Patrick Delaunay wrote: >> For boot with OP-TEE on STM32MP13, the communication with the secure >> world no more use STMicroelectronics SMC but communication with the >> BSEC TA, for data access (read/write) or lock operation: >> - all the request are sent to OP-TEE trusted application, >> - for upper OTP with ECC protection and with word programming only >> each OTP are permanently locked when programmed to avoid ECC error >> on the second write operation >> >> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> >> --- >> >> Changes in v2: >> - rebase series on linux-next/master >> - minor update after V1 revue >> - add missing sentinel in stm32_romem_of_match() >> - reorder function and remove prototypes for stm32_bsec_pta... functions >> - change stm32_bsec_pta_find to static >> - add return value in dev_err() >> - cleanups some comments, which can be on one line >> - remove test on priv->ctx in stm32_bsec_pta_remove >> - add missing tee_shm_free(shm) in stm32_bsec_pta_write() when >> tee_shm_get_va failed >> - return error in stm32_bsec_pta_find when devm_add_action_or_reset >> failed >> - handle driver_register error in stm32_romem_init >> >> drivers/nvmem/stm32-romem.c | 445 +++++++++++++++++++++++++++++++++++- >> 1 file changed, 441 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/nvmem/stm32-romem.c b/drivers/nvmem/stm32-romem.c >> index d1d03c2ad081..0a0e29d09b67 100644 >> --- a/drivers/nvmem/stm32-romem.c >> +++ b/drivers/nvmem/stm32-romem.c >> @@ -11,6 +11,7 @@ >> #include <linux/module.h> >> #include <linux/nvmem-provider.h> >> #include <linux/of_device.h> >> +#include <linux/tee_drv.h> >> /* BSEC secure service access from non-secure */ >> #define STM32_SMC_BSEC 0x82001003 >> @@ -25,14 +26,401 @@ >> struct stm32_romem_cfg { >> int size; >> u8 lower; >> + bool ta; >> }; >> struct stm32_romem_priv { >> void __iomem *base; >> struct nvmem_config cfg; >> u8 lower; >> + struct device *ta; >> }; >> +#if IS_ENABLED(CONFIG_OPTEE) >> +/* > > ... > >> + >> +static const struct tee_client_device_id stm32_bsec_id_table[] = { >> + { >> + UUID_INIT(0x94cf71ad, 0x80e6, 0x40b5, >> + 0xa7, 0xc6, 0x3d, 0xc5, 0x01, 0xeb, 0x28, 0x03) >> + }, >> + { } >> +}; >> + >> +MODULE_DEVICE_TABLE(tee, stm32_bsec_id_table); >> + >> +static struct tee_client_driver stm32_bsec_pta_driver = { >> + .id_table = stm32_bsec_id_table, >> + .driver = { >> + .name = "stm32-bsec-pta", >> + .bus = &tee_bus_type, >> + .probe = stm32_bsec_pta_probe, >> + .remove = stm32_bsec_pta_remove, >> + }, >> +}; >> + >> +static void stm32_bsec_put_device(void *data) >> +{ >> + put_device(data); >> +} >> + >> +static struct device *stm32_bsec_pta_find(struct device *dev) >> +{ >> + struct device *pta_dev; >> + int ret; >> + >> + pta_dev = driver_find_next_device(&stm32_bsec_pta_driver.driver, >> NULL); > > This is clearly not representing the dependencies in a proper device > model. > > > If the nvmem provider is a TEE client driver lets model it that way.. > brining in a additional device and somehow trying to link it with TEE > driver is a hack. >
TEE is a firmware which allow access to secure ressource... including BSEC ressources
I think it is also the case on a other driver = mson_sm.c
=> econfig->priv = fw;
fw is a handle to the firmware (secure monitor) which provide access to secure ressource
BSEC is a hardware device on the bus,
it it describe in the device tree, with a compatible,
the same description should be used for any SW, not only Linux kernel.
and the nvmem cell description are sub-node of BSEC node, used as phandle by other device.
I need to have a link between the NVMEM driver and the OP-TEE session;
But I use the tee bus discovery here it is a error,
because that create a second uneeded driver "stm32_bsec_pta_driver"...
I will remove this part, and only kept the PTA request with new lib functions "stm32_bsec_pta_XXX()".
> >> + >> + if (pta_dev) { >> + ret = devm_add_action_or_reset(dev, stm32_bsec_put_device, >> pta_dev); >> + if (ret) >> + dev_err(dev, "devm_add_action_or_reset() failed (%d)\n", >> ret); >> + >> + return ERR_PTR(ret); >> + } >> + >> + return pta_dev; >> +} >> + >> +#else >> +static int stm32_bsec_pta_read(void *context, unsigned int offset, >> void *buf, >> + size_t bytes) >> +{ >> + pr_debug("%s: TA BSEC request without OPTEE support\n", __func__); >> + >> + return -ENXIO; >> +} >> + >> +static int stm32_bsec_pta_write(void *context, unsigned int offset, >> void *buf, >> + size_t bytes) >> +{ >> + pr_debug("%s: TA BSEC request without OPTEE support\n", __func__); >> + >> + return -ENXIO; >> +} >> + >> +static struct device *stm32_bsec_pta_find(struct device *dev) >> +{ >> + pr_debug("%s: TA BSEC request without OPTEE support\n", __func__); >> + >> + return NULL; >> +} >> +#endif > > ifdefing inside the drvier is really ugly, please move this libary > functions to a seperate file and add dependecy properly in Kconfig.
Ok
regards
Patrick
| |