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

[Rob Clark]

On Wed, Oct 4, 2017 at 11:25 AM, Alexander Graf <agraf at 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 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;
> }

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