Re: [PATCH 1/2] drivers: tee: optee: discover OP-TEE services

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Etienne, 

On Wed, Jun 01, 2022 at 10:27:51AM +0200, Etienne Carriere wrote:
> This change defines resources for OP-TEE service drivers to register
> themselves for being bound to when OP-TEE firmware reports the related
> service is supported. OP-TEE services are discovered during optee
> driver probe sequence. Discovery of optee services and binding to
> related U-Boot drivers is embedded upon configuration switch
> CONFIG_OPTEE_SERVICE_DISCOVERY.
> 
> Cc: Jens Wiklander <jens.wiklander@linaro.org>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  drivers/tee/optee/Kconfig   |   8 ++
>  drivers/tee/optee/core.c    | 187 +++++++++++++++++++++++++++++++++++-
>  include/tee/optee_service.h |  29 ++++++
>  3 files changed, 223 insertions(+), 1 deletion(-)
>  create mode 100644 include/tee/optee_service.h
> 
> diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig
> index d03028070b..9dc65b0501 100644
> --- a/drivers/tee/optee/Kconfig
> +++ b/drivers/tee/optee/Kconfig
> @@ -37,6 +37,14 @@ config OPTEE_TA_SCP03
>  
 
[...]

> +static int enum_services(struct udevice *dev, struct tee_shm **shm, size_t *count, u32 tee_sess)
> +{
> +	size_t shm_size = 0;
> +	int ret;
> +
> +	ret = __enum_services(dev, NULL, &shm_size, tee_sess);
> +	if (ret)
> +		return ret;
> +
> +	ret = tee_shm_alloc(dev, shm_size, 0, shm);
> +	if (ret) {
> +		dev_err(dev, "Failed to allocated shared memory: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = __enum_services(dev, *shm, &shm_size, tee_sess);
> +	if (ret)
> +		tee_shm_free(*shm);

I'd prefer if we handled this a bit differently.  Instead of freeing the
buffer here, just release it on bind_service_drivers() always

> +	else
> +		*count = shm_size / sizeof(struct tee_optee_ta_uuid);
> +
> +	return ret;
> +}
> +
> +
>  static int optee_probe(struct udevice *dev)
>  {
>  	struct optee_pdata *pdata = dev_get_plat(dev);
>  	u32 sec_caps;
> -	struct udevice *child;
>  	int ret;
>  
>  	if (!is_optee_api(pdata->invoke_fn)) {
> @@ -668,15 +842,23 @@ static int optee_probe(struct udevice *dev)
>  		return -ENOENT;
>  	}
>  
> +	ret = bind_service_drivers(dev);
> +	if (ret)
> +		return ret;
> +
> +#ifndef CONFIG_OPTEE_SERVICE_DISCOVERY
>  	/*
>  	 * in U-Boot, the discovery of TA on the TEE bus is not supported:
>  	 * only bind the drivers associated to the supported OP-TEE TA
>  	 */
>  	if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
> +		struct udevice *child;
> +
>  		ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);

The same principle applies for fTPM.  Moreover the linux kernel supports
bus scanning, which creates a conflict when the fTPM is added on the .dts
(for u-boot to scan it).  

Can we make this a bit more generic, even though only the rng is added on
this patch?

something like 
struct devices {
	const char *drv_name;
	const char *dev_name;
} tee_bus_devices = {
	{
		"optee-rng",
		"optee-rng",
	},
}
and add an array of the 'scanable' devices?  It would make adding the ftpm
and other devices trivial

>  		if (ret)
>  			return ret;
>  	}
> +#endif
[...]


Thanks!
/Ilias