Re: [coreboot] TINT payload major update!

[Mike Banon]

Hi Martin, here is my new TINT commit - https://review.coreboot.org/#/c/17507/
(had to abandon a previous one)
I fixed all the problems that were noticed by you, and other reviewers.
Also:
[*] added check for archive before downloading it
[*] added SHA-1 checksum verification of the downloaded archive for TINT payload
[*] added USB keyboard support to nvramcui as well
[*] made a correct logic for USB enabling:
   #ifndef CONFIG_LP_USB
       #define CONFIG_LP_USB 0 <--- earlier it was 1, now it is 0,
because - if for some board CONFIG_LP_USB is not defined, USB should
not be initialized then
   #endif

#if IS_ENABLED(CONFIG_LP_USB)
       usb_initialize();
   #endif

[*] excluded nvramcui from kconfig_lint search (just like tint has
been excluded) --> for the same purpose

The only problem now is, because our kconfig_lint are different, my
commit conflicts with yours here:
https://review.coreboot.org/#/c/17443/1 . Very sorry for that, I'm new
to gerrit and made many mistakes before doing it right. This latest
TINT commit should be correct and final, I really hope

Mike

On Wed, Nov 16, 2016 at 6:52 AM, Martin Roth  wrote:
> Hi Mike, I'll take care of it.  Thanks for pointing it out.
>
> Martin
>
> On Tue, Nov 15, 2016 at 3:21 PM, Mike Banon  wrote:
> > Finally I found a way to commit this (after a few more changes, also
> > attaching a final version to this letter just in case) -
> > https://review.coreboot.org/#/c/17439/1
> >
> > Problem with this commit is a false positive error generated by
> > lint.lint-stable-008-kconfig :
> >
> > #!!!!! Error: IS_ENABLED() used on unknown value CONFIG_LP_USB at
> > payloads/external/tint/libpayload_tint.patch:378.
> > # 1 errors
> >
> > despite that CONFIG_LP_USB is always defined by:
> > #ifndef CONFIG_LP_USB
> > #define CONFIG_LP_USB 1
> > #endif
> > and also in libpayload.
> >
> > Please tell, how to suppress this error, or maybe there is a
> > workaround to make it disappear?
> >
> > Best regards,
> > Mike Banon
> >
> > On Mon, Nov 14, 2016 at 11:09 PM, Mike Banon  wrote:
> > > Please scroll down to the end of letter to download the attachments
> > > ("Non-text attachment was scrubbed..."), don't copy paste (because
> > > Google ate my tabs and broke the files)
> > >
> > > On Mon, Nov 14, 2016 at 10:50 PM, Mike Banon  wrote:
> > > > There are (were) five problems with a TINT payload:
> > > > It is outdated: version 0.03b vs the latest 0.04+nmu1 . Two buffer
> > > >
> > > > overflows have been fixed since that, as well as some other
> > > > improvements
> > > > 2) When you press 'p', "Paused - Press any key to continue" message is
> > > > not displayed - because there are no "refresh()" functions between
> > > > "out_printf" and "while" waiting cycle
> > > > 3) When you lose or quit, PC does not reboot - could only reboot by
> > > > the power button, really inconvenient. Meanwhile, coreinfo and
> > > > nvramcui payloads have a working reboot function "outb(0x6, 0xcf9);"
> > > > which could be borrowed
> > > > 4) libpayload_tint.patch is bloated - could reduce it in half for the
> > > > same effect (with a few changes to Makefile)
> > > > 5) Standard USB keyboard does not work, only internal PS2-like
> > > > keyboard works (at least it is this way on my G505S laptop). The same
> > > > USB keyboard works for me in coreinfo thanks to "#if
> > > > IS_ENABLED(CONFIG_LP_USB) usb_initialize(); #endif" right in the
> > > > beginning of "int main(void)" function at coreinfo.c file
> > > >
> > > > I have already fixed all these problems by making the changes to
> > > > Makefile and libpayload_tint.patch , but I have no idea how to make a
> > > > pull request to coreboot repository and don't have any commit rights!
> > > > So if you have the commit rights, please review the changes and commit
> > > > these updated versions of "coreboot/payloads/external/tint/Makefile"
> > > > and "coreboot/payloads/external/tint/libpayload_tint.patch" - files
> > > > attached below. Whoever commits them first could take a credit for my
> > > > work
> > > >
> > > > Best regards,
> > > > Mike Banon