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.
Hello,
This patchset fixes issues with probing() tee, optee and optee driver
if they were compiled into kernel, built as modules or any mixed
combination.
These changes require optee-os changes which already were merged.
Main corresponding commits are:
https://github.com/OP-TEE/optee_os/commit/9389d8030ef198c9d7b8ab7ea8e877e0ace3369d
https://github.com/OP-TEE/optee_os/commit/bc5921cdab538c8ae48422f5ffd600f1cbdd95b2
optee_enumerate_devices() which discovers Trusted Applications on tee
bus is split up on 2 changes. Do probe of drivers which do not require
userspace support of tee-supplicant and stage two to run drivers with
support of tee-supplicant only after tee supplicant run.
Best regards,
Maxim.
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
drivers/char/tpm/tpm_ftpm_tee.c | 70 ++++++++++++++++++++++++++-----
drivers/tee/optee/core.c | 28 +++++++++++--
drivers/tee/optee/device.c | 23 ++++++----
drivers/tee/optee/optee_private.h | 10 ++++-
4 files changed, 108 insertions(+), 23 deletions(-)
--
2.17.1
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 <[email protected]>
Suggested-by: Sumit Garg <[email protected]>
Suggested-by: Arnd Bergmann <[email protected]>
---
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 <[email protected]>");
MODULE_DESCRIPTION("TPM Driver for fTPM TA in TEE");
--
2.17.1
Optee device names for sysfs needed to be unique
and it's better if they will mean something. UUID for name
looks like good solution:
/sys/bus/tee/devices/optee-clnt-<uuid>
Signed-off-by: Maxim Uvarov <[email protected]>
---
drivers/tee/optee/device.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
index d4931dad07aa..aab917605e74 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-clnt-%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
On 5/25/20 1:52 PM, Maxim Uvarov wrote:
> Optee device names for sysfs needed to be unique
s/Optee/OP-TEE/
s/needed/need/
> and it's better if they will mean something. UUID for name
> looks like good solution:
> /sys/bus/tee/devices/optee-clnt-<uuid>
How about mentioning it is the UUID of the Trusted Application on the
TEE side?
>
> Signed-off-by: Maxim Uvarov <[email protected]>
> ---
> drivers/tee/optee/device.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Thanks,
--
Jerome
Some drivers (like ftpm) can operate only after tee-supplicant
runs becase of tee-supplicant provides things like storage
services. This patch splits probe of non tee-supplicant dependable
drivers to early stage, and after tee-supplicant run probe other
drivers.
Signed-off-by: Maxim Uvarov <[email protected]>
Suggested-by: Sumit Garg <[email protected]>
Suggested-by: Arnd Bergmann <[email protected]>
---
drivers/tee/optee/core.c | 28 +++++++++++++++++++++++++---
drivers/tee/optee/device.c | 17 +++++++++++------
drivers/tee/optee/optee_private.h | 10 +++++++++-
3 files changed, 45 insertions(+), 10 deletions(-)
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 99698b8a3a74..d059e3ac491c 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,15 @@ static void optee_get_version(struct tee_device *teedev,
*vers = v;
}
+static void optee_bus_scan(struct work_struct *work)
+{
+ int rc;
+
+ rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP);
+ if (rc)
+ pr_err("optee_enumerate_devices failed %d\n", rc);
+}
+
static int optee_open(struct tee_context *ctx)
{
struct optee_context_data *ctxdata;
@@ -241,8 +251,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) {
+ pr_err("optee: couldn't create workqueue\n");
+ 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 +316,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 +697,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(¶m, 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
On Mon, 2020-05-25 at 14:52 +0300, Maxim Uvarov wrote:
> Some drivers (like ftpm) can operate only after tee-supplicant
> runs becase of tee-supplicant provides things like storage
> services. This patch splits probe of non tee-supplicant dependable
> drivers to early stage, and after tee-supplicant run probe other
> drivers.
Fix the typos and remove extra spaces (one space after '.').
Why "tee-supplicant" and not "TEE-supplicant"?
>
> Signed-off-by: Maxim Uvarov <[email protected]>
> Suggested-by: Sumit Garg <[email protected]>
> Suggested-by: Arnd Bergmann <[email protected]>
> ---
> drivers/tee/optee/core.c | 28 +++++++++++++++++++++++++---
> drivers/tee/optee/device.c | 17 +++++++++++------
> drivers/tee/optee/optee_private.h | 10 +++++++++-
> 3 files changed, 45 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 99698b8a3a74..d059e3ac491c 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,15 @@ static void optee_get_version(struct tee_device *teedev,
> *vers = v;
> }
>
> +static void optee_bus_scan(struct work_struct *work)
> +{
> + int rc;
> +
> + rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP);
> + if (rc)
> + pr_err("optee_enumerate_devices failed %d\n", rc);
> +}
Is pr_err() required really here? Why not just pr_info()?
Why have it all and not just ftrace when needed? There is series going
on to add CONFIG_FTRACE_FUNC_PROTOTYPE to that to get return values and
arguments.
> +
> static int optee_open(struct tee_context *ctx)
> {
> struct optee_context_data *ctxdata;
> @@ -241,8 +251,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) {
> + pr_err("optee: couldn't create workqueue\n");
Neither get pr_err() here. Please remove this one completely.
> + 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 +316,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 +697,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(¶m, 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
No exalanation of "scan bus" neither in the comments nor in the code.
/Jarkko
On Wed, 27 May 2020 at 22:56, Jarkko Sakkinen
<[email protected]> wrote:
>
> On Mon, 2020-05-25 at 14:52 +0300, Maxim Uvarov wrote:
> > Some drivers (like ftpm) can operate only after tee-supplicant
> > runs becase of tee-supplicant provides things like storage
> > services. This patch splits probe of non tee-supplicant dependable
> > drivers to early stage, and after tee-supplicant run probe other
> > drivers.
>
> Fix the typos and remove extra spaces (one space after '.').
>
> Why "tee-supplicant" and not "TEE-supplicant"?
>
> >
> > Signed-off-by: Maxim Uvarov <[email protected]>
> > Suggested-by: Sumit Garg <[email protected]>
> > Suggested-by: Arnd Bergmann <[email protected]>
> > ---
> > drivers/tee/optee/core.c | 28 +++++++++++++++++++++++++---
> > drivers/tee/optee/device.c | 17 +++++++++++------
> > drivers/tee/optee/optee_private.h | 10 +++++++++-
> > 3 files changed, 45 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > index 99698b8a3a74..d059e3ac491c 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,15 @@ static void optee_get_version(struct tee_device *teedev,
> > *vers = v;
> > }
> >
> > +static void optee_bus_scan(struct work_struct *work)
> > +{
> > + int rc;
> > +
> > + rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP);
> > + if (rc)
> > + pr_err("optee_enumerate_devices failed %d\n", rc);
> > +}
>
> Is pr_err() required really here? Why not just pr_info()?
>
> Why have it all and not just ftrace when needed? There is series going
> on to add CONFIG_FTRACE_FUNC_PROTOTYPE to that to get return values and
> arguments.
>
Actually if optee_enumerate_devices() fail then I would say there is
something wrong with memory or Secure OS and more likely we should
stop execution.
I think something line WARN_ON might be more suitable here. What do you think?
> > +
> > static int optee_open(struct tee_context *ctx)
> > {
> > struct optee_context_data *ctxdata;
> > @@ -241,8 +251,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) {
> > + pr_err("optee: couldn't create workqueue\n");
>
> Neither get pr_err() here. Please remove this one completely.
>
What is the benefit of removing print here? Kernel binary size?
> > + 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 +316,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 +697,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(¶m, 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
>
> No exalanation of "scan bus" neither in the comments nor in the code.
>
Idea was when tee-supplicant opens /dev/tee-priv0 then we run
workqueue (to not hold current open()) and do bus scan for TA
applications which require tee-supplicant run. There is no "scan bus"
as API. The is optee_enumerate_devices() is split up on 2 stages.
Maxim.
> /Jarkko
>