<div class="gmail_quote">On Tue, Nov 23, 2010 at 11:50 AM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Hi David,<br>
<div class="im"><br>
On 23.11.2010 01:31, David Hendricks wrote:<br>
> Once upon a time, testing flashrom was simple: Read, erase, write, verify,<br>
> done.<br>
><br>
> Over the past few months, we've introduced some very important features to<br>
> Flashrom. Unfortunately, testing is no longer quite so simple. Partial<br>
> writes require that we test various patterns (and we do with a newly<br>
> introduced torture script), and we do that to some extend with the torture<br>
> test that we recently checked in. Embedded controllers introduce a whole<br>
> other world of pain. Long story short, a better testing strategy is needed.<br>
><br>
<br>
</div>Indeed. The biggest problem with any tests right now is that you can<br>
either write them for the flashrom-chromium tree or for mainline<br>
flashrom because the flashrom messages are incompatible/different.<br></blockquote><div><br></div><div>Ah yes... Flashrom messages have been a pain point for us. The upstream messages are too verbose, annoy developers, and confuse scripts. The messages we have now were designed to work around issues as we encountered them. I hope to work on a long-term fix soon...</div>
<div><br></div><meta http-equiv="content-type" content="text/html; charset=utf-8"><div>On Tue, Nov 23, 2010 at 11:50 AM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">> To get the ball rolling, I have a work-in-progress patch for the<br>
> Chromium.org branch of Flashrom right now to introduce a unit testing<br>
> framework and would appreciate thoughts / comments:<br>
> <a href="http://codereview.chromium.org/5136001" target="_blank">http://codereview.chromium.org/5136001</a> . You can download the unified diff<br>
> from there (as to avoid duplication here). It's written in shell script<br>
> (tested on dash and bash) since I felt that's the lowest common denominator<br>
> and the lowest barrier-to-entry for folks wanting to add/modify the<br>
> tests. Feel free to post comments in this thread, since the code review will<br>
> be closed once the code is checked in.<br>
><br>
<br>
</div>"Code review will be closed" means the comments will be unaccessible<br>
after checkin?<br></blockquote><div><br></div><div>No. You can still make comments after code is submitted, but new patches will have a different URL. Either way, I figure it's best to consolidate general discussion here so that we don't have the discussion spread across multiple code reviews.</div>
<div><br></div><meta http-equiv="content-type" content="text/html; charset=utf-8"><div>On Tue, Nov 23, 2010 at 11:50 AM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">> The idea is simple: One master script does generic setup, backs up the<br>
> firmware image, runs unit tests, and unconditionally restores the firmware<br>
> image at the end.<br>
<br>
</div>Agreed. Here is an interesting question: Given that the initial backup<br>
and the final restore are done with the system flashrom binary,<br>
shouldn't we try to restore the backup directly after making the backup?<br>
If that fails, we can't restore the image at the end, and we're better<br>
off detecting this problem before we mess with the chip.<br></blockquote><div><br></div><div>Good point! However, before we implement that, we'll need to modify flashrom to force rewrite all blocks. Otherwise, all blocks will simply be skipped.</div>
<div><br></div><div><meta http-equiv="content-type" content="text/html; charset=utf-8">On Tue, Nov 23, 2010 at 11:50 AM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">> The user sets a few (hopefully no more than two or three)<br>
> environment variables and runs the master script, specifying desired unit<br>
> tests as arguments on the command line. Each unit test should be<br>
> independent, and should have its own documentation in case it needs special<br>
> environment variables or something. Due to the variety of tests which may be<br>
> performed, it's up to the user to decide which tests to run on their<br>
> hardware.<br>
><br>
<br>
</div>In this case we should warn the user that each of the tests has the<br>
potential to brick his device. AFAICS the partial write torture test<br>
can/will result in a hanging EC if the EC code lives in the bottom 128<br>
kB of the flash chip.<br></blockquote><div><br></div><div>An interesting thought, but I think people get used to ignoring warnings. What if we created two (or more) sub-directories to categorize tests and keep dangerous ones separate? For starters, let's call them default_tests/ and unsafe_tests/.</div>
<div><br></div><div>default_tests/ would contain tests that any normal user can run relatively safely to test for simple regressions. We'd put a command on the wiki page for random interested people to run all of these tests. Maybe even have a machine @ <a href="http://coreboot.org">coreboot.org</a> automatically run them on patches sent to the mailing list and e-mail a response if it failed.</div>
<div><br></div><div>unsafe_tests/* would have the other crazy shit like EC tests. Users would need to be much more careful about hardware compatibility, special environment variables that need to be set, etc.</div><div><br>
</div><div><meta http-equiv="content-type" content="text/html; charset=utf-8">On Tue, Nov 23, 2010 at 11:50 AM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">> If you're curious to try the patch, here is a simple invocation that will<br>
> test partial writes for an x86 BIOS chip: ./do_tests.sh<br>
> partial_writes_x86_bios.sh<br>
><br>
> I've attached some sample console output for a test run that does partial<br>
> writes for x86 BIOS chip and some write protection commands. In this<br>
> example, I had to pass in some env variables and specify multiple tests as<br>
> arguments. The invocation: FLASHROM="../flashrom" FLASHROM_PARAM="-p<br>
> internal:bus=spi" sh do_tests.sh partial_writes_x86_bios.sh chip_size.sh<br>
> wp-toggle.sh wp-range.sh<br>
><br>
<br>
</div>Why the "sh" after FLASHROM_PARAM="..."?<br></blockquote><div><br></div><div>No reason. I often run with sh -x for debugging purposes, that is probably just a remnant.</div><div><br></div><meta http-equiv="content-type" content="text/html; charset=utf-8"><div>
On Tue, Nov 23, 2010 at 11:50 AM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">> If I were to test some EC stuff, I'd need to add additional parameters since<br>
> my EC test script requires a special layout file and an secondary EC<br>
> firmware image: LAYOUT_FILE=/tmp/ec_layout.txt<br>
> ALT_EC_IMAGE=/tmp/alt_ec_image.bin FLASHROM="../flashrom"<br>
> FLASHROM_PARAM="-p internal:bus=lpc" ./do_tests.sh partial_writes_ec.sh<br>
><br>
<br>
</div>The big question for the size and protection tests is what you actually<br>
intend to test and whether the test will work with any chips besides the<br>
one you wrote the tests for. This is not a complaint directed at the<br>
RFC, it is rather a hint that writing good tests is hard (but you<br>
already know that).<br></blockquote><div><br></div><div>Yes, indeed! There are numerous assumptions made about the size and capabilities of the chips that need to be abstracted better. I think your SPI chip emulator patch will help tremendously here.</div>
<div><br></div></div>-- <br>David Hendricks (dhendrix)<br>Systems Software Engineer, Google Inc.<br>