Re: [PATCH v6 3/5] test: add new driver_data load tester

[Luis R. Rodriguez]

On Fri, May 12, 2017 at 05:52:18PM +0200, Luis R. Rodriguez wrote:
> On Fri, May 12, 2017 at 09:20:24AM +0900, AKASHI Takahiro wrote:
> > On Thu, May 11, 2017 at 08:26:29PM +0200, Luis R. Rodriguez wrote:
> > > On Thu, May 11, 2017 at 07:46:27PM +0900, AKASHI Takahiro wrote:
> > > > On Fri, Apr 28, 2017 at 03:45:35AM +0200, Luis R. Rodriguez wrote:
> > > > > > > diff --git a/tools/testing/selftests/firmware/driver_data.sh b/tools/testing/selftests/firmware/driver_data.sh
> > > > > ...
> > > > > > > +ALL_TESTS="$ALL_TESTS 0012:1:1"
> > > > > > > +ALL_TESTS="$ALL_TESTS 0013:1:1"
> > > > > > 
> > > > > > Do you have good reasons for "the number of times" here?
> > > > > 
> > > > > Just that 1 was not enough and more than 10 seemed too much. As is the tests
> > > > 
> > > > In my opinion, "1," or "2" given the nature of firmware caching, is good
> > > > enough, but it's up to you.
> > > 
> > > No, firmware caching deserves its own test unit on its own, but that will be
> > > enabled through a separate test once we move DRIVER_DATA_PRIV_REQ_NO_CACHE 
> > > to DRIVER_DATA_REQ_NO_CACHE (a private feature to a publicly available
> > > enabled feature). This will be enabled for the upcoming work which enables
> > > FGPA firmware upload which will first enable request_firmware_into_buf()
> > > through the driver data API which uses this.
> > 
> > I thought of implementing NO_CACHE option by myself, but came up with
> > no practical use cases other than my test case :)
> 
> FWIW, my understanding is that such work is underway. I think it may be
> ready first than your signature work so it may make sense to coordinate
> with that developer on their work as you might be able to make use of their
> tests, etc.
> 
> > > > BTW, firmware caching is a bit annoying in my signature tests
> > > > because caching will bypass the verification checks when we iterates
> > > > tests with different conditions.
> > > 
> > > It would seems to make sense to me to only need to verify files when read
> > > for the first time, once its cache I don't see why we would re-verify them ?
> > 
> > Let me explain my test scenario:
> > case (a): firmware w/o signature for signature-not-required driver
> > case (b): firmware w/o signature for signature-required driver
> >    ... and so on
> > 
> > Case (a) and (b) are exercised with the same firmware blob but with
> > different 'test_config's of test_driver_data driver.
> > 
> > First do case (a) and it succeeds, caching the blob, then do case (b).
> > It should fail, but actually succeeds due to firmware caching.
> > 
> > Yes, we can change the order, (b) then (a), to make tests run correctly.
> 
> I see. OK we should consider if firmware requirements can ever vary for
> a file. If not then a cached firmware file can have a sig_check which
> any request can fill in. Upon request firmware, if the cache is present
> *but* the sig check requirements differ (in this simple case where
> signature requirements cannot vary) then the cache present will not
> suffice so we should allow a full new file check through to allow
> to fill the sig_check. If signatures requirements can vary then we
> could have a linked list of signature requirements and at cache check
> time we'd have to check if they match the requirements.
> 
> Just one way to try to address this I think. But hey this is all firmware
> signing related. I suppose a more relevant question to this thread is if
> there might be current criteria or criteria being introduced in this patch
> series which is similar to the signature check which should also be
> extended in these sorts of caching checks.
> 
> > But again, when we iterate this set of test cases under "-c" repeatedly,
> > this will happen again as you can imagine.
> 
> Makes sense.
> 
> > Thanks to firmware caching, each of iterations of tests is not executed
> > under the exactly same condition. That is the problem.
> 
> Sure, specially since asynchornous lookups won't be serialized we can't
> expect any proper order for which request goes in first. If the extra
> caching requirements were however taken into consideration I would expect
> this to not matter though.

Note that a bug was just opened on multiple fw requests and only one completes
fine, it sounds like a real bug, and if so sounds related to this topical case
you are testing against as well.

https://bugzilla.kernel.org/show_bug.cgi?id=195477

It has a proposed patch I have not yet had a chance to properly review / test:

https://bugzilla.kernel.org/attachment.cgi?id=256493&action=diff&collapsed=&headers=1&format=raw

  Luis