[PATCH 1/2] tee: optee: don't fail probe because of optee-rng

[PATCH 1/2] tee: optee: don't fail probe because of optee-rng

[Etienne Carriere]

Fixes optee-rng driver bind sequence in optee driver to print a warning
message but not report an error status when a optee-rng service driver
fails to be bound as the optee driver itself is still fully functional.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 drivers/tee/optee/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 9240277579..604fd1414f 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -834,7 +834,7 @@ static int optee_probe(struct udevice *dev)
 		 */
 		ret = device_bind_driver(dev, "optee-rng", "optee-rng", NULL);
 		if (ret)
-			return ret;
+			dev_warn(dev, "ftpm_tee failed to bind: %d\n", ret);
 	}
 
 	return 0;
-- 
2.25.1

[PATCH 2/2] tee: optee: discover services dependent on tee-supplicant

[Etienne Carriere]

Makes OP-TEE to enumerate also services depending on tee-supplicant
support in U-Boot. This change allows OP-TEE services like fTPM TA
to be discovered and get a TPM device registered in U-Boot.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 drivers/tee/optee/core.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 604fd1414f..b21031d7d8 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -102,13 +102,14 @@ static int bind_service_list(struct udevice *dev, struct tee_shm *service_list,
 	return 0;
 }
 
-static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm_size, u32 tee_sess)
+static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm_size, u32 tee_sess,
+			   unsigned int pta_cmd)
 {
 	struct tee_invoke_arg arg = { };
 	struct tee_param param = { };
 	int ret = 0;
 
-	arg.func = PTA_CMD_GET_DEVICES;
+	arg.func = pta_cmd;
 	arg.session = tee_sess;
 
 	/* Fill invoke cmd params */
@@ -118,7 +119,7 @@ static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm
 
 	ret = tee_invoke_func(dev, &arg, 1, &param);
 	if (ret || (arg.ret && arg.ret != TEE_ERROR_SHORT_BUFFER)) {
-		dev_err(dev, "PTA_CMD_GET_DEVICES invoke function err: 0x%x\n", arg.ret);
+		dev_err(dev, "Enumeration command 0x%x failed: 0x%x\n", pta_cmd, arg.ret);
 		return -EINVAL;
 	}
 
@@ -127,12 +128,13 @@ static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm
 	return 0;
 }
 
-static int enum_services(struct udevice *dev, struct tee_shm **shm, size_t *count, u32 tee_sess)
+static int enum_services(struct udevice *dev, struct tee_shm **shm, size_t *count, u32 tee_sess,
+			 unsigned int pta_cmd)
 {
 	size_t shm_size = 0;
 	int ret;
 
-	ret = __enum_services(dev, NULL, &shm_size, tee_sess);
+	ret = __enum_services(dev, NULL, &shm_size, tee_sess, pta_cmd);
 	if (ret)
 		return ret;
 
@@ -142,7 +144,7 @@ static int enum_services(struct udevice *dev, struct tee_shm **shm, size_t *coun
 		return ret;
 	}
 
-	ret = __enum_services(dev, *shm, &shm_size, tee_sess);
+	ret = __enum_services(dev, *shm, &shm_size, tee_sess, pta_cmd);
 	if (!ret)
 		*count = shm_size / sizeof(struct tee_optee_ta_uuid);
 
@@ -174,20 +176,32 @@ static int bind_service_drivers(struct udevice *dev)
 	struct tee_shm *service_list = NULL;
 	size_t service_count;
 	u32 tee_sess;
-	int ret;
+	int ret, ret2;
 
 	ret = open_enum_session(dev, &tee_sess);
 	if (ret)
 		return ret;
 
-	ret = enum_services(dev, &service_list, &service_count, tee_sess);
+	ret = enum_services(dev, &service_list, &service_count, tee_sess,
+			    PTA_CMD_GET_DEVICES);
 	if (!ret)
 		ret = bind_service_list(dev, service_list, service_count);
 
 	tee_shm_free(service_list);
+
+	ret2 = enum_services(dev, &service_list, &service_count, tee_sess,
+			     PTA_CMD_GET_DEVICES_SUPP);
+	if (!ret2)
+		ret2 = bind_service_list(dev, service_list, service_count);
+
+	tee_shm_free(service_list);
+
 	tee_close_session(dev, tee_sess);
 
-	return ret;
+	if (ret)
+		return ret;
+
+	return ret2;
 }
 
 /**
-- 
2.25.1

Re: [PATCH 1/2] tee: optee: don't fail probe because of optee-rng

[Ilias Apalodimas]

On Wed, Dec 07, 2022 at 04:29:59PM +0100, Etienne Carriere wrote:
> Fixes optee-rng driver bind sequence in optee driver to print a warning
> message but not report an error status when a optee-rng service driver
> fails to be bound as the optee driver itself is still fully functional.
>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  drivers/tee/optee/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 9240277579..604fd1414f 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -834,7 +834,7 @@ static int optee_probe(struct udevice *dev)
>  		 */
>  		ret = device_bind_driver(dev, "optee-rng", "optee-rng", NULL);
>  		if (ret)
> -			return ret;
> +			dev_warn(dev, "ftpm_tee failed to bind: %d\n", ret);
>  	}
>
>  	return 0;
> --
> 2.25.1
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 2/2] tee: optee: discover services dependent on tee-supplicant

