[{"id":1768833,"web_url":"http://patchwork.ozlabs.org/comment/1768833/","msgid":"<8c737e1d-9fd8-eec7-3148-b50bbc419a67@gmx.de>","list_archive_url":null,"date":"2017-09-14T20:51:05","subject":"Re: [U-Boot] efi_loader: fix events","submitter":{"id":61270,"url":"http://patchwork.ozlabs.org/api/people/61270/","name":"Heinrich Schuchardt","email":"xypron.glpk@gmx.de"},"content":"On 09/12/2017 06:35 PM, Rob Clark wrote:\n> An event can be created with type==0, Shell.efi does this for an event\n> that is set when Ctrl-C is typed.  So our current approach of having a\n> fixed set of timer slots, and determining which slots are unused by\n> type==0 doesn't work so well.  But we don't have any particularly good\n> reason to have a fixed table of events, so just dynamically allocate\n> them and keep a list.\n> \n> Also fixes an incorrect implementation of CheckEvent() which was (a)\n> incorrectly returning an error if type==0, and (b) didn't handle the\n> case of an unsignaled event with a notify callback.\n> \n> With these fixes (plus implementation of SIMPLE_TEXT_INPUT_EX protocol),\n> Ctrl-C works in Shell.efi.\n> \n> Signed-off-by: Rob Clark <robdclark@gmail.com>\n> ---\n>  include/efi_loader.h          |   1 +\n>  lib/efi_loader/efi_boottime.c | 167 +++++++++++++++++++-----------------------\n>  2 files changed, 78 insertions(+), 90 deletions(-)\n> \n> diff --git a/include/efi_loader.h b/include/efi_loader.h\n> index 291ea86568..9dc1ea4b88 100644\n> --- a/include/efi_loader.h\n> +++ b/include/efi_loader.h\n> @@ -149,6 +149,7 @@ struct efi_event {\n>  \tu64 trigger_time;\n>  \tenum efi_timer_delay trigger_type;\n>  \tint signaled;\n> +\tstruct list_head link;\n\nWouldn't we normally put this first in the structure?\n\n>  };\n>  \n>  \n> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c\n> index 5db8ea9090..be3c34b9e0 100644\n> --- a/lib/efi_loader/efi_boottime.c\n> +++ b/lib/efi_loader/efi_boottime.c\n> @@ -255,11 +255,7 @@ static efi_status_t efi_create_handle(void **handle)\n>  \treturn r;\n>  }\n>  \n> -/*\n> - * Our event capabilities are very limited. Only a small limited\n> - * number of events is allowed to coexist.\n> - */\n> -static struct efi_event efi_events[16];\n> +static LIST_HEAD(efi_events);\n>  \n>  efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl,\n>  \t\t\t      void (EFIAPI *notify_function) (\n> @@ -267,7 +263,7 @@ efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl,\n>  \t\t\t\t\tvoid *context),\n>  \t\t\t      void *notify_context, struct efi_event **event)\n>  {\n> -\tint i;\n> +\tstruct efi_event *evt;\n>  \n>  \tif (event == NULL)\n>  \t\treturn EFI_INVALID_PARAMETER;\n> @@ -279,20 +275,23 @@ efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl,\n>  \t    notify_function == NULL)\n>  \t\treturn EFI_INVALID_PARAMETER;\n>  \n> -\tfor (i = 0; i < ARRAY_SIZE(efi_events); ++i) {\n> -\t\tif (efi_events[i].type)\n> -\t\t\tcontinue;\n> -\t\tefi_events[i].type = type;\n> -\t\tefi_events[i].notify_tpl = notify_tpl;\n> -\t\tefi_events[i].notify_function = notify_function;\n> -\t\tefi_events[i].notify_context = notify_context;\n> -\t\t/* Disable timers on bootup */\n> -\t\tefi_events[i].trigger_next = -1ULL;\n> -\t\tefi_events[i].signaled = 0;\n> -\t\t*event = &efi_events[i];\n> -\t\treturn EFI_SUCCESS;\n> -\t}\n> -\treturn EFI_OUT_OF_RESOURCES;\n> +\tevt = calloc(1, sizeof(*evt));\n> +\tif (!evt)\n> +\t\treturn EFI_OUT_OF_RESOURCES;\n> +\n> +\tevt->type = type;\n> +\tevt->notify_tpl = notify_tpl;\n> +\tevt->notify_function = notify_function;\n> +\tevt->notify_context = notify_context;\n> +\t/* Disable timers on bootup */\n> +\tevt->trigger_next = -1ULL;\n> +\tevt->signaled = 0;\n> +\n> +\tlist_add_tail(&evt->link, &efi_events);\n> +\n> +\t*event = evt;\n> +\n> +\treturn EFI_SUCCESS;\n>  }\n>  \n>  static efi_status_t EFIAPI efi_create_event_ext(\n> @@ -315,21 +314,24 @@ static efi_status_t EFIAPI efi_create_event_ext(\n>   */\n>  void efi_timer_check(void)\n>  {\n> -\tint i;\n> +\tstruct efi_event *evt;\n>  \tu64 now = timer_get_us();\n>  \n> -\tfor (i = 0; i < ARRAY_SIZE(efi_events); ++i) {\n> -\t\tif (!efi_events[i].type ||\n> -\t\t    !(efi_events[i].type & EVT_TIMER) ||\n> -\t\t    efi_events[i].trigger_type == EFI_TIMER_STOP ||\n> -\t\t    now < efi_events[i].trigger_next)\n> +\t/*\n> +\t * TODO perhaps optimize a bit and track the time of next\n> +\t * timer expiration?\n> +\t */\n\nThe next expiration time is set below.\nWhat do you want to change?\n\n> +\tlist_for_each_entry(evt, &efi_events, link) {\n> +\t\tif (!evt->type || !(evt->type & EVT_TIMER) ||\n> +\t\t    evt->trigger_type == EFI_TIMER_STOP ||\n> +\t\t    now < evt->trigger_next)\n>  \t\t\tcontinue;\n> -\t\tif (efi_events[i].trigger_type == EFI_TIMER_PERIODIC) {\n> -\t\t\tefi_events[i].trigger_next +=\n> -\t\t\t\tefi_events[i].trigger_time;\n> -\t\t\tefi_events[i].signaled = 0;\n> +\t\tif (evt->trigger_type == EFI_TIMER_PERIODIC) {\n> +\t\t\tevt->trigger_next +=\n> +\t\t\t\tevt->trigger_time;\n> +\t\t\tevt->signaled = 0;\n>  \t\t}\n> -\t\tefi_signal_event(&efi_events[i]);\n> +\t\tefi_signal_event(evt);\n>  \t}\n>  \tWATCHDOG_RESET();\n>  }\n> @@ -337,37 +339,32 @@ void efi_timer_check(void)\n>  efi_status_t efi_set_timer(struct efi_event *event, enum efi_timer_delay type,\n>  \t\t\t   uint64_t trigger_time)\n>  {\n> -\tint i;\n> -\n>  \t/*\n>  \t * The parameter defines a multiple of 100ns.\n>  \t * We use multiples of 1000ns. So divide by 10.\n>  \t */\n>  \ttrigger_time = efi_div10(trigger_time);\n>  \n> -\tfor (i = 0; i < ARRAY_SIZE(efi_events); ++i) {\n> -\t\tif (event != &efi_events[i])\n> -\t\t\tcontinue;\n\nPlease, check that event relates to an event in our list.\nWe do not want to accept an invalid pointer.\n\n> +\tif (!(event->type & EVT_TIMER))\n> +\t\treturn EFI_INVALID_PARAMETER;\n>  \n> -\t\tif (!(event->type & EVT_TIMER))\n> -\t\t\tbreak;\n> -\t\tswitch (type) {\n> -\t\tcase EFI_TIMER_STOP:\n> -\t\t\tevent->trigger_next = -1ULL;\n> -\t\t\tbreak;\n> -\t\tcase EFI_TIMER_PERIODIC:\n> -\t\tcase EFI_TIMER_RELATIVE:\n> -\t\t\tevent->trigger_next =\n> +\tswitch (type) {\n> +\tcase EFI_TIMER_STOP:\n> +\t\tevent->trigger_next = -1ULL;\n> +\t\tbreak;\n> +\tcase EFI_TIMER_PERIODIC:\n> +\tcase EFI_TIMER_RELATIVE:\n> +\t\tevent->trigger_next =\n>  \t\t\t\ttimer_get_us() + trigger_time;\n> -\t\t\tbreak;\n> -\t\tdefault:\n> -\t\t\treturn EFI_INVALID_PARAMETER;\n> -\t\t}\n> -\t\tevent->trigger_type = type;\n> -\t\tevent->trigger_time = trigger_time;\n> -\t\treturn EFI_SUCCESS;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\treturn EFI_INVALID_PARAMETER;\n>  \t}\n> -\treturn EFI_INVALID_PARAMETER;\n> +\n> +\tevent->trigger_type = type;\n> +\tevent->trigger_time = trigger_time;\n> +\n> +\treturn EFI_SUCCESS;\n>  }\n>  \n>  static efi_status_t EFIAPI efi_set_timer_ext(struct efi_event *event,\n> @@ -382,7 +379,7 @@ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events,\n>  \t\t\t\t\t      struct efi_event **event,\n>  \t\t\t\t\t      unsigned long *index)\n>  {\n> -\tint i, j;\n> +\tint i;\n>  \n>  \tEFI_ENTRY(\"%ld, %p, %p\", num_events, event, index);\n>  \n> @@ -390,12 +387,6 @@ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events,\n>  \tif (!num_events || !event)\n>  \t\treturn EFI_EXIT(EFI_INVALID_PARAMETER);\n>  \tfor (i = 0; i < num_events; ++i) {\n> -\t\tfor (j = 0; j < ARRAY_SIZE(efi_events); ++j) {\n> -\t\t\tif (event[i] == &efi_events[j])\n> -\t\t\t\tgoto known_event;\n> -\t\t}\n> -\t\treturn EFI_EXIT(EFI_INVALID_PARAMETER);\n> -known_event:\n\nCheck that the event pointer is in our list.\n\n>  \t\tif (!event[i]->type || event[i]->type & EVT_NOTIFY_SIGNAL)\n>  \t\t\treturn EFI_EXIT(EFI_INVALID_PARAMETER);\n>  \t}\n> @@ -424,50 +415,46 @@ out:\n>  \n>  static efi_status_t EFIAPI efi_signal_event_ext(struct efi_event *event)\n>  {\n> -\tint i;\n> -\n>  \tEFI_ENTRY(\"%p\", event);\n> -\tfor (i = 0; i < ARRAY_SIZE(efi_events); ++i) {\n> -\t\tif (event != &efi_events[i])\n> -\t\t\tcontinue;\n> -\t\tefi_signal_event(event);\n> -\t\tbreak;\n> -\t}\n> +\tefi_signal_event(event);\n>  \treturn EFI_EXIT(EFI_SUCCESS);\n>  }\n>  \n>  static efi_status_t EFIAPI efi_close_event(struct efi_event *event)\n>  {\n> -\tint i;\n> -\n>  \tEFI_ENTRY(\"%p\", event);\n> -\tfor (i = 0; i < ARRAY_SIZE(efi_events); ++i) {\n> -\t\tif (event == &efi_events[i]) {\n> -\t\t\tevent->type = 0;\n> -\t\t\tevent->trigger_next = -1ULL;\n> -\t\t\tevent->signaled = 0;\n> -\t\t\treturn EFI_EXIT(EFI_SUCCESS);\n> -\t\t}\n> -\t}\n> -\treturn EFI_EXIT(EFI_INVALID_PARAMETER);\n\nReturn an error code if the event was not in our list.\n\n> +\tlist_del(&event->link);\n> +\tfree(event);\n> +\treturn EFI_EXIT(EFI_SUCCESS);\n>  }\n>  \n> +/*\n> + * - If Event is in the signaled state, it is cleared and EFI_SUCCESS\n> + *   is returned.\n> + *\n> + * - If Event is not in the signaled state and has no notification\n> + *   function, EFI_NOT_READY is returned.\n> + *\n> + * - If Event is not in the signaled state but does have a notification\n> + *   function, the notification function is queued at the event’s\n> + *   notification task priority level. If the execution of the\n> + *   notification function causes Event to be signaled, then the signaled\n> + *   state is cleared and EFI_SUCCESS is returned; if the Event is not\n> + *   signaled, then EFI_NOT_READY is returned.\n> + */\n>  static efi_status_t EFIAPI efi_check_event(struct efi_event *event)\n>  {\n> -\tint i;\n> -\n>  \tEFI_ENTRY(\"%p\", event);\n>  \tefi_timer_check();\n> -\tfor (i = 0; i < ARRAY_SIZE(efi_events); ++i) {\n> -\t\tif (event != &efi_events[i])\n> -\t\t\tcontinue;\n> -\t\tif (!event->type || event->type & EVT_NOTIFY_SIGNAL)\n> -\t\t\tbreak;\n> -\t\tif (event->signaled)\n> -\t\t\treturn EFI_EXIT(EFI_SUCCESS);\n> -\t\treturn EFI_EXIT(EFI_NOT_READY);\n\nCheck that the event is on our list.\n\n> +\tif (event->type & EVT_NOTIFY_SIGNAL)\n> +\t\treturn EFI_EXIT(EFI_INVALID_PARAMETER);\n> +\tif (!event->signaled && event->notify_function)\n> +\t\tEFI_CALL(event->notify_function(event, event->notify_context));\n> +\tif (event->signaled) {\n> +\t\tevent->signaled = 0;\n> +\t\treturn EFI_EXIT(EFI_SUCCESS);\n>  \t}\n> -\treturn EFI_EXIT(EFI_INVALID_PARAMETER);\n> +\treturn EFI_EXIT(EFI_NOT_READY);\n>  }\n>  \n>  static efi_status_t EFIAPI efi_install_protocol_interface(void **handle,\n> \n\nThe changes make sense. But unfortunately the patch series is not\napplicable to efi-next.\n\nEither rebase it or provide information concerning the prerequisite patches.\n\nPlease, check in all functions that the event parameter relates to a\npointer on our list. Otherwise return EFI_INVALID_PARAMETER.\n\nBest regards\n\nHeinrich","headers":{"Return-Path":"<u-boot-bounces@lists.denx.de>","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=<UNKNOWN>)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xtW0T6Hrmz9sCZ\n\tfor <incoming@patchwork.ozlabs.org>;\n\tFri, 15 Sep 2017 06:51:20 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 631AFC21EBE; Thu, 14 Sep 2017 20:51:16 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 1A0E7C21D5B;\n\tThu, 14 Sep 2017 20:51:13 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 749D9C21D5B; Thu, 14 Sep 2017 20:51:11 +0000 (UTC)","from mout.gmx.net (mout.gmx.net [212.227.15.18])\n\tby lists.denx.de (Postfix) with ESMTPS id 18911C21C6D\n\tfor <u-boot@lists.denx.de>; Thu, 14 Sep 2017 20:51:11 +0000 (UTC)","from [192.168.8.100] ([188.29.165.142]) by mail.gmx.com (mrgmx003\n\t[212.227.17.190]) with ESMTPSA (Nemesis) id\n\t0LwIuc-1dLv2w4Bzi-01864k; Thu, 14 Sep 2017 22:51:09 +0200"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-0.7 required=5.0 tests=FREEMAIL_FROM,\n\tRCVD_IN_DNSWL_LOW,\n\tRCVD_IN_MSPIKE_H2 autolearn=unavailable autolearn_force=no\n\tversion=3.4.0","To":"Rob Clark <robdclark@gmail.com>,\n\tU-Boot Mailing List <u-boot@lists.denx.de>","References":"<20170912163536.7349-1-robdclark@gmail.com>","From":"Heinrich Schuchardt <xypron.glpk@gmx.de>","Message-ID":"<8c737e1d-9fd8-eec7-3148-b50bbc419a67@gmx.de>","Date":"Thu, 14 Sep 2017 22:51:05 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170912163536.7349-1-robdclark@gmail.com>","Content-Language":"en-US","X-Provags-ID":"V03:K0:svacyhjgT1Vp20HrMdoQcl4UKFTwer1VwkP/iVmZC4lwtIGxOl1\n\tmroHpP1ONyB2nP59OyeUIyUZGCxxDD1HA9LhUvmyOISo7BAAhQgZ5uW6TWJi0vTAfmSipi1\n\ti43AAAigTofWCapE6NuPKc6COxzQVnYB0aKuVhWq5bY8lcMLNgPIhkCf6wlZueTssCtVPh9\n\tq8vXHP4hcMRBw9zT3whow==","X-UI-Out-Filterresults":"notjunk:1; V01:K0:HkZ1fq7D7xM=:C8zDptG62h1Q6rSttfklQ1\n\tHlZltJgDoPywthNHWYAT0HvhlmYWdQDFt8TsOTzXQC4ngUKkLGDIrhTEBmptxU3LijfToWUu0\n\t/M1svd8xFz2Aj/I4A4EHiK5yqr6OsC9FtRoVPMwopJs02ypxeVWJPxhITRrm/1R2whdpwsXeT\n\tO65xq/vgHzN22MmShqn1eCWBHEXoQBbf6YwBRLGgSa4P51tQi/ROoDMvgM71WtmnrMm4ndPvz\n\t406wturPnyLrqeqiCG90yTYteh8RaF8WNQQe1Q1RQyYE2nNR8ETCEb0/IJkSPoQTS+1Cc8aFR\n\tL20XOYdwqRU02W7Keyr9qe678IgRHEDDxevC0/hnLCN4B2YtJ3mqiIVUKwGoMfTFzkxabpOlW\n\td1AHP4thQhkxOkISFkf6o6sMysrzXxzSO+evukgCNdh2AI6lz9L6jRgfbqqv4DKcT3Vmz97y8\n\tSjKFYdCJXAuP7zThW7zCc4Tql9JFtjJ52jwKsLweEnLqsoB0Pw3+Yv9Vo3udAd7VR6Q0+a2Jg\n\tmq4ih6wMg+8Gbtrq4ZVuFdrN17HF6SLzOQsL3dh5K3i7osXrj1rcPlRx+iCEGwFh9dDT9TOP5\n\tZJN+T+z2GjIX8gZBrYXeY1cwh16HeNzwCTjLVy5hchJ3mMYI0nlrb20QPfjVVcWRJ6TnhEyaW\n\tuinlzcGCE9XCDJNuYB4Qd6+niFwq5Q7lBgbejWGlLrj8aTG2zT+KgXc/YLcy04pAw9QW30Mrr\n\tJkc1GBSLO8gUDfbHZ4hTnbmhctbN5jUyF/k3HKgK7L9Lk9G+9RaEDU8alCNrCXwJMzSc2bhl7\n\tmFuDyApjyG/Q7Z2BfYVmnRklH1M78xU7tq1kYxQDdJlUuXVXnE=","Subject":"Re: [U-Boot] efi_loader: fix events","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion <u-boot.lists.denx.de>","List-Unsubscribe":"<https://lists.denx.de/options/u-boot>,\n\t<mailto:u-boot-request@lists.denx.de?subject=unsubscribe>","List-Archive":"<http://lists.denx.de/pipermail/u-boot/>","List-Post":"<mailto:u-boot@lists.denx.de>","List-Help":"<mailto:u-boot-request@lists.denx.de?subject=help>","List-Subscribe":"<https://lists.denx.de/listinfo/u-boot>,\n\t<mailto:u-boot-request@lists.denx.de?subject=subscribe>","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" <u-boot-bounces@lists.denx.de>"}},{"id":1768839,"web_url":"http://patchwork.ozlabs.org/comment/1768839/","msgid":"<CAF6AEGv6YE71AdrHGcXqXVR2yfDA0LokCDJZiLg9v=WnSHf8xQ@mail.gmail.com>","list_archive_url":null,"date":"2017-09-14T21:14:06","subject":"Re: [U-Boot] efi_loader: fix events","submitter":{"id":18760,"url":"http://patchwork.ozlabs.org/api/people/18760/","name":"Rob Clark","email":"robdclark@gmail.com"},"content":"On Thu, Sep 14, 2017 at 4:51 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:\n> On 09/12/2017 06:35 PM, Rob Clark wrote:\n>> An event can be created with type==0, Shell.efi does this for an event\n>> that is set when Ctrl-C is typed.  So our current approach of having a\n>> fixed set of timer slots, and determining which slots are unused by\n>> type==0 doesn't work so well.  But we don't have any particularly good\n>> reason to have a fixed table of events, so just dynamically allocate\n>> them and keep a list.\n>>\n>> Also fixes an incorrect implementation of CheckEvent() which was (a)\n>> incorrectly returning an error if type==0, and (b) didn't handle the\n>> case of an unsignaled event with a notify callback.\n>>\n>> With these fixes (plus implementation of SIMPLE_TEXT_INPUT_EX protocol),\n>> Ctrl-C works in Shell.efi.\n>>\n>> Signed-off-by: Rob Clark <robdclark@gmail.com>\n>> ---\n>>  include/efi_loader.h          |   1 +\n>>  lib/efi_loader/efi_boottime.c | 167 +++++++++++++++++++-----------------------\n>>  2 files changed, 78 insertions(+), 90 deletions(-)\n>>\n>> diff --git a/include/efi_loader.h b/include/efi_loader.h\n>> index 291ea86568..9dc1ea4b88 100644\n>> --- a/include/efi_loader.h\n>> +++ b/include/efi_loader.h\n>> @@ -149,6 +149,7 @@ struct efi_event {\n>>       u64 trigger_time;\n>>       enum efi_timer_delay trigger_type;\n>>       int signaled;\n>> +     struct list_head link;\n>\n> Wouldn't we normally put this first in the structure?\n\nIt should not be required to be first, and anything that requires it\nto be first is a bug.. you could have an object in multiple different\nlists, for example.\n\n>>  };\n>>\n>>\n>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c\n>> index 5db8ea9090..be3c34b9e0 100644\n>> --- a/lib/efi_loader/efi_boottime.c\n>> +++ b/lib/efi_loader/efi_boottime.c\n>> @@ -255,11 +255,7 @@ static efi_status_t efi_create_handle(void **handle)\n>>       return r;\n>>  }\n>>\n>> -/*\n>> - * Our event capabilities are very limited. Only a small limited\n>> - * number of events is allowed to coexist.\n>> - */\n>> -static struct efi_event efi_events[16];\n>> +static LIST_HEAD(efi_events);\n>>\n>>  efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl,\n>>                             void (EFIAPI *notify_function) (\n>> @@ -267,7 +263,7 @@ efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl,\n>>                                       void *context),\n>>                             void *notify_context, struct efi_event **event)\n>>  {\n>> -     int i;\n>> +     struct efi_event *evt;\n>>\n>>       if (event == NULL)\n>>               return EFI_INVALID_PARAMETER;\n>> @@ -279,20 +275,23 @@ efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl,\n>>           notify_function == NULL)\n>>               return EFI_INVALID_PARAMETER;\n>>\n>> -     for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {\n>> -             if (efi_events[i].type)\n>> -                     continue;\n>> -             efi_events[i].type = type;\n>> -             efi_events[i].notify_tpl = notify_tpl;\n>> -             efi_events[i].notify_function = notify_function;\n>> -             efi_events[i].notify_context = notify_context;\n>> -             /* Disable timers on bootup */\n>> -             efi_events[i].trigger_next = -1ULL;\n>> -             efi_events[i].signaled = 0;\n>> -             *event = &efi_events[i];\n>> -             return EFI_SUCCESS;\n>> -     }\n>> -     return EFI_OUT_OF_RESOURCES;\n>> +     evt = calloc(1, sizeof(*evt));\n>> +     if (!evt)\n>> +             return EFI_OUT_OF_RESOURCES;\n>> +\n>> +     evt->type = type;\n>> +     evt->notify_tpl = notify_tpl;\n>> +     evt->notify_function = notify_function;\n>> +     evt->notify_context = notify_context;\n>> +     /* Disable timers on bootup */\n>> +     evt->trigger_next = -1ULL;\n>> +     evt->signaled = 0;\n>> +\n>> +     list_add_tail(&evt->link, &efi_events);\n>> +\n>> +     *event = evt;\n>> +\n>> +     return EFI_SUCCESS;\n>>  }\n>>\n>>  static efi_status_t EFIAPI efi_create_event_ext(\n>> @@ -315,21 +314,24 @@ static efi_status_t EFIAPI efi_create_event_ext(\n>>   */\n>>  void efi_timer_check(void)\n>>  {\n>> -     int i;\n>> +     struct efi_event *evt;\n>>       u64 now = timer_get_us();\n>>\n>> -     for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {\n>> -             if (!efi_events[i].type ||\n>> -                 !(efi_events[i].type & EVT_TIMER) ||\n>> -                 efi_events[i].trigger_type == EFI_TIMER_STOP ||\n>> -                 now < efi_events[i].trigger_next)\n>> +     /*\n>> +      * TODO perhaps optimize a bit and track the time of next\n>> +      * timer expiration?\n>> +      */\n>\n> The next expiration time is set below.\n> What do you want to change?\n\nI meant a global value for whichever is next timer to expire.. so\nefi_timer_check() could have a fast-path.\n\nbackground: I was thinking about adding efi_timer_check() to\nEFI_ENTRY() and/or EFI_EXIT().. something I was playing around with\nlocally but havent turned into something fully baked yet.  But I guess\nif we call efi_timer_check() so often we don't want to loop thru all\nthe events each time if there is nothing ready to expire.\n\nPossibly I could word that TODO comment better, it was mostly a note for myself.\n\n>> +     list_for_each_entry(evt, &efi_events, link) {\n>> +             if (!evt->type || !(evt->type & EVT_TIMER) ||\n>> +                 evt->trigger_type == EFI_TIMER_STOP ||\n>> +                 now < evt->trigger_next)\n>>                       continue;\n>> -             if (efi_events[i].trigger_type == EFI_TIMER_PERIODIC) {\n>> -                     efi_events[i].trigger_next +=\n>> -                             efi_events[i].trigger_time;\n>> -                     efi_events[i].signaled = 0;\n>> +             if (evt->trigger_type == EFI_TIMER_PERIODIC) {\n>> +                     evt->trigger_next +=\n>> +                             evt->trigger_time;\n>> +                     evt->signaled = 0;\n>>               }\n>> -             efi_signal_event(&efi_events[i]);\n>> +             efi_signal_event(evt);\n>>       }\n>>       WATCHDOG_RESET();\n>>  }\n>> @@ -337,37 +339,32 @@ void efi_timer_check(void)\n>>  efi_status_t efi_set_timer(struct efi_event *event, enum efi_timer_delay type,\n>>                          uint64_t trigger_time)\n>>  {\n>> -     int i;\n>> -\n>>       /*\n>>        * The parameter defines a multiple of 100ns.\n>>        * We use multiples of 1000ns. So divide by 10.\n>>        */\n>>       trigger_time = efi_div10(trigger_time);\n>>\n>> -     for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {\n>> -             if (event != &efi_events[i])\n>> -                     continue;\n>\n> Please, check that event relates to an event in our list.\n> We do not want to accept an invalid pointer.\n\nbut efi payload passing bogus pointer into EFI API is a generic\nproblem, not something specific to events.\n\nWe could add some type checking across the board by doing something\nlike adding a type-signature as first element in each object and\nchecking that the first (for example) four bytes match what we expect.\nI vaguely recall seeing some code in edk2 (I think?) doing something\nalong those lines.  Perhaps a useful thing to do, but I think that\nshould apply for all object types, and not be something event\nspecific.\n\nAnd we can't even do that for things like file objects, where the efi\npayload expects to dereference fxn ptrs in the struct.. maybe we can\nstuff the type signature *before* the beginning of the object??\n\nAnyways, I don't really believe that check that I dropped adds\nanything of value.\n\n>> +     if (!(event->type & EVT_TIMER))\n>> +             return EFI_INVALID_PARAMETER;\n>>\n>> -             if (!(event->type & EVT_TIMER))\n>> -                     break;\n>> -             switch (type) {\n>> -             case EFI_TIMER_STOP:\n>> -                     event->trigger_next = -1ULL;\n>> -                     break;\n>> -             case EFI_TIMER_PERIODIC:\n>> -             case EFI_TIMER_RELATIVE:\n>> -                     event->trigger_next =\n>> +     switch (type) {\n>> +     case EFI_TIMER_STOP:\n>> +             event->trigger_next = -1ULL;\n>> +             break;\n>> +     case EFI_TIMER_PERIODIC:\n>> +     case EFI_TIMER_RELATIVE:\n>> +             event->trigger_next =\n>>                               timer_get_us() + trigger_time;\n>> -                     break;\n>> -             default:\n>> -                     return EFI_INVALID_PARAMETER;\n>> -             }\n>> -             event->trigger_type = type;\n>> -             event->trigger_time = trigger_time;\n>> -             return EFI_SUCCESS;\n>> +             break;\n>> +     default:\n>> +             return EFI_INVALID_PARAMETER;\n>>       }\n>> -     return EFI_INVALID_PARAMETER;\n>> +\n>> +     event->trigger_type = type;\n>> +     event->trigger_time = trigger_time;\n>> +\n>> +     return EFI_SUCCESS;\n>>  }\n>>\n>>  static efi_status_t EFIAPI efi_set_timer_ext(struct efi_event *event,\n>> @@ -382,7 +379,7 @@ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events,\n>>                                             struct efi_event **event,\n>>                                             unsigned long *index)\n>>  {\n>> -     int i, j;\n>> +     int i;\n>>\n>>       EFI_ENTRY(\"%ld, %p, %p\", num_events, event, index);\n>>\n>> @@ -390,12 +387,6 @@ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events,\n>>       if (!num_events || !event)\n>>               return EFI_EXIT(EFI_INVALID_PARAMETER);\n>>       for (i = 0; i < num_events; ++i) {\n>> -             for (j = 0; j < ARRAY_SIZE(efi_events); ++j) {\n>> -                     if (event[i] == &efi_events[j])\n>> -                             goto known_event;\n>> -             }\n>> -             return EFI_EXIT(EFI_INVALID_PARAMETER);\n>> -known_event:\n>\n> Check that the event pointer is in our list.\n>\n>>               if (!event[i]->type || event[i]->type & EVT_NOTIFY_SIGNAL)\n>>                       return EFI_EXIT(EFI_INVALID_PARAMETER);\n>>       }\n>> @@ -424,50 +415,46 @@ out:\n>>\n>>  static efi_status_t EFIAPI efi_signal_event_ext(struct efi_event *event)\n>>  {\n>> -     int i;\n>> -\n>>       EFI_ENTRY(\"%p\", event);\n>> -     for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {\n>> -             if (event != &efi_events[i])\n>> -                     continue;\n>> -             efi_signal_event(event);\n>> -             break;\n>> -     }\n>> +     efi_signal_event(event);\n>>       return EFI_EXIT(EFI_SUCCESS);\n>>  }\n>>\n>>  static efi_status_t EFIAPI efi_close_event(struct efi_event *event)\n>>  {\n>> -     int i;\n>> -\n>>       EFI_ENTRY(\"%p\", event);\n>> -     for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {\n>> -             if (event == &efi_events[i]) {\n>> -                     event->type = 0;\n>> -                     event->trigger_next = -1ULL;\n>> -                     event->signaled = 0;\n>> -                     return EFI_EXIT(EFI_SUCCESS);\n>> -             }\n>> -     }\n>> -     return EFI_EXIT(EFI_INVALID_PARAMETER);\n>\n> Return an error code if the event was not in our list.\n>\n>> +     list_del(&event->link);\n>> +     free(event);\n>> +     return EFI_EXIT(EFI_SUCCESS);\n>>  }\n>>\n>> +/*\n>> + * - If Event is in the signaled state, it is cleared and EFI_SUCCESS\n>> + *   is returned.\n>> + *\n>> + * - If Event is not in the signaled state and has no notification\n>> + *   function, EFI_NOT_READY is returned.\n>> + *\n>> + * - If Event is not in the signaled state but does have a notification\n>> + *   function, the notification function is queued at the event’s\n>> + *   notification task priority level. If the execution of the\n>> + *   notification function causes Event to be signaled, then the signaled\n>> + *   state is cleared and EFI_SUCCESS is returned; if the Event is not\n>> + *   signaled, then EFI_NOT_READY is returned.\n>> + */\n>>  static efi_status_t EFIAPI efi_check_event(struct efi_event *event)\n>>  {\n>> -     int i;\n>> -\n>>       EFI_ENTRY(\"%p\", event);\n>>       efi_timer_check();\n>> -     for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {\n>> -             if (event != &efi_events[i])\n>> -                     continue;\n>> -             if (!event->type || event->type & EVT_NOTIFY_SIGNAL)\n>> -                     break;\n>> -             if (event->signaled)\n>> -                     return EFI_EXIT(EFI_SUCCESS);\n>> -             return EFI_EXIT(EFI_NOT_READY);\n>\n> Check that the event is on our list.\n>\n>> +     if (event->type & EVT_NOTIFY_SIGNAL)\n>> +             return EFI_EXIT(EFI_INVALID_PARAMETER);\n>> +     if (!event->signaled && event->notify_function)\n>> +             EFI_CALL(event->notify_function(event, event->notify_context));\n>> +     if (event->signaled) {\n>> +             event->signaled = 0;\n>> +             return EFI_EXIT(EFI_SUCCESS);\n>>       }\n>> -     return EFI_EXIT(EFI_INVALID_PARAMETER);\n>> +     return EFI_EXIT(EFI_NOT_READY);\n>>  }\n>>\n>>  static efi_status_t EFIAPI efi_install_protocol_interface(void **handle,\n>>\n>\n> The changes make sense. But unfortunately the patch series is not\n> applicable to efi-next.\n>\n> Either rebase it or provide information concerning the prerequisite patches.\n\natm I can't really rebase my stack of patches on efi-next until the\nreaddir patches land and efi-next is rebased.  Once that happens I'll\nrebase on efi-next.  (Hopefully that happens before next Tues since\nlatter part of next week I probably won't have an opportunity to test\nthe rebase on real hw.)\n\nBR,\n-R\n\n> Please, check in all functions that the event parameter relates to a\n> pointer on our list. Otherwise return EFI_INVALID_PARAMETER.\n>\n> Best regards\n>\n> Heinrich","headers":{"Return-Path":"<u-boot-bounces@lists.denx.de>","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=<UNKNOWN>)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"W0529zBY\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xtWVv1jNfz9s7h\n\tfor <incoming@patchwork.ozlabs.org>;\n\tFri, 15 Sep 2017 07:14:15 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 00CE4C21E26; Thu, 14 Sep 2017 21:14:13 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 17D1DC21CB1;\n\tThu, 14 Sep 2017 21:14:09 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 312A5C21CB1; Thu, 14 Sep 2017 21:14:08 +0000 (UTC)","from mail-lf0-f65.google.com (mail-lf0-f65.google.com\n\t[209.85.215.65])\n\tby lists.denx.de (Postfix) with ESMTPS id B25B1C21C6D\n\tfor <u-boot@lists.denx.de>; Thu, 14 Sep 2017 21:14:07 +0000 (UTC)","by mail-lf0-f65.google.com with SMTP id y15so279677lfd.0\n\tfor <u-boot@lists.denx.de>; Thu, 14 Sep 2017 14:14:07 -0700 (PDT)","by 10.46.41.75 with HTTP; Thu, 14 Sep 2017 14:14:06 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-0.0 required=5.0 tests=FREEMAIL_FROM,\n\tRCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,\n\tT_DKIM_INVALID\n\tautolearn=unavailable autolearn_force=no version=3.4.0","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=ZCtHz0xE/QwoKaHw0nuudSPq0MVpJPysgvogTTNhHrY=;\n\tb=W0529zBYvOIUKjf5WV7DbzwWEr1h3a2Lkf2i3lh7nxGUA60jbegqhWtiA//jfxpaOc\n\tbH5sNI/rsGiAO32atm+Jo0UAOy6zYQRZVX1zyR3wRTHJkfFVqgJM7pjKQRnDk2ktMxX9\n\tg00P8M5FbmFVorZiS/Z5g4TK9OB+SmT6r5c3r+wJbtz2Wp6GM0A4Q5xqsb1RpqsCuZ13\n\tq1+PQDEQFgoxNWYZZNCwcibfWFn93SbF4dZB7IqRkM9NMuaOoc7FKJPYXBXWckakoSXZ\n\tNmzkta0GE8TmF0XQjzPGiXfcj+hUT2QlcE2mn31ApjnZALzbylqDagWY1z38xvGJ4piF\n\tyk7w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=ZCtHz0xE/QwoKaHw0nuudSPq0MVpJPysgvogTTNhHrY=;\n\tb=YxcjXG8MbeNY/vg09a0k5xVyIDayKHRszNxA58Anta0Ey5jDr8bKh/tNytQ0LIcaYi\n\tZcG5i9Z3Z4ut1sYnEwor2vmzMTdGUpl/sErtGtlJD0FNfJl7PGwH5SKH1wmY/8ZVB/xZ\n\tm1QDXQXO7zEk+3n13n2oscl0A9q2cQaBx9T1IYBx96djZXiauztAaQDuBlvqKOob06aq\n\tQGjV1kSwM00HB7Vjdj3gOc4bho6Y0DD9GaX5qYHPNxbcx/IccAy18jLv/7bix9Ul4zeC\n\t5F4XGujUMymDZCAq/zp207xnDCXFKZasjhlP1u/pd0DUiTTIOYlb8WX3C0v3Y6HYkIDP\n\tUavA==","X-Gm-Message-State":"AHPjjUjYWQ9uTBw0gX2ScPP1NHryb9h8Ss4ooCtWte/LCsxAFfcBxaO+\n\tkx4OsG534pIxKgcHjdaW+LLk7UiLL5HNkxTPNE4=","X-Google-Smtp-Source":"AOwi7QC/IYs8PIQaLr30/pQZ7b6PgZwiAqgliuOKhEPLLrkGDyH3vVWX3smp94hbkenlPo65Rl2fK1bmWpn0UnE6fao=","X-Received":"by 10.25.78.153 with SMTP id u25mr747288lfk.111.1505423646978;\n\tThu, 14 Sep 2017 14:14:06 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<8c737e1d-9fd8-eec7-3148-b50bbc419a67@gmx.de>","References":"<20170912163536.7349-1-robdclark@gmail.com>\n\t<8c737e1d-9fd8-eec7-3148-b50bbc419a67@gmx.de>","From":"Rob Clark <robdclark@gmail.com>","Date":"Thu, 14 Sep 2017 17:14:06 -0400","Message-ID":"<CAF6AEGv6YE71AdrHGcXqXVR2yfDA0LokCDJZiLg9v=WnSHf8xQ@mail.gmail.com>","To":"Heinrich Schuchardt <xypron.glpk@gmx.de>","Cc":"U-Boot Mailing List <u-boot@lists.denx.de>","Subject":"Re: [U-Boot] efi_loader: fix events","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion <u-boot.lists.denx.de>","List-Unsubscribe":"<https://lists.denx.de/options/u-boot>,\n\t<mailto:u-boot-request@lists.denx.de?subject=unsubscribe>","List-Archive":"<http://lists.denx.de/pipermail/u-boot/>","List-Post":"<mailto:u-boot@lists.denx.de>","List-Help":"<mailto:u-boot-request@lists.denx.de?subject=help>","List-Subscribe":"<https://lists.denx.de/listinfo/u-boot>,\n\t<mailto:u-boot-request@lists.denx.de?subject=subscribe>","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" <u-boot-bounces@lists.denx.de>"}}]