[U-Boot] efi_loader: fix events
Heinrich Schuchardt
xypron.glpk at gmx.de
Thu Sep 14 20:51:05 UTC 2017
On 09/12/2017 06:35 PM, Rob Clark wrote:
> An event can be created with type==0, Shell.efi does this for an event
> that is set when Ctrl-C is typed. So our current approach of having a
> fixed set of timer slots, and determining which slots are unused by
> type==0 doesn't work so well. But we don't have any particularly good
> reason to have a fixed table of events, so just dynamically allocate
> them and keep a list.
>
> Also fixes an incorrect implementation of CheckEvent() which was (a)
> incorrectly returning an error if type==0, and (b) didn't handle the
> case of an unsignaled event with a notify callback.
>
> With these fixes (plus implementation of SIMPLE_TEXT_INPUT_EX protocol),
> Ctrl-C works in Shell.efi.
>
> Signed-off-by: Rob Clark <robdclark at gmail.com>
> ---
> include/efi_loader.h | 1 +
> lib/efi_loader/efi_boottime.c | 167 +++++++++++++++++++-----------------------
> 2 files changed, 78 insertions(+), 90 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 291ea86568..9dc1ea4b88 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -149,6 +149,7 @@ struct efi_event {
> u64 trigger_time;
> enum efi_timer_delay trigger_type;
> int signaled;
> + struct list_head link;
Wouldn't we normally put this first in the structure?
> };
>
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 5db8ea9090..be3c34b9e0 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -255,11 +255,7 @@ static efi_status_t efi_create_handle(void **handle)
> return r;
> }
>
> -/*
> - * Our event capabilities are very limited. Only a small limited
> - * number of events is allowed to coexist.
> - */
> -static struct efi_event efi_events[16];
> +static LIST_HEAD(efi_events);
>
> efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl,
> void (EFIAPI *notify_function) (
> @@ -267,7 +263,7 @@ efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl,
> void *context),
> void *notify_context, struct efi_event **event)
> {
> - int i;
> + struct efi_event *evt;
>
> if (event == NULL)
> return EFI_INVALID_PARAMETER;
> @@ -279,20 +275,23 @@ efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl,
> notify_function == NULL)
> return EFI_INVALID_PARAMETER;
>
> - for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
> - if (efi_events[i].type)
> - continue;
> - efi_events[i].type = type;
> - efi_events[i].notify_tpl = notify_tpl;
> - efi_events[i].notify_function = notify_function;
> - efi_events[i].notify_context = notify_context;
> - /* Disable timers on bootup */
> - efi_events[i].trigger_next = -1ULL;
> - efi_events[i].signaled = 0;
> - *event = &efi_events[i];
> - return EFI_SUCCESS;
> - }
> - return EFI_OUT_OF_RESOURCES;
> + evt = calloc(1, sizeof(*evt));
> + if (!evt)
> + return EFI_OUT_OF_RESOURCES;
> +
> + evt->type = type;
> + evt->notify_tpl = notify_tpl;
> + evt->notify_function = notify_function;
> + evt->notify_context = notify_context;
> + /* Disable timers on bootup */
> + evt->trigger_next = -1ULL;
> + evt->signaled = 0;
> +
> + list_add_tail(&evt->link, &efi_events);
> +
> + *event = evt;
> +
> + return EFI_SUCCESS;
> }
>
> static efi_status_t EFIAPI efi_create_event_ext(
> @@ -315,21 +314,24 @@ static efi_status_t EFIAPI efi_create_event_ext(
> */
> void efi_timer_check(void)
> {
> - int i;
> + struct efi_event *evt;
> u64 now = timer_get_us();
>
> - for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
> - if (!efi_events[i].type ||
> - !(efi_events[i].type & EVT_TIMER) ||
> - efi_events[i].trigger_type == EFI_TIMER_STOP ||
> - now < efi_events[i].trigger_next)
> + /*
> + * TODO perhaps optimize a bit and track the time of next
> + * timer expiration?
> + */
The next expiration time is set below.
What do you want to change?
> + list_for_each_entry(evt, &efi_events, link) {
> + if (!evt->type || !(evt->type & EVT_TIMER) ||
> + evt->trigger_type == EFI_TIMER_STOP ||
> + now < evt->trigger_next)
> continue;
> - if (efi_events[i].trigger_type == EFI_TIMER_PERIODIC) {
> - efi_events[i].trigger_next +=
> - efi_events[i].trigger_time;
> - efi_events[i].signaled = 0;
> + if (evt->trigger_type == EFI_TIMER_PERIODIC) {
> + evt->trigger_next +=
> + evt->trigger_time;
> + evt->signaled = 0;
> }
> - efi_signal_event(&efi_events[i]);
> + efi_signal_event(evt);
> }
> WATCHDOG_RESET();
> }
> @@ -337,37 +339,32 @@ void efi_timer_check(void)
> efi_status_t efi_set_timer(struct efi_event *event, enum efi_timer_delay type,
> uint64_t trigger_time)
> {
> - int i;
> -
> /*
> * The parameter defines a multiple of 100ns.
> * We use multiples of 1000ns. So divide by 10.
> */
> trigger_time = efi_div10(trigger_time);
>
> - for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
> - if (event != &efi_events[i])
> - continue;
Please, check that event relates to an event in our list.
We do not want to accept an invalid pointer.
> + if (!(event->type & EVT_TIMER))
> + return EFI_INVALID_PARAMETER;
>
> - if (!(event->type & EVT_TIMER))
> - break;
> - switch (type) {
> - case EFI_TIMER_STOP:
> - event->trigger_next = -1ULL;
> - break;
> - case EFI_TIMER_PERIODIC:
> - case EFI_TIMER_RELATIVE:
> - event->trigger_next =
> + switch (type) {
> + case EFI_TIMER_STOP:
> + event->trigger_next = -1ULL;
> + break;
> + case EFI_TIMER_PERIODIC:
> + case EFI_TIMER_RELATIVE:
> + event->trigger_next =
> timer_get_us() + trigger_time;
> - break;
> - default:
> - return EFI_INVALID_PARAMETER;
> - }
> - event->trigger_type = type;
> - event->trigger_time = trigger_time;
> - return EFI_SUCCESS;
> + break;
> + default:
> + return EFI_INVALID_PARAMETER;
> }
> - return EFI_INVALID_PARAMETER;
> +
> + event->trigger_type = type;
> + event->trigger_time = trigger_time;
> +
> + return EFI_SUCCESS;
> }
>
> static efi_status_t EFIAPI efi_set_timer_ext(struct efi_event *event,
> @@ -382,7 +379,7 @@ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events,
> struct efi_event **event,
> unsigned long *index)
> {
> - int i, j;
> + int i;
>
> EFI_ENTRY("%ld, %p, %p", num_events, event, index);
>
> @@ -390,12 +387,6 @@ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events,
> if (!num_events || !event)
> return EFI_EXIT(EFI_INVALID_PARAMETER);
> for (i = 0; i < num_events; ++i) {
> - for (j = 0; j < ARRAY_SIZE(efi_events); ++j) {
> - if (event[i] == &efi_events[j])
> - goto known_event;
> - }
> - return EFI_EXIT(EFI_INVALID_PARAMETER);
> -known_event:
Check that the event pointer is in our list.
> if (!event[i]->type || event[i]->type & EVT_NOTIFY_SIGNAL)
> return EFI_EXIT(EFI_INVALID_PARAMETER);
> }
> @@ -424,50 +415,46 @@ out:
>
> static efi_status_t EFIAPI efi_signal_event_ext(struct efi_event *event)
> {
> - int i;
> -
> EFI_ENTRY("%p", event);
> - for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
> - if (event != &efi_events[i])
> - continue;
> - efi_signal_event(event);
> - break;
> - }
> + efi_signal_event(event);
> return EFI_EXIT(EFI_SUCCESS);
> }
>
> static efi_status_t EFIAPI efi_close_event(struct efi_event *event)
> {
> - int i;
> -
> EFI_ENTRY("%p", event);
> - for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
> - if (event == &efi_events[i]) {
> - event->type = 0;
> - event->trigger_next = -1ULL;
> - event->signaled = 0;
> - return EFI_EXIT(EFI_SUCCESS);
> - }
> - }
> - return EFI_EXIT(EFI_INVALID_PARAMETER);
Return an error code if the event was not in our list.
> + list_del(&event->link);
> + free(event);
> + return EFI_EXIT(EFI_SUCCESS);
> }
>
> +/*
> + * - If Event is in the signaled state, it is cleared and EFI_SUCCESS
> + * is returned.
> + *
> + * - If Event is not in the signaled state and has no notification
> + * function, EFI_NOT_READY is returned.
> + *
> + * - If Event is not in the signaled state but does have a notification
> + * function, the notification function is queued at the event’s
> + * notification task priority level. If the execution of the
> + * notification function causes Event to be signaled, then the signaled
> + * state is cleared and EFI_SUCCESS is returned; if the Event is not
> + * signaled, then EFI_NOT_READY is returned.
> + */
> static efi_status_t EFIAPI efi_check_event(struct efi_event *event)
> {
> - int i;
> -
> EFI_ENTRY("%p", event);
> efi_timer_check();
> - for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
> - if (event != &efi_events[i])
> - continue;
> - if (!event->type || event->type & EVT_NOTIFY_SIGNAL)
> - break;
> - if (event->signaled)
> - return EFI_EXIT(EFI_SUCCESS);
> - return EFI_EXIT(EFI_NOT_READY);
Check that the event is on our list.
> + if (event->type & EVT_NOTIFY_SIGNAL)
> + return EFI_EXIT(EFI_INVALID_PARAMETER);
> + if (!event->signaled && event->notify_function)
> + EFI_CALL(event->notify_function(event, event->notify_context));
> + if (event->signaled) {
> + event->signaled = 0;
> + return EFI_EXIT(EFI_SUCCESS);
> }
> - return EFI_EXIT(EFI_INVALID_PARAMETER);
> + return EFI_EXIT(EFI_NOT_READY);
> }
>
> static efi_status_t EFIAPI efi_install_protocol_interface(void **handle,
>
The changes make sense. But unfortunately the patch series is not
applicable to efi-next.
Either rebase it or provide information concerning the prerequisite patches.
Please, check in all functions that the event parameter relates to a
pointer on our list. Otherwise return EFI_INVALID_PARAMETER.
Best regards
Heinrich
More information about the U-Boot
mailing list