* [PATCH v2] optee: add per cpu asynchronous notification
@ 2023-01-18 17:49 ` Etienne Carriere
0 siblings, 0 replies; 6+ messages in thread
From: Etienne Carriere @ 2023-01-18 17:49 UTC (permalink / raw)
To: linux-kernel
Cc: linux-arm-kernel, Etienne Carriere, Jens Wiklander, Sumit Garg,
Marc Zyngier, Alexandre Torgue
Implements use of per CPU irq for optee asynchronous notification.
Existing optee async notif implementation allows OP-TE world to
raise an interrupt for the Linux optee driver to query pending events
bound to waiting tasks in Linux world or threaded bottom half tasks
to be invoked in TEE world. This change allows the signaling interrupt
to be a per cpu interrupt as with Arm GIC PPIs.
Cc: Jens Wiklander <jens.wiklander@linaro.org>
Cc: Sumit Garg <sumit.garg@linaro.org>
Cc: Marc Zyngier <maz@kernel.org>
Co-developed-by: Alexandre Torgue <alexandre.torgue@foss.st.com>
Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
Changes since v1:
- Fixed missing __percpu attribute reported by kernel test robot.
- Rephrased commit message and added Cc tags.
---
drivers/tee/optee/optee_private.h | 22 ++++++
drivers/tee/optee/smc_abi.c | 107 ++++++++++++++++++++++++++++--
2 files changed, 124 insertions(+), 5 deletions(-)
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index 04ae58892608..e5bd3548691f 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -94,11 +94,33 @@ struct optee_supp {
struct completion reqs_c;
};
+/*
+ * struct optee_pcpu - per cpu notif private struct passed to work functions
+ * @optee optee device reference
+ */
+struct optee_pcpu {
+ struct optee *optee;
+};
+
+/*
+ * struct optee_smc - optee smc communication struct
+ * @invoke_fn handler function to invoke secure monitor
+ * @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
+ * @notif_irq interrupt used as async notification by OP-TEE or 0
+ * @optee_pcpu per_cpu optee instance for per cpu work or NULL
+ * @notif_pcpu_wq workqueue for per cpu aynchronous notification or NULL
+ * @notif_pcpu_work work for per cpu asynchronous notification
+ */
struct optee_smc {
optee_invoke_fn *invoke_fn;
void *memremaped_shm;
u32 sec_caps;
unsigned int notif_irq;
+ struct optee_pcpu __percpu *optee_pcpu;
+ struct workqueue_struct *notif_pcpu_wq;
+ struct work_struct notif_pcpu_work;
};
/**
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
index a1c1fa1a9c28..ffa3f3aa7244 100644
--- a/drivers/tee/optee/smc_abi.c
+++ b/drivers/tee/optee/smc_abi.c
@@ -993,12 +993,20 @@ static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
static irqreturn_t notif_irq_handler(int irq, void *dev_id)
{
- struct optee *optee = dev_id;
+ struct optee *optee;
bool do_bottom_half = false;
bool value_valid;
bool value_pending;
u32 value;
+ if (irq_is_percpu_devid(irq)) {
+ struct optee_pcpu __percpu *pcpu = (struct optee_pcpu *)dev_id;
+
+ optee = pcpu->optee;
+ } else {
+ optee = dev_id;
+ }
+
do {
value = get_async_notif_value(optee->smc.invoke_fn,
&value_valid, &value_pending);
@@ -1011,8 +1019,13 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id)
optee_notif_send(optee, value);
} while (value_pending);
- if (do_bottom_half)
- return IRQ_WAKE_THREAD;
+ if (do_bottom_half) {
+ if (irq_is_percpu_devid(irq))
+ queue_work(optee->smc.notif_pcpu_wq, &optee->smc.notif_pcpu_work);
+ else
+ return IRQ_WAKE_THREAD;
+ }
+
return IRQ_HANDLED;
}
@@ -1025,7 +1038,7 @@ static irqreturn_t notif_irq_thread_fn(int irq, void *dev_id)
return IRQ_HANDLED;
}
-static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
+static int init_irq(struct optee *optee, u_int irq)
{
int rc;
@@ -1040,12 +1053,96 @@ static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
return 0;
}
+static void notif_pcpu_irq_work_fn(struct work_struct *work)
+{
+ struct optee_smc *optee_smc = container_of(work, struct optee_smc, notif_pcpu_work);
+ struct optee *optee = container_of(optee_smc, struct optee, smc);
+
+ optee_smc_do_bottom_half(optee->ctx);
+}
+
+static int init_pcpu_irq(struct optee *optee, u_int irq)
+{
+ struct optee_pcpu __percpu *optee_pcpu;
+ spinlock_t lock;
+ int cpu;
+ int rc;
+
+ optee_pcpu = alloc_percpu(struct optee_pcpu);
+ if (!optee_pcpu)
+ return -ENOMEM;
+
+ for_each_present_cpu(cpu) {
+ struct optee_pcpu __percpu *p = per_cpu_ptr(optee_pcpu, cpu);
+
+ p->optee = optee;
+ }
+
+ rc = request_percpu_irq(irq, notif_irq_handler,
+ "optee_pcpu_notification", optee_pcpu);
+ if (rc)
+ goto err_free_pcpu;
+
+ spin_lock_init(&lock);
+
+ spin_lock(&lock);
+ enable_percpu_irq(irq, 0);
+ spin_unlock(&lock);
+
+ INIT_WORK(&optee->smc.notif_pcpu_work, notif_pcpu_irq_work_fn);
+ optee->smc.notif_pcpu_wq = create_workqueue("optee_pcpu_notification");
+ if (!optee->smc.notif_pcpu_wq) {
+ rc = -EINVAL;
+ goto err_free_pcpu_irq;
+ }
+
+ optee->smc.optee_pcpu = optee_pcpu;
+ optee->smc.notif_irq = irq;
+
+ return 0;
+
+err_free_pcpu_irq:
+ spin_lock(&lock);
+ disable_percpu_irq(irq);
+ spin_unlock(&lock);
+ free_percpu_irq(irq, optee_pcpu);
+err_free_pcpu:
+ free_percpu(optee_pcpu);
+
+ return rc;
+}
+
+static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
+{
+ if (irq_is_percpu_devid(irq))
+ return init_pcpu_irq(optee, irq);
+ else
+ return init_irq(optee, irq);
+}
+
+static void uninit_pcpu_irq(struct optee *optee)
+{
+ spinlock_t lock;
+
+ spin_lock_init(&lock);
+ spin_lock(&lock);
+ disable_percpu_irq(optee->smc.notif_irq);
+ spin_unlock(&lock);
+
+ free_percpu_irq(optee->smc.notif_irq, optee->smc.optee_pcpu);
+ free_percpu(optee->smc.optee_pcpu);
+}
+
static void optee_smc_notif_uninit_irq(struct optee *optee)
{
if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_ASYNC_NOTIF) {
optee_smc_stop_async_notif(optee->ctx);
if (optee->smc.notif_irq) {
- free_irq(optee->smc.notif_irq, optee);
+ if (irq_is_percpu_devid(optee->smc.notif_irq))
+ uninit_pcpu_irq(optee);
+ else
+ free_irq(optee->smc.notif_irq, optee);
+
irq_dispose_mapping(optee->smc.notif_irq);
}
}
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] optee: add per cpu asynchronous notification
@ 2023-01-18 17:49 ` Etienne Carriere
0 siblings, 0 replies; 6+ messages in thread
From: Etienne Carriere @ 2023-01-18 17:49 UTC (permalink / raw)
To: linux-kernel
Cc: linux-arm-kernel, Etienne Carriere, Jens Wiklander, Sumit Garg,
Marc Zyngier, Alexandre Torgue
Implements use of per CPU irq for optee asynchronous notification.
Existing optee async notif implementation allows OP-TE world to
raise an interrupt for the Linux optee driver to query pending events
bound to waiting tasks in Linux world or threaded bottom half tasks
to be invoked in TEE world. This change allows the signaling interrupt
to be a per cpu interrupt as with Arm GIC PPIs.
Cc: Jens Wiklander <jens.wiklander@linaro.org>
Cc: Sumit Garg <sumit.garg@linaro.org>
Cc: Marc Zyngier <maz@kernel.org>
Co-developed-by: Alexandre Torgue <alexandre.torgue@foss.st.com>
Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
Changes since v1:
- Fixed missing __percpu attribute reported by kernel test robot.
- Rephrased commit message and added Cc tags.
---
drivers/tee/optee/optee_private.h | 22 ++++++
drivers/tee/optee/smc_abi.c | 107 ++++++++++++++++++++++++++++--
2 files changed, 124 insertions(+), 5 deletions(-)
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index 04ae58892608..e5bd3548691f 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -94,11 +94,33 @@ struct optee_supp {
struct completion reqs_c;
};
+/*
+ * struct optee_pcpu - per cpu notif private struct passed to work functions
+ * @optee optee device reference
+ */
+struct optee_pcpu {
+ struct optee *optee;
+};
+
+/*
+ * struct optee_smc - optee smc communication struct
+ * @invoke_fn handler function to invoke secure monitor
+ * @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
+ * @notif_irq interrupt used as async notification by OP-TEE or 0
+ * @optee_pcpu per_cpu optee instance for per cpu work or NULL
+ * @notif_pcpu_wq workqueue for per cpu aynchronous notification or NULL
+ * @notif_pcpu_work work for per cpu asynchronous notification
+ */
struct optee_smc {
optee_invoke_fn *invoke_fn;
void *memremaped_shm;
u32 sec_caps;
unsigned int notif_irq;
+ struct optee_pcpu __percpu *optee_pcpu;
+ struct workqueue_struct *notif_pcpu_wq;
+ struct work_struct notif_pcpu_work;
};
/**
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
index a1c1fa1a9c28..ffa3f3aa7244 100644
--- a/drivers/tee/optee/smc_abi.c
+++ b/drivers/tee/optee/smc_abi.c
@@ -993,12 +993,20 @@ static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
static irqreturn_t notif_irq_handler(int irq, void *dev_id)
{
- struct optee *optee = dev_id;
+ struct optee *optee;
bool do_bottom_half = false;
bool value_valid;
bool value_pending;
u32 value;
+ if (irq_is_percpu_devid(irq)) {
+ struct optee_pcpu __percpu *pcpu = (struct optee_pcpu *)dev_id;
+
+ optee = pcpu->optee;
+ } else {
+ optee = dev_id;
+ }
+
do {
value = get_async_notif_value(optee->smc.invoke_fn,
&value_valid, &value_pending);
@@ -1011,8 +1019,13 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id)
optee_notif_send(optee, value);
} while (value_pending);
- if (do_bottom_half)
- return IRQ_WAKE_THREAD;
+ if (do_bottom_half) {
+ if (irq_is_percpu_devid(irq))
+ queue_work(optee->smc.notif_pcpu_wq, &optee->smc.notif_pcpu_work);
+ else
+ return IRQ_WAKE_THREAD;
+ }
+
return IRQ_HANDLED;
}
@@ -1025,7 +1038,7 @@ static irqreturn_t notif_irq_thread_fn(int irq, void *dev_id)
return IRQ_HANDLED;
}
-static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
+static int init_irq(struct optee *optee, u_int irq)
{
int rc;
@@ -1040,12 +1053,96 @@ static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
return 0;
}
+static void notif_pcpu_irq_work_fn(struct work_struct *work)
+{
+ struct optee_smc *optee_smc = container_of(work, struct optee_smc, notif_pcpu_work);
+ struct optee *optee = container_of(optee_smc, struct optee, smc);
+
+ optee_smc_do_bottom_half(optee->ctx);
+}
+
+static int init_pcpu_irq(struct optee *optee, u_int irq)
+{
+ struct optee_pcpu __percpu *optee_pcpu;
+ spinlock_t lock;
+ int cpu;
+ int rc;
+
+ optee_pcpu = alloc_percpu(struct optee_pcpu);
+ if (!optee_pcpu)
+ return -ENOMEM;
+
+ for_each_present_cpu(cpu) {
+ struct optee_pcpu __percpu *p = per_cpu_ptr(optee_pcpu, cpu);
+
+ p->optee = optee;
+ }
+
+ rc = request_percpu_irq(irq, notif_irq_handler,
+ "optee_pcpu_notification", optee_pcpu);
+ if (rc)
+ goto err_free_pcpu;
+
+ spin_lock_init(&lock);
+
+ spin_lock(&lock);
+ enable_percpu_irq(irq, 0);
+ spin_unlock(&lock);
+
+ INIT_WORK(&optee->smc.notif_pcpu_work, notif_pcpu_irq_work_fn);
+ optee->smc.notif_pcpu_wq = create_workqueue("optee_pcpu_notification");
+ if (!optee->smc.notif_pcpu_wq) {
+ rc = -EINVAL;
+ goto err_free_pcpu_irq;
+ }
+
+ optee->smc.optee_pcpu = optee_pcpu;
+ optee->smc.notif_irq = irq;
+
+ return 0;
+
+err_free_pcpu_irq:
+ spin_lock(&lock);
+ disable_percpu_irq(irq);
+ spin_unlock(&lock);
+ free_percpu_irq(irq, optee_pcpu);
+err_free_pcpu:
+ free_percpu(optee_pcpu);
+
+ return rc;
+}
+
+static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
+{
+ if (irq_is_percpu_devid(irq))
+ return init_pcpu_irq(optee, irq);
+ else
+ return init_irq(optee, irq);
+}
+
+static void uninit_pcpu_irq(struct optee *optee)
+{
+ spinlock_t lock;
+
+ spin_lock_init(&lock);
+ spin_lock(&lock);
+ disable_percpu_irq(optee->smc.notif_irq);
+ spin_unlock(&lock);
+
+ free_percpu_irq(optee->smc.notif_irq, optee->smc.optee_pcpu);
+ free_percpu(optee->smc.optee_pcpu);
+}
+
static void optee_smc_notif_uninit_irq(struct optee *optee)
{
if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_ASYNC_NOTIF) {
optee_smc_stop_async_notif(optee->ctx);
if (optee->smc.notif_irq) {
- free_irq(optee->smc.notif_irq, optee);
+ if (irq_is_percpu_devid(optee->smc.notif_irq))
+ uninit_pcpu_irq(optee);
+ else
+ free_irq(optee->smc.notif_irq, optee);
+
irq_dispose_mapping(optee->smc.notif_irq);
}
}
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] optee: add per cpu asynchronous notification
2023-01-18 17:49 ` Etienne Carriere
@ 2023-01-20 8:33 ` Jens Wiklander
-1 siblings, 0 replies; 6+ messages in thread
From: Jens Wiklander @ 2023-01-20 8:33 UTC (permalink / raw)
To: Etienne Carriere
Cc: linux-kernel, linux-arm-kernel, Sumit Garg, Marc Zyngier,
Alexandre Torgue
On Wed, Jan 18, 2023 at 06:49:09PM +0100, Etienne Carriere wrote:
> Implements use of per CPU irq for optee asynchronous notification.
>
> Existing optee async notif implementation allows OP-TE world to
allows OP-TEE in the secure world to
> raise an interrupt for the Linux optee driver to query pending events
> bound to waiting tasks in Linux world or threaded bottom half tasks
> to be invoked in TEE world. This change allows the signaling interrupt
> to be a per cpu interrupt as with Arm GIC PPIs.
>
> Cc: Jens Wiklander <jens.wiklander@linaro.org>
> Cc: Sumit Garg <sumit.garg@linaro.org>
> Cc: Marc Zyngier <maz@kernel.org>
>
> Co-developed-by: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since v1:
> - Fixed missing __percpu attribute reported by kernel test robot.
> - Rephrased commit message and added Cc tags.
> ---
> drivers/tee/optee/optee_private.h | 22 ++++++
> drivers/tee/optee/smc_abi.c | 107 ++++++++++++++++++++++++++++--
> 2 files changed, 124 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index 04ae58892608..e5bd3548691f 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -94,11 +94,33 @@ struct optee_supp {
> struct completion reqs_c;
> };
>
> +/*
> + * struct optee_pcpu - per cpu notif private struct passed to work functions
> + * @optee optee device reference
> + */
> +struct optee_pcpu {
> + struct optee *optee;
> +};
> +
> +/*
> + * struct optee_smc - optee smc communication struct
> + * @invoke_fn handler function to invoke secure monitor
> + * @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
> + * @notif_irq interrupt used as async notification by OP-TEE or 0
> + * @optee_pcpu per_cpu optee instance for per cpu work or NULL
> + * @notif_pcpu_wq workqueue for per cpu aynchronous notification or NULL
> + * @notif_pcpu_work work for per cpu asynchronous notification
> + */
> struct optee_smc {
> optee_invoke_fn *invoke_fn;
> void *memremaped_shm;
> u32 sec_caps;
> unsigned int notif_irq;
> + struct optee_pcpu __percpu *optee_pcpu;
> + struct workqueue_struct *notif_pcpu_wq;
> + struct work_struct notif_pcpu_work;
> };
>
> /**
> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> index a1c1fa1a9c28..ffa3f3aa7244 100644
> --- a/drivers/tee/optee/smc_abi.c
> +++ b/drivers/tee/optee/smc_abi.c
> @@ -993,12 +993,20 @@ static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
>
> static irqreturn_t notif_irq_handler(int irq, void *dev_id)
Wouldn't it be easier with one handler for shared irqs and one for
per-cpu irqs? The only common part is the do-while loop which I suppose
could go into a helper function.
> {
> - struct optee *optee = dev_id;
> + struct optee *optee;
> bool do_bottom_half = false;
> bool value_valid;
> bool value_pending;
> u32 value;
>
> + if (irq_is_percpu_devid(irq)) {
> + struct optee_pcpu __percpu *pcpu = (struct optee_pcpu *)dev_id;
> +
> + optee = pcpu->optee;
> + } else {
> + optee = dev_id;
> + }
> +
> do {
> value = get_async_notif_value(optee->smc.invoke_fn,
> &value_valid, &value_pending);
> @@ -1011,8 +1019,13 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id)
> optee_notif_send(optee, value);
> } while (value_pending);
>
> - if (do_bottom_half)
> - return IRQ_WAKE_THREAD;
> + if (do_bottom_half) {
> + if (irq_is_percpu_devid(irq))
> + queue_work(optee->smc.notif_pcpu_wq, &optee->smc.notif_pcpu_work);
This line is a bit long, please break it.
> + else
> + return IRQ_WAKE_THREAD;
> + }
> +
> return IRQ_HANDLED;
> }
>
> @@ -1025,7 +1038,7 @@ static irqreturn_t notif_irq_thread_fn(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> -static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
> +static int init_irq(struct optee *optee, u_int irq)
> {
> int rc;
>
> @@ -1040,12 +1053,96 @@ static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
> return 0;
> }
>
> +static void notif_pcpu_irq_work_fn(struct work_struct *work)
> +{
> + struct optee_smc *optee_smc = container_of(work, struct optee_smc, notif_pcpu_work);
This line is a bit long, please break it.
> + struct optee *optee = container_of(optee_smc, struct optee, smc);
> +
> + optee_smc_do_bottom_half(optee->ctx);
> +}
> +
> +static int init_pcpu_irq(struct optee *optee, u_int irq)
> +{
> + struct optee_pcpu __percpu *optee_pcpu;
> + spinlock_t lock;
> + int cpu;
> + int rc;
> +
> + optee_pcpu = alloc_percpu(struct optee_pcpu);
> + if (!optee_pcpu)
> + return -ENOMEM;
> +
> + for_each_present_cpu(cpu) {
> + struct optee_pcpu __percpu *p = per_cpu_ptr(optee_pcpu, cpu);
> +
> + p->optee = optee;
> + }
> +
> + rc = request_percpu_irq(irq, notif_irq_handler,
> + "optee_pcpu_notification", optee_pcpu);
> + if (rc)
> + goto err_free_pcpu;
> +
> + spin_lock_init(&lock);
> +
> + spin_lock(&lock);
What is the point with this spinlock?
> + enable_percpu_irq(irq, 0);
> + spin_unlock(&lock);
> +
> + INIT_WORK(&optee->smc.notif_pcpu_work, notif_pcpu_irq_work_fn);
> + optee->smc.notif_pcpu_wq = create_workqueue("optee_pcpu_notification");
> + if (!optee->smc.notif_pcpu_wq) {
> + rc = -EINVAL;
> + goto err_free_pcpu_irq;
> + }
> +
> + optee->smc.optee_pcpu = optee_pcpu;
> + optee->smc.notif_irq = irq;
> +
> + return 0;
> +
> +err_free_pcpu_irq:
> + spin_lock(&lock);
> + disable_percpu_irq(irq);
> + spin_unlock(&lock);
> + free_percpu_irq(irq, optee_pcpu);
> +err_free_pcpu:
> + free_percpu(optee_pcpu);
> +
> + return rc;
> +}
> +
> +static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
> +{
> + if (irq_is_percpu_devid(irq))
> + return init_pcpu_irq(optee, irq);
> + else
> + return init_irq(optee, irq);
> +}
> +
> +static void uninit_pcpu_irq(struct optee *optee)
> +{
> + spinlock_t lock;
> +
> + spin_lock_init(&lock);
> + spin_lock(&lock);
What's the point with this spinlock?
Cheers,
Jens
> + disable_percpu_irq(optee->smc.notif_irq);
> + spin_unlock(&lock);
> +
> + free_percpu_irq(optee->smc.notif_irq, optee->smc.optee_pcpu);
> + free_percpu(optee->smc.optee_pcpu);
> +}
> +
> static void optee_smc_notif_uninit_irq(struct optee *optee)
> {
> if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_ASYNC_NOTIF) {
> optee_smc_stop_async_notif(optee->ctx);
> if (optee->smc.notif_irq) {
> - free_irq(optee->smc.notif_irq, optee);
> + if (irq_is_percpu_devid(optee->smc.notif_irq))
> + uninit_pcpu_irq(optee);
> + else
> + free_irq(optee->smc.notif_irq, optee);
> +
> irq_dispose_mapping(optee->smc.notif_irq);
> }
> }
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] optee: add per cpu asynchronous notification
@ 2023-01-20 8:33 ` Jens Wiklander
0 siblings, 0 replies; 6+ messages in thread
From: Jens Wiklander @ 2023-01-20 8:33 UTC (permalink / raw)
To: Etienne Carriere
Cc: linux-kernel, linux-arm-kernel, Sumit Garg, Marc Zyngier,
Alexandre Torgue
On Wed, Jan 18, 2023 at 06:49:09PM +0100, Etienne Carriere wrote:
> Implements use of per CPU irq for optee asynchronous notification.
>
> Existing optee async notif implementation allows OP-TE world to
allows OP-TEE in the secure world to
> raise an interrupt for the Linux optee driver to query pending events
> bound to waiting tasks in Linux world or threaded bottom half tasks
> to be invoked in TEE world. This change allows the signaling interrupt
> to be a per cpu interrupt as with Arm GIC PPIs.
>
> Cc: Jens Wiklander <jens.wiklander@linaro.org>
> Cc: Sumit Garg <sumit.garg@linaro.org>
> Cc: Marc Zyngier <maz@kernel.org>
>
> Co-developed-by: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since v1:
> - Fixed missing __percpu attribute reported by kernel test robot.
> - Rephrased commit message and added Cc tags.
> ---
> drivers/tee/optee/optee_private.h | 22 ++++++
> drivers/tee/optee/smc_abi.c | 107 ++++++++++++++++++++++++++++--
> 2 files changed, 124 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index 04ae58892608..e5bd3548691f 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -94,11 +94,33 @@ struct optee_supp {
> struct completion reqs_c;
> };
>
> +/*
> + * struct optee_pcpu - per cpu notif private struct passed to work functions
> + * @optee optee device reference
> + */
> +struct optee_pcpu {
> + struct optee *optee;
> +};
> +
> +/*
> + * struct optee_smc - optee smc communication struct
> + * @invoke_fn handler function to invoke secure monitor
> + * @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
> + * @notif_irq interrupt used as async notification by OP-TEE or 0
> + * @optee_pcpu per_cpu optee instance for per cpu work or NULL
> + * @notif_pcpu_wq workqueue for per cpu aynchronous notification or NULL
> + * @notif_pcpu_work work for per cpu asynchronous notification
> + */
> struct optee_smc {
> optee_invoke_fn *invoke_fn;
> void *memremaped_shm;
> u32 sec_caps;
> unsigned int notif_irq;
> + struct optee_pcpu __percpu *optee_pcpu;
> + struct workqueue_struct *notif_pcpu_wq;
> + struct work_struct notif_pcpu_work;
> };
>
> /**
> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> index a1c1fa1a9c28..ffa3f3aa7244 100644
> --- a/drivers/tee/optee/smc_abi.c
> +++ b/drivers/tee/optee/smc_abi.c
> @@ -993,12 +993,20 @@ static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
>
> static irqreturn_t notif_irq_handler(int irq, void *dev_id)
Wouldn't it be easier with one handler for shared irqs and one for
per-cpu irqs? The only common part is the do-while loop which I suppose
could go into a helper function.
> {
> - struct optee *optee = dev_id;
> + struct optee *optee;
> bool do_bottom_half = false;
> bool value_valid;
> bool value_pending;
> u32 value;
>
> + if (irq_is_percpu_devid(irq)) {
> + struct optee_pcpu __percpu *pcpu = (struct optee_pcpu *)dev_id;
> +
> + optee = pcpu->optee;
> + } else {
> + optee = dev_id;
> + }
> +
> do {
> value = get_async_notif_value(optee->smc.invoke_fn,
> &value_valid, &value_pending);
> @@ -1011,8 +1019,13 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id)
> optee_notif_send(optee, value);
> } while (value_pending);
>
> - if (do_bottom_half)
> - return IRQ_WAKE_THREAD;
> + if (do_bottom_half) {
> + if (irq_is_percpu_devid(irq))
> + queue_work(optee->smc.notif_pcpu_wq, &optee->smc.notif_pcpu_work);
This line is a bit long, please break it.
> + else
> + return IRQ_WAKE_THREAD;
> + }
> +
> return IRQ_HANDLED;
> }
>
> @@ -1025,7 +1038,7 @@ static irqreturn_t notif_irq_thread_fn(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> -static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
> +static int init_irq(struct optee *optee, u_int irq)
> {
> int rc;
>
> @@ -1040,12 +1053,96 @@ static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
> return 0;
> }
>
> +static void notif_pcpu_irq_work_fn(struct work_struct *work)
> +{
> + struct optee_smc *optee_smc = container_of(work, struct optee_smc, notif_pcpu_work);
This line is a bit long, please break it.
> + struct optee *optee = container_of(optee_smc, struct optee, smc);
> +
> + optee_smc_do_bottom_half(optee->ctx);
> +}
> +
> +static int init_pcpu_irq(struct optee *optee, u_int irq)
> +{
> + struct optee_pcpu __percpu *optee_pcpu;
> + spinlock_t lock;
> + int cpu;
> + int rc;
> +
> + optee_pcpu = alloc_percpu(struct optee_pcpu);
> + if (!optee_pcpu)
> + return -ENOMEM;
> +
> + for_each_present_cpu(cpu) {
> + struct optee_pcpu __percpu *p = per_cpu_ptr(optee_pcpu, cpu);
> +
> + p->optee = optee;
> + }
> +
> + rc = request_percpu_irq(irq, notif_irq_handler,
> + "optee_pcpu_notification", optee_pcpu);
> + if (rc)
> + goto err_free_pcpu;
> +
> + spin_lock_init(&lock);
> +
> + spin_lock(&lock);
What is the point with this spinlock?
> + enable_percpu_irq(irq, 0);
> + spin_unlock(&lock);
> +
> + INIT_WORK(&optee->smc.notif_pcpu_work, notif_pcpu_irq_work_fn);
> + optee->smc.notif_pcpu_wq = create_workqueue("optee_pcpu_notification");
> + if (!optee->smc.notif_pcpu_wq) {
> + rc = -EINVAL;
> + goto err_free_pcpu_irq;
> + }
> +
> + optee->smc.optee_pcpu = optee_pcpu;
> + optee->smc.notif_irq = irq;
> +
> + return 0;
> +
> +err_free_pcpu_irq:
> + spin_lock(&lock);
> + disable_percpu_irq(irq);
> + spin_unlock(&lock);
> + free_percpu_irq(irq, optee_pcpu);
> +err_free_pcpu:
> + free_percpu(optee_pcpu);
> +
> + return rc;
> +}
> +
> +static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
> +{
> + if (irq_is_percpu_devid(irq))
> + return init_pcpu_irq(optee, irq);
> + else
> + return init_irq(optee, irq);
> +}
> +
> +static void uninit_pcpu_irq(struct optee *optee)
> +{
> + spinlock_t lock;
> +
> + spin_lock_init(&lock);
> + spin_lock(&lock);
What's the point with this spinlock?
Cheers,
Jens
> + disable_percpu_irq(optee->smc.notif_irq);
> + spin_unlock(&lock);
> +
> + free_percpu_irq(optee->smc.notif_irq, optee->smc.optee_pcpu);
> + free_percpu(optee->smc.optee_pcpu);
> +}
> +
> static void optee_smc_notif_uninit_irq(struct optee *optee)
> {
> if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_ASYNC_NOTIF) {
> optee_smc_stop_async_notif(optee->ctx);
> if (optee->smc.notif_irq) {
> - free_irq(optee->smc.notif_irq, optee);
> + if (irq_is_percpu_devid(optee->smc.notif_irq))
> + uninit_pcpu_irq(optee);
> + else
> + free_irq(optee->smc.notif_irq, optee);
> +
> irq_dispose_mapping(optee->smc.notif_irq);
> }
> }
> --
> 2.25.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] optee: add per cpu asynchronous notification
2023-01-20 8:33 ` Jens Wiklander
@ 2023-01-20 10:09 ` Etienne Carriere
-1 siblings, 0 replies; 6+ messages in thread
From: Etienne Carriere @ 2023-01-20 10:09 UTC (permalink / raw)
To: Jens Wiklander
Cc: linux-kernel, linux-arm-kernel, Sumit Garg, Marc Zyngier,
Alexandre Torgue
Hello Jens,
Thanks for the feedback.
On Fri, 20 Jan 2023 at 09:33, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Wed, Jan 18, 2023 at 06:49:09PM +0100, Etienne Carriere wrote:
> > Implements use of per CPU irq for optee asynchronous notification.
> >
> > Existing optee async notif implementation allows OP-TE world to
>
> allows OP-TEE in the secure world to
>
> > raise an interrupt for the Linux optee driver to query pending events
> > bound to waiting tasks in Linux world or threaded bottom half tasks
> > to be invoked in TEE world. This change allows the signaling interrupt
> > to be a per cpu interrupt as with Arm GIC PPIs.
> >
> > Cc: Jens Wiklander <jens.wiklander@linaro.org>
> > Cc: Sumit Garg <sumit.garg@linaro.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> >
> > Co-developed-by: Alexandre Torgue <alexandre.torgue@foss.st.com>
> > Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com>
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> > Changes since v1:
> > - Fixed missing __percpu attribute reported by kernel test robot.
> > - Rephrased commit message and added Cc tags.
> > ---
> > drivers/tee/optee/optee_private.h | 22 ++++++
> > drivers/tee/optee/smc_abi.c | 107 ++++++++++++++++++++++++++++--
> > 2 files changed, 124 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index 04ae58892608..e5bd3548691f 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -94,11 +94,33 @@ struct optee_supp {
> > struct completion reqs_c;
> > };
> >
> > +/*
> > + * struct optee_pcpu - per cpu notif private struct passed to work functions
> > + * @optee optee device reference
> > + */
> > +struct optee_pcpu {
> > + struct optee *optee;
> > +};
> > +
> > +/*
> > + * struct optee_smc - optee smc communication struct
> > + * @invoke_fn handler function to invoke secure monitor
> > + * @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
> > + * @notif_irq interrupt used as async notification by OP-TEE or 0
> > + * @optee_pcpu per_cpu optee instance for per cpu work or NULL
> > + * @notif_pcpu_wq workqueue for per cpu aynchronous notification or NULL
> > + * @notif_pcpu_work work for per cpu asynchronous notification
> > + */
> > struct optee_smc {
> > optee_invoke_fn *invoke_fn;
> > void *memremaped_shm;
> > u32 sec_caps;
> > unsigned int notif_irq;
> > + struct optee_pcpu __percpu *optee_pcpu;
> > + struct workqueue_struct *notif_pcpu_wq;
> > + struct work_struct notif_pcpu_work;
> > };
> >
> > /**
> > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > index a1c1fa1a9c28..ffa3f3aa7244 100644
> > --- a/drivers/tee/optee/smc_abi.c
> > +++ b/drivers/tee/optee/smc_abi.c
> > @@ -993,12 +993,20 @@ static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
> >
> > static irqreturn_t notif_irq_handler(int irq, void *dev_id)
>
> Wouldn't it be easier with one handler for shared irqs and one for
> per-cpu irqs? The only common part is the do-while loop which I suppose
> could go into a helper function.
Ok, I do that.
>
> > {
> > - struct optee *optee = dev_id;
> > + struct optee *optee;
> > bool do_bottom_half = false;
> > bool value_valid;
> > bool value_pending;
> > u32 value;
> >
> > + if (irq_is_percpu_devid(irq)) {
> > + struct optee_pcpu __percpu *pcpu = (struct optee_pcpu *)dev_id;
> > +
> > + optee = pcpu->optee;
> > + } else {
> > + optee = dev_id;
> > + }
> > +
> > do {
> > value = get_async_notif_value(optee->smc.invoke_fn,
> > &value_valid, &value_pending);
> > @@ -1011,8 +1019,13 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id)
> > optee_notif_send(optee, value);
> > } while (value_pending);
> >
> > - if (do_bottom_half)
> > - return IRQ_WAKE_THREAD;
> > + if (do_bottom_half) {
> > + if (irq_is_percpu_devid(irq))
> > + queue_work(optee->smc.notif_pcpu_wq, &optee->smc.notif_pcpu_work);
>
> This line is a bit long, please break it.
ok, thanks.
>
> > + else
> > + return IRQ_WAKE_THREAD;
> > + }
> > +
> > return IRQ_HANDLED;
> > }
> >
> > @@ -1025,7 +1038,7 @@ static irqreturn_t notif_irq_thread_fn(int irq, void *dev_id)
> > return IRQ_HANDLED;
> > }
> >
> > -static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
> > +static int init_irq(struct optee *optee, u_int irq)
> > {
> > int rc;
> >
> > @@ -1040,12 +1053,96 @@ static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
> > return 0;
> > }
> >
> > +static void notif_pcpu_irq_work_fn(struct work_struct *work)
> > +{
> > + struct optee_smc *optee_smc = container_of(work, struct optee_smc, notif_pcpu_work);
>
> This line is a bit long, please break it.
ok.
>
> > + struct optee *optee = container_of(optee_smc, struct optee, smc);
> > +
> > + optee_smc_do_bottom_half(optee->ctx);
> > +}
> > +
> > +static int init_pcpu_irq(struct optee *optee, u_int irq)
> > +{
> > + struct optee_pcpu __percpu *optee_pcpu;
> > + spinlock_t lock;
> > + int cpu;
> > + int rc;
> > +
> > + optee_pcpu = alloc_percpu(struct optee_pcpu);
> > + if (!optee_pcpu)
> > + return -ENOMEM;
> > +
> > + for_each_present_cpu(cpu) {
> > + struct optee_pcpu __percpu *p = per_cpu_ptr(optee_pcpu, cpu);
> > +
> > + p->optee = optee;
> > + }
> > +
> > + rc = request_percpu_irq(irq, notif_irq_handler,
> > + "optee_pcpu_notification", optee_pcpu);
> > + if (rc)
> > + goto err_free_pcpu;
> > +
> > + spin_lock_init(&lock);
> > +
> > + spin_lock(&lock);
>
> What is the point with this spinlock?
Hmm... indeed. I'll remove.
>
> > + enable_percpu_irq(irq, 0);
> > + spin_unlock(&lock);
> > +
> > + INIT_WORK(&optee->smc.notif_pcpu_work, notif_pcpu_irq_work_fn);
> > + optee->smc.notif_pcpu_wq = create_workqueue("optee_pcpu_notification");
> > + if (!optee->smc.notif_pcpu_wq) {
> > + rc = -EINVAL;
> > + goto err_free_pcpu_irq;
> > + }
> > +
> > + optee->smc.optee_pcpu = optee_pcpu;
> > + optee->smc.notif_irq = irq;
> > +
> > + return 0;
> > +
> > +err_free_pcpu_irq:
> > + spin_lock(&lock);
> > + disable_percpu_irq(irq);
> > + spin_unlock(&lock);
> > + free_percpu_irq(irq, optee_pcpu);
> > +err_free_pcpu:
> > + free_percpu(optee_pcpu);
> > +
> > + return rc;
> > +}
> > +
> > +static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
> > +{
> > + if (irq_is_percpu_devid(irq))
> > + return init_pcpu_irq(optee, irq);
> > + else
> > + return init_irq(optee, irq);
> > +}
> > +
> > +static void uninit_pcpu_irq(struct optee *optee)
> > +{
> > + spinlock_t lock;
> > +
> > + spin_lock_init(&lock);
> > + spin_lock(&lock);
>
> What's the point with this spinlock?
>
>
> Cheers,
> Jens
>
> > + disable_percpu_irq(optee->smc.notif_irq);
> > + spin_unlock(&lock);
> > +
> > + free_percpu_irq(optee->smc.notif_irq, optee->smc.optee_pcpu);
> > + free_percpu(optee->smc.optee_pcpu);
> > +}
> > +
> > static void optee_smc_notif_uninit_irq(struct optee *optee)
> > {
> > if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_ASYNC_NOTIF) {
> > optee_smc_stop_async_notif(optee->ctx);
> > if (optee->smc.notif_irq) {
> > - free_irq(optee->smc.notif_irq, optee);
> > + if (irq_is_percpu_devid(optee->smc.notif_irq))
> > + uninit_pcpu_irq(optee);
> > + else
> > + free_irq(optee->smc.notif_irq, optee);
> > +
> > irq_dispose_mapping(optee->smc.notif_irq);
> > }
> > }
> > --
> > 2.25.1
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] optee: add per cpu asynchronous notification
@ 2023-01-20 10:09 ` Etienne Carriere
0 siblings, 0 replies; 6+ messages in thread
From: Etienne Carriere @ 2023-01-20 10:09 UTC (permalink / raw)
To: Jens Wiklander
Cc: linux-kernel, linux-arm-kernel, Sumit Garg, Marc Zyngier,
Alexandre Torgue
Hello Jens,
Thanks for the feedback.
On Fri, 20 Jan 2023 at 09:33, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Wed, Jan 18, 2023 at 06:49:09PM +0100, Etienne Carriere wrote:
> > Implements use of per CPU irq for optee asynchronous notification.
> >
> > Existing optee async notif implementation allows OP-TE world to
>
> allows OP-TEE in the secure world to
>
> > raise an interrupt for the Linux optee driver to query pending events
> > bound to waiting tasks in Linux world or threaded bottom half tasks
> > to be invoked in TEE world. This change allows the signaling interrupt
> > to be a per cpu interrupt as with Arm GIC PPIs.
> >
> > Cc: Jens Wiklander <jens.wiklander@linaro.org>
> > Cc: Sumit Garg <sumit.garg@linaro.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> >
> > Co-developed-by: Alexandre Torgue <alexandre.torgue@foss.st.com>
> > Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com>
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> > Changes since v1:
> > - Fixed missing __percpu attribute reported by kernel test robot.
> > - Rephrased commit message and added Cc tags.
> > ---
> > drivers/tee/optee/optee_private.h | 22 ++++++
> > drivers/tee/optee/smc_abi.c | 107 ++++++++++++++++++++++++++++--
> > 2 files changed, 124 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index 04ae58892608..e5bd3548691f 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -94,11 +94,33 @@ struct optee_supp {
> > struct completion reqs_c;
> > };
> >
> > +/*
> > + * struct optee_pcpu - per cpu notif private struct passed to work functions
> > + * @optee optee device reference
> > + */
> > +struct optee_pcpu {
> > + struct optee *optee;
> > +};
> > +
> > +/*
> > + * struct optee_smc - optee smc communication struct
> > + * @invoke_fn handler function to invoke secure monitor
> > + * @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
> > + * @notif_irq interrupt used as async notification by OP-TEE or 0
> > + * @optee_pcpu per_cpu optee instance for per cpu work or NULL
> > + * @notif_pcpu_wq workqueue for per cpu aynchronous notification or NULL
> > + * @notif_pcpu_work work for per cpu asynchronous notification
> > + */
> > struct optee_smc {
> > optee_invoke_fn *invoke_fn;
> > void *memremaped_shm;
> > u32 sec_caps;
> > unsigned int notif_irq;
> > + struct optee_pcpu __percpu *optee_pcpu;
> > + struct workqueue_struct *notif_pcpu_wq;
> > + struct work_struct notif_pcpu_work;
> > };
> >
> > /**
> > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > index a1c1fa1a9c28..ffa3f3aa7244 100644
> > --- a/drivers/tee/optee/smc_abi.c
> > +++ b/drivers/tee/optee/smc_abi.c
> > @@ -993,12 +993,20 @@ static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
> >
> > static irqreturn_t notif_irq_handler(int irq, void *dev_id)
>
> Wouldn't it be easier with one handler for shared irqs and one for
> per-cpu irqs? The only common part is the do-while loop which I suppose
> could go into a helper function.
Ok, I do that.
>
> > {
> > - struct optee *optee = dev_id;
> > + struct optee *optee;
> > bool do_bottom_half = false;
> > bool value_valid;
> > bool value_pending;
> > u32 value;
> >
> > + if (irq_is_percpu_devid(irq)) {
> > + struct optee_pcpu __percpu *pcpu = (struct optee_pcpu *)dev_id;
> > +
> > + optee = pcpu->optee;
> > + } else {
> > + optee = dev_id;
> > + }
> > +
> > do {
> > value = get_async_notif_value(optee->smc.invoke_fn,
> > &value_valid, &value_pending);
> > @@ -1011,8 +1019,13 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id)
> > optee_notif_send(optee, value);
> > } while (value_pending);
> >
> > - if (do_bottom_half)
> > - return IRQ_WAKE_THREAD;
> > + if (do_bottom_half) {
> > + if (irq_is_percpu_devid(irq))
> > + queue_work(optee->smc.notif_pcpu_wq, &optee->smc.notif_pcpu_work);
>
> This line is a bit long, please break it.
ok, thanks.
>
> > + else
> > + return IRQ_WAKE_THREAD;
> > + }
> > +
> > return IRQ_HANDLED;
> > }
> >
> > @@ -1025,7 +1038,7 @@ static irqreturn_t notif_irq_thread_fn(int irq, void *dev_id)
> > return IRQ_HANDLED;
> > }
> >
> > -static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
> > +static int init_irq(struct optee *optee, u_int irq)
> > {
> > int rc;
> >
> > @@ -1040,12 +1053,96 @@ static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
> > return 0;
> > }
> >
> > +static void notif_pcpu_irq_work_fn(struct work_struct *work)
> > +{
> > + struct optee_smc *optee_smc = container_of(work, struct optee_smc, notif_pcpu_work);
>
> This line is a bit long, please break it.
ok.
>
> > + struct optee *optee = container_of(optee_smc, struct optee, smc);
> > +
> > + optee_smc_do_bottom_half(optee->ctx);
> > +}
> > +
> > +static int init_pcpu_irq(struct optee *optee, u_int irq)
> > +{
> > + struct optee_pcpu __percpu *optee_pcpu;
> > + spinlock_t lock;
> > + int cpu;
> > + int rc;
> > +
> > + optee_pcpu = alloc_percpu(struct optee_pcpu);
> > + if (!optee_pcpu)
> > + return -ENOMEM;
> > +
> > + for_each_present_cpu(cpu) {
> > + struct optee_pcpu __percpu *p = per_cpu_ptr(optee_pcpu, cpu);
> > +
> > + p->optee = optee;
> > + }
> > +
> > + rc = request_percpu_irq(irq, notif_irq_handler,
> > + "optee_pcpu_notification", optee_pcpu);
> > + if (rc)
> > + goto err_free_pcpu;
> > +
> > + spin_lock_init(&lock);
> > +
> > + spin_lock(&lock);
>
> What is the point with this spinlock?
Hmm... indeed. I'll remove.
>
> > + enable_percpu_irq(irq, 0);
> > + spin_unlock(&lock);
> > +
> > + INIT_WORK(&optee->smc.notif_pcpu_work, notif_pcpu_irq_work_fn);
> > + optee->smc.notif_pcpu_wq = create_workqueue("optee_pcpu_notification");
> > + if (!optee->smc.notif_pcpu_wq) {
> > + rc = -EINVAL;
> > + goto err_free_pcpu_irq;
> > + }
> > +
> > + optee->smc.optee_pcpu = optee_pcpu;
> > + optee->smc.notif_irq = irq;
> > +
> > + return 0;
> > +
> > +err_free_pcpu_irq:
> > + spin_lock(&lock);
> > + disable_percpu_irq(irq);
> > + spin_unlock(&lock);
> > + free_percpu_irq(irq, optee_pcpu);
> > +err_free_pcpu:
> > + free_percpu(optee_pcpu);
> > +
> > + return rc;
> > +}
> > +
> > +static int optee_smc_notif_init_irq(struct optee *optee, u_int irq)
> > +{
> > + if (irq_is_percpu_devid(irq))
> > + return init_pcpu_irq(optee, irq);
> > + else
> > + return init_irq(optee, irq);
> > +}
> > +
> > +static void uninit_pcpu_irq(struct optee *optee)
> > +{
> > + spinlock_t lock;
> > +
> > + spin_lock_init(&lock);
> > + spin_lock(&lock);
>
> What's the point with this spinlock?
>
>
> Cheers,
> Jens
>
> > + disable_percpu_irq(optee->smc.notif_irq);
> > + spin_unlock(&lock);
> > +
> > + free_percpu_irq(optee->smc.notif_irq, optee->smc.optee_pcpu);
> > + free_percpu(optee->smc.optee_pcpu);
> > +}
> > +
> > static void optee_smc_notif_uninit_irq(struct optee *optee)
> > {
> > if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_ASYNC_NOTIF) {
> > optee_smc_stop_async_notif(optee->ctx);
> > if (optee->smc.notif_irq) {
> > - free_irq(optee->smc.notif_irq, optee);
> > + if (irq_is_percpu_devid(optee->smc.notif_irq))
> > + uninit_pcpu_irq(optee);
> > + else
> > + free_irq(optee->smc.notif_irq, optee);
> > +
> > irq_dispose_mapping(optee->smc.notif_irq);
> > }
> > }
> > --
> > 2.25.1
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-20 10:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-18 17:49 [PATCH v2] optee: add per cpu asynchronous notification Etienne Carriere
2023-01-18 17:49 ` Etienne Carriere
2023-01-20 8:33 ` Jens Wiklander
2023-01-20 8:33 ` Jens Wiklander
2023-01-20 10:09 ` Etienne Carriere
2023-01-20 10:09 ` Etienne Carriere
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.