[Ilias Apalodimas]

On Wed, Dec 07, 2022 at 04:30:00PM +0100, Etienne Carriere wrote:
> Makes OP-TEE to enumerate also services depending on tee-supplicant
> support in U-Boot. This change allows OP-TEE services like fTPM TA
> to be discovered and get a TPM device registered in U-Boot.
>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  drivers/tee/optee/core.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 604fd1414f..b21031d7d8 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -102,13 +102,14 @@ static int bind_service_list(struct udevice *dev, struct tee_shm *service_list,
>  	return 0;
>  }
>
> -static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm_size, u32 tee_sess)
> +static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm_size, u32 tee_sess,
> +			   unsigned int pta_cmd)
>  {
>  	struct tee_invoke_arg arg = { };
>  	struct tee_param param = { };
>  	int ret = 0;
>
> -	arg.func = PTA_CMD_GET_DEVICES;
> +	arg.func = pta_cmd;
>  	arg.session = tee_sess;
>
>  	/* Fill invoke cmd params */
> @@ -118,7 +119,7 @@ static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm
>
>  	ret = tee_invoke_func(dev, &arg, 1, &param);
>  	if (ret || (arg.ret && arg.ret != TEE_ERROR_SHORT_BUFFER)) {
> -		dev_err(dev, "PTA_CMD_GET_DEVICES invoke function err: 0x%x\n", arg.ret);
> +		dev_err(dev, "Enumeration command 0x%x failed: 0x%x\n", pta_cmd, arg.ret);
>  		return -EINVAL;
>  	}
>
> @@ -127,12 +128,13 @@ static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm
>  	return 0;
>  }
>
> -static int enum_services(struct udevice *dev, struct tee_shm **shm, size_t *count, u32 tee_sess)
> +static int enum_services(struct udevice *dev, struct tee_shm **shm, size_t *count, u32 tee_sess,
> +			 unsigned int pta_cmd)
>  {
>  	size_t shm_size = 0;
>  	int ret;
>
> -	ret = __enum_services(dev, NULL, &shm_size, tee_sess);
> +	ret = __enum_services(dev, NULL, &shm_size, tee_sess, pta_cmd);
>  	if (ret)
>  		return ret;
>
> @@ -142,7 +144,7 @@ static int enum_services(struct udevice *dev, struct tee_shm **shm, size_t *coun
>  		return ret;
>  	}
>
> -	ret = __enum_services(dev, *shm, &shm_size, tee_sess);
> +	ret = __enum_services(dev, *shm, &shm_size, tee_sess, pta_cmd);
>  	if (!ret)
>  		*count = shm_size / sizeof(struct tee_optee_ta_uuid);
>
> @@ -174,20 +176,32 @@ static int bind_service_drivers(struct udevice *dev)
>  	struct tee_shm *service_list = NULL;
>  	size_t service_count;
>  	u32 tee_sess;
> -	int ret;
> +	int ret, ret2;
>
>  	ret = open_enum_session(dev, &tee_sess);
>  	if (ret)
>  		return ret;
>
> -	ret = enum_services(dev, &service_list, &service_count, tee_sess);
> +	ret = enum_services(dev, &service_list, &service_count, tee_sess,
> +			    PTA_CMD_GET_DEVICES);
>  	if (!ret)
>  		ret = bind_service_list(dev, service_list, service_count);
>
>  	tee_shm_free(service_list);
> +
> +	ret2 = enum_services(dev, &service_list, &service_count, tee_sess,
> +			     PTA_CMD_GET_DEVICES_SUPP);
> +	if (!ret2)
> +		ret2 = bind_service_list(dev, service_list, service_count);
> +
> +	tee_shm_free(service_list);
> +
>  	tee_close_session(dev, tee_sess);
>
> -	return ret;
> +	if (ret)
> +		return ret;
> +
> +	return ret2;

All looks good, mind sending a v2 converting this to a ternary operator?

with or without
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Regards
/Ilias



>  }
>
>  /**
> --
> 2.25.1
>

Re: [PATCH 2/2] tee: optee: discover services dependent on tee-supplicant

[Jens Wiklander]

On Wed, Dec 7, 2022 at 4:30 PM Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Makes OP-TEE to enumerate also services depending on tee-supplicant
> support in U-Boot. This change allows OP-TEE services like fTPM TA
> to be discovered and get a TPM device registered in U-Boot.
>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  drivers/tee/optee/core.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Thanks,
Jens

>
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 604fd1414f..b21031d7d8 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -102,13 +102,14 @@ static int bind_service_list(struct udevice *dev, struct tee_shm *service_list,
>         return 0;
>  }
>
> -static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm_size, u32 tee_sess)
> +static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm_size, u32 tee_sess,
> +                          unsigned int pta_cmd)
>  {
>         struct tee_invoke_arg arg = { };
>         struct tee_param param = { };
>         int ret = 0;
>
> -       arg.func = PTA_CMD_GET_DEVICES;
> +       arg.func = pta_cmd;
>         arg.session = tee_sess;
>
>         /* Fill invoke cmd params */
> @@ -118,7 +119,7 @@ static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm
>
>         ret = tee_invoke_func(dev, &arg, 1, &param);
>         if (ret || (arg.ret && arg.ret != TEE_ERROR_SHORT_BUFFER)) {
> -               dev_err(dev, "PTA_CMD_GET_DEVICES invoke function err: 0x%x\n", arg.ret);
> +               dev_err(dev, "Enumeration command 0x%x failed: 0x%x\n", pta_cmd, arg.ret);
>                 return -EINVAL;
>         }
>
> @@ -127,12 +128,13 @@ static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm
>         return 0;
>  }
>
> -static int enum_services(struct udevice *dev, struct tee_shm **shm, size_t *count, u32 tee_sess)
> +static int enum_services(struct udevice *dev, struct tee_shm **shm, size_t *count, u32 tee_sess,
> +                        unsigned int pta_cmd)
>  {
>         size_t shm_size = 0;
>         int ret;
>
> -       ret = __enum_services(dev, NULL, &shm_size, tee_sess);
> +       ret = __enum_services(dev, NULL, &shm_size, tee_sess, pta_cmd);
>         if (ret)
>                 return ret;
>
> @@ -142,7 +144,7 @@ static int enum_services(struct udevice *dev, struct tee_shm **shm, size_t *coun
>                 return ret;
>         }
>
> -       ret = __enum_services(dev, *shm, &shm_size, tee_sess);
> +       ret = __enum_services(dev, *shm, &shm_size, tee_sess, pta_cmd);
>         if (!ret)
>                 *count = shm_size / sizeof(struct tee_optee_ta_uuid);
>
> @@ -174,20 +176,32 @@ static int bind_service_drivers(struct udevice *dev)
>         struct tee_shm *service_list = NULL;
>         size_t service_count;
>         u32 tee_sess;
> -       int ret;
> +       int ret, ret2;
>
>         ret = open_enum_session(dev, &tee_sess);
>         if (ret)
>                 return ret;
>
> -       ret = enum_services(dev, &service_list, &service_count, tee_sess);
> +       ret = enum_services(dev, &service_list, &service_count, tee_sess,
> +                           PTA_CMD_GET_DEVICES);
>         if (!ret)
>                 ret = bind_service_list(dev, service_list, service_count);
>
>         tee_shm_free(service_list);
> +
> +       ret2 = enum_services(dev, &service_list, &service_count, tee_sess,
> +                            PTA_CMD_GET_DEVICES_SUPP);
> +       if (!ret2)
> +               ret2 = bind_service_list(dev, service_list, service_count);
> +
> +       tee_shm_free(service_list);
> +
>         tee_close_session(dev, tee_sess);
>
> -       return ret;
> +       if (ret)
> +               return ret;
> +
> +       return ret2;
>  }
>
>  /**
> --
> 2.25.1
>

Re: [PATCH 1/2] tee: optee: don't fail probe because of optee-rng

[Jens Wiklander]

On Wed, Dec 7, 2022 at 4:30 PM Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Fixes optee-rng driver bind sequence in optee driver to print a warning
> message but not report an error status when a optee-rng service driver
> fails to be bound as the optee driver itself is still fully functional.
>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  drivers/tee/optee/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Thanks,
Jens

>
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 9240277579..604fd1414f 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -834,7 +834,7 @@ static int optee_probe(struct udevice *dev)
>                  */
>                 ret = device_bind_driver(dev, "optee-rng", "optee-rng", NULL);
>                 if (ret)
> -                       return ret;
> +                       dev_warn(dev, "ftpm_tee failed to bind: %d\n", ret);
>         }
>
>         return 0;
> --
> 2.25.1
>

