RE: [PATCH V3 2/3] efi: Introduce efi_queue_work() to queue any efi_runtime_service() on efi_rts_wq

[Prakhya, Sai Praneeth]

> On Mon, May 21, 2018 at 08:13:03PM -0700, Sai Praneeth Prakhya wrote:
> > +   /*  \
> > +* queue_work() returns 0 if work was already on queue, \
> > +* _ideally_ this should never happen.  \
> > +*/ \
> > +   if (queue_work(efi_rts_wq, _rts_work.work))
>   \
> > +   flush_work(_rts_work.work);
>   \
> 
> Since you're _always_ going to wait for it, it is _much_ cheaper to put a
> completion in your actual work and wait for that.

Sure! I will change it.
I also noticed that flush_work() in turn calls wait_for_completion().

Will also wait for a couple of days before posting V4.

Regards,
Sai
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/3] efi: Introduce efi_queue_work() to queue any efi_runtime_service() on efi_rts_wq

[Peter Zijlstra]

On Mon, May 21, 2018 at 08:13:03PM -0700, Sai Praneeth Prakhya wrote:
> + /*  \
> +  * queue_work() returns 0 if work was already on queue, \
> +  * _ideally_ this should never happen.  \
> +  */ \
> + if (queue_work(efi_rts_wq, _rts_work.work)) \
> + flush_work(_rts_work.work); \

Since you're _always_ going to wait for it, it is _much_ cheaper to put
a completion in your actual work and wait for that.

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V3 2/3] efi: Introduce efi_queue_work() to queue any efi_runtime_service() on efi_rts_wq

[Sai Praneeth Prakhya]

From: Sai Praneeth 

When a process requests the kernel to execute any efi_runtime_service(),
the requested efi_runtime_service (represented as an identifier) and its
arguments are packed into a struct named efi_runtime_work and queued
onto work queue named efi_rts_wq. The caller then waits until the work
is completed.

Introduce efi_queue_work() that 1. Populates efi_runtime_work 2. Queues
work onto efi_rts_wq and 3. Waits until worker thread returns.

The caller thread has to wait until the worker thread returns, because
it depends on the return status of efi_runtime_service() and, in
specific cases, the arguments populated by efi_runtime_service(). Some
efi_runtime_services() takes a pointer to buffer as an argument and
fills up the buffer with requested data. For instance,
efi_get_variable() and efi_get_next_variable(). Hence, caller process
cannot just post the work and get going.

Some facts about efi_runtime_services():
1. A quick look at all the efi_runtime_services() shows that any
efi_runtime_service() has five or less arguments.
2. An argument of efi_runtime_service() can be a value (of any type) or
a pointer (of any type).
Hence, efi_runtime_work has five void pointers to store these arguments.

Signed-off-by: Sai Praneeth Prakhya 
Suggested-by: Andy Lutomirski 
Cc: Lee Chun-Yi 
Cc: Borislav Petkov 
Cc: Tony Luck 
Cc: Will Deacon 
Cc: Dave Hansen 
Cc: Mark Rutland 
Cc: Bhupesh Sharma 
Cc: Naresh Bhat 
Cc: Ricardo Neri 
Cc: Peter Zijlstra 
Cc: Ravi Shankar 
Cc: Matt Fleming 
Cc: Dan Williams 
Cc: Ard Biesheuvel 
Cc: Miguel Ojeda 
---
 drivers/firmware/efi/runtime-wrappers.c | 80 +
 1 file changed, 80 insertions(+)

diff --git a/drivers/firmware/efi/runtime-wrappers.c 
b/drivers/firmware/efi/runtime-wrappers.c
index ae54870b2788..a9866045ed52 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -1,6 +1,14 @@
 /*
  * runtime-wrappers.c - Runtime Services function call wrappers
  *
+ * Implementation summary:
+ * ---
+ * 1. When user/kernel thread requests to execute efi_runtime_service(),
+ * enqueue work to efi_rts_wq.
+ * 2. Caller thread waits until the work is finished because it's
+ * dependent on the return status and execution of efi_runtime_service().
+ * For instance, get_variable() and get_next_variable().
+ *
  * Copyright (C) 2014 Linaro Ltd. 
  *
  * Split off from arch/x86/platform/efi/efi.c
@@ -22,6 +30,8 @@
 #include 
 #include 
 #include 
+#include 
+
 #include 
 
 /*
@@ -33,6 +43,76 @@
 #define __efi_call_virt(f, args...) \
__efi_call_virt_pointer(efi.systab->runtime, f, args)
 
+/* efi_runtime_service() function identifiers */
+enum efi_rts_ids {
+   GET_TIME,
+   SET_TIME,
+   GET_WAKEUP_TIME,
+   SET_WAKEUP_TIME,
+   GET_VARIABLE,
+   GET_NEXT_VARIABLE,
+   SET_VARIABLE,
+   SET_VARIABLE_NONBLOCKING,
+   QUERY_VARIABLE_INFO,
+   QUERY_VARIABLE_INFO_NONBLOCKING,
+   GET_NEXT_HIGH_MONO_COUNT,
+   RESET_SYSTEM,
+   UPDATE_CAPSULE,
+   QUERY_CAPSULE_CAPS,
+};
+
+/*
+ * efi_runtime_work:   Details of EFI Runtime Service work
+ * @func:  EFI Runtime Service function identifier
+ * @arg<1-5>:  EFI Runtime Service function arguments
+ * @status:Status of executing EFI Runtime Service
+ */
+struct efi_runtime_work {
+   void *arg1;
+   void *arg2;
+   void *arg3;
+   void *arg4;
+   void *arg5;
+   efi_status_t status;
+   struct work_struct work;
+   enum efi_rts_ids efi_rts_id;
+};
+
+/*
+ * efi_queue_work: Queue efi_runtime_service() and wait until it's done
+ * @rts:   efi_runtime_service() function identifier
+ * @rts_arg<1-5>:  efi_runtime_service() function arguments
+ *
+ * Accesses to efi_runtime_services() are serialized by a binary
+ * semaphore (efi_runtime_lock) and caller waits until the work is
+ * finished, hence _only_ one work is queued at a time and the queued
+ * work gets flushed.
+ */
+#define efi_queue_work(_rts, _arg1, _arg2, _arg3, _arg4, _arg5)\
+({ \
+   struct efi_runtime_work efi_rts_work;   \
+   efi_rts_work.status = EFI_ABORTED;  \
+   \
+   INIT_WORK_ONSTACK(_rts_work.work, efi_call_rts);\
+   efi_rts_work.arg1 = _arg1;