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

[Takashi Iwai]

On Mon, 20 May 2019 17:18:48 +0200,
Takashi Iwai wrote:
> 
> On Mon, 20 May 2019 16:39:37 +0200,
> Takashi Iwai wrote:
> > 
> > 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?
> 
> This seems to be a regression on 5.2-rc1.
> The tests on 4.20 worked fine.  5.1 worked, but gave the error at
> fallback test instead of skipping.  This is likely another regression,
> but irrelevant with the major issue as above.
> 
> Now bisecting...

Still in bisection, but it's timeout, I'll have to leave now...

FWIW, the regression seems to have been introduced around the latest
kselftest merge.
The commit 4c7b63a32d54 is bad, and the commit 9cbda1bddb4c good.

Shuah, could you check it?


thanks,

Takashi