Re: [PATCH 0/5] firmware: Add support for loading compressed files

[Takashi Iwai]

On Mon, 20 May 2019 11:39:29 +0200,
Greg Kroah-Hartman wrote:
> 
> On Mon, May 20, 2019 at 11:26:42AM +0200, Takashi Iwai wrote:
> > Hi,
> > 
> > this is a patch set to add the support for loading compressed firmware
> > files.
> > 
> > The primary motivation is to reduce the storage size; e.g. currently
> > the amount of /lib/firmware on my machine counts up to 419MB, and this
> > can be reduced to 130MB file compression.  No bad deal.
> > 
> > The feature adds only fallback to the compressed file, so it should
> > work as it was as long as the normal firmware file is present.  The
> > f/w loader decompresses the content, so that there is no change needed
> > in the caller side.
> > 
> > Currently only XZ format is supported.  A caveat is that the kernel XZ
> > helper code supports only CRC32 (or none) integrity check type, so
> > you'll have to compress the files via xz -C crc32 option.
> > 
> > The patch set begins with a few other improvements and refactoring,
> > followed by the compression support.
> > 
> > In addition to this, dracut needs a small fix to deal with the *.xz
> > files.
> > 
> > Also, the latest patchset is found in topic/fw-decompress branch of my
> > sound.git tree:
> >   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> 
> After a quick review, these all look good to me, nice job.
> 
> One recommendation, can we add support for testing this to the
> tools/testing/selftests/firmware/ tests?  And you did run those
> regression tests to verify that you didn't get any of the config options
> messed up, right? :)

Now I've been testing the firmware selftest, and this turned out to be
surprisingly difficult on my system.  By some reason, the test always
fails at the point triggering the request (line 58 of
fw_filesystem.sh):

  if ! echo -n "$NAME" >"$DIR"/trigger_request ; then
	....

Judging from the strace output, this echo writes only the first byte
of $NAME.  Then kernfs write op is invoked and it deals this one byte
input as if a whole argument were passed, leading to an error.

My temporary workaround was to replace the all "echo" call with
"/usr/bin/echo".

Then it hits a similar write error at the places like:

	echo 1 > $DIR/config_sync_direct

This could be worked around by adding -n option to echo.

Finally, I noticed that the user-fallback doesn't work on my system
any longer and the test stopped.  This is expected, so it implies that
all direct loading tests passed.

FWIW, my system is openSUSE Leap 15.1.  Does anyone experience a
similar problem?


BTW, about adding the new tests for the compressed file support.
Would we want to add a new proc entry for toggling the behavior, like
other tested stuff?  Or just check CONFIG_FW_LOADER_COMPRESS and test
the compressed files conditionally?


thanks,

Takashi