| Message ID | 20171004130324.20673-1-xypron.glpk@gmx.de |
|---|---|
| State | Accepted |
| Commit | e190e8972faf4d5b09e2a92fefc54a1832fd67da |
| Headers | show |
| Series | [U-Boot,1/1] efi_loader: use type bool for event states | expand |
On Wed, Oct 4, 2017 at 9:03 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > Queued and signaled describe boolean states of events. > So let's use type bool and rename the structure members to is_queued > and is_signaled. > > Update the comments for is_queued and is_signaled. Reviewed-by: Rob Clark <robdclark@gmail.com> It would be kinda nice to merge my efi_event fixes and rework to use an arbitrary sized list of events before making too many more efi_event changes, since that is kind of annoying to keep rebasing ;-) BR, -R > Reported-by: Simon Glass <sjg@chromium.org> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > include/efi_loader.h | 8 ++++---- > lib/efi_loader/efi_boottime.c | 32 ++++++++++++++++---------------- > lib/efi_loader/efi_console.c | 2 +- > 3 files changed, 21 insertions(+), 21 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 2f081f8996..1148db2b00 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -136,8 +136,8 @@ struct efi_object { > * @nofify_function: Function to call when the event is triggered > * @notify_context: Data to be passed to the notify function > * @trigger_type: Type of timer, see efi_set_timer > - * @queued: The notification functionis queued > - * @signaled: The event occured > + * @queued: The notification function is queued > + * @signaled: The event occurred. The event is in the signaled state. > */ > struct efi_event { > uint32_t type; > @@ -147,8 +147,8 @@ struct efi_event { > u64 trigger_next; > u64 trigger_time; > enum efi_timer_delay trigger_type; > - int queued; > - int signaled; > + bool is_queued; > + bool is_signaled; > }; > > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > index d63c66dd45..8975fc4ec1 100644 > --- a/lib/efi_loader/efi_boottime.c > +++ b/lib/efi_loader/efi_boottime.c > @@ -132,14 +132,14 @@ const char *__efi_nesting_dec(void) > void efi_signal_event(struct efi_event *event) > { > if (event->notify_function) { > - event->queued = 1; > + event->is_queued = true; > /* Check TPL */ > if (efi_tpl >= event->notify_tpl) > return; > EFI_CALL_VOID(event->notify_function(event, > event->notify_context)); > } > - event->queued = 0; > + event->is_queued = false; > } > > static efi_status_t efi_unsupported(const char *funcname) > @@ -267,8 +267,8 @@ efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl, > efi_events[i].notify_context = notify_context; > /* Disable timers on bootup */ > efi_events[i].trigger_next = -1ULL; > - efi_events[i].queued = 0; > - efi_events[i].signaled = 0; > + efi_events[i].is_queued = false; > + efi_events[i].is_signaled = false; > *event = &efi_events[i]; > return EFI_SUCCESS; > } > @@ -301,7 +301,7 @@ void efi_timer_check(void) > for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { > if (!efi_events[i].type) > continue; > - if (efi_events[i].queued) > + if (efi_events[i].is_queued) > efi_signal_event(&efi_events[i]); > if (!(efi_events[i].type & EVT_TIMER) || > now < efi_events[i].trigger_next) > @@ -317,7 +317,7 @@ void efi_timer_check(void) > default: > continue; > } > - efi_events[i].signaled = 1; > + efi_events[i].is_signaled = true; > efi_signal_event(&efi_events[i]); > } > WATCHDOG_RESET(); > @@ -354,7 +354,7 @@ efi_status_t efi_set_timer(struct efi_event *event, enum efi_timer_delay type, > } > event->trigger_type = type; > event->trigger_time = trigger_time; > - event->signaled = 0; > + event->is_signaled = false; > return EFI_SUCCESS; > } > return EFI_INVALID_PARAMETER; > @@ -391,14 +391,14 @@ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events, > known_event: > if (!event[i]->type || event[i]->type & EVT_NOTIFY_SIGNAL) > return EFI_EXIT(EFI_INVALID_PARAMETER); > - if (!event[i]->signaled) > + if (!event[i]->is_signaled) > efi_signal_event(event[i]); > } > > /* Wait for signal */ > for (;;) { > for (i = 0; i < num_events; ++i) { > - if (event[i]->signaled) > + if (event[i]->is_signaled) > goto out; > } > /* Allow events to occur. */ > @@ -410,7 +410,7 @@ out: > * Reset the signal which is passed to the caller to allow periodic > * events to occur. > */ > - event[i]->signaled = 0; > + event[i]->is_signaled = false; > if (index) > *index = i; > > @@ -425,9 +425,9 @@ static efi_status_t EFIAPI efi_signal_event_ext(struct efi_event *event) > for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { > if (event != &efi_events[i]) > continue; > - if (event->signaled) > + if (event->is_signaled) > break; > - event->signaled = 1; > + event->is_signaled = true; > if (event->type & EVT_NOTIFY_SIGNAL) > efi_signal_event(event); > break; > @@ -444,8 +444,8 @@ static efi_status_t EFIAPI efi_close_event(struct efi_event *event) > if (event == &efi_events[i]) { > event->type = 0; > event->trigger_next = -1ULL; > - event->queued = 0; > - event->signaled = 0; > + event->is_queued = false; > + event->is_signaled = false; > return EFI_EXIT(EFI_SUCCESS); > } > } > @@ -463,9 +463,9 @@ static efi_status_t EFIAPI efi_check_event(struct efi_event *event) > continue; > if (!event->type || event->type & EVT_NOTIFY_SIGNAL) > break; > - if (!event->signaled) > + if (!event->is_signaled) > efi_signal_event(event); > - if (event->signaled) > + if (event->is_signaled) > return EFI_EXIT(EFI_SUCCESS); > return EFI_EXIT(EFI_NOT_READY); > } > diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c > index fd5398d61d..1bdf36b4ae 100644 > --- a/lib/efi_loader/efi_console.c > +++ b/lib/efi_loader/efi_console.c > @@ -460,7 +460,7 @@ static void EFIAPI efi_console_timer_notify(struct efi_event *event, > { > EFI_ENTRY("%p, %p", event, context); > if (tstc()) { > - efi_con_in.wait_for_key->signaled = 1; > + efi_con_in.wait_for_key->is_signaled = true; > efi_signal_event(efi_con_in.wait_for_key); > } > EFI_EXIT(EFI_SUCCESS); > -- > 2.14.1 >
On 10/04/2017 04:14 PM, Rob Clark wrote: > On Wed, Oct 4, 2017 at 9:03 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> Queued and signaled describe boolean states of events. >> So let's use type bool and rename the structure members to is_queued >> and is_signaled. >> >> Update the comments for is_queued and is_signaled. > > Reviewed-by: Rob Clark <robdclark@gmail.com> > > It would be kinda nice to merge my efi_event fixes and rework to use > an arbitrary sized list of events before making too many more > efi_event changes, since that is kind of annoying to keep rebasing ;-) > > BR, > -R I would not mind if you patch went first. But your patch https://patchwork.ozlabs.org/patch/812967/ is not applicable to U-Boot master and needs rebasing anyway. Please, add the missing check that the event pointer is valid. The EFI code checks other arguments rigorously so we should do the same for pointers. It would be very hard to debug a problem in an EFI application otherwise. @Alexander I guess with Suseconf you were quite busy in the last weeks. Did you already make up your mind in which sequence we should prepare the EFI patches? The following of my patches could be directly applied to efi-next: [U-Boot,1/1] efi_selftest: enable CONFIG_CMD_BOOTEFI_SELFTEST https://patchwork.ozlabs.org/patch/816412/ [U-Boot,1/1] efi_loader: replace efi_div10 by div64_u64 https://patchwork.ozlabs.org/patch/820731/ [U-Boot,1/1] efi_selftest: use efi_st_error for all error messages https://patchwork.ozlabs.org/patch/821237/ [U-Boot,1/1] efi_loader: use type bool for event states https://patchwork.ozlabs.org/patch/821302/ [U-Boot,v2,1/1] efi_selftest: make tests easier to read https://patchwork.ozlabs.org/patch/821306/ I guess Rob was refering to [U-Boot,1/1] efi_loader: use type bool for event states Best regards Heinrich
On Wed, Oct 4, 2017 at 10:46 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 10/04/2017 04:14 PM, Rob Clark wrote: >> On Wed, Oct 4, 2017 at 9:03 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>> Queued and signaled describe boolean states of events. >>> So let's use type bool and rename the structure members to is_queued >>> and is_signaled. >>> >>> Update the comments for is_queued and is_signaled. >> >> Reviewed-by: Rob Clark <robdclark@gmail.com> >> >> It would be kinda nice to merge my efi_event fixes and rework to use >> an arbitrary sized list of events before making too many more >> efi_event changes, since that is kind of annoying to keep rebasing ;-) >> >> BR, >> -R > > I would not mind if you patch went first. > > But your patch > https://patchwork.ozlabs.org/patch/812967/ > is not applicable to U-Boot master and needs rebasing anyway. jfyi, I have it (and other pending patches) rebased on latest master (as of ~yesterday) here: https://github.com/robclark/u-boot/commits/wip-enough-uefi-for-shell I wasn't planning on resending until I get further with FAT write stuff (currently on a local branch, although I might not get much time to work on in the next week or two).. although I can re-send it or any of the other patches to get Shell.efi working if wanted. (Note that I'm also using your patch for efi watchdog support, that was one of the other required bits.) Not sure what agraf's plan is but I think the needed bits for Shell.efi are mergable already. > Please, add the missing check that the event pointer is valid. > The EFI code checks other arguments rigorously so we should do the same > for pointers. It would be very hard to debug a problem in an EFI > application otherwise. I'm a bit undecided on this, since we have other places where there is no good way to check the validity of a pointer. (For example file or disk objects.) I was thinking about perhaps implementing a compile-time optional feature using a hashtable to map objects to type so we can add in some type checking, at the expense of extra runtime overhead. Probably not something you'd want to ship enabled, but it would be useful for debugging. BR, -R > @Alexander > I guess with Suseconf you were quite busy in the last weeks. Did you > already make up your mind in which sequence we should prepare the EFI > patches? > > The following of my patches could be directly applied to efi-next: > > [U-Boot,1/1] efi_selftest: enable CONFIG_CMD_BOOTEFI_SELFTEST > https://patchwork.ozlabs.org/patch/816412/ > [U-Boot,1/1] efi_loader: replace efi_div10 by div64_u64 > https://patchwork.ozlabs.org/patch/820731/ > [U-Boot,1/1] efi_selftest: use efi_st_error for all error messages > https://patchwork.ozlabs.org/patch/821237/ > [U-Boot,1/1] efi_loader: use type bool for event states > https://patchwork.ozlabs.org/patch/821302/ > [U-Boot,v2,1/1] efi_selftest: make tests easier to read > https://patchwork.ozlabs.org/patch/821306/ > > I guess Rob was refering to > [U-Boot,1/1] efi_loader: use type bool for event states > > Best regards > > Heinrich
On 04.10.17 16:57, Rob Clark wrote: > On Wed, Oct 4, 2017 at 10:46 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> On 10/04/2017 04:14 PM, Rob Clark wrote: >>> On Wed, Oct 4, 2017 at 9:03 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>>> Queued and signaled describe boolean states of events. >>>> So let's use type bool and rename the structure members to is_queued >>>> and is_signaled. >>>> >>>> Update the comments for is_queued and is_signaled. >>> >>> Reviewed-by: Rob Clark <robdclark@gmail.com> >>> >>> It would be kinda nice to merge my efi_event fixes and rework to use >>> an arbitrary sized list of events before making too many more >>> efi_event changes, since that is kind of annoying to keep rebasing ;-) >>> >>> BR, >>> -R >> >> I would not mind if you patch went first. >> >> But your patch >> https://patchwork.ozlabs.org/patch/812967/ >> is not applicable to U-Boot master and needs rebasing anyway. > > jfyi, I have it (and other pending patches) rebased on latest master > (as of ~yesterday) here: > > https://github.com/robclark/u-boot/commits/wip-enough-uefi-for-shell > > I wasn't planning on resending until I get further with FAT write > stuff (currently on a local branch, although I might not get much time > to work on in the next week or two).. although I can re-send it or any > of the other patches to get Shell.efi working if wanted. (Note that > I'm also using your patch for efi watchdog support, that was one of > the other required bits.) > > Not sure what agraf's plan is but I think the needed bits for > Shell.efi are mergable already. I don't have a concrete plan - in general I consider patches that have unaddressed review comments as "not to be applied atm" though and I'm not sure I have anything pending from you that would not fall into that category :). Can you split off a series that has Heinrich's blessing to get us as far as we can, so we can keep your queue short? > >> Please, add the missing check that the event pointer is valid. >> The EFI code checks other arguments rigorously so we should do the same >> for pointers. It would be very hard to debug a problem in an EFI >> application otherwise. > > I'm a bit undecided on this, since we have other places where there is > no good way to check the validity of a pointer. (For example file or > disk objects.) I was thinking about perhaps implementing a > compile-time optional feature using a hashtable to map objects to type > so we can add in some type checking, at the expense of extra runtime > overhead. Probably not something you'd want to ship enabled, but it > would be useful for debugging. Feel free to implement just the boilerplate for it with a function that always returns true: static int efi_ptr_valid(void *foo) { return 1; } which we can then later on improve to do actual checking if we care. The least we can do is probably to check for alignment problems. Alex
On 10/04/2017 04:57 PM, Rob Clark wrote: > On Wed, Oct 4, 2017 at 10:46 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> On 10/04/2017 04:14 PM, Rob Clark wrote: >>> On Wed, Oct 4, 2017 at 9:03 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>>> Queued and signaled describe boolean states of events. >>>> So let's use type bool and rename the structure members to is_queued >>>> and is_signaled. >>>> >>>> Update the comments for is_queued and is_signaled. >>> >>> Reviewed-by: Rob Clark <robdclark@gmail.com> >>> >>> It would be kinda nice to merge my efi_event fixes and rework to use >>> an arbitrary sized list of events before making too many more >>> efi_event changes, since that is kind of annoying to keep rebasing ;-) >>> >>> BR, >>> -R >> >> I would not mind if you patch went first. >> >> But your patch >> https://patchwork.ozlabs.org/patch/812967/ >> is not applicable to U-Boot master and needs rebasing anyway. > > jfyi, I have it (and other pending patches) rebased on latest master > (as of ~yesterday) here: > > https://github.com/robclark/u-boot/commits/wip-enough-uefi-for-shell You have a Travis CI build error. Could you, please, fix that. efi_loader: implement SetWatchdogTimer is the last patch in your branch that is not called "HACK ...". So could you, please, prepare a branch that ends here and let Travis test it. > > I wasn't planning on resending until I get further with FAT write > stuff (currently on a local branch, although I might not get much time > to work on in the next week or two).. although I can re-send it or any > of the other patches to get Shell.efi working if wanted. (Note that > I'm also using your patch for efi watchdog support, that was one of > the other required bits.) I have also a lot of patches in the pipeline. We should not block each other. So, please, either resend the patch series (but please nothing called HACK) for review now or let my patches below enter efi-next. Some review comments for your branch: Patch efi_loader: add stub HII protocols uses UINTN heavily. In a recent comment Simon remarked that types should not be uppercase. So I was planning to replace all occurences of UINTN by size_t in an upcoming patch. Patch efi_loader: add EFI_UNICODE_COLLATION_PROTOCOL stub has EFI_EXIT(0). We should yuse a constant like EFI_SUCCESS here. In patch efi_loader: Decouple EFI input/output from stdin/stdout the commit message is insufficient. Please, explain how the output device will be controlled. I am managing my machines remotely via serial. Some of my machines have video but not monitor attached. I would not be able to accept no longer seeing EFI output on my serial. Patch efi_loader: fix events still has the TODO text that you wanted to remove. And of cause the missing pointer validity check. Best regards Heinrich > > Not sure what agraf's plan is but I think the needed bits for > Shell.efi are mergable already. > >> Please, add the missing check that the event pointer is valid. >> The EFI code checks other arguments rigorously so we should do the same >> for pointers. It would be very hard to debug a problem in an EFI >> application otherwise. > > I'm a bit undecided on this, since we have other places where there is > no good way to check the validity of a pointer. (For example file or > disk objects.) I was thinking about perhaps implementing a > compile-time optional feature using a hashtable to map objects to type > so we can add in some type checking, at the expense of extra runtime > overhead. Probably not something you'd want to ship enabled, but it > would be useful for debugging. > > BR, > -R > >> @Alexander >> I guess with Suseconf you were quite busy in the last weeks. Did you >> already make up your mind in which sequence we should prepare the EFI >> patches? >> >> The following of my patches could be directly applied to efi-next: >> >> [U-Boot,1/1] efi_selftest: enable CONFIG_CMD_BOOTEFI_SELFTEST >> https://patchwork.ozlabs.org/patch/816412/ >> [U-Boot,1/1] efi_loader: replace efi_div10 by div64_u64 >> https://patchwork.ozlabs.org/patch/820731/ >> [U-Boot,1/1] efi_selftest: use efi_st_error for all error messages >> https://patchwork.ozlabs.org/patch/821237/ >> [U-Boot,1/1] efi_loader: use type bool for event states >> https://patchwork.ozlabs.org/patch/821302/ >> [U-Boot,v2,1/1] efi_selftest: make tests easier to read >> https://patchwork.ozlabs.org/patch/821306/ >> >> I guess Rob was refering to >> [U-Boot,1/1] efi_loader: use type bool for event states >> >> Best regards >> >> Heinrich >
On Wed, Oct 4, 2017 at 11:25 AM, Alexander Graf <agraf@suse.de> wrote: > > > On 04.10.17 16:57, Rob Clark wrote: >> >> On Wed, Oct 4, 2017 at 10:46 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> >> wrote: >>> >>> On 10/04/2017 04:14 PM, Rob Clark wrote: >>>> >>>> On Wed, Oct 4, 2017 at 9:03 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> >>>> wrote: >>>>> >>>>> Queued and signaled describe boolean states of events. >>>>> So let's use type bool and rename the structure members to is_queued >>>>> and is_signaled. >>>>> >>>>> Update the comments for is_queued and is_signaled. >>>> >>>> >>>> Reviewed-by: Rob Clark <robdclark@gmail.com> >>>> >>>> It would be kinda nice to merge my efi_event fixes and rework to use >>>> an arbitrary sized list of events before making too many more >>>> efi_event changes, since that is kind of annoying to keep rebasing ;-) >>>> >>>> BR, >>>> -R >>> >>> >>> I would not mind if you patch went first. >>> >>> But your patch >>> https://patchwork.ozlabs.org/patch/812967/ >>> is not applicable to U-Boot master and needs rebasing anyway. >> >> >> jfyi, I have it (and other pending patches) rebased on latest master >> (as of ~yesterday) here: >> >> https://github.com/robclark/u-boot/commits/wip-enough-uefi-for-shell >> >> I wasn't planning on resending until I get further with FAT write >> stuff (currently on a local branch, although I might not get much time >> to work on in the next week or two).. although I can re-send it or any >> of the other patches to get Shell.efi working if wanted. (Note that >> I'm also using your patch for efi watchdog support, that was one of >> the other required bits.) >> >> Not sure what agraf's plan is but I think the needed bits for >> Shell.efi are mergable already. > > > I don't have a concrete plan - in general I consider patches that have > unaddressed review comments as "not to be applied atm" though and I'm not > sure I have anything pending from you that would not fall into that category > :). > > Can you split off a series that has Heinrich's blessing to get us as far as > we can, so we can keep your queue short? > >> >>> Please, add the missing check that the event pointer is valid. >>> The EFI code checks other arguments rigorously so we should do the same >>> for pointers. It would be very hard to debug a problem in an EFI >>> application otherwise. >> >> >> I'm a bit undecided on this, since we have other places where there is >> no good way to check the validity of a pointer. (For example file or >> disk objects.) I was thinking about perhaps implementing a >> compile-time optional feature using a hashtable to map objects to type >> so we can add in some type checking, at the expense of extra runtime >> overhead. Probably not something you'd want to ship enabled, but it >> would be useful for debugging. > > > Feel free to implement just the boilerplate for it with a function that > always returns true: > > static int efi_ptr_valid(void *foo) > { > return 1; > } that is reasonable.. I do think we want something to identify types (which could just be a unique global/const ptr for now), so like: static inline bool efi_ptr_valid(void *ptr, efi_type_t *type) (which would let us change what a type is later if we needed) I'll try to cook something up (which might not be until the weekend.. still getting caught up after two weeks of conferences) BR, -R > which we can then later on improve to do actual checking if we care. The > least we can do is probably to check for alignment problems. > > > Alex
On 10/04/2017 05:25 PM, Alexander Graf wrote: > > > On 04.10.17 16:57, Rob Clark wrote: >> On Wed, Oct 4, 2017 at 10:46 AM, Heinrich Schuchardt >> <xypron.glpk@gmx.de> wrote: >>> On 10/04/2017 04:14 PM, Rob Clark wrote: >>>> On Wed, Oct 4, 2017 at 9:03 AM, Heinrich Schuchardt >>>> <xypron.glpk@gmx.de> wrote: >>>>> Queued and signaled describe boolean states of events. >>>>> So let's use type bool and rename the structure members to is_queued >>>>> and is_signaled. >>>>> >>>>> Update the comments for is_queued and is_signaled. >>>> >>>> Reviewed-by: Rob Clark <robdclark@gmail.com> >>>> >>>> It would be kinda nice to merge my efi_event fixes and rework to use >>>> an arbitrary sized list of events before making too many more >>>> efi_event changes, since that is kind of annoying to keep rebasing ;-) >>>> >>>> BR, >>>> -R >>> >>> I would not mind if you patch went first. >>> >>> But your patch >>> https://patchwork.ozlabs.org/patch/812967/ >>> is not applicable to U-Boot master and needs rebasing anyway. >> >> jfyi, I have it (and other pending patches) rebased on latest master >> (as of ~yesterday) here: >> >> https://github.com/robclark/u-boot/commits/wip-enough-uefi-for-shell >> >> I wasn't planning on resending until I get further with FAT write >> stuff (currently on a local branch, although I might not get much time >> to work on in the next week or two).. although I can re-send it or any >> of the other patches to get Shell.efi working if wanted. (Note that >> I'm also using your patch for efi watchdog support, that was one of >> the other required bits.) >> >> Not sure what agraf's plan is but I think the needed bits for >> Shell.efi are mergable already. > > I don't have a concrete plan - in general I consider patches that have > unaddressed review comments as "not to be applied atm" though and I'm > not sure I have anything pending from you that would not fall into that > category :). > > Can you split off a series that has Heinrich's blessing to get us as far > as we can, so we can keep your queue short? > These are the two first patches in Rob's queue: efi_loader: support 16 protocols per efi_object https://patchwork.ozlabs.org/patch/806167/ efi_loader: allow creating new handles https://patchwork.ozlabs.org/patch/806173/ Please, merge these. I am aware that we need to convert the protocols list to a linked list and will do so in a later patch. Please, also merge this patch that definitively does not interfere with Rob's work: efi_selftest: enable CONFIG_CMD_BOOTEFI_SELFTEST https://patchwork.ozlabs.org/patch/816412/ Best regards Heinrich >> >>> Please, add the missing check that the event pointer is valid. >>> The EFI code checks other arguments rigorously so we should do the same >>> for pointers. It would be very hard to debug a problem in an EFI >>> application otherwise. >> >> I'm a bit undecided on this, since we have other places where there is >> no good way to check the validity of a pointer. (For example file or >> disk objects.) I was thinking about perhaps implementing a >> compile-time optional feature using a hashtable to map objects to type >> so we can add in some type checking, at the expense of extra runtime >> overhead. Probably not something you'd want to ship enabled, but it >> would be useful for debugging. > > Feel free to implement just the boilerplate for it with a function that > always returns true: > > static int efi_ptr_valid(void *foo) > { > return 1; > } > > which we can then later on improve to do actual checking if we care. The > least we can do is probably to check for alignment problems. > > > Alex >
On Wed, Oct 4, 2017 at 12:02 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 10/04/2017 04:57 PM, Rob Clark wrote: >> On Wed, Oct 4, 2017 at 10:46 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>> On 10/04/2017 04:14 PM, Rob Clark wrote: >>>> On Wed, Oct 4, 2017 at 9:03 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>>>> Queued and signaled describe boolean states of events. >>>>> So let's use type bool and rename the structure members to is_queued >>>>> and is_signaled. >>>>> >>>>> Update the comments for is_queued and is_signaled. >>>> >>>> Reviewed-by: Rob Clark <robdclark@gmail.com> >>>> >>>> It would be kinda nice to merge my efi_event fixes and rework to use >>>> an arbitrary sized list of events before making too many more >>>> efi_event changes, since that is kind of annoying to keep rebasing ;-) >>>> >>>> BR, >>>> -R >>> >>> I would not mind if you patch went first. >>> >>> But your patch >>> https://patchwork.ozlabs.org/patch/812967/ >>> is not applicable to U-Boot master and needs rebasing anyway. >> >> jfyi, I have it (and other pending patches) rebased on latest master >> (as of ~yesterday) here: >> >> https://github.com/robclark/u-boot/commits/wip-enough-uefi-for-shell > > You have a Travis CI build error. Could you, please, fix that. > > efi_loader: implement SetWatchdogTimer > is the last patch in your branch that is not called "HACK ...". > > So could you, please, prepare a branch that ends here and let Travis > test it. > >> >> I wasn't planning on resending until I get further with FAT write >> stuff (currently on a local branch, although I might not get much time >> to work on in the next week or two).. although I can re-send it or any >> of the other patches to get Shell.efi working if wanted. (Note that >> I'm also using your patch for efi watchdog support, that was one of >> the other required bits.) > > I have also a lot of patches in the pipeline. We should not block each > other. So, please, either resend the patch series (but please nothing > called HACK) for review now or let my patches below enter efi-next. sorry, I didn't have time to push the non "wip-" version of the branch (which does not have other patches+hacks to make things work on db410c), but ofc I wouldn't send those as part of a patchset btw, did you plan any further changes for your watchdog patch? I've rebased it already, but let me know if I should pull in something newer. > Some review comments for your branch: > > Patch efi_loader: add stub HII protocols uses UINTN heavily. In a recent > comment Simon remarked that types should not be uppercase. So I was > planning to replace all occurences of UINTN by size_t in an upcoming patch. ok, I also wanted to clean up efi strings and introduce a typedef for efi 16b chars too.. and this might be easier just to do on top of the other patches. If you were planning on doing this before the weekend, I'll rebase on top.. if you weren't going to have time for it in the next week or so I can take a stab at it. I guess both the cleanups are just a big chunk of mechanical churn, but otherwise not so difficult. > Patch efi_loader: add EFI_UNICODE_COLLATION_PROTOCOL stub > has EFI_EXIT(0). We should yuse a constant like EFI_SUCCESS here. > > In patch efi_loader: Decouple EFI input/output from stdin/stdout > the commit message is insufficient. Please, explain how the output > device will be controlled. I am managing my machines remotely via > serial. Some of my machines have video but not monitor attached. I would > not be able to accept no longer seeing EFI output on my serial. ok.. fwiw, this introduces two new env vars, efiin and efiout, so you can still configure output/input as you prefer. I was planning to add efiout=vidconsole\0efiin=usbkbd to include/configs/dragonboard410c (although maybe there is a better place.. either way setting the vars isn't part of this patch) > Patch efi_loader: fix events > still has the TODO text that you wanted to remove. And of cause the > missing pointer validity check. I did update the TODO text to clarify better, but I could remove it. It was mostly an idea for a future optimization which might (or might not) be useful, not something I was planning to add as part of this patch. BR, -R > Best regards > > Heinrich > >> >> Not sure what agraf's plan is but I think the needed bits for >> Shell.efi are mergable already. >> >>> Please, add the missing check that the event pointer is valid. >>> The EFI code checks other arguments rigorously so we should do the same >>> for pointers. It would be very hard to debug a problem in an EFI >>> application otherwise. >> >> I'm a bit undecided on this, since we have other places where there is >> no good way to check the validity of a pointer. (For example file or >> disk objects.) I was thinking about perhaps implementing a >> compile-time optional feature using a hashtable to map objects to type >> so we can add in some type checking, at the expense of extra runtime >> overhead. Probably not something you'd want to ship enabled, but it >> would be useful for debugging. >> >> BR, >> -R >> >>> @Alexander >>> I guess with Suseconf you were quite busy in the last weeks. Did you >>> already make up your mind in which sequence we should prepare the EFI >>> patches? >>> >>> The following of my patches could be directly applied to efi-next: >>> >>> [U-Boot,1/1] efi_selftest: enable CONFIG_CMD_BOOTEFI_SELFTEST >>> https://patchwork.ozlabs.org/patch/816412/ >>> [U-Boot,1/1] efi_loader: replace efi_div10 by div64_u64 >>> https://patchwork.ozlabs.org/patch/820731/ >>> [U-Boot,1/1] efi_selftest: use efi_st_error for all error messages >>> https://patchwork.ozlabs.org/patch/821237/ >>> [U-Boot,1/1] efi_loader: use type bool for event states >>> https://patchwork.ozlabs.org/patch/821302/ >>> [U-Boot,v2,1/1] efi_selftest: make tests easier to read >>> https://patchwork.ozlabs.org/patch/821306/ >>> >>> I guess Rob was refering to >>> [U-Boot,1/1] efi_loader: use type bool for event states >>> >>> Best regards >>> >>> Heinrich >> >
On Wed, Oct 4, 2017 at 12:29 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 10/04/2017 05:25 PM, Alexander Graf wrote: >> >> >> On 04.10.17 16:57, Rob Clark wrote: >>> On Wed, Oct 4, 2017 at 10:46 AM, Heinrich Schuchardt >>> <xypron.glpk@gmx.de> wrote: >>>> On 10/04/2017 04:14 PM, Rob Clark wrote: >>>>> On Wed, Oct 4, 2017 at 9:03 AM, Heinrich Schuchardt >>>>> <xypron.glpk@gmx.de> wrote: >>>>>> Queued and signaled describe boolean states of events. >>>>>> So let's use type bool and rename the structure members to is_queued >>>>>> and is_signaled. >>>>>> >>>>>> Update the comments for is_queued and is_signaled. >>>>> >>>>> Reviewed-by: Rob Clark <robdclark@gmail.com> >>>>> >>>>> It would be kinda nice to merge my efi_event fixes and rework to use >>>>> an arbitrary sized list of events before making too many more >>>>> efi_event changes, since that is kind of annoying to keep rebasing ;-) >>>>> >>>>> BR, >>>>> -R >>>> >>>> I would not mind if you patch went first. >>>> >>>> But your patch >>>> https://patchwork.ozlabs.org/patch/812967/ >>>> is not applicable to U-Boot master and needs rebasing anyway. >>> >>> jfyi, I have it (and other pending patches) rebased on latest master >>> (as of ~yesterday) here: >>> >>> https://github.com/robclark/u-boot/commits/wip-enough-uefi-for-shell >>> >>> I wasn't planning on resending until I get further with FAT write >>> stuff (currently on a local branch, although I might not get much time >>> to work on in the next week or two).. although I can re-send it or any >>> of the other patches to get Shell.efi working if wanted. (Note that >>> I'm also using your patch for efi watchdog support, that was one of >>> the other required bits.) >>> >>> Not sure what agraf's plan is but I think the needed bits for >>> Shell.efi are mergable already. >> >> I don't have a concrete plan - in general I consider patches that have >> unaddressed review comments as "not to be applied atm" though and I'm >> not sure I have anything pending from you that would not fall into that >> category :). >> >> Can you split off a series that has Heinrich's blessing to get us as far >> as we can, so we can keep your queue short? >> > > These are the two first patches in Rob's queue: > > efi_loader: support 16 protocols per efi_object > https://patchwork.ozlabs.org/patch/806167/ > > efi_loader: allow creating new handles > https://patchwork.ozlabs.org/patch/806173/ btw, you can add for these two: Reviewed-by: Rob Clark <robdclark@gmail.com> > Please, merge these. > > I am aware that we need to convert the protocols list to a linked list > and will do so in a later patch. > > Please, also merge this patch that definitively does not interfere with > Rob's work: > > efi_selftest: enable CONFIG_CMD_BOOTEFI_SELFTEST > https://patchwork.ozlabs.org/patch/816412/ > > Best regards > > Heinrich > >>> >>>> Please, add the missing check that the event pointer is valid. >>>> The EFI code checks other arguments rigorously so we should do the same >>>> for pointers. It would be very hard to debug a problem in an EFI >>>> application otherwise. >>> >>> I'm a bit undecided on this, since we have other places where there is >>> no good way to check the validity of a pointer. (For example file or >>> disk objects.) I was thinking about perhaps implementing a >>> compile-time optional feature using a hashtable to map objects to type >>> so we can add in some type checking, at the expense of extra runtime >>> overhead. Probably not something you'd want to ship enabled, but it >>> would be useful for debugging. >> >> Feel free to implement just the boilerplate for it with a function that >> always returns true: >> >> static int efi_ptr_valid(void *foo) >> { >> return 1; >> } >> >> which we can then later on improve to do actual checking if we care. The >> least we can do is probably to check for alignment problems. >> >> >> Alex >> >
On 04.10.17 18:29, Heinrich Schuchardt wrote: > On 10/04/2017 05:25 PM, Alexander Graf wrote: >> >> >> On 04.10.17 16:57, Rob Clark wrote: >>> On Wed, Oct 4, 2017 at 10:46 AM, Heinrich Schuchardt >>> <xypron.glpk@gmx.de> wrote: >>>> On 10/04/2017 04:14 PM, Rob Clark wrote: >>>>> On Wed, Oct 4, 2017 at 9:03 AM, Heinrich Schuchardt >>>>> <xypron.glpk@gmx.de> wrote: >>>>>> Queued and signaled describe boolean states of events. >>>>>> So let's use type bool and rename the structure members to is_queued >>>>>> and is_signaled. >>>>>> >>>>>> Update the comments for is_queued and is_signaled. >>>>> >>>>> Reviewed-by: Rob Clark <robdclark@gmail.com> >>>>> >>>>> It would be kinda nice to merge my efi_event fixes and rework to use >>>>> an arbitrary sized list of events before making too many more >>>>> efi_event changes, since that is kind of annoying to keep rebasing ;-) >>>>> >>>>> BR, >>>>> -R >>>> >>>> I would not mind if you patch went first. >>>> >>>> But your patch >>>> https://patchwork.ozlabs.org/patch/812967/ >>>> is not applicable to U-Boot master and needs rebasing anyway. >>> >>> jfyi, I have it (and other pending patches) rebased on latest master >>> (as of ~yesterday) here: >>> >>> https://github.com/robclark/u-boot/commits/wip-enough-uefi-for-shell >>> >>> I wasn't planning on resending until I get further with FAT write >>> stuff (currently on a local branch, although I might not get much time >>> to work on in the next week or two).. although I can re-send it or any >>> of the other patches to get Shell.efi working if wanted. (Note that >>> I'm also using your patch for efi watchdog support, that was one of >>> the other required bits.) >>> >>> Not sure what agraf's plan is but I think the needed bits for >>> Shell.efi are mergable already. >> >> I don't have a concrete plan - in general I consider patches that have >> unaddressed review comments as "not to be applied atm" though and I'm >> not sure I have anything pending from you that would not fall into that >> category :). >> >> Can you split off a series that has Heinrich's blessing to get us as far >> as we can, so we can keep your queue short? >> > > These are the two first patches in Rob's queue: > > efi_loader: support 16 protocols per efi_object > https://patchwork.ozlabs.org/patch/806167/ > > efi_loader: allow creating new handles > https://patchwork.ozlabs.org/patch/806173/ > > Please, merge these. Done. > > I am aware that we need to convert the protocols list to a linked list > and will do so in a later patch. > > Please, also merge this patch that definitively does not interfere with > Rob's work: > > efi_selftest: enable CONFIG_CMD_BOOTEFI_SELFTEST > https://patchwork.ozlabs.org/patch/816412/ Done. Alex
On 04.10.17 18:39, Rob Clark wrote: > On Wed, Oct 4, 2017 at 12:29 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> On 10/04/2017 05:25 PM, Alexander Graf wrote: >>> >>> >>> On 04.10.17 16:57, Rob Clark wrote: >>>> On Wed, Oct 4, 2017 at 10:46 AM, Heinrich Schuchardt >>>> <xypron.glpk@gmx.de> wrote: >>>>> On 10/04/2017 04:14 PM, Rob Clark wrote: >>>>>> On Wed, Oct 4, 2017 at 9:03 AM, Heinrich Schuchardt >>>>>> <xypron.glpk@gmx.de> wrote: >>>>>>> Queued and signaled describe boolean states of events. >>>>>>> So let's use type bool and rename the structure members to is_queued >>>>>>> and is_signaled. >>>>>>> >>>>>>> Update the comments for is_queued and is_signaled. >>>>>> >>>>>> Reviewed-by: Rob Clark <robdclark@gmail.com> >>>>>> >>>>>> It would be kinda nice to merge my efi_event fixes and rework to use >>>>>> an arbitrary sized list of events before making too many more >>>>>> efi_event changes, since that is kind of annoying to keep rebasing ;-) >>>>>> >>>>>> BR, >>>>>> -R >>>>> >>>>> I would not mind if you patch went first. >>>>> >>>>> But your patch >>>>> https://patchwork.ozlabs.org/patch/812967/ >>>>> is not applicable to U-Boot master and needs rebasing anyway. >>>> >>>> jfyi, I have it (and other pending patches) rebased on latest master >>>> (as of ~yesterday) here: >>>> >>>> https://github.com/robclark/u-boot/commits/wip-enough-uefi-for-shell >>>> >>>> I wasn't planning on resending until I get further with FAT write >>>> stuff (currently on a local branch, although I might not get much time >>>> to work on in the next week or two).. although I can re-send it or any >>>> of the other patches to get Shell.efi working if wanted. (Note that >>>> I'm also using your patch for efi watchdog support, that was one of >>>> the other required bits.) >>>> >>>> Not sure what agraf's plan is but I think the needed bits for >>>> Shell.efi are mergable already. >>> >>> I don't have a concrete plan - in general I consider patches that have >>> unaddressed review comments as "not to be applied atm" though and I'm >>> not sure I have anything pending from you that would not fall into that >>> category :). >>> >>> Can you split off a series that has Heinrich's blessing to get us as far >>> as we can, so we can keep your queue short? >>> >> >> These are the two first patches in Rob's queue: >> >> efi_loader: support 16 protocols per efi_object >> https://patchwork.ozlabs.org/patch/806167/ >> >> efi_loader: allow creating new handles >> https://patchwork.ozlabs.org/patch/806173/ > > btw, you can add for these two: > > Reviewed-by: Rob Clark <robdclark@gmail.com> I usually apply patches using patchwork which gives me tags that were emailed directly onto the mail thread integrated into the mbox I download. In other words, if you reply to the patch with a reviewed-by tag, I'll automatically have it when applying the patch. If you mention it in an email somewhere chances are quite good it slips through the cracks :) (and yes, I've added those two manually now) Alex
On Wed, Oct 4, 2017 at 12:02 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > Patch efi_loader: add stub HII protocols uses UINTN heavily. In a recent > comment Simon remarked that types should not be uppercase. So I was > planning to replace all occurences of UINTN by size_t in an upcoming patch. btw, how would you feed about: typedef size_t efi_uintn_t; instead of using size_t directly? UINTN is used in a bunch of places where size_t, although it might be the right size, does not feel appropriate.. BR, -R
> Am 08.10.2017 um 17:54 schrieb Rob Clark <robdclark@gmail.com>: > >> On Wed, Oct 4, 2017 at 12:02 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> Patch efi_loader: add stub HII protocols uses UINTN heavily. In a recent >> comment Simon remarked that types should not be uppercase. So I was >> planning to replace all occurences of UINTN by size_t in an upcoming patch. > > btw, how would you feed about: > > typedef size_t efi_uintn_t; > > instead of using size_t directly? UINTN is used in a bunch of places > where size_t, although it might be the right size, does not feel > appropriate.. I like the idea :) Alex
On Sun, Oct 8, 2017 at 12:41 PM, Alexander Graf <agraf@suse.de> wrote: > > >> Am 08.10.2017 um 17:54 schrieb Rob Clark <robdclark@gmail.com>: >> >>> On Wed, Oct 4, 2017 at 12:02 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>> Patch efi_loader: add stub HII protocols uses UINTN heavily. In a recent >>> comment Simon remarked that types should not be uppercase. So I was >>> planning to replace all occurences of UINTN by size_t in an upcoming patch. >> >> btw, how would you feed about: >> >> typedef size_t efi_uintn_t; >> >> instead of using size_t directly? UINTN is used in a bunch of places >> where size_t, although it might be the right size, does not feel >> appropriate.. > > I like the idea :) > btw, 2nd benefit that I didn't immediately think of.. but if I'm comparing UEFI spec and edk2 headers, if I see size_t in u-boot's headers but UINTN in UEFI spec / edk2, then I have to stop and think and remember how UINTN is defined in the spec. If I see efi_uintn_t, I don't ;-) I'll convert the efi_loader patches for Shell.efi to efi_uintn_t/efi_intn_t.. we can handle the rest of conversion and removal of UINTN as a follow-on patch, imho. BR, -R
diff --git a/include/efi_loader.h b/include/efi_loader.h index 2f081f8996..1148db2b00 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -136,8 +136,8 @@ struct efi_object { * @nofify_function: Function to call when the event is triggered * @notify_context: Data to be passed to the notify function * @trigger_type: Type of timer, see efi_set_timer - * @queued: The notification functionis queued - * @signaled: The event occured + * @queued: The notification function is queued + * @signaled: The event occurred. The event is in the signaled state. */ struct efi_event { uint32_t type; @@ -147,8 +147,8 @@ struct efi_event { u64 trigger_next; u64 trigger_time; enum efi_timer_delay trigger_type; - int queued; - int signaled; + bool is_queued; + bool is_signaled; }; diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index d63c66dd45..8975fc4ec1 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -132,14 +132,14 @@ const char *__efi_nesting_dec(void) void efi_signal_event(struct efi_event *event) { if (event->notify_function) { - event->queued = 1; + event->is_queued = true; /* Check TPL */ if (efi_tpl >= event->notify_tpl) return; EFI_CALL_VOID(event->notify_function(event, event->notify_context)); } - event->queued = 0; + event->is_queued = false; } static efi_status_t efi_unsupported(const char *funcname) @@ -267,8 +267,8 @@ efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl, efi_events[i].notify_context = notify_context; /* Disable timers on bootup */ efi_events[i].trigger_next = -1ULL; - efi_events[i].queued = 0; - efi_events[i].signaled = 0; + efi_events[i].is_queued = false; + efi_events[i].is_signaled = false; *event = &efi_events[i]; return EFI_SUCCESS; } @@ -301,7 +301,7 @@ void efi_timer_check(void) for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { if (!efi_events[i].type) continue; - if (efi_events[i].queued) + if (efi_events[i].is_queued) efi_signal_event(&efi_events[i]); if (!(efi_events[i].type & EVT_TIMER) || now < efi_events[i].trigger_next) @@ -317,7 +317,7 @@ void efi_timer_check(void) default: continue; } - efi_events[i].signaled = 1; + efi_events[i].is_signaled = true; efi_signal_event(&efi_events[i]); } WATCHDOG_RESET(); @@ -354,7 +354,7 @@ efi_status_t efi_set_timer(struct efi_event *event, enum efi_timer_delay type, } event->trigger_type = type; event->trigger_time = trigger_time; - event->signaled = 0; + event->is_signaled = false; return EFI_SUCCESS; } return EFI_INVALID_PARAMETER; @@ -391,14 +391,14 @@ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events, known_event: if (!event[i]->type || event[i]->type & EVT_NOTIFY_SIGNAL) return EFI_EXIT(EFI_INVALID_PARAMETER); - if (!event[i]->signaled) + if (!event[i]->is_signaled) efi_signal_event(event[i]); } /* Wait for signal */ for (;;) { for (i = 0; i < num_events; ++i) { - if (event[i]->signaled) + if (event[i]->is_signaled) goto out; } /* Allow events to occur. */ @@ -410,7 +410,7 @@ out: * Reset the signal which is passed to the caller to allow periodic * events to occur. */ - event[i]->signaled = 0; + event[i]->is_signaled = false; if (index) *index = i; @@ -425,9 +425,9 @@ static efi_status_t EFIAPI efi_signal_event_ext(struct efi_event *event) for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { if (event != &efi_events[i]) continue; - if (event->signaled) + if (event->is_signaled) break; - event->signaled = 1; + event->is_signaled = true; if (event->type & EVT_NOTIFY_SIGNAL) efi_signal_event(event); break; @@ -444,8 +444,8 @@ static efi_status_t EFIAPI efi_close_event(struct efi_event *event) if (event == &efi_events[i]) { event->type = 0; event->trigger_next = -1ULL; - event->queued = 0; - event->signaled = 0; + event->is_queued = false; + event->is_signaled = false; return EFI_EXIT(EFI_SUCCESS); } } @@ -463,9 +463,9 @@ static efi_status_t EFIAPI efi_check_event(struct efi_event *event) continue; if (!event->type || event->type & EVT_NOTIFY_SIGNAL) break; - if (!event->signaled) + if (!event->is_signaled) efi_signal_event(event); - if (event->signaled) + if (event->is_signaled) return EFI_EXIT(EFI_SUCCESS); return EFI_EXIT(EFI_NOT_READY); } diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index fd5398d61d..1bdf36b4ae 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -460,7 +460,7 @@ static void EFIAPI efi_console_timer_notify(struct efi_event *event, { EFI_ENTRY("%p, %p", event, context); if (tstc()) { - efi_con_in.wait_for_key->signaled = 1; + efi_con_in.wait_for_key->is_signaled = true; efi_signal_event(efi_con_in.wait_for_key); } EFI_EXIT(EFI_SUCCESS);
Queued and signaled describe boolean states of events. So let's use type bool and rename the structure members to is_queued and is_signaled. Update the comments for is_queued and is_signaled. Reported-by: Simon Glass <sjg@chromium.org> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- include/efi_loader.h | 8 ++++---- lib/efi_loader/efi_boottime.c | 32 ++++++++++++++++---------------- lib/efi_loader/efi_console.c | 2 +- 3 files changed, 21 insertions(+), 21 deletions(-)