Move the pool alloc and free helper functions from the OP-TEE driver to
the TEE subsystem, since these could be reused in other TEE drivers.
This patch is not supposed to change behavior, it's only reorganizing
the code.
Suggested-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Balint Dobszay <balint.dobszay@arm.com>
---
drivers/tee/optee/core.c | 64 ------------------------------
drivers/tee/optee/ffa_abi.c | 6 +--
drivers/tee/optee/optee_private.h | 12 ------
drivers/tee/optee/smc_abi.c | 11 +++---
drivers/tee/tee_shm.c | 65 +++++++++++++++++++++++++++++++
include/linux/tee_drv.h | 11 ++++++
6 files changed, 85 insertions(+), 84 deletions(-)
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 3aed554bc8d8..9390f21f9902 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -9,7 +9,6 @@
#include <linux/crash_dump.h>
#include <linux/errno.h>
#include <linux/io.h>
-#include <linux/mm.h>
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/string.h>
@@ -17,69 +16,6 @@
#include <linux/types.h>
#include "optee_private.h"
-int optee_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
- size_t size, size_t align,
- int (*shm_register)(struct tee_context *ctx,
- struct tee_shm *shm,
- struct page **pages,
- size_t num_pages,
- unsigned long start))
-{
- size_t nr_pages = roundup(size, PAGE_SIZE) / PAGE_SIZE;
- struct page **pages;
- unsigned int i;
- int rc = 0;
-
- /*
- * Ignore alignment since this is already going to be page aligned
- * and there's no need for any larger alignment.
- */
- shm->kaddr = alloc_pages_exact(nr_pages * PAGE_SIZE,
- GFP_KERNEL | __GFP_ZERO);
- if (!shm->kaddr)
- return -ENOMEM;
-
- shm->paddr = virt_to_phys(shm->kaddr);
- shm->size = nr_pages * PAGE_SIZE;
-
- pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
- if (!pages) {
- rc = -ENOMEM;
- goto err;
- }
-
- for (i = 0; i < nr_pages; i++)
- pages[i] = virt_to_page((u8 *)shm->kaddr + i * PAGE_SIZE);
-
- shm->pages = pages;
- shm->num_pages = nr_pages;
-
- if (shm_register) {
- rc = shm_register(shm->ctx, shm, pages, nr_pages,
- (unsigned long)shm->kaddr);
- if (rc)
- goto err;
- }
-
- return 0;
-err:
- free_pages_exact(shm->kaddr, shm->size);
- shm->kaddr = NULL;
- return rc;
-}
-
-void optee_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
- int (*shm_unregister)(struct tee_context *ctx,
- struct tee_shm *shm))
-{
- if (shm_unregister)
- shm_unregister(shm->ctx, shm);
- free_pages_exact(shm->kaddr, shm->size);
- shm->kaddr = NULL;
- kfree(shm->pages);
- shm->pages = NULL;
-}
-
static void optee_bus_scan(struct work_struct *work)
{
WARN_ON(optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP));
diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
index ecb5eb079408..ee11918a2b35 100644
--- a/drivers/tee/optee/ffa_abi.c
+++ b/drivers/tee/optee/ffa_abi.c
@@ -374,14 +374,14 @@ static int optee_ffa_shm_unregister_supp(struct tee_context *ctx,
static int pool_ffa_op_alloc(struct tee_shm_pool *pool,
struct tee_shm *shm, size_t size, size_t align)
{
- return optee_pool_op_alloc_helper(pool, shm, size, align,
- optee_ffa_shm_register);
+ return tee_shm_pool_op_alloc_helper(pool, shm, size, align,
+ optee_ffa_shm_register);
}
static void pool_ffa_op_free(struct tee_shm_pool *pool,
struct tee_shm *shm)
{
- optee_pool_op_free_helper(pool, shm, optee_ffa_shm_unregister);
+ tee_shm_pool_op_free_helper(pool, shm, optee_ffa_shm_unregister);
}
static void pool_ffa_op_destroy_pool(struct tee_shm_pool *pool)
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index 7a5243c78b55..a153285a1919 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -283,18 +283,6 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session);
int optee_enumerate_devices(u32 func);
void optee_unregister_devices(void);
-int optee_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
- size_t size, size_t align,
- int (*shm_register)(struct tee_context *ctx,
- struct tee_shm *shm,
- struct page **pages,
- size_t num_pages,
- unsigned long start));
-void optee_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
- int (*shm_unregister)(struct tee_context *ctx,
- struct tee_shm *shm));
-
-
void optee_remove_common(struct optee *optee);
int optee_open(struct tee_context *ctx, bool cap_memref_null);
void optee_release(struct tee_context *ctx);
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
index a37f87087e5c..b0c616b6870d 100644
--- a/drivers/tee/optee/smc_abi.c
+++ b/drivers/tee/optee/smc_abi.c
@@ -592,19 +592,20 @@ static int pool_op_alloc(struct tee_shm_pool *pool,
* to be registered with OP-TEE.
*/
if (shm->flags & TEE_SHM_PRIV)
- return optee_pool_op_alloc_helper(pool, shm, size, align, NULL);
+ return tee_shm_pool_op_alloc_helper(pool, shm, size, align,
+ NULL);
- return optee_pool_op_alloc_helper(pool, shm, size, align,
- optee_shm_register);
+ return tee_shm_pool_op_alloc_helper(pool, shm, size, align,
+ optee_shm_register);
}
static void pool_op_free(struct tee_shm_pool *pool,
struct tee_shm *shm)
{
if (!(shm->flags & TEE_SHM_PRIV))
- optee_pool_op_free_helper(pool, shm, optee_shm_unregister);
+ tee_shm_pool_op_free_helper(pool, shm, optee_shm_unregister);
else
- optee_pool_op_free_helper(pool, shm, NULL);
+ tee_shm_pool_op_free_helper(pool, shm, NULL);
}
static void pool_op_destroy_pool(struct tee_shm_pool *pool)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 731d9028b67f..641aad92ffe2 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -202,6 +202,71 @@ struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size)
}
EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf);
+int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
+ size_t size, size_t align,
+ int (*shm_register)(struct tee_context *ctx,
+ struct tee_shm *shm,
+ struct page **pages,
+ size_t num_pages,
+ unsigned long start))
+{
+ size_t nr_pages = roundup(size, PAGE_SIZE) / PAGE_SIZE;
+ struct page **pages;
+ unsigned int i;
+ int rc = 0;
+
+ /*
+ * Ignore alignment since this is already going to be page aligned
+ * and there's no need for any larger alignment.
+ */
+ shm->kaddr = alloc_pages_exact(nr_pages * PAGE_SIZE,
+ GFP_KERNEL | __GFP_ZERO);
+ if (!shm->kaddr)
+ return -ENOMEM;
+
+ shm->paddr = virt_to_phys(shm->kaddr);
+ shm->size = nr_pages * PAGE_SIZE;
+
+ pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
+ if (!pages) {
+ rc = -ENOMEM;
+ goto err;
+ }
+
+ for (i = 0; i < nr_pages; i++)
+ pages[i] = virt_to_page((u8 *)shm->kaddr + i * PAGE_SIZE);
+
+ shm->pages = pages;
+ shm->num_pages = nr_pages;
+
+ if (shm_register) {
+ rc = shm_register(shm->ctx, shm, pages, nr_pages,
+ (unsigned long)shm->kaddr);
+ if (rc)
+ goto err;
+ }
+
+ return 0;
+err:
+ free_pages_exact(shm->kaddr, shm->size);
+ shm->kaddr = NULL;
+ return rc;
+}
+EXPORT_SYMBOL_GPL(tee_shm_pool_op_alloc_helper);
+
+void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
+ int (*shm_unregister)(struct tee_context *ctx,
+ struct tee_shm *shm))
+{
+ if (shm_unregister)
+ shm_unregister(shm->ctx, shm);
+ free_pages_exact(shm->kaddr, shm->size);
+ shm->kaddr = NULL;
+ kfree(shm->pages);
+ shm->pages = NULL;
+}
+EXPORT_SYMBOL_GPL(tee_shm_pool_op_free_helper);
+
static struct tee_shm *
register_shm_helper(struct tee_context *ctx, struct iov_iter *iter, u32 flags,
int id)
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 911ddf92dcee..4cf402424e71 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -275,6 +275,17 @@ void *tee_get_drvdata(struct tee_device *teedev);
struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size);
struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
+int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
+ size_t size, size_t align,
+ int (*shm_register)(struct tee_context *ctx,
+ struct tee_shm *shm,
+ struct page **pages,
+ size_t num_pages,
+ unsigned long start));
+void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
+ int (*shm_unregister)(struct tee_context *ctx,
+ struct tee_shm *shm));
+
struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx,
void *addr, size_t length);
--
2.34.1Hi Balint,
On Fri, 23 Feb 2024 at 15:22, Balint Dobszay <balint.dobszay@arm.com> wrote:
>
> Move the pool alloc and free helper functions from the OP-TEE driver to
> the TEE subsystem, since these could be reused in other TEE drivers.
> This patch is not supposed to change behavior, it's only reorganizing
> the code.
>
> Suggested-by: Jens Wiklander <jens.wiklander@linaro.org>
> Signed-off-by: Balint Dobszay <balint.dobszay@arm.com>
> ---
> drivers/tee/optee/core.c | 64 ------------------------------
> drivers/tee/optee/ffa_abi.c | 6 +--
> drivers/tee/optee/optee_private.h | 12 ------
> drivers/tee/optee/smc_abi.c | 11 +++---
> drivers/tee/tee_shm.c | 65 +++++++++++++++++++++++++++++++
> include/linux/tee_drv.h | 11 ++++++
> 6 files changed, 85 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 3aed554bc8d8..9390f21f9902 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -9,7 +9,6 @@
> #include <linux/crash_dump.h>
> #include <linux/errno.h>
> #include <linux/io.h>
> -#include <linux/mm.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/string.h>
> @@ -17,69 +16,6 @@
> #include <linux/types.h>
> #include "optee_private.h"
>
> -int optee_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> - size_t size, size_t align,
> - int (*shm_register)(struct tee_context *ctx,
> - struct tee_shm *shm,
> - struct page **pages,
> - size_t num_pages,
> - unsigned long start))
> -{
> - size_t nr_pages = roundup(size, PAGE_SIZE) / PAGE_SIZE;
> - struct page **pages;
> - unsigned int i;
> - int rc = 0;
> -
> - /*
> - * Ignore alignment since this is already going to be page aligned
> - * and there's no need for any larger alignment.
> - */
> - shm->kaddr = alloc_pages_exact(nr_pages * PAGE_SIZE,
> - GFP_KERNEL | __GFP_ZERO);
> - if (!shm->kaddr)
> - return -ENOMEM;
> -
> - shm->paddr = virt_to_phys(shm->kaddr);
> - shm->size = nr_pages * PAGE_SIZE;
> -
> - pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
> - if (!pages) {
> - rc = -ENOMEM;
> - goto err;
> - }
> -
> - for (i = 0; i < nr_pages; i++)
> - pages[i] = virt_to_page((u8 *)shm->kaddr + i * PAGE_SIZE);
> -
> - shm->pages = pages;
> - shm->num_pages = nr_pages;
> -
> - if (shm_register) {
> - rc = shm_register(shm->ctx, shm, pages, nr_pages,
> - (unsigned long)shm->kaddr);
> - if (rc)
> - goto err;
> - }
> -
> - return 0;
> -err:
> - free_pages_exact(shm->kaddr, shm->size);
> - shm->kaddr = NULL;
> - return rc;
> -}
> -
> -void optee_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> - int (*shm_unregister)(struct tee_context *ctx,
> - struct tee_shm *shm))
> -{
> - if (shm_unregister)
> - shm_unregister(shm->ctx, shm);
> - free_pages_exact(shm->kaddr, shm->size);
> - shm->kaddr = NULL;
> - kfree(shm->pages);
> - shm->pages = NULL;
> -}
> -
> static void optee_bus_scan(struct work_struct *work)
> {
> WARN_ON(optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP));
> diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> index ecb5eb079408..ee11918a2b35 100644
> --- a/drivers/tee/optee/ffa_abi.c
> +++ b/drivers/tee/optee/ffa_abi.c
> @@ -374,14 +374,14 @@ static int optee_ffa_shm_unregister_supp(struct tee_context *ctx,
> static int pool_ffa_op_alloc(struct tee_shm_pool *pool,
> struct tee_shm *shm, size_t size, size_t align)
> {
> - return optee_pool_op_alloc_helper(pool, shm, size, align,
> - optee_ffa_shm_register);
> + return tee_shm_pool_op_alloc_helper(pool, shm, size, align,
> + optee_ffa_shm_register);
> }
>
> static void pool_ffa_op_free(struct tee_shm_pool *pool,
> struct tee_shm *shm)
> {
> - optee_pool_op_free_helper(pool, shm, optee_ffa_shm_unregister);
> + tee_shm_pool_op_free_helper(pool, shm, optee_ffa_shm_unregister);
> }
>
> static void pool_ffa_op_destroy_pool(struct tee_shm_pool *pool)
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index 7a5243c78b55..a153285a1919 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -283,18 +283,6 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session);
> int optee_enumerate_devices(u32 func);
> void optee_unregister_devices(void);
>
> -int optee_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> - size_t size, size_t align,
> - int (*shm_register)(struct tee_context *ctx,
> - struct tee_shm *shm,
> - struct page **pages,
> - size_t num_pages,
> - unsigned long start));
> -void optee_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> - int (*shm_unregister)(struct tee_context *ctx,
> - struct tee_shm *shm));
> -
> -
> void optee_remove_common(struct optee *optee);
> int optee_open(struct tee_context *ctx, bool cap_memref_null);
> void optee_release(struct tee_context *ctx);
> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> index a37f87087e5c..b0c616b6870d 100644
> --- a/drivers/tee/optee/smc_abi.c
> +++ b/drivers/tee/optee/smc_abi.c
> @@ -592,19 +592,20 @@ static int pool_op_alloc(struct tee_shm_pool *pool,
> * to be registered with OP-TEE.
> */
> if (shm->flags & TEE_SHM_PRIV)
> - return optee_pool_op_alloc_helper(pool, shm, size, align, NULL);
> + return tee_shm_pool_op_alloc_helper(pool, shm, size, align,
> + NULL);
>
> - return optee_pool_op_alloc_helper(pool, shm, size, align,
> - optee_shm_register);
> + return tee_shm_pool_op_alloc_helper(pool, shm, size, align,
> + optee_shm_register);
> }
>
> static void pool_op_free(struct tee_shm_pool *pool,
> struct tee_shm *shm)
> {
> if (!(shm->flags & TEE_SHM_PRIV))
> - optee_pool_op_free_helper(pool, shm, optee_shm_unregister);
> + tee_shm_pool_op_free_helper(pool, shm, optee_shm_unregister);
> else
> - optee_pool_op_free_helper(pool, shm, NULL);
> + tee_shm_pool_op_free_helper(pool, shm, NULL);
> }
>
> static void pool_op_destroy_pool(struct tee_shm_pool *pool)
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index 731d9028b67f..641aad92ffe2 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -202,6 +202,71 @@ struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size)
> }
> EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf);
>
> +int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
I don't see the first argument (struct tee_shm_pool *pool) being used,
so drop that. Also, we can just rename it as
tee_dyn_shm_alloc_helper().
> + size_t size, size_t align,
> + int (*shm_register)(struct tee_context *ctx,
> + struct tee_shm *shm,
> + struct page **pages,
> + size_t num_pages,
> + unsigned long start))
> +{
> + size_t nr_pages = roundup(size, PAGE_SIZE) / PAGE_SIZE;
> + struct page **pages;
> + unsigned int i;
> + int rc = 0;
> +
> + /*
> + * Ignore alignment since this is already going to be page aligned
> + * and there's no need for any larger alignment.
> + */
> + shm->kaddr = alloc_pages_exact(nr_pages * PAGE_SIZE,
> + GFP_KERNEL | __GFP_ZERO);
> + if (!shm->kaddr)
> + return -ENOMEM;
> +
> + shm->paddr = virt_to_phys(shm->kaddr);
> + shm->size = nr_pages * PAGE_SIZE;
> +
> + pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
> + if (!pages) {
> + rc = -ENOMEM;
> + goto err;
> + }
> +
> + for (i = 0; i < nr_pages; i++)
> + pages[i] = virt_to_page((u8 *)shm->kaddr + i * PAGE_SIZE);
> +
> + shm->pages = pages;
> + shm->num_pages = nr_pages;
> +
> + if (shm_register) {
> + rc = shm_register(shm->ctx, shm, pages, nr_pages,
> + (unsigned long)shm->kaddr);
> + if (rc)
> + goto err;
> + }
> +
> + return 0;
> +err:
> + free_pages_exact(shm->kaddr, shm->size);
> + shm->kaddr = NULL;
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(tee_shm_pool_op_alloc_helper);
> +
> +void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
Ditto tee_shm_pool_op_free_helper() -> tee_dyn_shm_free_helper()
> + int (*shm_unregister)(struct tee_context *ctx,
> + struct tee_shm *shm))
> +{
> + if (shm_unregister)
> + shm_unregister(shm->ctx, shm);
> + free_pages_exact(shm->kaddr, shm->size);
> + shm->kaddr = NULL;
> + kfree(shm->pages);
> + shm->pages = NULL;
> +}
> +EXPORT_SYMBOL_GPL(tee_shm_pool_op_free_helper);
> +
> static struct tee_shm *
> register_shm_helper(struct tee_context *ctx, struct iov_iter *iter, u32 flags,
> int id)
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 911ddf92dcee..4cf402424e71 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -275,6 +275,17 @@ void *tee_get_drvdata(struct tee_device *teedev);
> struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size);
> struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
>
> +int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> + size_t size, size_t align,
> + int (*shm_register)(struct tee_context *ctx,
> + struct tee_shm *shm,
> + struct page **pages,
> + size_t num_pages,
> + unsigned long start));
> +void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> + int (*shm_unregister)(struct tee_context *ctx,
> + struct tee_shm *shm));
> +
These rather belong to drivers/tee/tee_private.h as we shouldn't
expose them to other kernel client drivers.
-Sumit
> struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx,
> void *addr, size_t length);
>
> --
> 2.34.1
>
Hi Sumit,
On 27 Feb 2024, at 7:06, Sumit Garg wrote:
> Hi Balint,
>
> On Fri, 23 Feb 2024 at 15:22, Balint Dobszay <balint.dobszay@arm.com> wrote:
>>
>> Move the pool alloc and free helper functions from the OP-TEE driver to
>> the TEE subsystem, since these could be reused in other TEE drivers.
>> This patch is not supposed to change behavior, it's only reorganizing
>> the code.
>>
>> Suggested-by: Jens Wiklander <jens.wiklander@linaro.org>
>> Signed-off-by: Balint Dobszay <balint.dobszay@arm.com>
>> ---
>> drivers/tee/optee/core.c | 64 ------------------------------
>> drivers/tee/optee/ffa_abi.c | 6 +--
>> drivers/tee/optee/optee_private.h | 12 ------
>> drivers/tee/optee/smc_abi.c | 11 +++---
>> drivers/tee/tee_shm.c | 65 +++++++++++++++++++++++++++++++
>> include/linux/tee_drv.h | 11 ++++++
>> 6 files changed, 85 insertions(+), 84 deletions(-)
>>
>> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
>> index 3aed554bc8d8..9390f21f9902 100644
>> --- a/drivers/tee/optee/core.c
>> +++ b/drivers/tee/optee/core.c
>> @@ -9,7 +9,6 @@
>> #include <linux/crash_dump.h>
>> #include <linux/errno.h>
>> #include <linux/io.h>
>> -#include <linux/mm.h>
>> #include <linux/module.h>
>> #include <linux/slab.h>
>> #include <linux/string.h>
>> @@ -17,69 +16,6 @@
>> #include <linux/types.h>
>> #include "optee_private.h"
>>
>> -int optee_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
>> - size_t size, size_t align,
>> - int (*shm_register)(struct tee_context *ctx,
>> - struct tee_shm *shm,
>> - struct page **pages,
>> - size_t num_pages,
>> - unsigned long start))
>> -{
>> - size_t nr_pages = roundup(size, PAGE_SIZE) / PAGE_SIZE;
>> - struct page **pages;
>> - unsigned int i;
>> - int rc = 0;
>> -
>> - /*
>> - * Ignore alignment since this is already going to be page aligned
>> - * and there's no need for any larger alignment.
>> - */
>> - shm->kaddr = alloc_pages_exact(nr_pages * PAGE_SIZE,
>> - GFP_KERNEL | __GFP_ZERO);
>> - if (!shm->kaddr)
>> - return -ENOMEM;
>> -
>> - shm->paddr = virt_to_phys(shm->kaddr);
>> - shm->size = nr_pages * PAGE_SIZE;
>> -
>> - pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
>> - if (!pages) {
>> - rc = -ENOMEM;
>> - goto err;
>> - }
>> -
>> - for (i = 0; i < nr_pages; i++)
>> - pages[i] = virt_to_page((u8 *)shm->kaddr + i * PAGE_SIZE);
>> -
>> - shm->pages = pages;
>> - shm->num_pages = nr_pages;
>> -
>> - if (shm_register) {
>> - rc = shm_register(shm->ctx, shm, pages, nr_pages,
>> - (unsigned long)shm->kaddr);
>> - if (rc)
>> - goto err;
>> - }
>> -
>> - return 0;
>> -err:
>> - free_pages_exact(shm->kaddr, shm->size);
>> - shm->kaddr = NULL;
>> - return rc;
>> -}
>> -
>> -void optee_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
>> - int (*shm_unregister)(struct tee_context *ctx,
>> - struct tee_shm *shm))
>> -{
>> - if (shm_unregister)
>> - shm_unregister(shm->ctx, shm);
>> - free_pages_exact(shm->kaddr, shm->size);
>> - shm->kaddr = NULL;
>> - kfree(shm->pages);
>> - shm->pages = NULL;
>> -}
>> -
>> static void optee_bus_scan(struct work_struct *work)
>> {
>> WARN_ON(optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP));
>> diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
>> index ecb5eb079408..ee11918a2b35 100644
>> --- a/drivers/tee/optee/ffa_abi.c
>> +++ b/drivers/tee/optee/ffa_abi.c
>> @@ -374,14 +374,14 @@ static int optee_ffa_shm_unregister_supp(struct tee_context *ctx,
>> static int pool_ffa_op_alloc(struct tee_shm_pool *pool,
>> struct tee_shm *shm, size_t size, size_t align)
>> {
>> - return optee_pool_op_alloc_helper(pool, shm, size, align,
>> - optee_ffa_shm_register);
>> + return tee_shm_pool_op_alloc_helper(pool, shm, size, align,
>> + optee_ffa_shm_register);
>> }
>>
>> static void pool_ffa_op_free(struct tee_shm_pool *pool,
>> struct tee_shm *shm)
>> {
>> - optee_pool_op_free_helper(pool, shm, optee_ffa_shm_unregister);
>> + tee_shm_pool_op_free_helper(pool, shm, optee_ffa_shm_unregister);
>> }
>>
>> static void pool_ffa_op_destroy_pool(struct tee_shm_pool *pool)
>> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
>> index 7a5243c78b55..a153285a1919 100644
>> --- a/drivers/tee/optee/optee_private.h
>> +++ b/drivers/tee/optee/optee_private.h
>> @@ -283,18 +283,6 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session);
>> int optee_enumerate_devices(u32 func);
>> void optee_unregister_devices(void);
>>
>> -int optee_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
>> - size_t size, size_t align,
>> - int (*shm_register)(struct tee_context *ctx,
>> - struct tee_shm *shm,
>> - struct page **pages,
>> - size_t num_pages,
>> - unsigned long start));
>> -void optee_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
>> - int (*shm_unregister)(struct tee_context *ctx,
>> - struct tee_shm *shm));
>> -
>> -
>> void optee_remove_common(struct optee *optee);
>> int optee_open(struct tee_context *ctx, bool cap_memref_null);
>> void optee_release(struct tee_context *ctx);
>> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
>> index a37f87087e5c..b0c616b6870d 100644
>> --- a/drivers/tee/optee/smc_abi.c
>> +++ b/drivers/tee/optee/smc_abi.c
>> @@ -592,19 +592,20 @@ static int pool_op_alloc(struct tee_shm_pool *pool,
>> * to be registered with OP-TEE.
>> */
>> if (shm->flags & TEE_SHM_PRIV)
>> - return optee_pool_op_alloc_helper(pool, shm, size, align, NULL);
>> + return tee_shm_pool_op_alloc_helper(pool, shm, size, align,
>> + NULL);
>>
>> - return optee_pool_op_alloc_helper(pool, shm, size, align,
>> - optee_shm_register);
>> + return tee_shm_pool_op_alloc_helper(pool, shm, size, align,
>> + optee_shm_register);
>> }
>>
>> static void pool_op_free(struct tee_shm_pool *pool,
>> struct tee_shm *shm)
>> {
>> if (!(shm->flags & TEE_SHM_PRIV))
>> - optee_pool_op_free_helper(pool, shm, optee_shm_unregister);
>> + tee_shm_pool_op_free_helper(pool, shm, optee_shm_unregister);
>> else
>> - optee_pool_op_free_helper(pool, shm, NULL);
>> + tee_shm_pool_op_free_helper(pool, shm, NULL);
>> }
>>
>> static void pool_op_destroy_pool(struct tee_shm_pool *pool)
>> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
>> index 731d9028b67f..641aad92ffe2 100644
>> --- a/drivers/tee/tee_shm.c
>> +++ b/drivers/tee/tee_shm.c
>> @@ -202,6 +202,71 @@ struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size)
>> }
>> EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf);
>>
>> +int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
>
> I don't see the first argument (struct tee_shm_pool *pool) being used,
> so drop that. Also, we can just rename it as
> tee_dyn_shm_alloc_helper().
Okay, I'll refactor.
>> + size_t size, size_t align,
>> + int (*shm_register)(struct tee_context *ctx,
>> + struct tee_shm *shm,
>> + struct page **pages,
>> + size_t num_pages,
>> + unsigned long start))
>> +{
>> + size_t nr_pages = roundup(size, PAGE_SIZE) / PAGE_SIZE;
>> + struct page **pages;
>> + unsigned int i;
>> + int rc = 0;
>> +
>> + /*
>> + * Ignore alignment since this is already going to be page aligned
>> + * and there's no need for any larger alignment.
>> + */
>> + shm->kaddr = alloc_pages_exact(nr_pages * PAGE_SIZE,
>> + GFP_KERNEL | __GFP_ZERO);
>> + if (!shm->kaddr)
>> + return -ENOMEM;
>> +
>> + shm->paddr = virt_to_phys(shm->kaddr);
>> + shm->size = nr_pages * PAGE_SIZE;
>> +
>> + pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
>> + if (!pages) {
>> + rc = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + for (i = 0; i < nr_pages; i++)
>> + pages[i] = virt_to_page((u8 *)shm->kaddr + i * PAGE_SIZE);
>> +
>> + shm->pages = pages;
>> + shm->num_pages = nr_pages;
>> +
>> + if (shm_register) {
>> + rc = shm_register(shm->ctx, shm, pages, nr_pages,
>> + (unsigned long)shm->kaddr);
>> + if (rc)
>> + goto err;
>> + }
>> +
>> + return 0;
>> +err:
>> + free_pages_exact(shm->kaddr, shm->size);
>> + shm->kaddr = NULL;
>> + return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(tee_shm_pool_op_alloc_helper);
>> +
>> +void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
>
> Ditto tee_shm_pool_op_free_helper() -> tee_dyn_shm_free_helper()
Okay, actually the first argument is unused here too, I'll drop it.
>> + int (*shm_unregister)(struct tee_context *ctx,
>> + struct tee_shm *shm))
>> +{
>> + if (shm_unregister)
>> + shm_unregister(shm->ctx, shm);
>> + free_pages_exact(shm->kaddr, shm->size);
>> + shm->kaddr = NULL;
>> + kfree(shm->pages);
>> + shm->pages = NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(tee_shm_pool_op_free_helper);
>> +
>> static struct tee_shm *
>> register_shm_helper(struct tee_context *ctx, struct iov_iter *iter, u32 flags,
>> int id)
>> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
>> index 911ddf92dcee..4cf402424e71 100644
>> --- a/include/linux/tee_drv.h
>> +++ b/include/linux/tee_drv.h
>> @@ -275,6 +275,17 @@ void *tee_get_drvdata(struct tee_device *teedev);
>> struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size);
>> struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
>>
>> +int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
>> + size_t size, size_t align,
>> + int (*shm_register)(struct tee_context *ctx,
>> + struct tee_shm *shm,
>> + struct page **pages,
>> + size_t num_pages,
>> + unsigned long start));
>> +void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
>> + int (*shm_unregister)(struct tee_context *ctx,
>> + struct tee_shm *shm));
>> +
>
> These rather belong to drivers/tee/tee_private.h as we shouldn't
> expose them to other kernel client drivers.
As per the discussion in the other thread I'll ignore this.
Regards,
Balint
>> struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx,
>> void *addr, size_t length);
>>
>> --
>> 2.34.1
>>
Hi Balint, On Mon, 4 Mar 2024 at 14:33, Balint Dobszay <balint.dobszay@arm.com> wrote: > > Hi Sumit, > [snip] > >> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h > >> index 911ddf92dcee..4cf402424e71 100644 > >> --- a/include/linux/tee_drv.h > >> +++ b/include/linux/tee_drv.h > >> @@ -275,6 +275,17 @@ void *tee_get_drvdata(struct tee_device *teedev); > >> struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size); > >> struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size); > >> > >> +int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm, > >> + size_t size, size_t align, > >> + int (*shm_register)(struct tee_context *ctx, > >> + struct tee_shm *shm, > >> + struct page **pages, > >> + size_t num_pages, > >> + unsigned long start)); > >> +void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm, > >> + int (*shm_unregister)(struct tee_context *ctx, > >> + struct tee_shm *shm)); > >> + > > > > These rather belong to drivers/tee/tee_private.h as we shouldn't > > expose them to other kernel client drivers. > > As per the discussion in the other thread I'll ignore this. > Then it will have conflicts with this [1] patch. If you are fine to incorporate [1] in your series then the right place for these function declarations should be include/linux/tee_core.h. [1] https://www.spinics.net/lists/kernel/msg5122983.html -Sumit > Regards, > Balint > > >> struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx, > >> void *addr, size_t length); > >> > >> -- > >> 2.34.1 > >>
On 4 Mar 2024, at 10:17, Sumit Garg wrote: > Hi Balint, > > On Mon, 4 Mar 2024 at 14:33, Balint Dobszay <balint.dobszay@arm.com> wrote: >> >> Hi Sumit, >> > > [snip] > >>>> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h >>>> index 911ddf92dcee..4cf402424e71 100644 >>>> --- a/include/linux/tee_drv.h >>>> +++ b/include/linux/tee_drv.h >>>> @@ -275,6 +275,17 @@ void *tee_get_drvdata(struct tee_device *teedev); >>>> struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size); >>>> struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size); >>>> >>>> +int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm, >>>> + size_t size, size_t align, >>>> + int (*shm_register)(struct tee_context *ctx, >>>> + struct tee_shm *shm, >>>> + struct page **pages, >>>> + size_t num_pages, >>>> + unsigned long start)); >>>> +void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm, >>>> + int (*shm_unregister)(struct tee_context *ctx, >>>> + struct tee_shm *shm)); >>>> + >>> >>> These rather belong to drivers/tee/tee_private.h as we shouldn't >>> expose them to other kernel client drivers. >> >> As per the discussion in the other thread I'll ignore this. >> > > Then it will have conflicts with this [1] patch. If you are fine to > incorporate [1] in your series then the right place for these function > declarations should be include/linux/tee_core.h. > > [1] https://www.spinics.net/lists/kernel/msg5122983.html You're right, I'll rebase my patches on this. By incorporating your patch in my series, do you mean that I should just add it as the first patch in the series for the next version? Or keep my series as is (do the rebase of course) and just mention that it's using your patch as base? Regards, Balint >> >>>> struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx, >>>> void *addr, size_t length); >>>> >>>> -- >>>> 2.34.1 >>>>
On Mon, 4 Mar 2024 at 19:57, Balint Dobszay <balint.dobszay@arm.com> wrote: > > On 4 Mar 2024, at 10:17, Sumit Garg wrote: > > > Hi Balint, > > > > On Mon, 4 Mar 2024 at 14:33, Balint Dobszay <balint.dobszay@arm.com> wrote: > >> > >> Hi Sumit, > >> > > > > [snip] > > > >>>> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h > >>>> index 911ddf92dcee..4cf402424e71 100644 > >>>> --- a/include/linux/tee_drv.h > >>>> +++ b/include/linux/tee_drv.h > >>>> @@ -275,6 +275,17 @@ void *tee_get_drvdata(struct tee_device *teedev); > >>>> struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size); > >>>> struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size); > >>>> > >>>> +int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm, > >>>> + size_t size, size_t align, > >>>> + int (*shm_register)(struct tee_context *ctx, > >>>> + struct tee_shm *shm, > >>>> + struct page **pages, > >>>> + size_t num_pages, > >>>> + unsigned long start)); > >>>> +void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm, > >>>> + int (*shm_unregister)(struct tee_context *ctx, > >>>> + struct tee_shm *shm)); > >>>> + > >>> > >>> These rather belong to drivers/tee/tee_private.h as we shouldn't > >>> expose them to other kernel client drivers. > >> > >> As per the discussion in the other thread I'll ignore this. > >> > > > > Then it will have conflicts with this [1] patch. If you are fine to > > incorporate [1] in your series then the right place for these function > > declarations should be include/linux/tee_core.h. > > > > [1] https://www.spinics.net/lists/kernel/msg5122983.html > > You're right, I'll rebase my patches on this. > > By incorporating your patch in my series, do you mean that I should just > add it as the first patch in the series for the next version? If you are happy to carry it then I would be in favour of this to take care of dependency. -Sumit > Or keep my > series as is (do the rebase of course) and just mention that it's using > your patch as base? > > Regards, > Balint > > >> > >>>> struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx, > >>>> void *addr, size_t length); > >>>> > >>>> -- > >>>> 2.34.1 > >>>>
On 5 Mar 2024, at 6:36, Sumit Garg wrote: > On Mon, 4 Mar 2024 at 19:57, Balint Dobszay <balint.dobszay@arm.com> wrote: >> >> On 4 Mar 2024, at 10:17, Sumit Garg wrote: >> >>> Hi Balint, >>> >>> On Mon, 4 Mar 2024 at 14:33, Balint Dobszay <balint.dobszay@arm.com> wrote: >>>> >>>> Hi Sumit, >>>> >>> >>> [snip] >>> >>>>>> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h >>>>>> index 911ddf92dcee..4cf402424e71 100644 >>>>>> --- a/include/linux/tee_drv.h >>>>>> +++ b/include/linux/tee_drv.h >>>>>> @@ -275,6 +275,17 @@ void *tee_get_drvdata(struct tee_device *teedev); >>>>>> struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size); >>>>>> struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size); >>>>>> >>>>>> +int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm, >>>>>> + size_t size, size_t align, >>>>>> + int (*shm_register)(struct tee_context *ctx, >>>>>> + struct tee_shm *shm, >>>>>> + struct page **pages, >>>>>> + size_t num_pages, >>>>>> + unsigned long start)); >>>>>> +void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm, >>>>>> + int (*shm_unregister)(struct tee_context *ctx, >>>>>> + struct tee_shm *shm)); >>>>>> + >>>>> >>>>> These rather belong to drivers/tee/tee_private.h as we shouldn't >>>>> expose them to other kernel client drivers. >>>> >>>> As per the discussion in the other thread I'll ignore this. >>>> >>> >>> Then it will have conflicts with this [1] patch. If you are fine to >>> incorporate [1] in your series then the right place for these function >>> declarations should be include/linux/tee_core.h. >>> >>> [1] https://www.spinics.net/lists/kernel/msg5122983.html >> >> You're right, I'll rebase my patches on this. >> >> By incorporating your patch in my series, do you mean that I should just >> add it as the first patch in the series for the next version? > > If you are happy to carry it then I would be in favour of this to take > care of dependency. Sure, I've added it to v3. Regards, Balint >> Or keep my >> series as is (do the rebase of course) and just mention that it's using >> your patch as base? >> >> Regards, >> Balint >> >>>> >>>>>> struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx, >>>>>> void *addr, size_t length); >>>>>> >>>>>> -- >>>>>> 2.34.1 >>>>>>
On Tue, Feb 27, 2024 at 7:06 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Hi Balint,
>
> On Fri, 23 Feb 2024 at 15:22, Balint Dobszay <balint.dobszay@arm.com> wrote:
> >
> > Move the pool alloc and free helper functions from the OP-TEE driver to
> > the TEE subsystem, since these could be reused in other TEE drivers.
> > This patch is not supposed to change behavior, it's only reorganizing
> > the code.
> >
> > Suggested-by: Jens Wiklander <jens.wiklander@linaro.org>
> > Signed-off-by: Balint Dobszay <balint.dobszay@arm.com>
> > ---
> > drivers/tee/optee/core.c | 64 ------------------------------
> > drivers/tee/optee/ffa_abi.c | 6 +--
> > drivers/tee/optee/optee_private.h | 12 ------
> > drivers/tee/optee/smc_abi.c | 11 +++---
> > drivers/tee/tee_shm.c | 65 +++++++++++++++++++++++++++++++
> > include/linux/tee_drv.h | 11 ++++++
> > 6 files changed, 85 insertions(+), 84 deletions(-)
> >
> > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > index 3aed554bc8d8..9390f21f9902 100644
> > --- a/drivers/tee/optee/core.c
> > +++ b/drivers/tee/optee/core.c
> > @@ -9,7 +9,6 @@
> > #include <linux/crash_dump.h>
> > #include <linux/errno.h>
> > #include <linux/io.h>
> > -#include <linux/mm.h>
> > #include <linux/module.h>
> > #include <linux/slab.h>
> > #include <linux/string.h>
> > @@ -17,69 +16,6 @@
> > #include <linux/types.h>
> > #include "optee_private.h"
> >
> > -int optee_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > - size_t size, size_t align,
> > - int (*shm_register)(struct tee_context *ctx,
> > - struct tee_shm *shm,
> > - struct page **pages,
> > - size_t num_pages,
> > - unsigned long start))
> > -{
> > - size_t nr_pages = roundup(size, PAGE_SIZE) / PAGE_SIZE;
> > - struct page **pages;
> > - unsigned int i;
> > - int rc = 0;
> > -
> > - /*
> > - * Ignore alignment since this is already going to be page aligned
> > - * and there's no need for any larger alignment.
> > - */
> > - shm->kaddr = alloc_pages_exact(nr_pages * PAGE_SIZE,
> > - GFP_KERNEL | __GFP_ZERO);
> > - if (!shm->kaddr)
> > - return -ENOMEM;
> > -
> > - shm->paddr = virt_to_phys(shm->kaddr);
> > - shm->size = nr_pages * PAGE_SIZE;
> > -
> > - pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
> > - if (!pages) {
> > - rc = -ENOMEM;
> > - goto err;
> > - }
> > -
> > - for (i = 0; i < nr_pages; i++)
> > - pages[i] = virt_to_page((u8 *)shm->kaddr + i * PAGE_SIZE);
> > -
> > - shm->pages = pages;
> > - shm->num_pages = nr_pages;
> > -
> > - if (shm_register) {
> > - rc = shm_register(shm->ctx, shm, pages, nr_pages,
> > - (unsigned long)shm->kaddr);
> > - if (rc)
> > - goto err;
> > - }
> > -
> > - return 0;
> > -err:
> > - free_pages_exact(shm->kaddr, shm->size);
> > - shm->kaddr = NULL;
> > - return rc;
> > -}
> > -
> > -void optee_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > - int (*shm_unregister)(struct tee_context *ctx,
> > - struct tee_shm *shm))
> > -{
> > - if (shm_unregister)
> > - shm_unregister(shm->ctx, shm);
> > - free_pages_exact(shm->kaddr, shm->size);
> > - shm->kaddr = NULL;
> > - kfree(shm->pages);
> > - shm->pages = NULL;
> > -}
> > -
> > static void optee_bus_scan(struct work_struct *work)
> > {
> > WARN_ON(optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP));
> > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> > index ecb5eb079408..ee11918a2b35 100644
> > --- a/drivers/tee/optee/ffa_abi.c
> > +++ b/drivers/tee/optee/ffa_abi.c
> > @@ -374,14 +374,14 @@ static int optee_ffa_shm_unregister_supp(struct tee_context *ctx,
> > static int pool_ffa_op_alloc(struct tee_shm_pool *pool,
> > struct tee_shm *shm, size_t size, size_t align)
> > {
> > - return optee_pool_op_alloc_helper(pool, shm, size, align,
> > - optee_ffa_shm_register);
> > + return tee_shm_pool_op_alloc_helper(pool, shm, size, align,
> > + optee_ffa_shm_register);
> > }
> >
> > static void pool_ffa_op_free(struct tee_shm_pool *pool,
> > struct tee_shm *shm)
> > {
> > - optee_pool_op_free_helper(pool, shm, optee_ffa_shm_unregister);
> > + tee_shm_pool_op_free_helper(pool, shm, optee_ffa_shm_unregister);
> > }
> >
> > static void pool_ffa_op_destroy_pool(struct tee_shm_pool *pool)
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index 7a5243c78b55..a153285a1919 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -283,18 +283,6 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session);
> > int optee_enumerate_devices(u32 func);
> > void optee_unregister_devices(void);
> >
> > -int optee_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > - size_t size, size_t align,
> > - int (*shm_register)(struct tee_context *ctx,
> > - struct tee_shm *shm,
> > - struct page **pages,
> > - size_t num_pages,
> > - unsigned long start));
> > -void optee_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > - int (*shm_unregister)(struct tee_context *ctx,
> > - struct tee_shm *shm));
> > -
> > -
> > void optee_remove_common(struct optee *optee);
> > int optee_open(struct tee_context *ctx, bool cap_memref_null);
> > void optee_release(struct tee_context *ctx);
> > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > index a37f87087e5c..b0c616b6870d 100644
> > --- a/drivers/tee/optee/smc_abi.c
> > +++ b/drivers/tee/optee/smc_abi.c
> > @@ -592,19 +592,20 @@ static int pool_op_alloc(struct tee_shm_pool *pool,
> > * to be registered with OP-TEE.
> > */
> > if (shm->flags & TEE_SHM_PRIV)
> > - return optee_pool_op_alloc_helper(pool, shm, size, align, NULL);
> > + return tee_shm_pool_op_alloc_helper(pool, shm, size, align,
> > + NULL);
> >
> > - return optee_pool_op_alloc_helper(pool, shm, size, align,
> > - optee_shm_register);
> > + return tee_shm_pool_op_alloc_helper(pool, shm, size, align,
> > + optee_shm_register);
> > }
> >
> > static void pool_op_free(struct tee_shm_pool *pool,
> > struct tee_shm *shm)
> > {
> > if (!(shm->flags & TEE_SHM_PRIV))
> > - optee_pool_op_free_helper(pool, shm, optee_shm_unregister);
> > + tee_shm_pool_op_free_helper(pool, shm, optee_shm_unregister);
> > else
> > - optee_pool_op_free_helper(pool, shm, NULL);
> > + tee_shm_pool_op_free_helper(pool, shm, NULL);
> > }
> >T
> > static void pool_op_destroy_pool(struct tee_shm_pool *pool)
> > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > index 731d9028b67f..641aad92ffe2 100644
> > --- a/drivers/tee/tee_shm.c
> > +++ b/drivers/tee/tee_shm.c
> > @@ -202,6 +202,71 @@ struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size)
> > }
> > EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf);
> >
> > +int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
>
> I don't see the first argument (struct tee_shm_pool *pool) being used,
> so drop that. Also, we can just rename it as
> tee_dyn_shm_alloc_helper().
>
> > + size_t size, size_t align,
> > + int (*shm_register)(struct tee_context *ctx,
> > + struct tee_shm *shm,
> > + struct page **pages,
> > + size_t num_pages,
> > + unsigned long start))
> > +{
> > + size_t nr_pages = roundup(size, PAGE_SIZE) / PAGE_SIZE;
> > + struct page **pages;
> > + unsigned int i;
> > + int rc = 0;
> > +
> > + /*
> > + * Ignore alignment since this is already going to be page aligned
> > + * and there's no need for any larger alignment.
> > + */
> > + shm->kaddr = alloc_pages_exact(nr_pages * PAGE_SIZE,
> > + GFP_KERNEL | __GFP_ZERO);
> > + if (!shm->kaddr)
> > + return -ENOMEM;
> > +
> > + shm->paddr = virt_to_phys(shm->kaddr);
> > + shm->size = nr_pages * PAGE_SIZE;
> > +
> > + pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
> > + if (!pages) {
> > + rc = -ENOMEM;
> > + goto err;
> > + }
> > +
> > + for (i = 0; i < nr_pages; i++)
> > + pages[i] = virt_to_page((u8 *)shm->kaddr + i * PAGE_SIZE);
> > +
> > + shm->pages = pages;
> > + shm->num_pages = nr_pages;
> > +
> > + if (shm_register) {
> > + rc = shm_register(shm->ctx, shm, pages, nr_pages,
> > + (unsigned long)shm->kaddr);
> > + if (rc)
> > + goto err;
> > + }
> > +
> > + return 0;
> > +err:
> > + free_pages_exact(shm->kaddr, shm->size);
> > + shm->kaddr = NULL;
> > + return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(tee_shm_pool_op_alloc_helper);
> > +
> > +void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
>
> Ditto tee_shm_pool_op_free_helper() -> tee_dyn_shm_free_helper()
>
> > + int (*shm_unregister)(struct tee_context *ctx,
> > + struct tee_shm *shm))
> > +{
> > + if (shm_unregister)
> > + shm_unregister(shm->ctx, shm);
> > + free_pages_exact(shm->kaddr, shm->size);
> > + shm->kaddr = NULL;
> > + kfree(shm->pages);
> > + shm->pages = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(tee_shm_pool_op_free_helper);
> > +
> > static struct tee_shm *
> > register_shm_helper(struct tee_context *ctx, struct iov_iter *iter, u32 flags,
> > int id)
> > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> > index 911ddf92dcee..4cf402424e71 100644
> > --- a/include/linux/tee_drv.h
> > +++ b/include/linux/tee_drv.h
> > @@ -275,6 +275,17 @@ void *tee_get_drvdata(struct tee_device *teedev);
> > struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size);
> > struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
> >
> > +int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > + size_t size, size_t align,
> > + int (*shm_register)(struct tee_context *ctx,
> > + struct tee_shm *shm,
> > + struct page **pages,
> > + size_t num_pages,
> > + unsigned long start));
> > +void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > + int (*shm_unregister)(struct tee_context *ctx,
> > + struct tee_shm *shm));
> > +
>
> These rather belong to drivers/tee/tee_private.h as we shouldn't
> expose them to other kernel client drivers.
This is the right place, this .h file is for TEE drivers too.
Cheers,
Jens
>
> -Sumit
>
> > struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx,
> > void *addr, size_t length);
> >
> > --
> > 2.34.1
> >
On Tue, 27 Feb 2024 at 21:20, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Tue, Feb 27, 2024 at 7:06 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Hi Balint,
> >
> > On Fri, 23 Feb 2024 at 15:22, Balint Dobszay <balint.dobszay@arm.com> wrote:
> > >
> > > Move the pool alloc and free helper functions from the OP-TEE driver to
> > > the TEE subsystem, since these could be reused in other TEE drivers.
> > > This patch is not supposed to change behavior, it's only reorganizing
> > > the code.
> > >
> > > Suggested-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > Signed-off-by: Balint Dobszay <balint.dobszay@arm.com>
> > > ---
> > > drivers/tee/optee/core.c | 64 ------------------------------
> > > drivers/tee/optee/ffa_abi.c | 6 +--
> > > drivers/tee/optee/optee_private.h | 12 ------
> > > drivers/tee/optee/smc_abi.c | 11 +++---
> > > drivers/tee/tee_shm.c | 65 +++++++++++++++++++++++++++++++
> > > include/linux/tee_drv.h | 11 ++++++
> > > 6 files changed, 85 insertions(+), 84 deletions(-)
> > >
> > > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > > index 3aed554bc8d8..9390f21f9902 100644
> > > --- a/drivers/tee/optee/core.c
> > > +++ b/drivers/tee/optee/core.c
> > > @@ -9,7 +9,6 @@
> > > #include <linux/crash_dump.h>
> > > #include <linux/errno.h>
> > > #include <linux/io.h>
> > > -#include <linux/mm.h>
> > > #include <linux/module.h>
> > > #include <linux/slab.h>
> > > #include <linux/string.h>
> > > @@ -17,69 +16,6 @@
> > > #include <linux/types.h>
> > > #include "optee_private.h"
> > >
> > > -int optee_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > > - size_t size, size_t align,
> > > - int (*shm_register)(struct tee_context *ctx,
> > > - struct tee_shm *shm,
> > > - struct page **pages,
> > > - size_t num_pages,
> > > - unsigned long start))
> > > -{
> > > - size_t nr_pages = roundup(size, PAGE_SIZE) / PAGE_SIZE;
> > > - struct page **pages;
> > > - unsigned int i;
> > > - int rc = 0;
> > > -
> > > - /*
> > > - * Ignore alignment since this is already going to be page aligned
> > > - * and there's no need for any larger alignment.
> > > - */
> > > - shm->kaddr = alloc_pages_exact(nr_pages * PAGE_SIZE,
> > > - GFP_KERNEL | __GFP_ZERO);
> > > - if (!shm->kaddr)
> > > - return -ENOMEM;
> > > -
> > > - shm->paddr = virt_to_phys(shm->kaddr);
> > > - shm->size = nr_pages * PAGE_SIZE;
> > > -
> > > - pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
> > > - if (!pages) {
> > > - rc = -ENOMEM;
> > > - goto err;
> > > - }
> > > -
> > > - for (i = 0; i < nr_pages; i++)
> > > - pages[i] = virt_to_page((u8 *)shm->kaddr + i * PAGE_SIZE);
> > > -
> > > - shm->pages = pages;
> > > - shm->num_pages = nr_pages;
> > > -
> > > - if (shm_register) {
> > > - rc = shm_register(shm->ctx, shm, pages, nr_pages,
> > > - (unsigned long)shm->kaddr);
> > > - if (rc)
> > > - goto err;
> > > - }
> > > -
> > > - return 0;
> > > -err:
> > > - free_pages_exact(shm->kaddr, shm->size);
> > > - shm->kaddr = NULL;
> > > - return rc;
> > > -}
> > > -
> > > -void optee_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > > - int (*shm_unregister)(struct tee_context *ctx,
> > > - struct tee_shm *shm))
> > > -{
> > > - if (shm_unregister)
> > > - shm_unregister(shm->ctx, shm);
> > > - free_pages_exact(shm->kaddr, shm->size);
> > > - shm->kaddr = NULL;
> > > - kfree(shm->pages);
> > > - shm->pages = NULL;
> > > -}
> > > -
> > > static void optee_bus_scan(struct work_struct *work)
> > > {
> > > WARN_ON(optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP));
> > > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> > > index ecb5eb079408..ee11918a2b35 100644
> > > --- a/drivers/tee/optee/ffa_abi.c
> > > +++ b/drivers/tee/optee/ffa_abi.c
> > > @@ -374,14 +374,14 @@ static int optee_ffa_shm_unregister_supp(struct tee_context *ctx,
> > > static int pool_ffa_op_alloc(struct tee_shm_pool *pool,
> > > struct tee_shm *shm, size_t size, size_t align)
> > > {
> > > - return optee_pool_op_alloc_helper(pool, shm, size, align,
> > > - optee_ffa_shm_register);
> > > + return tee_shm_pool_op_alloc_helper(pool, shm, size, align,
> > > + optee_ffa_shm_register);
> > > }
> > >
> > > static void pool_ffa_op_free(struct tee_shm_pool *pool,
> > > struct tee_shm *shm)
> > > {
> > > - optee_pool_op_free_helper(pool, shm, optee_ffa_shm_unregister);
> > > + tee_shm_pool_op_free_helper(pool, shm, optee_ffa_shm_unregister);
> > > }
> > >
> > > static void pool_ffa_op_destroy_pool(struct tee_shm_pool *pool)
> > > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > > index 7a5243c78b55..a153285a1919 100644
> > > --- a/drivers/tee/optee/optee_private.h
> > > +++ b/drivers/tee/optee/optee_private.h
> > > @@ -283,18 +283,6 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session);
> > > int optee_enumerate_devices(u32 func);
> > > void optee_unregister_devices(void);
> > >
> > > -int optee_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > > - size_t size, size_t align,
> > > - int (*shm_register)(struct tee_context *ctx,
> > > - struct tee_shm *shm,
> > > - struct page **pages,
> > > - size_t num_pages,
> > > - unsigned long start));
> > > -void optee_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > > - int (*shm_unregister)(struct tee_context *ctx,
> > > - struct tee_shm *shm));
> > > -
> > > -
> > > void optee_remove_common(struct optee *optee);
> > > int optee_open(struct tee_context *ctx, bool cap_memref_null);
> > > void optee_release(struct tee_context *ctx);
> > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > > index a37f87087e5c..b0c616b6870d 100644
> > > --- a/drivers/tee/optee/smc_abi.c
> > > +++ b/drivers/tee/optee/smc_abi.c
> > > @@ -592,19 +592,20 @@ static int pool_op_alloc(struct tee_shm_pool *pool,
> > > * to be registered with OP-TEE.
> > > */
> > > if (shm->flags & TEE_SHM_PRIV)
> > > - return optee_pool_op_alloc_helper(pool, shm, size, align, NULL);
> > > + return tee_shm_pool_op_alloc_helper(pool, shm, size, align,
> > > + NULL);
> > >
> > > - return optee_pool_op_alloc_helper(pool, shm, size, align,
> > > - optee_shm_register);
> > > + return tee_shm_pool_op_alloc_helper(pool, shm, size, align,
> > > + optee_shm_register);
> > > }
> > >
> > > static void pool_op_free(struct tee_shm_pool *pool,
> > > struct tee_shm *shm)
> > > {
> > > if (!(shm->flags & TEE_SHM_PRIV))
> > > - optee_pool_op_free_helper(pool, shm, optee_shm_unregister);
> > > + tee_shm_pool_op_free_helper(pool, shm, optee_shm_unregister);
> > > else
> > > - optee_pool_op_free_helper(pool, shm, NULL);
> > > + tee_shm_pool_op_free_helper(pool, shm, NULL);
> > > }
> > >T
> > > static void pool_op_destroy_pool(struct tee_shm_pool *pool)
> > > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > > index 731d9028b67f..641aad92ffe2 100644
> > > --- a/drivers/tee/tee_shm.c
> > > +++ b/drivers/tee/tee_shm.c
> > > @@ -202,6 +202,71 @@ struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size)
> > > }
> > > EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf);
> > >
> > > +int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> >
> > I don't see the first argument (struct tee_shm_pool *pool) being used,
> > so drop that. Also, we can just rename it as
> > tee_dyn_shm_alloc_helper().
> >
> > > + size_t size, size_t align,
> > > + int (*shm_register)(struct tee_context *ctx,
> > > + struct tee_shm *shm,
> > > + struct page **pages,
> > > + size_t num_pages,
> > > + unsigned long start))
> > > +{
> > > + size_t nr_pages = roundup(size, PAGE_SIZE) / PAGE_SIZE;
> > > + struct page **pages;
> > > + unsigned int i;
> > > + int rc = 0;
> > > +
> > > + /*
> > > + * Ignore alignment since this is already going to be page aligned
> > > + * and there's no need for any larger alignment.
> > > + */
> > > + shm->kaddr = alloc_pages_exact(nr_pages * PAGE_SIZE,
> > > + GFP_KERNEL | __GFP_ZERO);
> > > + if (!shm->kaddr)
> > > + return -ENOMEM;
> > > +
> > > + shm->paddr = virt_to_phys(shm->kaddr);
> > > + shm->size = nr_pages * PAGE_SIZE;
> > > +
> > > + pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
> > > + if (!pages) {
> > > + rc = -ENOMEM;
> > > + goto err;
> > > + }
> > > +
> > > + for (i = 0; i < nr_pages; i++)
> > > + pages[i] = virt_to_page((u8 *)shm->kaddr + i * PAGE_SIZE);
> > > +
> > > + shm->pages = pages;
> > > + shm->num_pages = nr_pages;
> > > +
> > > + if (shm_register) {
> > > + rc = shm_register(shm->ctx, shm, pages, nr_pages,
> > > + (unsigned long)shm->kaddr);
> > > + if (rc)
> > > + goto err;
> > > + }
> > > +
> > > + return 0;
> > > +err:
> > > + free_pages_exact(shm->kaddr, shm->size);
> > > + shm->kaddr = NULL;
> > > + return rc;
> > > +}
> > > +EXPORT_SYMBOL_GPL(tee_shm_pool_op_alloc_helper);
> > > +
> > > +void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> >
> > Ditto tee_shm_pool_op_free_helper() -> tee_dyn_shm_free_helper()
> >
> > > + int (*shm_unregister)(struct tee_context *ctx,
> > > + struct tee_shm *shm))
> > > +{
> > > + if (shm_unregister)
> > > + shm_unregister(shm->ctx, shm);
> > > + free_pages_exact(shm->kaddr, shm->size);
> > > + shm->kaddr = NULL;
> > > + kfree(shm->pages);
> > > + shm->pages = NULL;
> > > +}
> > > +EXPORT_SYMBOL_GPL(tee_shm_pool_op_free_helper);
> > > +
> > > static struct tee_shm *
> > > register_shm_helper(struct tee_context *ctx, struct iov_iter *iter, u32 flags,
> > > int id)
> > > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> > > index 911ddf92dcee..4cf402424e71 100644
> > > --- a/include/linux/tee_drv.h
> > > +++ b/include/linux/tee_drv.h
> > > @@ -275,6 +275,17 @@ void *tee_get_drvdata(struct tee_device *teedev);
> > > struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size);
> > > struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
> > >
> > > +int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > > + size_t size, size_t align,
> > > + int (*shm_register)(struct tee_context *ctx,
> > > + struct tee_shm *shm,
> > > + struct page **pages,
> > > + size_t num_pages,
> > > + unsigned long start));
> > > +void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > > + int (*shm_unregister)(struct tee_context *ctx,
> > > + struct tee_shm *shm));
> > > +
> >
> > These rather belong to drivers/tee/tee_private.h as we shouldn't
> > expose them to other kernel client drivers.
>
> This is the right place, this .h file is for TEE drivers too.
>
But this is shared with other kernel TEE client drivers and we
shouldn't expose internal APIs which aren't meant for them with a side
effect of API abuse too. Any particular reason to not use
drivers/tee/tee_private.h?
-Sumit
> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > > struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx,
> > > void *addr, size_t length);
> > >
> > > --
> > > 2.34.1
> > >
On Wed, Feb 28, 2024 at 6:58 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Tue, 27 Feb 2024 at 21:20, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > On Tue, Feb 27, 2024 at 7:06 AM Sumit Garg <sumit.garg@linaro.org> wrote:
[snip]
> > > > --- a/include/linux/tee_drv.h
> > > > +++ b/include/linux/tee_drv.h
> > > > @@ -275,6 +275,17 @@ void *tee_get_drvdata(struct tee_device *teedev);
> > > > struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size);
> > > > struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
> > > >
> > > > +int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > > > + size_t size, size_t align,
> > > > + int (*shm_register)(struct tee_context *ctx,
> > > > + struct tee_shm *shm,
> > > > + struct page **pages,
> > > > + size_t num_pages,
> > > > + unsigned long start));
> > > > +void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > > > + int (*shm_unregister)(struct tee_context *ctx,
> > > > + struct tee_shm *shm));
> > > > +
> > >
> > > These rather belong to drivers/tee/tee_private.h as we shouldn't
> > > expose them to other kernel client drivers.
> >
> > This is the right place, this .h file is for TEE drivers too.
> >
>
> But this is shared with other kernel TEE client drivers and we
> shouldn't expose internal APIs which aren't meant for them with a side
> effect of API abuse too. Any particular reason to not use
> drivers/tee/tee_private.h?
drivers/tee/tee_private.h is supposed to be used internally by only
the files in drivers/tee. If you look in include/linux/tee_drv.h
you'll find a few functions and other definitions that a kernel TEE
client driver should not use, for instance, tee_device_register() and
tee_device_unregister(). This kernel TEE client interface was
introduced with commit 25559c22cef8 ("tee: add kernel internal client
interface"). include/linux/tee_drv.h existed before we even had any
kernel TEE client interface.
Cheers,
Jens
On Wed, 28 Feb 2024 at 14:11, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Wed, Feb 28, 2024 at 6:58 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Tue, 27 Feb 2024 at 21:20, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > On Tue, Feb 27, 2024 at 7:06 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> [snip]
> > > > > --- a/include/linux/tee_drv.h
> > > > > +++ b/include/linux/tee_drv.h
> > > > > @@ -275,6 +275,17 @@ void *tee_get_drvdata(struct tee_device *teedev);
> > > > > struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size);
> > > > > struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
> > > > >
> > > > > +int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > > > > + size_t size, size_t align,
> > > > > + int (*shm_register)(struct tee_context *ctx,
> > > > > + struct tee_shm *shm,
> > > > > + struct page **pages,
> > > > > + size_t num_pages,
> > > > > + unsigned long start));
> > > > > +void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > > > > + int (*shm_unregister)(struct tee_context *ctx,
> > > > > + struct tee_shm *shm));
> > > > > +
> > > >
> > > > These rather belong to drivers/tee/tee_private.h as we shouldn't
> > > > expose them to other kernel client drivers.
> > >
> > > This is the right place, this .h file is for TEE drivers too.
> > >
> >
> > But this is shared with other kernel TEE client drivers and we
> > shouldn't expose internal APIs which aren't meant for them with a side
> > effect of API abuse too. Any particular reason to not use
> > drivers/tee/tee_private.h?
>
> drivers/tee/tee_private.h is supposed to be used internally by only
> the files in drivers/tee.
How about "struct tee_device" being in drivers/tee/tee_private.h?
> If you look in include/linux/tee_drv.h
> you'll find a few functions and other definitions that a kernel TEE
> client driver should not use, for instance, tee_device_register() and
> tee_device_unregister(). This kernel TEE client interface was
> introduced with commit 25559c22cef8 ("tee: add kernel internal client
> interface"). include/linux/tee_drv.h existed before we even had any
> kernel TEE client interface.
Anyhow, it looks like there is a chance for refactoring here. How
about splitting this header further in something like
include/linux/tee_core.h which will contain all the pieces relevant to
TEE drivers?
BTW, this patch series can keep using include/linux/tee_drv.h for the
time being.
-Sumit
>
> Cheers,
> Jens
On Wed, Feb 28, 2024 at 12:20 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Wed, 28 Feb 2024 at 14:11, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > On Wed, Feb 28, 2024 at 6:58 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > On Tue, 27 Feb 2024 at 21:20, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > >
> > > > On Tue, Feb 27, 2024 at 7:06 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > [snip]
> > > > > > --- a/include/linux/tee_drv.h
> > > > > > +++ b/include/linux/tee_drv.h
> > > > > > @@ -275,6 +275,17 @@ void *tee_get_drvdata(struct tee_device *teedev);
> > > > > > struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size);
> > > > > > struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
> > > > > >
> > > > > > +int tee_shm_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > > > > > + size_t size, size_t align,
> > > > > > + int (*shm_register)(struct tee_context *ctx,
> > > > > > + struct tee_shm *shm,
> > > > > > + struct page **pages,
> > > > > > + size_t num_pages,
> > > > > > + unsigned long start));
> > > > > > +void tee_shm_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> > > > > > + int (*shm_unregister)(struct tee_context *ctx,
> > > > > > + struct tee_shm *shm));
> > > > > > +
> > > > >
> > > > > These rather belong to drivers/tee/tee_private.h as we shouldn't
> > > > > expose them to other kernel client drivers.
> > > >
> > > > This is the right place, this .h file is for TEE drivers too.
> > > >
> > >
> > > But this is shared with other kernel TEE client drivers and we
> > > shouldn't expose internal APIs which aren't meant for them with a side
> > > effect of API abuse too. Any particular reason to not use
> > > drivers/tee/tee_private.h?
> >
> > drivers/tee/tee_private.h is supposed to be used internally by only
> > the files in drivers/tee.
>
> How about "struct tee_device" being in drivers/tee/tee_private.h?
What about it?
>
> > If you look in include/linux/tee_drv.h
> > you'll find a few functions and other definitions that a kernel TEE
> > client driver should not use, for instance, tee_device_register() and
> > tee_device_unregister(). This kernel TEE client interface was
> > introduced with commit 25559c22cef8 ("tee: add kernel internal client
> > interface"). include/linux/tee_drv.h existed before we even had any
> > kernel TEE client interface.
>
> Anyhow, it looks like there is a chance for refactoring here. How
> about splitting this header further in something like
> include/linux/tee_core.h which will contain all the pieces relevant to
> TEE drivers?
Sure.
>
> BTW, this patch series can keep using include/linux/tee_drv.h for the
> time being.
Agreed.
Cheers,
Jens
>
> -Sumit
>
> >
> > Cheers,
> > Jens
© 2016 - 2026 Red Hat, Inc.