Re: [PATCH v2 2/3] nvmem: stm32: add OP-TEE support for STM32MP13x

[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@xxxxxxxxxxx>
> > ---
> >
> > 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