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

From: shuah
Date: Mon May 20 2019 - 13:29:41 EST


On 5/20/19 10:22 AM, Takashi Iwai wrote:
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?



Kees,

Could this be related to your selftest Makefile test run output
refactoring work?

I did a quick test on 5.2 after my first kselftest update without
the refactor work - firmware test worked.

I am thinking this is related to

eff3ee303d0d
selftests: Extract single-test shell logic from lib.mk

bash vs. sh difference in "echo" command behavior is the cause
is my best guess. Will you be able to take a look at this?

sudo make -C tools/testing/selftests/firmware/ run_tests

will show the difference.

thanks,
-- Shuah