[RFC PATCH 0/2] allow optee to be exposed on ACPI systems

[RFC PATCH 0/2] allow optee to be exposed on ACPI systems

[Ard Biesheuvel]

Similar to how OP-TEE is exposed as a pseudo device under /firmware/optee
on DT systems, permit OP-TEE presence to be exposed via a device object
in the ACPI namespace. This makes it possible to model the OP-TEE interface
as a platform device gets instantiated automatically both on DT and ACPI
systems, and implement the driver as a platform driver that is able to
use the generic device properties API to access the 'method' attribute
as well as potential future extensions to the binding that introduce
new attributes.

What remains to be discussed is how to expose OP-TEE pseudo devices,
e.g., Sumit's RNG implementation on SynQuacer which we would like to
bind a Linux driver to.

Cc: Jens Wiklander <jens.wiklander@linaro.org>
Cc: Sumit Garg <sumit.garg@linaro.org>
Cc: Graeme Gregory <graeme.gregory@linaro.org>
Cc: Jerome Forissier <jerome.forissier@linaro.org>

Ard Biesheuvel (2):
  optee: model OP-TEE as a platform device/driver
  optee: add ACPI support

 drivers/tee/optee/core.c | 94 +++++++++-----------
 1 file changed, 41 insertions(+), 53 deletions(-)

-- 
2.19.2

[RFC PATCH 1/2] optee: model OP-TEE as a platform device/driver

[Ard Biesheuvel]

To simplify adding ACPI support to the OP-TEE driver, model it as
a platform driver. This will permit us to use the generic device
property layer for parsing additional properties, regardless of
whether DT or ACPI is being used.

Note that this change will result in the OP-TEE driver to be loaded
automatically on systems that advertise the presence of OP-TEE via
the device tree.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/tee/optee/core.c | 86 ++++++++------------
 1 file changed, 32 insertions(+), 54 deletions(-)

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index e1aafe842d66..61ea65cfaedd 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -529,13 +529,13 @@ static void optee_smccc_hvc(unsigned long a0, unsigned long a1,
 	arm_smccc_hvc(a0, a1, a2, a3, a4, a5, a6, a7, res);
 }
 
