Re: [PATCH 2/7] firmware: add offset to request_firmware_into_buf

[Takashi Iwai <tiwai@suse.de>]

On Mon, 26 Aug 2019 17:41:40 +0200,
Scott Branden wrote:
> 
> HI Takashi,
> 
> On 2019-08-26 8:20 a.m., Takashi Iwai wrote:
> > On Fri, 23 Aug 2019 21:44:42 +0200,
> > Scott Branden wrote:
> >> Hi Takashi,
> >>
> >> Thanks for review.  comments below.
> >>
> >> On 2019-08-23 3:05 a.m., Takashi Iwai wrote:
> >>> On Thu, 22 Aug 2019 21:24:46 +0200,
> >>> Scott Branden wrote:
> >>>> Add offset to request_firmware_into_buf to allow for portions
> >>>> of firmware file to be read into a buffer.  Necessary where firmware
> >>>> needs to be loaded in portions from file in memory constrained systems.
> >>> AFAIU, this won't work with the fallback user helper, right?
> >> Seems to work fine in the fw_run_tests.sh with fallbacks.
> > But how?  You patch doesn't change anything about the fallback loading
> > mechanism.
> Correct - I didn't change any of the underlying mechanisms,
> so however request_firmware_into_buf worked before it still does.

But how?  That's the question.

If I understand correctly, essentially your patch changes the call of
kernel_read_file_from_path() with additional offset and partial size
parameters.  i.e. the partial read behavior itself purely relies on
the kernel_read_file_from_path().
And, if the file isn't read via this function, the f/w loader falls
back to the UMH.  Since fallback.c has no idea about the partial read,
it shall return the full content of the file.  Then this must
contradict against the expected result, no?

> >   Or, if the expected behavior is to load the whole content
> > and then copy a part, what's the merit of this API?
> The merit of the API is that the entire file is not copied into a buffer.
> In my use case, the buffer is a memory region in PCIe space that isn't
> even large enough for the whole file.  So the only way to get the file
> is to read it
> in portions.

But you read not in portions but the whole content, in the case of
fallback mode...

> >>> Also it won't work for the compressed firmware files as-is.
> >> Although unnecessary, seems to work fine in the fw_run_tests.sh with
> >> "both" and "xzonly" options.
> > This looks also suspicious.  Loading a part of the file from the
> > middle and decompression won't work together, from obvious reasons.
> I don't know what the underlying mechanisms are doing right now.
> If they decompress the whole file then that is why it's working.

No, it shouldn't be a complete read.  As already mentioned, the patch
changes only the call pattern of kernel_read_file_from_path().  The
decompression is done after that, so it must be applied to the
partially read content which cannot be decompressed properly.

> An obvious improvement that could be made later is to only read
> a portion of the file before writing it into the buffer in the non-xz case.
>
> > If the test passes, it means that the test itself is more likely
> > incorrect, I'm afraid.
> Then all of the tests for "both" and "xzonly" could be broken.

I suspect that the fallback test is also broken.


thanks,

Takashi