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

[Takashi Iwai]

On Mon, 20 May 2019 11:56:07 +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? :)
> 
> Oh, do you believe I'm a so modern person who lets computer working on
> everything? ;)  I only tested manually, so far, this will be my
> homework today.

After fixing the regression in kselftest, I could verify and confirm
that no regression was introduced by my patchset.

Also, below is the patch to add tests for the compressed firmware
load.  I'll add to the series at the next respin, if needed.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@xxxxxxx>
Subject: [PATCH] selftests: firmware: Add compressed firmware tests

This patch adds the test cases for checking compressed firmware load.
Two more cases are added to fw_filesystem.sh:
- Both a plain file and an xz file are present, and load the former
- Only an xz file is present, and load without '.xz' suffix

The tests are enabled only when CONFIG_FW_LOADER_COMPRESS is enabled
and xz program is installed.

Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
---
 tools/testing/selftests/firmware/fw_filesystem.sh | 73 +++++++++++++++++++----
 tools/testing/selftests/firmware/fw_lib.sh        |  7 +++
 tools/testing/selftests/firmware/fw_run_tests.sh  |  1 +
 3 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh
index a4320c4b44dc..f901076aa2ea 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -153,13 +153,18 @@ config_set_read_fw_idx()
 
 read_firmwares()
 {
+	if [ "$1" = "xzonly" ]; then
+		fwfile="${FW}-orig"
+	else
+		fwfile="$FW"
+	fi
 	for i in $(seq 0 3); do
 		config_set_read_fw_idx $i
 		# Verify the contents are what we expect.
 		# -Z required for now -- check for yourself, md5sum
 		# on $FW and DIR/read_firmware will yield the same. Even
 		# cmp agrees, so something is off.
-		if ! diff -q -Z "$FW" $DIR/read_firmware 2>/dev/null ; then
+		if ! diff -q -Z "$fwfile" $DIR/read_firmware 2>/dev/null ; then
 			echo "request #$i: firmware was not loaded" >&2
 			exit 1
 		fi
@@ -246,17 +251,17 @@ test_request_firmware_nowait_custom_nofile()
 
 test_batched_request_firmware()
 {
-	echo -n "Batched request_firmware() try #$1: "
+	echo -n "Batched request_firmware() $2 try #$1: "
 	config_reset
 	config_trigger_sync
-	read_firmwares
+	read_firmwares $2
 	release_all_firmware
 	echo "OK"
 }
 
 test_batched_request_firmware_direct()
 {
-	echo -n "Batched request_firmware_direct() try #$1: "
+	echo -n "Batched request_firmware_direct() $2 try #$1: "
 	config_reset
 	config_set_sync_direct
 	config_trigger_sync
@@ -266,7 +271,7 @@ test_batched_request_firmware_direct()
 
 test_request_firmware_nowait_uevent()
 {
-	echo -n "Batched request_firmware_nowait(uevent=true) try #$1: "
+	echo -n "Batched request_firmware_nowait(uevent=true) $2 try #$1: "
 	config_reset
 	config_trigger_async
 	release_all_firmware
@@ -275,11 +280,16 @@ test_request_firmware_nowait_uevent()
 
 test_request_firmware_nowait_custom()
 {
-	echo -n "Batched request_firmware_nowait(uevent=false) try #$1: "
+	echo -n "Batched request_firmware_nowait(uevent=false) $2 try #$1: "
 	config_reset
 	config_unset_uevent
 	RANDOM_FILE_PATH=$(setup_random_file)
 	RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+	if [ "$2" = "both" ]; then
+		xz -9 -C crc32 -k $RANDOM_FILE_PATH
+	elif [ "$2" = "xzonly" ]; then
+		xz -9 -C crc32 $RANDOM_FILE_PATH
+	fi
 	config_set_name $RANDOM_FILE
 	config_trigger_async
 	release_all_firmware
@@ -294,19 +304,19 @@ test_config_present
 echo
 echo "Testing with the file present..."
 for i in $(seq 1 5); do
-	test_batched_request_firmware $i
+	test_batched_request_firmware $i normal
 done
 
 for i in $(seq 1 5); do
-	test_batched_request_firmware_direct $i
+	test_batched_request_firmware_direct $i normal
 done
 
 for i in $(seq 1 5); do
-	test_request_firmware_nowait_uevent $i
+	test_request_firmware_nowait_uevent $i normal
 done
 
 for i in $(seq 1 5); do
-	test_request_firmware_nowait_custom $i
+	test_request_firmware_nowait_custom $i normal
 done
 
 # Test for file not found, errors are expected, the failure would be
@@ -329,4 +339,47 @@ for i in $(seq 1 5); do
 	test_request_firmware_nowait_custom_nofile $i
 done
 
+test "$HAS_FW_LOADER_COMPRESS" != "yes" && exit 0
+
+# test with both files present
+xz -9 -C crc32 -k $FW
+config_set_name $NAME
+echo
+echo "Testing with both plain and xz files present..."
+for i in $(seq 1 5); do
+	test_batched_request_firmware $i both
+done
+
+for i in $(seq 1 5); do
+	test_batched_request_firmware_direct $i both
+done
+
+for i in $(seq 1 5); do
+	test_request_firmware_nowait_uevent $i both
+done
+
+for i in $(seq 1 5); do
+	test_request_firmware_nowait_custom $i both
+done
+
+# test with only xz file present
+mv "$FW" "${FW}-orig"
+echo
+echo "Testing with only xz file present..."
+for i in $(seq 1 5); do
+	test_batched_request_firmware $i xzonly
+done
+
+for i in $(seq 1 5); do
+	test_batched_request_firmware_direct $i xzonly
+done
+
+for i in $(seq 1 5); do
+	test_request_firmware_nowait_uevent $i xzonly
+done
+
+for i in $(seq 1 5); do
+	test_request_firmware_nowait_custom $i xzonly
+done
+
 exit 0
diff --git a/tools/testing/selftests/firmware/fw_lib.sh b/tools/testing/selftests/firmware/fw_lib.sh
index 1cbb12e284a6..f236cc295450 100755
--- a/tools/testing/selftests/firmware/fw_lib.sh
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -50,6 +50,7 @@ check_setup()
 {
 	HAS_FW_LOADER_USER_HELPER="$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y)"
 	HAS_FW_LOADER_USER_HELPER_FALLBACK="$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y)"
+	HAS_FW_LOADER_COMPRESS="$(kconfig_has CONFIG_FW_LOADER_COMPRESS=y)"
 	PROC_FW_IGNORE_SYSFS_FALLBACK="0"
 	PROC_FW_FORCE_SYSFS_FALLBACK="0"
 
@@ -84,6 +85,12 @@ check_setup()
 	fi
 
 	OLD_FWPATH="$(cat /sys/module/firmware_class/parameters/path)"
+
+	if [ "$HAS_FW_LOADER_COMPRESS" = "yes" ]; then
+		if ! which xz 2> /dev/null > /dev/null; then
+			HAS_FW_LOADER_COMPRESS=""
+		fi
+	fi
 }
 
 verify_reqs()
diff --git a/tools/testing/selftests/firmware/fw_run_tests.sh b/tools/testing/selftests/firmware/fw_run_tests.sh
index cffdd4eb0a57..8e14d555c197 100755
--- a/tools/testing/selftests/firmware/fw_run_tests.sh
+++ b/tools/testing/selftests/firmware/fw_run_tests.sh
@@ -11,6 +11,7 @@ source $TEST_DIR/fw_lib.sh
 
 export HAS_FW_LOADER_USER_HELPER=""
 export HAS_FW_LOADER_USER_HELPER_FALLBACK=""
+export HAS_FW_LOADER_COMPRESS=""
 
 run_tests()
 {
-- 
2.16.4