[PATCHv5 0/3] optee: register drivers on optee bus

[PATCHv5 0/3] optee: register drivers on optee bus

[Maxim Uvarov]

v5: - removed pr_err and fix typos in description (Jarkko Sakkinen)
    - added missed kfree in optee_open()
v4: - sysfs entry is optee-ta-uuid (Jerome Forissier, Sumit Garg)
    - added Documentation/ABI/testing/sysfs-bus-optee-devices (Greg Kroah-Hartman)
v3: - support tee-suppicant restart (Jens Wiklander)
    - description and comments (Jarkko Sakkinen)
    - do not name optee drivers by index in sysfs (Sumit Garg)
v2: - write TEE with capital letters.
    - declare __optee_enumerate_device() as static.

Maxim Uvarov (3):
  optee: do drivers initialization before and after tee-supplicant run
  optee: use uuid for sysfs driver entry
  tpm_ftpm_tee: register driver on TEE bus

 .../ABI/testing/sysfs-bus-optee-devices       |  8 +++
 MAINTAINERS                                   |  2 +
 drivers/char/tpm/tpm_ftpm_tee.c               | 70 ++++++++++++++++---
 drivers/tee/optee/core.c                      | 24 ++++++-
 drivers/tee/optee/device.c                    | 23 +++---
 drivers/tee/optee/optee_private.h             | 10 ++-
 6 files changed, 114 insertions(+), 23 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices

-- 
2.17.1

[PATCHv5 1/3] optee: do drivers initialization before and after tee-supplicant run

[Maxim Uvarov]

Some drivers (like ftpm) can operate only after tee-supplicant
runs because of tee-supplicant provides things like storage
services.  This patch splits probe of non tee-supplicant dependable
drivers to the early stage, and after tee-supplicant run probe other
drivers.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
Suggested-by: Sumit Garg <sumit.garg@linaro.org>
Suggested-by: Arnd Bergmann <arnd@linaro.org>
---
 drivers/tee/optee/core.c          | 24 +++++++++++++++++++++---
 drivers/tee/optee/device.c        | 17 +++++++++++------
 drivers/tee/optee/optee_private.h | 10 +++++++++-
 3 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 99698b8a3a74..bf0851fdf108 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -17,6 +17,7 @@
 #include <linux/tee_drv.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
+#include <linux/workqueue.h>
 #include "optee_private.h"
 #include "optee_smc.h"
 #include "shm_pool.h"
@@ -218,6 +219,11 @@ static void optee_get_version(struct tee_device *teedev,
 	*vers = v;
 }
 
+static void optee_bus_scan(struct work_struct *work)
+{
+	WARN_ON(optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP));
+}
+
 static int optee_open(struct tee_context *ctx)
 {
 	struct optee_context_data *ctxdata;
@@ -241,8 +247,18 @@ static int optee_open(struct tee_context *ctx)
 			kfree(ctxdata);
 			return -EBUSY;
 		}
-	}
 
+		if (!optee->scan_bus_done) {
+			INIT_WORK(&optee->scan_bus_work, optee_bus_scan);
+			optee->scan_bus_wq = create_workqueue("optee_bus_scan");
+			if (!optee->scan_bus_wq) {
+				kfree(ctxdata);
+				return -ECHILD;
+			}
+			queue_work(optee->scan_bus_wq, &optee->scan_bus_work);
+			optee->scan_bus_done = true;
+		}
+	}
 	mutex_init(&ctxdata->mutex);
 	INIT_LIST_HEAD(&ctxdata->sess_list);
 
@@ -296,8 +312,10 @@ static void optee_release(struct tee_context *ctx)
 
 	ctx->data = NULL;
 
-	if (teedev == optee->supp_teedev)
+	if (teedev == optee->supp_teedev) {
+		destroy_workqueue(optee->scan_bus_wq);
 		optee_supp_release(&optee->supp);
+	}
 }
 
 static const struct tee_driver_ops optee_ops = {
@@ -675,7 +693,7 @@ static int optee_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, optee);
 
-	rc = optee_enumerate_devices();
+	rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES);
 	if (rc) {
 		optee_remove(pdev);
 		return rc;
diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
index e3a148521ec1..d4931dad07aa 100644
--- a/drivers/tee/optee/device.c
+++ b/drivers/tee/optee/device.c
@@ -21,7 +21,6 @@
  * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
  * TEE_ERROR_SHORT_BUFFER - Output buffer size less than required
  */
-#define PTA_CMD_GET_DEVICES		0x0
 
 static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
 {
@@ -32,7 +31,8 @@ static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
 }
 
 static int get_devices(struct tee_context *ctx, u32 session,
-		       struct tee_shm *device_shm, u32 *shm_size)
+		       struct tee_shm *device_shm, u32 *shm_size,
+		       u32 func)
 {
 	int ret = 0;
 	struct tee_ioctl_invoke_arg inv_arg;
@@ -42,7 +42,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
 	memset(&param, 0, sizeof(param));
 
 	/* Invoke PTA_CMD_GET_DEVICES function */
-	inv_arg.func = PTA_CMD_GET_DEVICES;
+	inv_arg.func = func;
 	inv_arg.session = session;
 	inv_arg.num_params = 4;
 
@@ -87,7 +87,7 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
 	return rc;
 }
 
