On 07/25/2017 07:52 PM, Rob Clark wrote: > On Tue, Jul 25, 2017 at 12:56 PM, Heinrich Schuchardt > <xypron.g...@gmx.de> wrote: >> On 07/25/2017 04:28 PM, Alexander Graf wrote: >>> >>> >>> On 25.07.17 14:28, Rob Clark wrote: >>>> On Tue, Jul 25, 2017 at 8:16 AM, Alexander Graf <ag...@suse.de> wrote: >>>>> >>>>> >>>>> On 25.07.17 14:08, Rob Clark wrote: >>>>>> >>>>>> On Tue, Jul 25, 2017 at 7:17 AM, Alexander Graf <ag...@suse.de> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 25.07.17 13:10, Rob Clark wrote: >>>>>>>> >>>>>>>> >>>>>>>> On Tue, Jul 25, 2017 at 4:10 AM, Alexander Graf <ag...@suse.de> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On 25.07.17 01:47, Rob Clark wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> To implement efi_load_image() for the case of loading an image >>>>>>>>>> from a >>>>>>>>>> device path rather than image already loaded into source_buffer, >>>>>>>>>> it is >>>>>>>>>> convenient to be able to re-use simple-file-system and efi-file >>>>>>>>>> interfaces. But these are already using EFI_ENTER()/EFI_EXIT(). >>>>>>>>>> Allow >>>>>>>>>> entry into EFI interfaces to be recursive, since this greatly >>>>>>>>>> simplifies >>>>>>>>>> things. >>>>>>>>>> >>>>>>>>>> (And hypothetically this would be needed anyways to allow grub >>>>>>>>>> to call >>>>>>>>>> into interfaces installed by something outside of grub.) >>>>>>>>>> >>>>>>>>>> Signed-off-by: Rob Clark <robdcl...@gmail.com> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> So there are 2 ways to do this: >>>>>>>>> >>>>>>>>> 1) Keep a refcount, only transition when passing the 0 level >>>>>>>>> 2) Explicitly split functions with ENTRY/EXIT from their core >>>>>>>>> functions >>>>>>>>> >>>>>>>>> So far we used approach 2, splitting functions that are used by both >>>>>>>>> internal and external code into _ext (for externally called) and >>>>>>>>> normal >>>>>>>>> functions. You can see this pattern quite a few times throughout >>>>>>>>> efi_loader. >>>>>>>>> >>>>>>>>> I definitely did try the refcount approach back when I decided for >>>>>>>>> approach >>>>>>>>> 2 and it failed on me in some case, but I can't remember where. >>>>>>>>> >>>>>>>>> Either way, we should definitely be consistent. Either we always use >>>>>>>>> refcounting or we shouldn't bother with it. So if you can make a >>>>>>>>> version >>>>>>>>> work where all _ext variants disappear and we're magically >>>>>>>>> reentrant, >>>>>>>>> I'll >>>>>>>>> be happy to take that. I'm fairly sure it'll break somewhere >>>>>>>>> though :). >>>>>>>>> >>>>>>>> >>>>>>>> for load_image via file-path, we end up needing a *bunch* of >>>>>>>> functions >>>>>>>> normally called via EFI.. so it is going to be a lot more _ext >>>>>>>> variants. >>>>>>> >>>>>>> >>>>>>> >>>>>>> Yes, but imagine a case like this: >>>>>>> >>>>>>> open_protocol >>>>>>> -> open() >>>>>>> -> random_junk() >>>>>>> -> efi_timer_check() >>>>>>> -> timer notifier >>>>>>> -> console print() >>>>>>> >>>>>>> Here the refcounting will simply fail on us, as the timer notifier >>>>>>> needs >>>>>>> to >>>>>>> be run in UEFI context while efi_timer_check() as well as console >>>>>>> print() >>>>>>> need to be run in U-Boot context. >>>>>>> >>>>>>> And if we start to mix and mesh the 2 approaches, I can promise you >>>>>>> that >>>>>>> 2 >>>>>>> weeks down some corner case nobody thought of falls apart because >>>>>>> people >>>>>>> don't manage to fully grasp the code flow anymore. >>>>>>> >>>>>> >>>>>> ugg.. gd is a gd pita ;-) >>>>>> >>>>>> how many places do we have callbacks into UEFI context like this? If >>>>>> just one or two maybe we can suppress them while in u-boot context and >>>>>> handle in efi_exit_func() when entry_count drops to zero? >>>>> >>>>> >>>>> What do you mean by suppressing? We need to call those helpers >>>>> synchronously. And yes, we can probably hand massage the refcount on the >>>>> ones that we remember, but I'm just afraid that the whole thing will >>>>> be so >>>>> complicated eventually that nobody understands what's going on. >>>> >>>> Maybe suppress isn't the right word.. I was thinking of delaying the >>>> callback until EFI_EXIT() that transitions back to the UEFI world. So >>>> from the PoV of the UEFI world, it is still synchronous. >>>> >>>> I haven't looked at the efi_event stuff until just now, but from a >>>> 30sec look, maybe efi_signal_event() could just put the event on a >>>> list of signalled events and not immediately call >>>> event->notify_function(), and handle the calling of notify_function() >>>> in the last EFI_EXIT()?? >>> >>> Maybe, I'll leave Heinrich to comment on that. >> >> Let the application call WaitForEvent for a keyboard event. >> Rob suggests that neither timer events nor any other events are served >> until the user possibly presses a key. >> >> No, we have to call notification functions immediately to serve time >> critical communication like network traffic. >> > > Does WaitForEvent or anything else that dispatches events dispatch > more than one event, or is it just one event and then return to UEFI > world to poll again? We aren't really multi-threaded so there can't > really be async callbacks. > > Either way, I think we can make this work by changing > EFI_EXIT()/callback/EFI_ENTER() to EFI_CALLBACK(callback).. for the > non-nested EFI_ENTER() case we don't have to do anything special (and > the nested case probably should never happen in places where we need > EFI_CALLBACK())
WaitForEvent and CheckEvent calls will serve all notify_functions of timer events this includes a notify_function checking the keyboard and in future I want to check the watchdog in a notify_function. Please, consider that in the notify functions any number of UEFI API functions will be called by the EFI application which themselves may signal events which in turn call notify_functions in which again UEFI API functions may be called. So make no assumptions whatsoever about what is going to happen in a notify function or about the level of nesting. Best regards Heinrich _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