Re: [PATCH 1/2] tee: optee: don't fail probe because of optee-rng

[Patrick DELAUNAY]

Hi,

On 12/7/22 16:29, Etienne Carriere wrote:
> Fixes optee-rng driver bind sequence in optee driver to print a warning
> message but not report an error status when a optee-rng service driver
> fails to be bound as the optee driver itself is still fully functional.
>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>   drivers/tee/optee/core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 9240277579..604fd1414f 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -834,7 +834,7 @@ static int optee_probe(struct udevice *dev)
>   		 */
>   		ret = device_bind_driver(dev, "optee-rng", "optee-rng", NULL);
>   		if (ret)
> -			return ret;
> +			dev_warn(dev, "ftpm_tee failed to bind: %d\n", ret);
>   	}
>   
>   	return 0;



Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>

Thanks
Patrick

Re: [PATCH 2/2] tee: optee: discover services dependent on tee-supplicant

[Patrick DELAUNAY]

Hi,

On 12/7/22 16:30, Etienne Carriere wrote:
> Makes OP-TEE to enumerate also services depending on tee-supplicant
> support in U-Boot. This change allows OP-TEE services like fTPM TA
> to be discovered and get a TPM device registered in U-Boot.
>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>   drivers/tee/optee/core.c | 32 +++++++++++++++++++++++---------
>   1 file changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 604fd1414f..b21031d7d8 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -102,13 +102,14 @@ static int bind_service_list(struct udevice *dev, struct tee_shm *service_list,
>   	return 0;
>   }
>   
> -static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm_size, u32 tee_sess)
> +static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm_size, u32 tee_sess,
> +			   unsigned int pta_cmd)
>   {
>   	struct tee_invoke_arg arg = { };
>   	struct tee_param param = { };
>   	int ret = 0;
>   
> -	arg.func = PTA_CMD_GET_DEVICES;
> +	arg.func = pta_cmd;
>   	arg.session = tee_sess;
>   
>   	/* Fill invoke cmd params */
> @@ -118,7 +119,7 @@ static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm
>   
>   	ret = tee_invoke_func(dev, &arg, 1, &param);
>   	if (ret || (arg.ret && arg.ret != TEE_ERROR_SHORT_BUFFER)) {
> -		dev_err(dev, "PTA_CMD_GET_DEVICES invoke function err: 0x%x\n", arg.ret);
> +		dev_err(dev, "Enumeration command 0x%x failed: 0x%x\n", pta_cmd, arg.ret);
>   		return -EINVAL;
>   	}
>   
> @@ -127,12 +128,13 @@ static int __enum_services(struct udevice *dev, struct tee_shm *shm, size_t *shm
>   	return 0;
>   }
>   
> -static int enum_services(struct udevice *dev, struct tee_shm **shm, size_t *count, u32 tee_sess)
> +static int enum_services(struct udevice *dev, struct tee_shm **shm, size_t *count, u32 tee_sess,
> +			 unsigned int pta_cmd)
>   {
>   	size_t shm_size = 0;
>   	int ret;
>   
> -	ret = __enum_services(dev, NULL, &shm_size, tee_sess);
> +	ret = __enum_services(dev, NULL, &shm_size, tee_sess, pta_cmd);
>   	if (ret)
>   		return ret;
>   
> @@ -142,7 +144,7 @@ static int enum_services(struct udevice *dev, struct tee_shm **shm, size_t *coun
>   		return ret;
>   	}
>   
> -	ret = __enum_services(dev, *shm, &shm_size, tee_sess);
> +	ret = __enum_services(dev, *shm, &shm_size, tee_sess, pta_cmd);
>   	if (!ret)
>   		*count = shm_size / sizeof(struct tee_optee_ta_uuid);
>   
> @@ -174,20 +176,32 @@ static int bind_service_drivers(struct udevice *dev)
>   	struct tee_shm *service_list = NULL;
>   	size_t service_count;
>   	u32 tee_sess;
> -	int ret;
> +	int ret, ret2;
>   
>   	ret = open_enum_session(dev, &tee_sess);
>   	if (ret)
>   		return ret;
>   
> -	ret = enum_services(dev, &service_list, &service_count, tee_sess);
> +	ret = enum_services(dev, &service_list, &service_count, tee_sess,
> +			    PTA_CMD_GET_DEVICES);
>   	if (!ret)
>   		ret = bind_service_list(dev, service_list, service_count);
>   
>   	tee_shm_free(service_list);

can you move part of duplicate code in enum_services() function ?
=> variables service_list and service_count
=> call to bind_service_list()
=> call to tee_shm_free(service_list);

then the call is more simple :

static int bind_service_drivers(struct udevice *dev)
  	u32 tee_sess;
	int ret, ret2;

  	ret = open_enum_session(dev, &tee_sess);
  	if (ret)
  		return ret;

	ret = enum_services(dev, tee_sess, PTA_CMD_GET_DEVICES);
	ret2 = enum_services(dev, tee_sess, PTA_CMD_GET_DEVICES_SUPP);

  	tee_close_session(dev, tee_sess);
  
	if (ret)
		return ret;

	return ret2;

}

> +
> +	ret2 = enum_services(dev, &service_list, &service_count, tee_sess,
> +			     PTA_CMD_GET_DEVICES_SUPP);
> +	if (!ret2)
> +		ret2 = bind_service_list(dev, service_list, service_count);
> +
> +	tee_shm_free(service_list);
> +
>   	tee_close_session(dev, tee_sess);
>   
> -	return ret;
> +	if (ret)
> +		return ret;
> +
> +	return ret2;
>   }
>   
>   /**

with or without


Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>

Thanks
Patrick