"pcidev.c: Factor out pcidev_validate() into pure fn" broke PCI programmers
Hi, commit e28d75ed7204d7fac2c0fac13978098530b0574e dropped some logic during refactoring. PCI-based programmers now fail to initialize IFF the desired PCI device not the last one enumerated by libpci. Given that this refactoring was done to make unit tests easier, I'd expect the unit tests to have caught that problem. This raises a few questions: - Why did unit tests not find that problem? - If there are no unit tests testing for such a problem, why was the code refactored in the first place? - Do we need CI tests with real hardware or at least with qemu to avoid such problems in the future? Regards, Carl-Daniel
Hi, On 14.01.20 17:10, Carl-Daniel Hailfinger wrote:
commit e28d75ed7204d7fac2c0fac13978098530b0574e dropped some logic during refactoring. PCI-based programmers now fail to initialize IFF the desired PCI device not the last one enumerated by libpci.
yup, I already commented on that change post-merge. Just waited for the holidays to pass, no reaction after all so I pushed a revert: https://review.coreboot.org/c/flashrom/+/38319 Nico
Hi, On Sat, Jan 18, 2020 at 7:31 PM Nico Huber <nico.h@gmx.de> wrote:
Hi,
On 14.01.20 17:10, Carl-Daniel Hailfinger wrote:
commit e28d75ed7204d7fac2c0fac13978098530b0574e dropped some logic during refactoring. PCI-based programmers now fail to initialize IFF the desired PCI device not the last one enumerated by libpci.
yup, I already commented on that change post-merge. Just waited for the holidays to pass, no reaction after all so I pushed a revert: https://review.coreboot.org/c/flashrom/+/38319
I noticed the revert today. I'm not sure what unit tests are good for without, for example, regression tests on hardware.
Nico _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-leave@flashrom.org
Best regards, Angel
Hey guys, Sorry for the delays in response due to local issues here.. Thanks for spotting this issue that slipped through the cracks. I actually think this came about from a local merge conflict resolution. We had breakages internally for all sorts of random reasons as well while trying the other way around to bring the higher quality upstream code back down into our tree. :< I am working locally on building out a test infrastructure that uses EM100's to emulate flashchips and a bunch of different Chromebooks to exercise flashrom more. The idea I have is to turn this into a upstream CI bot somehow but I have to work out how that would work in practice and if it is sufficient to use EM100's to test most things? The problem with QEMU is that only a limited chipset range is there so I guess that would only exercise the core flashrom logic itself? I am open to more ideas on what we need as a community setup to help catch these things better than having breakage merged which is less than ideal! Kind Regards, Edward. On Mon, Jan 20, 2020 at 8:32 AM Angel Pons <th3fanbus@gmail.com> wrote:
Hi,
On Sat, Jan 18, 2020 at 7:31 PM Nico Huber <nico.h@gmx.de> wrote:
Hi,
On 14.01.20 17:10, Carl-Daniel Hailfinger wrote:
commit e28d75ed7204d7fac2c0fac13978098530b0574e dropped some logic during refactoring. PCI-based programmers now fail to initialize IFF the desired PCI device not the last one enumerated by libpci.
yup, I already commented on that change post-merge. Just waited for the holidays to pass, no reaction after all so I pushed a revert: https://review.coreboot.org/c/flashrom/+/38319
I noticed the revert today. I'm not sure what unit tests are good for without, for example, regression tests on hardware.
Nico _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-leave@flashrom.org
Best regards,
Angel
I am working locally on building out a test infrastructure that uses EM100's to emulate flashchips and a bunch of different Chromebooks to exercise flashrom more. The idea I have is to turn this into a upstream CI bot somehow but I have to work out how that would work in practice and if it is sufficient to use EM100's to test most things? The problem with QEMU is that only a limited chipset range is there so I guess that would only exercise the core flashrom logic itself? I am open to more ideas on what we need as a community setup to help catch these things better than having breakage merged which is less than ideal!
Great to see more interest in this :-) EM100 testing would be great to help test on all the various chipsets you use, and save some time too. Though I'm wondering if the EM100 really helps much in your case. For one, the EM100 doesn't emulate write protection capabilities AFAIK (maybe I'm wrong?). Also, if you're worried about wearing down NOR flash parts, you could test erase/writes on a small region (maybe 8 blocks at a time selected at random, or something to that effect). And if there are flash chips in Chromebooks that wear out faster than expected then that's probably something you want to know ;-) It would be nice to see dummyflasher used more. Some simple sanity check could even be made into part of the git presubmit hooks. I also attempted to port the ginormous test_v2.sh script to upstream a while back, but it never made it in: https://review.coreboot.org/c/flashrom/+/23025 (documentation is still at https://goo.gl/qNwdmm). It's kind of the brute force approach and got unwieldy, but IMO with some work it can still be useful to have upstream especially if there's some easier to access documentation to help users with it. There are also some opportunities for whitebox testing since flashrom knows all about block erasers and write protections for the chip/programmer being used. I hacked up something a while back to demonstrate this with NOR flash write protections: https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+... Lots of fun and exciting things to do here for whoever has time...
participants (5)
-
Angel Pons -
Carl-Daniel Hailfinger -
David Hendricks -
Edward O'Callaghan -
Nico Huber