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

[Rob Clark]

On Wed, Oct 4, 2017 at 12:02 PM, Heinrich Schuchardt <xypron.glpk at 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 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
>
> 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
>>
>