-int optee_enumerate_devices(void)
+static int __optee_enumerate_devices(u32 func)
 {
 	const uuid_t pta_uuid =
 		UUID_INIT(0x7011a688, 0xddde, 0x4053,
@@ -118,7 +118,7 @@ int optee_enumerate_devices(void)
 		goto out_ctx;
 	}
 
-	rc = get_devices(ctx, sess_arg.session, NULL, &shm_size);
+	rc = get_devices(ctx, sess_arg.session, NULL, &shm_size, func);
 	if (rc < 0 || !shm_size)
 		goto out_sess;
 
@@ -130,7 +130,7 @@ int optee_enumerate_devices(void)
 		goto out_sess;
 	}
 
-	rc = get_devices(ctx, sess_arg.session, device_shm, &shm_size);
+	rc = get_devices(ctx, sess_arg.session, device_shm, &shm_size, func);
 	if (rc < 0)
 		goto out_shm;
 
@@ -158,3 +158,8 @@ int optee_enumerate_devices(void)
 
 	return rc;
 }
+
+int optee_enumerate_devices(u32 func)
+{
+	return  __optee_enumerate_devices(func);
+}
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index d9c5037b4e03..8b71839a357e 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -78,6 +78,9 @@ struct optee_supp {
  * @memremaped_shm	virtual address of memory in shared memory pool
  * @sec_caps:		secure world capabilities defined by
  *			OPTEE_SMC_SEC_CAP_* in optee_smc.h
+ * @scan_bus_done	flag if device registation was already done.
+ * @scan_bus_wq		workqueue to scan optee bus and register optee drivers
+ * @scan_bus_work	workq to scan optee bus and register optee drivers
  */
 struct optee {
 	struct tee_device *supp_teedev;
@@ -89,6 +92,9 @@ struct optee {
 	struct tee_shm_pool *pool;
 	void *memremaped_shm;
 	u32 sec_caps;
+	bool   scan_bus_done;
+	struct workqueue_struct *scan_bus_wq;
+	struct work_struct scan_bus_work;
 };
 
 struct optee_session {
@@ -173,7 +179,9 @@ void optee_free_pages_list(void *array, size_t num_entries);
 void optee_fill_pages_list(u64 *dst, struct page **pages, int num_pages,
 			   size_t page_offset);
 
-int optee_enumerate_devices(void);
+#define PTA_CMD_GET_DEVICES		0x0
+#define PTA_CMD_GET_DEVICES_SUPP	0x1
+int optee_enumerate_devices(u32 func);
 
 /*
  * Small helpers
-- 
2.17.1

Re: [PATCHv5 1/3] optee: do drivers initialization before and after tee-supplicant run

[Jarkko Sakkinen]

On Fri, May 29, 2020 at 11:26:59AM +0300, Maxim Uvarov wrote:
> Some drivers (like ftpm) can operate only after tee-supplicant
> runs because of tee-supplicant provides things like storage
> services.  This patch splits probe of non tee-supplicant dependable
> drivers to the early stage, and after tee-supplicant run probe other
> drivers.
> 
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> Suggested-by: Sumit Garg <sumit.garg@linaro.org>
> Suggested-by: Arnd Bergmann <arnd@linaro.org>

$ scripts/get_maintainer.pl drivers/tee 
Jens Wiklander <jens.wiklander@linaro.org> (maintainer:TEE SUBSYSTEM)
tee-dev@lists.linaro.org (open list:TEE SUBSYSTEM)
linux-kernel@vger.kernel.org (open list)

tee-dev is missing. Please resend.

/Jarkko

Re: [PATCHv5 1/3] optee: do drivers initialization before and after tee-supplicant run

[Maxim Uvarov]

On Mon, 1 Jun 2020 at 04:49, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Fri, May 29, 2020 at 11:26:59AM +0300, Maxim Uvarov wrote:
> > Some drivers (like ftpm) can operate only after tee-supplicant
> > runs because of tee-supplicant provides things like storage
> > services.  This patch splits probe of non tee-supplicant dependable
> > drivers to the early stage, and after tee-supplicant run probe other
> > drivers.
> >
> > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> > Suggested-by: Sumit Garg <sumit.garg@linaro.org>
> > Suggested-by: Arnd Bergmann <arnd@linaro.org>
>
> $ scripts/get_maintainer.pl drivers/tee
> Jens Wiklander <jens.wiklander@linaro.org> (maintainer:TEE SUBSYSTEM)
> tee-dev@lists.linaro.org (open list:TEE SUBSYSTEM)
> linux-kernel@vger.kernel.org (open list)
>
> tee-dev is missing. Please resend.
>
> /Jarkko

tee-dev@ is in CC.

Re: [PATCHv5 1/3] optee: do drivers initialization before and after tee-supplicant run

[Jarkko Sakkinen]

On Mon, Jun 01, 2020 at 10:13:40AM +0300, Maxim Uvarov wrote:
> On Mon, 1 Jun 2020 at 04:49, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Fri, May 29, 2020 at 11:26:59AM +0300, Maxim Uvarov wrote:
> > > Some drivers (like ftpm) can operate only after tee-supplicant
> > > runs because of tee-supplicant provides things like storage
> > > services.  This patch splits probe of non tee-supplicant dependable
> > > drivers to the early stage, and after tee-supplicant run probe other
> > > drivers.
> > >
> > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> > > Suggested-by: Sumit Garg <sumit.garg@linaro.org>
> > > Suggested-by: Arnd Bergmann <arnd@linaro.org>
> >
> > $ scripts/get_maintainer.pl drivers/tee
> > Jens Wiklander <jens.wiklander@linaro.org> (maintainer:TEE SUBSYSTEM)
> > tee-dev@lists.linaro.org (open list:TEE SUBSYSTEM)
> > linux-kernel@vger.kernel.org (open list)
> >
> > tee-dev is missing. Please resend.
> >
> > /Jarkko
> 
> tee-dev@ is in CC.

Oops. I'm sorry, missed it.

/Jarkko

Re: [PATCHv5 1/3] optee: do drivers initialization before and after tee-supplicant run

[Sumit Garg]

On Fri, 29 May 2020 at 13:57, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
> Some drivers (like ftpm) can operate only after tee-supplicant
> runs because of tee-supplicant provides things like storage
> services.  This patch splits probe of non tee-supplicant dependable
> drivers to the early stage, and after tee-supplicant run probe other
> drivers.
>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> Suggested-by: Sumit Garg <sumit.garg@linaro.org>
> Suggested-by: Arnd Bergmann <arnd@linaro.org>
> ---
>  drivers/tee/optee/core.c          | 24 +++++++++++++++++++++---
>  drivers/tee/optee/device.c        | 17 +++++++++++------
>  drivers/tee/optee/optee_private.h | 10 +++++++++-
>  3 files changed, 41 insertions(+), 10 deletions(-)
>

Commit subject sounds a little vague, so how about:

====
optee: enable support for multi-stage bus enumeration
====

Then in the commit description, you can elaborate on what it actually means.

> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 99698b8a3a74..bf0851fdf108 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -17,6 +17,7 @@
>  #include <linux/tee_drv.h>
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
> +#include <linux/workqueue.h>
>  #include "optee_private.h"
>  #include "optee_smc.h"
>  #include "shm_pool.h"
> @@ -218,6 +219,11 @@ static void optee_get_version(struct tee_device *teedev,
>         *vers = v;
>  }
>
> +static void optee_bus_scan(struct work_struct *work)
> +{
> +       WARN_ON(optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP));
> +}
> +
>  static int optee_open(struct tee_context *ctx)
>  {
>         struct optee_context_data *ctxdata;
> @@ -241,8 +247,18 @@ static int optee_open(struct tee_context *ctx)
>                         kfree(ctxdata);
>                         return -EBUSY;
>                 }
> -       }
>
> +               if (!optee->scan_bus_done) {
> +                       INIT_WORK(&optee->scan_bus_work, optee_bus_scan);
> +                       optee->scan_bus_wq = create_workqueue("optee_bus_scan");
> +                       if (!optee->scan_bus_wq) {
> +                               kfree(ctxdata);
> +                               return -ECHILD;
> +                       }
> +                       queue_work(optee->scan_bus_wq, &optee->scan_bus_work);
> +                       optee->scan_bus_done = true;
> +               }
> +       }
>         mutex_init(&ctxdata->mutex);
>         INIT_LIST_HEAD(&ctxdata->sess_list);
>
> @@ -296,8 +312,10 @@ static void optee_release(struct tee_context *ctx)
>
>         ctx->data = NULL;
>
> -       if (teedev == optee->supp_teedev)
> +       if (teedev == optee->supp_teedev) {
> +               destroy_workqueue(optee->scan_bus_wq);

Doesn't it deserve a prior check "if(optee->scan_bus_wq)" as we only
allocate it once during multiple tee-supplicant instances?

>                 optee_supp_release(&optee->supp);
> +       }
>  }
>
>  static const struct tee_driver_ops optee_ops = {
> @@ -675,7 +693,7 @@ static int optee_probe(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, optee);
>
> -       rc = optee_enumerate_devices();
> +       rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES);
>         if (rc) {
>                 optee_remove(pdev);
>                 return rc;
> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> index e3a148521ec1..d4931dad07aa 100644
> --- a/drivers/tee/optee/device.c
> +++ b/drivers/tee/optee/device.c
> @@ -21,7 +21,6 @@
>   * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
>   * TEE_ERROR_SHORT_BUFFER - Output buffer size less than required
>   */

This comment needs to be moved as well.

> -#define PTA_CMD_GET_DEVICES            0x0
>
>  static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
>  {
> @@ -32,7 +31,8 @@ static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
>  }
>
>  static int get_devices(struct tee_context *ctx, u32 session,
> -                      struct tee_shm *device_shm, u32 *shm_size)
> +                      struct tee_shm *device_shm, u32 *shm_size,
> +                      u32 func)
>  {
>         int ret = 0;
>         struct tee_ioctl_invoke_arg inv_arg;
> @@ -42,7 +42,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
>         memset(&param, 0, sizeof(param));
>
>         /* Invoke PTA_CMD_GET_DEVICES function */

You can get rid of this comment.

-Sumit

> -       inv_arg.func = PTA_CMD_GET_DEVICES;
> +       inv_arg.func = func;
>         inv_arg.session = session;
>         inv_arg.num_params = 4;
>
> @@ -87,7 +87,7 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
>         return rc;
>  }
>
> -int optee_enumerate_devices(void)
> +static int __optee_enumerate_devices(u32 func)
>  {
>         const uuid_t pta_uuid =
>                 UUID_INIT(0x7011a688, 0xddde, 0x4053,
> @@ -118,7 +118,7 @@ int optee_enumerate_devices(void)
>                 goto out_ctx;
>         }
>
> -       rc = get_devices(ctx, sess_arg.session, NULL, &shm_size);
> +       rc = get_devices(ctx, sess_arg.session, NULL, &shm_size, func);
>         if (rc < 0 || !shm_size)
>                 goto out_sess;
>
> @@ -130,7 +130,7 @@ int optee_enumerate_devices(void)
>                 goto out_sess;
>         }
>
> -       rc = get_devices(ctx, sess_arg.session, device_shm, &shm_size);
> +       rc = get_devices(ctx, sess_arg.session, device_shm, &shm_size, func);
>         if (rc < 0)
>                 goto out_shm;
>
> @@ -158,3 +158,8 @@ int optee_enumerate_devices(void)
>
>         return rc;
>  }
> +
> +int optee_enumerate_devices(u32 func)
> +{
> +       return  __optee_enumerate_devices(func);
> +}
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index d9c5037b4e03..8b71839a357e 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -78,6 +78,9 @@ struct optee_supp {
>   * @memremaped_shm     virtual address of memory in shared memory pool
>   * @sec_caps:          secure world capabilities defined by
>   *                     OPTEE_SMC_SEC_CAP_* in optee_smc.h
> + * @scan_bus_done      flag if device registation was already done.
> + * @scan_bus_wq                workqueue to scan optee bus and register optee drivers
> + * @scan_bus_work      workq to scan optee bus and register optee drivers
>   */
>  struct optee {
>         struct tee_device *supp_teedev;
> @@ -89,6 +92,9 @@ struct optee {
>         struct tee_shm_pool *pool;
>         void *memremaped_shm;
>         u32 sec_caps;
> +       bool   scan_bus_done;
> +       struct workqueue_struct *scan_bus_wq;
> +       struct work_struct scan_bus_work;
>  };
>
>  struct optee_session {
> @@ -173,7 +179,9 @@ void optee_free_pages_list(void *array, size_t num_entries);
>  void optee_fill_pages_list(u64 *dst, struct page **pages, int num_pages,
>                            size_t page_offset);
>
> -int optee_enumerate_devices(void);
> +#define PTA_CMD_GET_DEVICES            0x0
> +#define PTA_CMD_GET_DEVICES_SUPP       0x1
> +int optee_enumerate_devices(u32 func);
>
>  /*
>   * Small helpers
> --
> 2.17.1
>

[PATCHv5 2/3] optee: use uuid for sysfs driver entry

[Maxim Uvarov]

OP-TEE device names for sysfs need to be unique
and it's better if they will mean something. UUID for name
looks like good solution:
/sys/bus/tee/devices/optee-ta-<uuid>

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 Documentation/ABI/testing/sysfs-bus-optee-devices | 8 ++++++++
 MAINTAINERS                                       | 2 ++
 drivers/tee/optee/device.c                        | 6 +++---
 3 files changed, 13 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices

diff --git a/Documentation/ABI/testing/sysfs-bus-optee-devices b/Documentation/ABI/testing/sysfs-bus-optee-devices
new file mode 100644
index 000000000000..0ae04ae5374a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-optee-devices
@@ -0,0 +1,8 @@
+What:		/sys/bus/tee/devices/optee-ta-<uuid>/
+Date:           May 2020
+KernelVersion   5.7
+Contact:        tee-dev@lists.linaro.org
+Description:
+		OP-TEE bus provides reference to registered drivers under this directory. The <uuid>
+		matches Trusted Application (TA) driver and corresponding TA in secure OS. Drivers
+		are free to create needed API under optee-ta-<uuid> directory.
diff --git a/MAINTAINERS b/MAINTAINERS
index ecc0749810b0..52717ede29fc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12516,8 +12516,10 @@ OP-TEE DRIVER
 M:	Jens Wiklander <jens.wiklander@linaro.org>
 L:	tee-dev@lists.linaro.org
 S:	Maintained
+F:	Documentation/ABI/testing/sysfs-bus-optee-devices
 F:	drivers/tee/optee/
 
+
 OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER
 M:	Sumit Garg <sumit.garg@linaro.org>
 L:	tee-dev@lists.linaro.org
diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
index d4931dad07aa..2eb1c0283aec 100644
--- a/drivers/tee/optee/device.c
+++ b/drivers/tee/optee/device.c
@@ -65,7 +65,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
 	return 0;
 }
 
-static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
+static int optee_register_device(const uuid_t *device_uuid)
 {
 	struct tee_client_device *optee_device = NULL;
 	int rc;
@@ -75,7 +75,7 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
 		return -ENOMEM;
 
 	optee_device->dev.bus = &tee_bus_type;
-	dev_set_name(&optee_device->dev, "optee-clnt%u", device_id);
+	dev_set_name(&optee_device->dev, "optee-ta-%pUl", device_uuid);
 	uuid_copy(&optee_device->id.uuid, device_uuid);
 
 	rc = device_register(&optee_device->dev);
@@ -144,7 +144,7 @@ static int __optee_enumerate_devices(u32 func)
 	num_devices = shm_size / sizeof(uuid_t);
 
 	for (idx = 0; idx < num_devices; idx++) {
-		rc = optee_register_device(&device_uuid[idx], idx);
+		rc = optee_register_device(&device_uuid[idx]);
 		if (rc)
 			goto out_shm;
 	}
-- 
2.17.1

Re: [PATCHv5 2/3] optee: use uuid for sysfs driver entry

[Sumit Garg]

On Fri, 29 May 2020 at 13:57, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
> OP-TEE device names for sysfs need to be unique
> and it's better if they will mean something. UUID for name
> looks like good solution:
> /sys/bus/tee/devices/optee-ta-<uuid>
>

I think this description is a little vague here which fails to explain
why we are doing this. How about:

=======
With the evolving use-cases for TEE bus, now it's required to support
multi-stage enumeration process. But using a simple index doesn't
suffice this requirement and instead leads to duplicate sysfs entries.
So instead switch to use more informative device UUID for sysfs entry
like:

/sys/bus/tee/devices/optee-ta-<uuid>
========

> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  Documentation/ABI/testing/sysfs-bus-optee-devices | 8 ++++++++
>  MAINTAINERS                                       | 2 ++
>  drivers/tee/optee/device.c                        | 6 +++---
>  3 files changed, 13 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
>

I think this patch belongs as patch #1 in this series given the dependency.

> diff --git a/Documentation/ABI/testing/sysfs-bus-optee-devices b/Documentation/ABI/testing/sysfs-bus-optee-devices
> new file mode 100644
> index 000000000000..0ae04ae5374a
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-optee-devices
> @@ -0,0 +1,8 @@
> +What:          /sys/bus/tee/devices/optee-ta-<uuid>/
> +Date:           May 2020
> +KernelVersion   5.7
> +Contact:        tee-dev@lists.linaro.org
> +Description:
> +               OP-TEE bus provides reference to registered drivers under this directory. The <uuid>
> +               matches Trusted Application (TA) driver and corresponding TA in secure OS. Drivers
> +               are free to create needed API under optee-ta-<uuid> directory.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ecc0749810b0..52717ede29fc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12516,8 +12516,10 @@ OP-TEE DRIVER
>  M:     Jens Wiklander <jens.wiklander@linaro.org>
>  L:     tee-dev@lists.linaro.org
>  S:     Maintained
> +F:     Documentation/ABI/testing/sysfs-bus-optee-devices
>  F:     drivers/tee/optee/
>
> +

Unnecessary blank line.

-Sumit

>  OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER
>  M:     Sumit Garg <sumit.garg@linaro.org>
>  L:     tee-dev@lists.linaro.org
> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> index d4931dad07aa..2eb1c0283aec 100644
> --- a/drivers/tee/optee/device.c
> +++ b/drivers/tee/optee/device.c
> @@ -65,7 +65,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
>         return 0;
>  }
>
> -static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> +static int optee_register_device(const uuid_t *device_uuid)
>  {
>         struct tee_client_device *optee_device = NULL;
>         int rc;
> @@ -75,7 +75,7 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
>                 return -ENOMEM;
>
>         optee_device->dev.bus = &tee_bus_type;
> -       dev_set_name(&optee_device->dev, "optee-clnt%u", device_id);
> +       dev_set_name(&optee_device->dev, "optee-ta-%pUl", device_uuid);
>         uuid_copy(&optee_device->id.uuid, device_uuid);
>
>         rc = device_register(&optee_device->dev);
> @@ -144,7 +144,7 @@ static int __optee_enumerate_devices(u32 func)
>         num_devices = shm_size / sizeof(uuid_t);
>
>         for (idx = 0; idx < num_devices; idx++) {
> -               rc = optee_register_device(&device_uuid[idx], idx);
> +               rc = optee_register_device(&device_uuid[idx]);
>                 if (rc)
>                         goto out_shm;
>         }
> --
> 2.17.1
>

[PATCHv5 3/3] tpm_ftpm_tee: register driver on TEE bus

[Maxim Uvarov]

Register driver on the TEE bus. The module tee registers bus,
and module optee calls optee_enumerate_devices() to scan
all devices on the bus. Trusted Application for this driver
can be Early TA's (can be compiled into optee-os). In that
case it will be on OPTEE bus before linux booting. Also
optee-suplicant application is needed to be loaded between
OPTEE module and ftpm module to maintain functionality
for fTPM driver.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
Suggested-by: Sumit Garg <sumit.garg@linaro.org>
Suggested-by: Arnd Bergmann <arnd@linaro.org>
---
 drivers/char/tpm/tpm_ftpm_tee.c | 70 ++++++++++++++++++++++++++++-----
 1 file changed, 60 insertions(+), 10 deletions(-)

diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
index 22bf553ccf9d..28da638360d8 100644
--- a/drivers/char/tpm/tpm_ftpm_tee.c
+++ b/drivers/char/tpm/tpm_ftpm_tee.c
@@ -214,11 +214,10 @@ static int ftpm_tee_match(struct tee_ioctl_version_data *ver, const void *data)
  * Return:
  *	On success, 0. On failure, -errno.
  */
-static int ftpm_tee_probe(struct platform_device *pdev)
+static int ftpm_tee_probe(struct device *dev)
 {
 	int rc;
 	struct tpm_chip *chip;
-	struct device *dev = &pdev->dev;
 	struct ftpm_tee_private *pvt_data = NULL;
 	struct tee_ioctl_open_session_arg sess_arg;
 
@@ -297,6 +296,13 @@ static int ftpm_tee_probe(struct platform_device *pdev)
 	return rc;
 }
 
+static int ftpm_plat_tee_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	return ftpm_tee_probe(dev);
+}
+
 /**
  * ftpm_tee_remove() - remove the TPM device
  * @pdev: the platform_device description.
@@ -304,9 +310,9 @@ static int ftpm_tee_probe(struct platform_device *pdev)
  * Return:
  *	0 always.
  */
-static int ftpm_tee_remove(struct platform_device *pdev)
+static int ftpm_tee_remove(struct device *dev)
 {
-	struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);
+	struct ftpm_tee_private *pvt_data = dev_get_drvdata(dev);
 
 	/* Release the chip */
 	tpm_chip_unregister(pvt_data->chip);
@@ -328,11 +334,18 @@ static int ftpm_tee_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int ftpm_plat_tee_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	return ftpm_tee_remove(dev);
+}
+
 /**
  * ftpm_tee_shutdown() - shutdown the TPM device
  * @pdev: the platform_device description.
  */
-static void ftpm_tee_shutdown(struct platform_device *pdev)
+static void ftpm_plat_tee_shutdown(struct platform_device *pdev)
 {
 	struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);
 
@@ -347,17 +360,54 @@ static const struct of_device_id of_ftpm_tee_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, of_ftpm_tee_ids);
 
-static struct platform_driver ftpm_tee_driver = {
+static struct platform_driver ftpm_tee_plat_driver = {
 	.driver = {
 		.name = "ftpm-tee",
 		.of_match_table = of_match_ptr(of_ftpm_tee_ids),
 	},
-	.probe = ftpm_tee_probe,
-	.remove = ftpm_tee_remove,
-	.shutdown = ftpm_tee_shutdown,
+	.shutdown = ftpm_plat_tee_shutdown,
+	.probe = ftpm_plat_tee_probe,
+	.remove = ftpm_plat_tee_remove,
+};
+
+/* UUID of the fTPM TA */
+static const struct tee_client_device_id optee_ftpm_id_table[] = {
+	{UUID_INIT(0xbc50d971, 0xd4c9, 0x42c4,
+		   0x82, 0xcb, 0x34, 0x3f, 0xb7, 0xf3, 0x78, 0x96)},
+	{}
 };
 
-module_platform_driver(ftpm_tee_driver);
+MODULE_DEVICE_TABLE(tee, optee_ftpm_id_table);
+
+static struct tee_client_driver ftpm_tee_driver = {
+	.id_table	= optee_ftpm_id_table,
+	.driver		= {
+		.name		= "optee-ftpm",
+		.bus		= &tee_bus_type,
+		.probe		= ftpm_tee_probe,
+		.remove		= ftpm_tee_remove,
+	},
+};
+
+static int __init ftpm_mod_init(void)
+{
+	int rc;
+
+	rc = platform_driver_register(&ftpm_tee_plat_driver);
+	if (rc)
+		return rc;
+
+	return driver_register(&ftpm_tee_driver.driver);
+}
+
+static void __exit ftpm_mod_exit(void)
+{
+	platform_driver_unregister(&ftpm_tee_plat_driver);
+	driver_unregister(&ftpm_tee_driver.driver);
+}
+
+module_init(ftpm_mod_init);
+module_exit(ftpm_mod_exit);
 
 MODULE_AUTHOR("Thirupathaiah Annapureddy <thiruan@microsoft.com>");
 MODULE_DESCRIPTION("TPM Driver for fTPM TA in TEE");
-- 
2.17.1

Re: [PATCHv5 3/3] tpm_ftpm_tee: register driver on TEE bus

[Sumit Garg]

On Fri, 29 May 2020 at 13:57, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
> Register driver on the TEE bus. The module tee registers bus,
> and module optee calls optee_enumerate_devices() to scan
> all devices on the bus. Trusted Application for this driver
> can be Early TA's (can be compiled into optee-os). In that
> case it will be on OPTEE bus before linux booting. Also
> optee-suplicant application is needed to be loaded between
> OPTEE module and ftpm module to maintain functionality
> for fTPM driver.

I think this description merely describes the functioning of TEE bus
and misses what value add does TEE bus provide compared to platform
bus.

Consider:

====
OP-TEE based fTPM Trusted Application depends on tee-supplicant to
provide NV RAM implementation based on RPMB secure storage. So this
dependency can be resolved via TEE bus where we only invoke fTPM
driver probe once fTPM device is registered on the bus which is only
true after the tee-supplicant is up and running. Additionally, TEE bus
provides auto device enumeration.
====

With that, implementation looks good to me. So feel free to add:

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> Suggested-by: Sumit Garg <sumit.garg@linaro.org>
> Suggested-by: Arnd Bergmann <arnd@linaro.org>
> ---
>  drivers/char/tpm/tpm_ftpm_tee.c | 70 ++++++++++++++++++++++++++++-----
>  1 file changed, 60 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> index 22bf553ccf9d..28da638360d8 100644
> --- a/drivers/char/tpm/tpm_ftpm_tee.c
> +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> @@ -214,11 +214,10 @@ static int ftpm_tee_match(struct tee_ioctl_version_data *ver, const void *data)
>   * Return:
>   *     On success, 0. On failure, -errno.
>   */
> -static int ftpm_tee_probe(struct platform_device *pdev)
> +static int ftpm_tee_probe(struct device *dev)
>  {
>         int rc;
>         struct tpm_chip *chip;
> -       struct device *dev = &pdev->dev;
>         struct ftpm_tee_private *pvt_data = NULL;
>         struct tee_ioctl_open_session_arg sess_arg;
>
> @@ -297,6 +296,13 @@ static int ftpm_tee_probe(struct platform_device *pdev)
>         return rc;
>  }
>
> +static int ftpm_plat_tee_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +
> +       return ftpm_tee_probe(dev);
> +}
> +
>  /**
>   * ftpm_tee_remove() - remove the TPM device
>   * @pdev: the platform_device description.
> @@ -304,9 +310,9 @@ static int ftpm_tee_probe(struct platform_device *pdev)
>   * Return:
>   *     0 always.
>   */
> -static int ftpm_tee_remove(struct platform_device *pdev)
> +static int ftpm_tee_remove(struct device *dev)
>  {
> -       struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);
> +       struct ftpm_tee_private *pvt_data = dev_get_drvdata(dev);
>
>         /* Release the chip */
>         tpm_chip_unregister(pvt_data->chip);
> @@ -328,11 +334,18 @@ static int ftpm_tee_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +static int ftpm_plat_tee_remove(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +
> +       return ftpm_tee_remove(dev);
> +}
> +
>  /**
>   * ftpm_tee_shutdown() - shutdown the TPM device
>   * @pdev: the platform_device description.
>   */
> -static void ftpm_tee_shutdown(struct platform_device *pdev)
> +static void ftpm_plat_tee_shutdown(struct platform_device *pdev)
>  {
>         struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);
>
> @@ -347,17 +360,54 @@ static const struct of_device_id of_ftpm_tee_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, of_ftpm_tee_ids);
>
> -static struct platform_driver ftpm_tee_driver = {
> +static struct platform_driver ftpm_tee_plat_driver = {
>         .driver = {
>                 .name = "ftpm-tee",
>                 .of_match_table = of_match_ptr(of_ftpm_tee_ids),
>         },
> -       .probe = ftpm_tee_probe,
> -       .remove = ftpm_tee_remove,
> -       .shutdown = ftpm_tee_shutdown,
> +       .shutdown = ftpm_plat_tee_shutdown,
> +       .probe = ftpm_plat_tee_probe,
> +       .remove = ftpm_plat_tee_remove,
> +};
> +
> +/* UUID of the fTPM TA */
> +static const struct tee_client_device_id optee_ftpm_id_table[] = {
> +       {UUID_INIT(0xbc50d971, 0xd4c9, 0x42c4,
> +                  0x82, 0xcb, 0x34, 0x3f, 0xb7, 0xf3, 0x78, 0x96)},
> +       {}
>  };
>
> -module_platform_driver(ftpm_tee_driver);
> +MODULE_DEVICE_TABLE(tee, optee_ftpm_id_table);
> +
> +static struct tee_client_driver ftpm_tee_driver = {
> +       .id_table       = optee_ftpm_id_table,
> +       .driver         = {
> +               .name           = "optee-ftpm",
> +               .bus            = &tee_bus_type,
> +               .probe          = ftpm_tee_probe,
> +               .remove         = ftpm_tee_remove,
> +       },
> +};
> +
> +static int __init ftpm_mod_init(void)
> +{
> +       int rc;
> +
> +       rc = platform_driver_register(&ftpm_tee_plat_driver);
> +       if (rc)
> +               return rc;
> +
> +       return driver_register(&ftpm_tee_driver.driver);
> +}
> +
> +static void __exit ftpm_mod_exit(void)
> +{
> +       platform_driver_unregister(&ftpm_tee_plat_driver);
> +       driver_unregister(&ftpm_tee_driver.driver);
> +}
> +
> +module_init(ftpm_mod_init);
> +module_exit(ftpm_mod_exit);
>
>  MODULE_AUTHOR("Thirupathaiah Annapureddy <thiruan@microsoft.com>");
>  MODULE_DESCRIPTION("TPM Driver for fTPM TA in TEE");
> --
> 2.17.1
>

Re: [PATCHv5 3/3] tpm_ftpm_tee: register driver on TEE bus

[Maxim Uvarov]

thanks, will send an updated version soon.

On Mon, 1 Jun 2020 at 14:12, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Fri, 29 May 2020 at 13:57, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> >
> > Register driver on the TEE bus. The module tee registers bus,
> > and module optee calls optee_enumerate_devices() to scan
> > all devices on the bus. Trusted Application for this driver
> > can be Early TA's (can be compiled into optee-os). In that
> > case it will be on OPTEE bus before linux booting. Also
> > optee-suplicant application is needed to be loaded between
> > OPTEE module and ftpm module to maintain functionality
> > for fTPM driver.
>
> I think this description merely describes the functioning of TEE bus
> and misses what value add does TEE bus provide compared to platform
> bus.
>
> Consider:
>
> ====
> OP-TEE based fTPM Trusted Application depends on tee-supplicant to
> provide NV RAM implementation based on RPMB secure storage. So this
> dependency can be resolved via TEE bus where we only invoke fTPM
> driver probe once fTPM device is registered on the bus which is only
> true after the tee-supplicant is up and running. Additionally, TEE bus
> provides auto device enumeration.
> ====
>
> With that, implementation looks good to me. So feel free to add:
>
> Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
>
> -Sumit
>
> >
> > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> > Suggested-by: Sumit Garg <sumit.garg@linaro.org>
> > Suggested-by: Arnd Bergmann <arnd@linaro.org>
> > ---
> >  drivers/char/tpm/tpm_ftpm_tee.c | 70 ++++++++++++++++++++++++++++-----
> >  1 file changed, 60 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> > index 22bf553ccf9d..28da638360d8 100644
> > --- a/drivers/char/tpm/tpm_ftpm_tee.c
> > +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> > @@ -214,11 +214,10 @@ static int ftpm_tee_match(struct tee_ioctl_version_data *ver, const void *data)
> >   * Return:
> >   *     On success, 0. On failure, -errno.
> >   */
> > -static int ftpm_tee_probe(struct platform_device *pdev)
> > +static int ftpm_tee_probe(struct device *dev)
> >  {
> >         int rc;
> >         struct tpm_chip *chip;
> > -       struct device *dev = &pdev->dev;
> >         struct ftpm_tee_private *pvt_data = NULL;
> >         struct tee_ioctl_open_session_arg sess_arg;
> >
> > @@ -297,6 +296,13 @@ static int ftpm_tee_probe(struct platform_device *pdev)
> >         return rc;
> >  }
> >
> > +static int ftpm_plat_tee_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +
> > +       return ftpm_tee_probe(dev);
> > +}
> > +
> >  /**
> >   * ftpm_tee_remove() - remove the TPM device
> >   * @pdev: the platform_device description.
> > @@ -304,9 +310,9 @@ static int ftpm_tee_probe(struct platform_device *pdev)
> >   * Return:
> >   *     0 always.
> >   */
> > -static int ftpm_tee_remove(struct platform_device *pdev)
> > +static int ftpm_tee_remove(struct device *dev)
> >  {
> > -       struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);
> > +       struct ftpm_tee_private *pvt_data = dev_get_drvdata(dev);
> >
> >         /* Release the chip */
> >         tpm_chip_unregister(pvt_data->chip);
> > @@ -328,11 +334,18 @@ static int ftpm_tee_remove(struct platform_device *pdev)
> >         return 0;
> >  }
> >
> > +static int ftpm_plat_tee_remove(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +
> > +       return ftpm_tee_remove(dev);
> > +}
> > +
> >  /**
> >   * ftpm_tee_shutdown() - shutdown the TPM device
> >   * @pdev: the platform_device description.
> >   */
> > -static void ftpm_tee_shutdown(struct platform_device *pdev)
> > +static void ftpm_plat_tee_shutdown(struct platform_device *pdev)
> >  {
> >         struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);
> >
> > @@ -347,17 +360,54 @@ static const struct of_device_id of_ftpm_tee_ids[] = {
> >  };
> >  MODULE_DEVICE_TABLE(of, of_ftpm_tee_ids);
> >
> > -static struct platform_driver ftpm_tee_driver = {
> > +static struct platform_driver ftpm_tee_plat_driver = {
> >         .driver = {
> >                 .name = "ftpm-tee",
> >                 .of_match_table = of_match_ptr(of_ftpm_tee_ids),
> >         },
> > -       .probe = ftpm_tee_probe,
> > -       .remove = ftpm_tee_remove,
> > -       .shutdown = ftpm_tee_shutdown,
> > +       .shutdown = ftpm_plat_tee_shutdown,
> > +       .probe = ftpm_plat_tee_probe,
> > +       .remove = ftpm_plat_tee_remove,
> > +};
> > +
> > +/* UUID of the fTPM TA */
> > +static const struct tee_client_device_id optee_ftpm_id_table[] = {
> > +       {UUID_INIT(0xbc50d971, 0xd4c9, 0x42c4,
> > +                  0x82, 0xcb, 0x34, 0x3f, 0xb7, 0xf3, 0x78, 0x96)},
> > +       {}
> >  };
> >
> > -module_platform_driver(ftpm_tee_driver);
> > +MODULE_DEVICE_TABLE(tee, optee_ftpm_id_table);
> > +
> > +static struct tee_client_driver ftpm_tee_driver = {
> > +       .id_table       = optee_ftpm_id_table,
> > +       .driver         = {
> > +               .name           = "optee-ftpm",
> > +               .bus            = &tee_bus_type,
> > +               .probe          = ftpm_tee_probe,
> > +               .remove         = ftpm_tee_remove,
> > +       },
> > +};
> > +
> > +static int __init ftpm_mod_init(void)
> > +{
> > +       int rc;
> > +
> > +       rc = platform_driver_register(&ftpm_tee_plat_driver);
> > +       if (rc)
> > +               return rc;
> > +
> > +       return driver_register(&ftpm_tee_driver.driver);
> > +}
> > +
> > +static void __exit ftpm_mod_exit(void)
> > +{
> > +       platform_driver_unregister(&ftpm_tee_plat_driver);
> > +       driver_unregister(&ftpm_tee_driver.driver);
> > +}
> > +
> > +module_init(ftpm_mod_init);
> > +module_exit(ftpm_mod_exit);
> >
> >  MODULE_AUTHOR("Thirupathaiah Annapureddy <thiruan@microsoft.com>");
> >  MODULE_DESCRIPTION("TPM Driver for fTPM TA in TEE");
> > --
> > 2.17.1
> >