On Tue, Oct 12, 2021 at 10:27 AM Sumit Garg [off-list ref] wrote:

Hi Jens,

On Tue, 12 Oct 2021 at 13:29, Jens Wiklander [off-list ref] wrote:

quoted

Hi Sumit,

On Tue, Oct 12, 2021 at 9:31 AM Sumit Garg [off-list ref] wrote:

quoted

When OP-TEE driver is built as a module, OP-TEE client devices
registered on TEE bus during probe should be unregistered during
optee_remove. So implement optee_unregister_devices() accordingly.

Fixes: c3fa24af9244 ("tee: optee: add TEE bus device enumeration support")
Reported-by: Sudeep Holla <redacted>
Signed-off-by: Sumit Garg <redacted>
---
 drivers/tee/optee/core.c          |  3 +++
 drivers/tee/optee/device.c        | 22 ++++++++++++++++++++++
 drivers/tee/optee/optee_private.h |  1 +
 3 files changed, 26 insertions(+)

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index ccad3c7c8f6d..3915dc574503 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c

@@ -586,6 +586,9 @@ static int optee_remove(struct platform_device *pdev)
 {
        struct optee *optee = platform_get_drvdata(pdev);

+       /* Unregister OP-TEE specific client devices on TEE bus */
+       optee_unregister_devices();
+
        /*
         * Ask OP-TEE to free all cached shared memory objects to decrease
         * reference counters and also avoid wild pointers in secure world

diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
index ec1d24693eba..128a2d2a50a1 100644
--- a/drivers/tee/optee/device.c
+++ b/drivers/tee/optee/device.c

@@ -53,6 +53,13 @@ static int get_devices(struct tee_context *ctx, u32 session,
        return 0;
 }

+static void optee_release_device(struct device *dev)
+{
+       struct tee_client_device *optee_device = to_tee_client_device(dev);
+
+       kfree(optee_device);
+}
+
 static int optee_register_device(const uuid_t *device_uuid)
 {
        struct tee_client_device *optee_device = NULL;

@@ -63,6 +70,7 @@ static int optee_register_device(const uuid_t *device_uuid)
                return -ENOMEM;

        optee_device->dev.bus = &tee_bus_type;
+       optee_device->dev.release = optee_release_device;
        if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
                kfree(optee_device);
                return -ENOMEM;

@@ -154,3 +162,17 @@ int optee_enumerate_devices(u32 func)
 {
        return  __optee_enumerate_devices(func);
 }
+
+static int __optee_unregister_device(struct device *dev, void *data)
+{
+       if (!strncmp(dev_name(dev), "optee-ta", strlen("optee-ta")))

The issue you described below should be handled by this check as we
would register TAs with unique name corresponding to each OP-TEE
driver.

quoted

quoted

+               device_unregister(dev);
+
+       return 0;
+}
+
+void optee_unregister_devices(void)
+{
+       bus_for_each_dev(&tee_bus_type, NULL, NULL,
+                        __optee_unregister_device);

I had something like this in mind too, but there's one potential
problem with this approach. What if there's more than one OP-TEE
driver with TAs here? It seems that we'll remove TAs from other
drivers too then.

We should be able to easily differentiate among TAs associated with
any of multiple OP-TEE drivers based on their unique device name.

quoted

This is not likely to be a problem at upstream for the moment so I
might be enough just to keep this in mind if/when the OP-TEE driver is
extended in a way that there can be multiple OP-TEEs handled.

Given above comments, I think it should be easily handled.

OK, thanks. The patch looks good and it works when testing it on the
upstream kernel and also with the FF-A patch set.
I'm picking up this now.

Cheers,
Jens

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel