[U-Boot] [PATCH 1/1] efi_loader: use type bool for event states

[Alexander Graf]

On 04.10.17 16:57, Rob Clark wrote:
> On Wed, Oct 4, 2017 at 10:46 AM, Heinrich Schuchardt <xypron.glpk at 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 at 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 at 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