"pcidev.c: Factor out pcidev_validate() into pure fn" broke PCI programmers

[Carl-Daniel Hailfinger]

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

[Nico Huber]

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

[Angel Pons]

Hi,

On Sat, Jan 18, 2020 at 7:31 PM Nico Huber  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 -- [e-mail]
> To unsubscribe send an email to [e-mail]

Best regards,

Angel

[Edward O'Callaghan]

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  wrote:
> Hi,
>
> On Sat, Jan 18, 2020 at 7:31 PM Nico Huber  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 -- [e-mail]
> > To unsubscribe send an email to [e-mail]
>
> Best regards,
>
> Angel

[David Hendricks]

> 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...