-static optee_invoke_fn *get_invoke_func(struct device_node *np)
+static optee_invoke_fn *get_invoke_func(struct device *dev)
 {
 	const char *method;
 
-	pr_info("probing for conduit method from DT.\n");
+	pr_info("probing for conduit method.\n");
 
-	if (of_property_read_string(np, "method", &method)) {
+	if (device_property_read_string(dev, "method", &method)) {
 		pr_warn("missing \"method\" property\n");
 		return ERR_PTR(-ENXIO);
 	}
@@ -549,7 +549,7 @@ static optee_invoke_fn *get_invoke_func(struct device_node *np)
 	return ERR_PTR(-EINVAL);
 }
 
-static struct optee *optee_probe(struct device_node *np)
+static int optee_probe(struct platform_device *pdev)
 {
 	optee_invoke_fn *invoke_fn;
 	struct tee_shm_pool *pool;
@@ -559,25 +559,25 @@ static struct optee *optee_probe(struct device_node *np)
 	u32 sec_caps;
 	int rc;
 
-	invoke_fn = get_invoke_func(np);
+	invoke_fn = get_invoke_func(&pdev->dev);
 	if (IS_ERR(invoke_fn))
-		return (void *)invoke_fn;
+		return PTR_ERR(invoke_fn);
 
 	if (!optee_msg_api_uid_is_optee_api(invoke_fn)) {
 		pr_warn("api uid mismatch\n");
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
 
 	optee_msg_get_os_revision(invoke_fn);
 
 	if (!optee_msg_api_revision_is_compatible(invoke_fn)) {
 		pr_warn("api revision mismatch\n");
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
 
 	if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps)) {
 		pr_warn("capabilities mismatch\n");
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
 
 	/*
@@ -585,11 +585,11 @@ static struct optee *optee_probe(struct device_node *np)
 	 * doesn't have any reserved memory we can use we can't continue.
 	 */
 	if (!(sec_caps & OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM))
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 
 	pool = optee_config_shm_memremap(invoke_fn, &memremaped_shm, sec_caps);
 	if (IS_ERR(pool))
-		return (void *)pool;
+		return PTR_ERR(pool);
 
 	optee = kzalloc(sizeof(*optee), GFP_KERNEL);
 	if (!optee) {
@@ -600,6 +600,8 @@ static struct optee *optee_probe(struct device_node *np)
 	optee->invoke_fn = invoke_fn;
 	optee->sec_caps = sec_caps;
 
+	platform_set_drvdata(pdev, optee);
+
 	teedev = tee_device_alloc(&optee_desc, NULL, pool, optee);
 	if (IS_ERR(teedev)) {
 		rc = PTR_ERR(teedev);
@@ -632,7 +634,7 @@ static struct optee *optee_probe(struct device_node *np)
 	optee_enable_shm_cache(optee);
 
 	pr_info("initialized driver\n");
-	return optee;
+	return 0;
 err:
 	if (optee) {
 		/*
@@ -648,11 +650,13 @@ static struct optee *optee_probe(struct device_node *np)
 		tee_shm_pool_free(pool);
 	if (memremaped_shm)
 		memunmap(memremaped_shm);
-	return ERR_PTR(rc);
+	return rc;
 }
 
-static void optee_remove(struct optee *optee)
+static int optee_remove(struct platform_device *pdev)
 {
+	struct optee *optee = platform_get_drvdata(pdev);
+
 	/*
 	 * Ask OP-TEE to free all cached shared memory objects to decrease
 	 * reference counters and also avoid wild pointers in secure world
@@ -675,51 +679,25 @@ static void optee_remove(struct optee *optee)
 	mutex_destroy(&optee->call_queue.mutex);
 
 	kfree(optee);
+
+	return 0;
 }
 
-static const struct of_device_id optee_match[] = {
+static const struct of_device_id optee_dt_match[] = {
 	{ .compatible = "linaro,optee-tz" },
 	{},
 };
-
-static struct optee *optee_svc;
-
-static int __init optee_driver_init(void)
-{
-	struct device_node *fw_np;
-	struct device_node *np;
-	struct optee *optee;
-
-	/* Node is supposed to be below /firmware */
-	fw_np = of_find_node_by_name(NULL, "firmware");
-	if (!fw_np)
-		return -ENODEV;
-
-	np = of_find_matching_node(fw_np, optee_match);
-	if (!np)
-		return -ENODEV;
-
-	optee = optee_probe(np);
-	of_node_put(np);
-
-	if (IS_ERR(optee))
-		return PTR_ERR(optee);
-
-	optee_svc = optee;
-
-	return 0;
-}
-module_init(optee_driver_init);
-
-static void __exit optee_driver_exit(void)
-{
-	struct optee *optee = optee_svc;
-
-	optee_svc = NULL;
-	if (optee)
-		optee_remove(optee);
-}
-module_exit(optee_driver_exit);
+MODULE_DEVICE_TABLE(of, optee_dt_match);
+
+static struct platform_driver optee_driver = {
+	.probe  = optee_probe,
+	.remove = optee_remove,
+	.driver = {
+		.name = "optee",
+		.of_match_table = optee_dt_match,
+	},
+};
+module_platform_driver(optee_driver);
 
 MODULE_AUTHOR("Linaro");
 MODULE_DESCRIPTION("OP-TEE driver");
-- 
2.19.2

[RFC PATCH 2/2] optee: add ACPI support

[Ard Biesheuvel]

Add support for devices that expose the presence of OPTEE via device
object in the ACPI namespace.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/tee/optee/core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 61ea65cfaedd..94b2fd08b446 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -14,6 +14,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/acpi.h>
 #include <linux/arm-smccc.h>
 #include <linux/errno.h>
 #include <linux/io.h>
@@ -689,12 +690,21 @@ static const struct of_device_id optee_dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, optee_dt_match);
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id optee_acpi_match[] = {
+	{ "LNRO0020" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, optee_acpi_match);
+#endif
+
 static struct platform_driver optee_driver = {
 	.probe  = optee_probe,
 	.remove = optee_remove,
 	.driver = {
 		.name = "optee",
 		.of_match_table = optee_dt_match,
+		.acpi_match_table = ACPI_PTR(optee_acpi_match),
 	},
 };
 module_platform_driver(optee_driver);
-- 
2.19.2

Re: [RFC PATCH 0/2] allow optee to be exposed on ACPI systems

[Sumit Garg]

On Fri, 28 Dec 2018 at 00:31, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> Similar to how OP-TEE is exposed as a pseudo device under /firmware/optee
> on DT systems, permit OP-TEE presence to be exposed via a device object
> in the ACPI namespace. This makes it possible to model the OP-TEE interface
> as a platform device gets instantiated automatically both on DT and ACPI
> systems, and implement the driver as a platform driver that is able to
> use the generic device properties API to access the 'method' attribute
> as well as potential future extensions to the binding that introduce
> new attributes.
>
> What remains to be discussed is how to expose OP-TEE pseudo devices,
> e.g., Sumit's RNG implementation on SynQuacer which we would like to
> bind a Linux driver to.
>
> Cc: Jens Wiklander <jens.wiklander@linaro.org>
> Cc: Sumit Garg <sumit.garg@linaro.org>
> Cc: Graeme Gregory <graeme.gregory@linaro.org>
> Cc: Jerome Forissier <jerome.forissier@linaro.org>
>
> Ard Biesheuvel (2):
>   optee: model OP-TEE as a platform device/driver
>   optee: add ACPI support
>
>  drivers/tee/optee/core.c | 94 +++++++++-----------
>  1 file changed, 41 insertions(+), 53 deletions(-)
>

Looks good to me.

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


> --
> 2.19.2
>

Re: [RFC PATCH 1/2] optee: model OP-TEE as a platform device/driver

[Jens Wiklander]

Hi Ard and Arnd,

On Thu, Dec 27, 2018 at 8:01 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> To simplify adding ACPI support to the OP-TEE driver, model it as
> a platform driver. This will permit us to use the generic device
> property layer for parsing additional properties, regardless of
> whether DT or ACPI is being used.
>
> Note that this change will result in the OP-TEE driver to be loaded
> automatically on systems that advertise the presence of OP-TEE via
> the device tree.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/tee/optee/core.c | 86 ++++++++------------
>  1 file changed, 32 insertions(+), 54 deletions(-)

In the earlier postings of the TEE subsystem (v8) the OP-TEE device
was based on a platform device. Arnd didn't like this back then so it
was dropped. The situation has changed a bit now. A platform device
will make it easier to add ACPI support. Arnd, what do you think?

Thanks,
Jens

>
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index e1aafe842d66..61ea65cfaedd 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -529,13 +529,13 @@ static void optee_smccc_hvc(unsigned long a0, unsigned long a1,
>         arm_smccc_hvc(a0, a1, a2, a3, a4, a5, a6, a7, res);
>  }
>
> -static optee_invoke_fn *get_invoke_func(struct device_node *np)
> +static optee_invoke_fn *get_invoke_func(struct device *dev)
>  {
>         const char *method;
>
> -       pr_info("probing for conduit method from DT.\n");
> +       pr_info("probing for conduit method.\n");
>
> -       if (of_property_read_string(np, "method", &method)) {
> +       if (device_property_read_string(dev, "method", &method)) {
>                 pr_warn("missing \"method\" property\n");
>                 return ERR_PTR(-ENXIO);
>         }
> @@ -549,7 +549,7 @@ static optee_invoke_fn *get_invoke_func(struct device_node *np)
>         return ERR_PTR(-EINVAL);
>  }
>
> -static struct optee *optee_probe(struct device_node *np)
> +static int optee_probe(struct platform_device *pdev)
>  {
>         optee_invoke_fn *invoke_fn;
>         struct tee_shm_pool *pool;
> @@ -559,25 +559,25 @@ static struct optee *optee_probe(struct device_node *np)
>         u32 sec_caps;
>         int rc;
>
> -       invoke_fn = get_invoke_func(np);
> +       invoke_fn = get_invoke_func(&pdev->dev);
>         if (IS_ERR(invoke_fn))
> -               return (void *)invoke_fn;
> +               return PTR_ERR(invoke_fn);
>
>         if (!optee_msg_api_uid_is_optee_api(invoke_fn)) {
>                 pr_warn("api uid mismatch\n");
> -               return ERR_PTR(-EINVAL);
> +               return -EINVAL;
>         }
>
>         optee_msg_get_os_revision(invoke_fn);
>
>         if (!optee_msg_api_revision_is_compatible(invoke_fn)) {
>                 pr_warn("api revision mismatch\n");
> -               return ERR_PTR(-EINVAL);
> +               return -EINVAL;
>         }
>
>         if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps)) {
>                 pr_warn("capabilities mismatch\n");
> -               return ERR_PTR(-EINVAL);
> +               return -EINVAL;
>         }
>
>         /*
> @@ -585,11 +585,11 @@ static struct optee *optee_probe(struct device_node *np)
>          * doesn't have any reserved memory we can use we can't continue.
>          */
>         if (!(sec_caps & OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM))
> -               return ERR_PTR(-EINVAL);
> +               return -EINVAL;
>
>         pool = optee_config_shm_memremap(invoke_fn, &memremaped_shm, sec_caps);
>         if (IS_ERR(pool))
> -               return (void *)pool;
> +               return PTR_ERR(pool);
>
>         optee = kzalloc(sizeof(*optee), GFP_KERNEL);
>         if (!optee) {
> @@ -600,6 +600,8 @@ static struct optee *optee_probe(struct device_node *np)
>         optee->invoke_fn = invoke_fn;
>         optee->sec_caps = sec_caps;
>
> +       platform_set_drvdata(pdev, optee);
> +
>         teedev = tee_device_alloc(&optee_desc, NULL, pool, optee);
>         if (IS_ERR(teedev)) {
>                 rc = PTR_ERR(teedev);
> @@ -632,7 +634,7 @@ static struct optee *optee_probe(struct device_node *np)
>         optee_enable_shm_cache(optee);
>
>         pr_info("initialized driver\n");
> -       return optee;
> +       return 0;
>  err:
>         if (optee) {
>                 /*
> @@ -648,11 +650,13 @@ static struct optee *optee_probe(struct device_node *np)
>                 tee_shm_pool_free(pool);
>         if (memremaped_shm)
>                 memunmap(memremaped_shm);
> -       return ERR_PTR(rc);
> +       return rc;
>  }
>
> -static void optee_remove(struct optee *optee)
> +static int optee_remove(struct platform_device *pdev)
>  {
> +       struct optee *optee = platform_get_drvdata(pdev);
> +
>         /*
>          * Ask OP-TEE to free all cached shared memory objects to decrease
>          * reference counters and also avoid wild pointers in secure world
> @@ -675,51 +679,25 @@ static void optee_remove(struct optee *optee)
>         mutex_destroy(&optee->call_queue.mutex);
>
>         kfree(optee);
> +
> +       return 0;
>  }
>
> -static const struct of_device_id optee_match[] = {
> +static const struct of_device_id optee_dt_match[] = {
>         { .compatible = "linaro,optee-tz" },
>         {},
>  };
> -
> -static struct optee *optee_svc;
> -
> -static int __init optee_driver_init(void)
> -{
> -       struct device_node *fw_np;
> -       struct device_node *np;
> -       struct optee *optee;
> -
> -       /* Node is supposed to be below /firmware */
> -       fw_np = of_find_node_by_name(NULL, "firmware");
> -       if (!fw_np)
> -               return -ENODEV;
> -
> -       np = of_find_matching_node(fw_np, optee_match);
> -       if (!np)
> -               return -ENODEV;
> -
> -       optee = optee_probe(np);
> -       of_node_put(np);
> -
> -       if (IS_ERR(optee))
> -               return PTR_ERR(optee);
> -
> -       optee_svc = optee;
> -
> -       return 0;
> -}
> -module_init(optee_driver_init);
> -
> -static void __exit optee_driver_exit(void)
> -{
> -       struct optee *optee = optee_svc;
> -
> -       optee_svc = NULL;
> -       if (optee)
> -               optee_remove(optee);
> -}
> -module_exit(optee_driver_exit);
> +MODULE_DEVICE_TABLE(of, optee_dt_match);
> +
> +static struct platform_driver optee_driver = {
> +       .probe  = optee_probe,
> +       .remove = optee_remove,
> +       .driver = {
> +               .name = "optee",
> +               .of_match_table = optee_dt_match,
> +       },
> +};
> +module_platform_driver(optee_driver);
>
>  MODULE_AUTHOR("Linaro");
>  MODULE_DESCRIPTION("OP-TEE driver");
> --
> 2.19.2
>