[PATCH v2.5 0/3] firmware: Add support for loading compressed files

[PATCH v2.5 0/3] firmware: Add support for loading compressed files

[Takashi Iwai]

[resubmitted with the missing patch]

Hi,

here are the rest and the main part of patches to add the support for
loading the compressed firmware files.  The patch was slightly
refactored for more easily enhancing for other compression formats (if
anyone wants).  Also the selftest patch is included.  The
functionality doesn't change from the previous patchset.


thanks,

Takashi

===

Takashi Iwai (3):
  firmware: Factor out the paged buffer handling code
  firmware: Add support for loading compressed files
  selftests: firmware: Add compressed firmware tests

 drivers/base/firmware_loader/Kconfig              |  18 ++
 drivers/base/firmware_loader/fallback.c           |  61 +------
 drivers/base/firmware_loader/firmware.h           |  12 +-
 drivers/base/firmware_loader/main.c               | 199 +++++++++++++++++++++-
 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 +
 7 files changed, 295 insertions(+), 76 deletions(-)

-- 
2.16.4

[PATCH v2.5 1/3] firmware: Factor out the paged buffer handling code

[Takashi Iwai]

This is merely a preparation for the upcoming compressed firmware
support and no functional changes.  It moves the code to handle the
paged buffer allocation and mapping out of fallback.c into the main
code, so that they can be used commonly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/firmware_loader/fallback.c | 61 ++++-----------------------------
 drivers/base/firmware_loader/firmware.h |  4 +++
 drivers/base/firmware_loader/main.c     | 52 ++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 54 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index b5cd96fd0e77..80b20f6c494f 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -219,25 +219,6 @@ static ssize_t firmware_loading_show(struct device *dev,
 	return sprintf(buf, "%d\n", loading);
 }
 
-/* one pages buffer should be mapped/unmapped only once */
-static int map_fw_priv_pages(struct fw_priv *fw_priv)
-{
-	if (!fw_priv->pages)
-		return 0;
-
-	vunmap(fw_priv->data);
-	fw_priv->data = vmap(fw_priv->pages, fw_priv->nr_pages, 0,
-			     PAGE_KERNEL_RO);
-	if (!fw_priv->data)
-		return -ENOMEM;
-
-	/* page table is no longer needed after mapping, let's free */
-	kvfree(fw_priv->pages);
-	fw_priv->pages = NULL;
-
-	return 0;
-}
-
 /**
  * firmware_loading_store() - set value in the 'loading' control file
  * @dev: device pointer
@@ -283,7 +264,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 			 * see the mapped 'buf->data' once the loading
 			 * is completed.
 			 * */
-			rc = map_fw_priv_pages(fw_priv);
+			rc = fw_map_paged_buf(fw_priv);
 			if (rc)
 				dev_err(dev, "%s: map pages failed\n",
 					__func__);
@@ -388,41 +369,13 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 
 static int fw_realloc_pages(struct fw_sysfs *fw_sysfs, int min_size)
 {
-	struct fw_priv *fw_priv= fw_sysfs->fw_priv;
-	int pages_needed = PAGE_ALIGN(min_size) >> PAGE_SHIFT;
-
-	/* If the array of pages is too small, grow it... */
-	if (fw_priv->page_array_size < pages_needed) {
-		int new_array_size = max(pages_needed,
-					 fw_priv->page_array_size * 2);
-		struct page **new_pages;
+	int err;
 
-		new_pages = kvmalloc_array(new_array_size, sizeof(void *),
-					   GFP_KERNEL);
-		if (!new_pages) {
-			fw_load_abort(fw_sysfs);
-			return -ENOMEM;
-		}
-		memcpy(new_pages, fw_priv->pages,
-		       fw_priv->page_array_size * sizeof(void *));
-		memset(&new_pages[fw_priv->page_array_size], 0, sizeof(void *) *
-		       (new_array_size - fw_priv->page_array_size));
-		kvfree(fw_priv->pages);
-		fw_priv->pages = new_pages;
-		fw_priv->page_array_size = new_array_size;
-	}
-
-	while (fw_priv->nr_pages < pages_needed) {
-		fw_priv->pages[fw_priv->nr_pages] =
-			alloc_page(GFP_KERNEL | __GFP_HIGHMEM);
-
-		if (!fw_priv->pages[fw_priv->nr_pages]) {
-			fw_load_abort(fw_sysfs);
-			return -ENOMEM;
-		}
-		fw_priv->nr_pages++;
-	}
-	return 0;
+	err = fw_grow_paged_buf(fw_sysfs->fw_priv,
+				PAGE_ALIGN(min_size) >> PAGE_SHIFT);
+	if (err)
+		fw_load_abort(fw_sysfs);
+	return err;
 }
 
 /**
diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index d20d4e7f9e71..35f4e58b2d98 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -135,8 +135,12 @@ int assign_fw(struct firmware *fw, struct device *device,
 
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 void fw_free_paged_buf(struct fw_priv *fw_priv);
+int fw_grow_paged_buf(struct fw_priv *fw_priv, int pages_needed);
+int fw_map_paged_buf(struct fw_priv *fw_priv);
 #else
 static inline void fw_free_paged_buf(struct fw_priv *fw_priv) {}
+int fw_grow_paged_buf(struct fw_priv *fw_priv, int pages_needed) { return -ENXIO; }
+int fw_map_paged_buf(struct fw_priv *fw_priv) { return -ENXIO; }
 #endif
 
 #endif /* __FIRMWARE_LOADER_H */
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 2e74a1b73dae..7e12732f4705 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -281,6 +281,58 @@ void fw_free_paged_buf(struct fw_priv *fw_priv)
 	fw_priv->page_array_size = 0;
 	fw_priv->nr_pages = 0;
 }
+
+int fw_grow_paged_buf(struct fw_priv *fw_priv, int pages_needed)
+{
+	/* If the array of pages is too small, grow it */
+	if (fw_priv->page_array_size < pages_needed) {
+		int new_array_size = max(pages_needed,
+					 fw_priv->page_array_size * 2);
+		struct page **new_pages;
+
+		new_pages = kvmalloc_array(new_array_size, sizeof(void *),
+					   GFP_KERNEL);
+		if (!new_pages)
+			return -ENOMEM;
+		memcpy(new_pages, fw_priv->pages,
+		       fw_priv->page_array_size * sizeof(void *));
+		memset(&new_pages[fw_priv->page_array_size], 0, sizeof(void *) *
+		       (new_array_size - fw_priv->page_array_size));
+		kvfree(fw_priv->pages);
+		fw_priv->pages = new_pages;
+		fw_priv->page_array_size = new_array_size;
+	}
+
+	while (fw_priv->nr_pages < pages_needed) {
+		fw_priv->pages[fw_priv->nr_pages] =
+			alloc_page(GFP_KERNEL | __GFP_HIGHMEM);
+
+		if (!fw_priv->pages[fw_priv->nr_pages])
+			return -ENOMEM;
+		fw_priv->nr_pages++;
+	}
+
+	return 0;
+}
+
+int fw_map_paged_buf(struct fw_priv *fw_priv)
+{
+	/* one pages buffer should be mapped/unmapped only once */
+	if (!fw_priv->pages)
+		return 0;
+
+	vunmap(fw_priv->data);
+	fw_priv->data = vmap(fw_priv->pages, fw_priv->nr_pages, 0,
+			     PAGE_KERNEL_RO);
+	if (!fw_priv->data)
+		return -ENOMEM;
+
+	/* page table is no longer needed after mapping, let's free */
+	kvfree(fw_priv->pages);
+	fw_priv->pages = NULL;
+
+	return 0;
+}
 #endif
 
 /* direct firmware loading support */
-- 
2.16.4

[PATCH v2.5 2/3] firmware: Add support for loading compressed files

[Takashi Iwai]

This patch adds the support for loading compressed firmware files.
The primary motivation is to reduce the storage size; e.g. currently
the files in /lib/firmware on my machine counts up to 419MB, while
they can be reduced to 130MB by file compression.

The patch introduces a new kconfig option CONFIG_FW_LOADER_COMPRESS.
Even with this option set, the firmware loader still tries to load the
original firmware file as-is at first, but then falls back to the file
with ".xz" extension when it's not found, and the decompressed file
content is returned to the caller of request_firmware().  So, no
change is needed for the rest.

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.

Since we can't determine the expanded size immediately from an XZ
file, the patch re-uses the paged buffer that was used for the
user-mode fallback; it puts the decompressed content page, which are
vmapped at the end.  The paged buffer code is conditionally built with
a new Kconfig that is selected automatically.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2.5: Slight code refactoring, no functional changes

 drivers/base/firmware_loader/Kconfig    |  18 ++++
 drivers/base/firmware_loader/firmware.h |   8 +-
 drivers/base/firmware_loader/main.c     | 147 ++++++++++++++++++++++++++++++--
 3 files changed, 161 insertions(+), 12 deletions(-)

diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig
index 38f2da6f5c2b..3f9e274e2ed3 100644
--- a/drivers/base/firmware_loader/Kconfig
+++ b/drivers/base/firmware_loader/Kconfig
@@ -26,6 +26,9 @@ config FW_LOADER
 
 if FW_LOADER
 
+config FW_LOADER_PAGED_BUF
+	bool
+
 config EXTRA_FIRMWARE
 	string "Build named firmware blobs into the kernel binary"
 	help
@@ -67,6 +70,7 @@ config EXTRA_FIRMWARE_DIR
 
 config FW_LOADER_USER_HELPER
 	bool "Enable the firmware sysfs fallback mechanism"
+	select FW_LOADER_PAGED_BUF
 	help
 	  This option enables a sysfs loading facility to enable firmware
 	  loading to the kernel through userspace as a fallback mechanism
@@ -151,5 +155,19 @@ config FW_LOADER_USER_HELPER_FALLBACK
 
 	  If you are unsure about this, say N here.
 
+config FW_LOADER_COMPRESS
+	bool "Enable compressed firmware support"
+	select FW_LOADER_PAGED_BUF
+	select XZ_DEC
+	help
+	  This option enables the support for loading compressed firmware
+	  files. The caller of firmware API receives the decompressed file
+	  content. The compressed file is loaded as a fallback, only after
+	  loading the raw file failed at first.
+
+	  Currently only XZ-compressed files are supported, and they have to
+	  be compressed with either none or crc32 integrity check type (pass
+	  "-C crc32" option to xz command).
+
 endif # FW_LOADER
 endmenu
diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index 35f4e58b2d98..7048a41973ed 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -64,12 +64,14 @@ struct fw_priv {
 	void *data;
 	size_t size;
 	size_t allocated_size;
-#ifdef CONFIG_FW_LOADER_USER_HELPER
+#ifdef CONFIG_FW_LOADER_PAGED_BUF
 	bool is_paged_buf;
-	bool need_uevent;
 	struct page **pages;
 	int nr_pages;
 	int page_array_size;
+#endif
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+	bool need_uevent;
 	struct list_head pending_list;
 #endif
 	const char *fw_name;
@@ -133,7 +135,7 @@ static inline void fw_state_done(struct fw_priv *fw_priv)
 int assign_fw(struct firmware *fw, struct device *device,
 	      enum fw_opt opt_flags);
 
-#ifdef CONFIG_FW_LOADER_USER_HELPER
+#ifdef CONFIG_FW_LOADER_PAGED_BUF
 void fw_free_paged_buf(struct fw_priv *fw_priv);
 int fw_grow_paged_buf(struct fw_priv *fw_priv, int pages_needed);
 int fw_map_paged_buf(struct fw_priv *fw_priv);
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 7e12732f4705..bf44c79beae9 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -33,6 +33,7 @@
 #include <linux/syscore_ops.h>
 #include <linux/reboot.h>
 #include <linux/security.h>
+#include <linux/xz.h>
 
 #include <generated/utsrelease.h>
 
@@ -266,7 +267,7 @@ static void free_fw_priv(struct fw_priv *fw_priv)
 		spin_unlock(&fwc->lock);
 }
 
-#ifdef CONFIG_FW_LOADER_USER_HELPER
+#ifdef CONFIG_FW_LOADER_PAGED_BUF
 void fw_free_paged_buf(struct fw_priv *fw_priv)
 {
 	int i;
@@ -335,6 +336,105 @@ int fw_map_paged_buf(struct fw_priv *fw_priv)
 }
 #endif
 
+/*
+ * XZ-compressed firmware support
+ */
+#ifdef CONFIG_FW_LOADER_COMPRESS
+/* show an error and return the standard error code */
+static int fw_decompress_xz_error(struct device *dev, enum xz_ret xz_ret)
+{
+	if (xz_ret != XZ_STREAM_END) {
+		dev_warn(dev, "xz decompression failed (xz_ret=%d)\n", xz_ret);
+		return xz_ret == XZ_MEM_ERROR ? -ENOMEM : -EINVAL;
+	}
+	return 0;
+}
+
+/* single-shot decompression onto the pre-allocated buffer */
+static int fw_decompress_xz_single(struct device *dev, struct fw_priv *fw_priv,
+				   size_t in_size, const void *in_buffer)
+{
+	struct xz_dec *xz_dec;
+	struct xz_buf xz_buf;
+	enum xz_ret xz_ret;
+
+	xz_dec = xz_dec_init(XZ_SINGLE, (u32)-1);
+	if (!xz_dec)
+		return -ENOMEM;
+
+	xz_buf.in_size = in_size;
+	xz_buf.in = in_buffer;
+	xz_buf.in_pos = 0;
+	xz_buf.out_size = fw_priv->allocated_size;
+	xz_buf.out = fw_priv->data;
+	xz_buf.out_pos = 0;
+
+	xz_ret = xz_dec_run(xz_dec, &xz_buf);
+	xz_dec_end(xz_dec);
+
+	fw_priv->size = xz_buf.out_pos;
+	return fw_decompress_xz_error(dev, xz_ret);
+}
+
+/* decompression on paged buffer and map it */
+static int fw_decompress_xz_pages(struct device *dev, struct fw_priv *fw_priv,
+				  size_t in_size, const void *in_buffer)
+{
+	struct xz_dec *xz_dec;
+	struct xz_buf xz_buf;
+	enum xz_ret xz_ret;
+	struct page *page;
+	int err = 0;
+
+	xz_dec = xz_dec_init(XZ_DYNALLOC, (u32)-1);
+	if (!xz_dec)
+		return -ENOMEM;
+
+	xz_buf.in_size = in_size;
+	xz_buf.in = in_buffer;
+	xz_buf.in_pos = 0;
+
+	fw_priv->is_paged_buf = true;
+	fw_priv->size = 0;
+	do {
+		if (fw_grow_paged_buf(fw_priv, fw_priv->nr_pages + 1)) {
+			err = -ENOMEM;
+			goto out;
+		}
+
+		/* decompress onto the new allocated page */
+		page = fw_priv->pages[fw_priv->nr_pages - 1];
+		xz_buf.out = kmap(page);
+		xz_buf.out_pos = 0;
+		xz_buf.out_size = PAGE_SIZE;
+		xz_ret = xz_dec_run(xz_dec, &xz_buf);
+		kunmap(page);
+		fw_priv->size += xz_buf.out_pos;
+		/* partial decompression means either end or error */
+		if (xz_buf.out_pos != PAGE_SIZE)
+			break;
+	} while (xz_ret == XZ_OK);
+
+	err = fw_decompress_xz_error(dev, xz_ret);
+	if (!err)
+		err = fw_map_paged_buf(fw_priv);
+
+ out:
+	xz_dec_end(xz_dec);
+	return err;
+}
+
+static int fw_decompress_xz(struct device *dev, struct fw_priv *fw_priv,
+			    size_t in_size, const void *in_buffer)
+{
+	/* if the buffer is pre-allocated, we can perform in single-shot mode */
+	if (fw_priv->data)
+		return fw_decompress_xz_single(dev, fw_priv, in_size, in_buffer);
+	else
+		return fw_decompress_xz_pages(dev, fw_priv, in_size, in_buffer);
+}
+#endif /* CONFIG_FW_LOADER_COMPRESS */
+
 /* direct firmware loading support */
 static char fw_path_para[256];
 static const char * const fw_path[] = {
@@ -354,7 +454,12 @@ module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
 MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
 
 static int
-fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
+fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
+			   const char *suffix,
+			   int (*decompress)(struct device *dev,
+					     struct fw_priv *fw_priv,
+					     size_t in_size,
+					     const void *in_buffer))
 {
 	loff_t size;
 	int i, len;
@@ -362,9 +467,11 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
 	char *path;
 	enum kernel_read_file_id id = READING_FIRMWARE;
 	size_t msize = INT_MAX;
+	void *buffer = NULL;
 
 	/* Already populated data member means we're loading into a buffer */
-	if (fw_priv->data) {
+	if (!decompress && fw_priv->data) {
+		buffer = fw_priv->data;
 		id = READING_FIRMWARE_PREALLOC_BUFFER;
 		msize = fw_priv->allocated_size;
 	}
@@ -378,15 +485,15 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
 		if (!fw_path[i][0])
 			continue;
 
-		len = snprintf(path, PATH_MAX, "%s/%s",
-			       fw_path[i], fw_priv->fw_name);
+		len = snprintf(path, PATH_MAX, "%s/%s%s",
+			       fw_path[i], fw_priv->fw_name, suffix);
 		if (len >= PATH_MAX) {
 			rc = -ENAMETOOLONG;
 			break;
 		}
 
 		fw_priv->size = 0;
-		rc = kernel_read_file_from_path(path, &fw_priv->data, &size,
+		rc = kernel_read_file_from_path(path, &buffer, &size,
 						msize, id);
 		if (rc) {
 			if (rc != -ENOENT)
@@ -397,8 +504,24 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
 					 path);
 			continue;
 		}
-		dev_dbg(device, "direct-loading %s\n", fw_priv->fw_name);
-		fw_priv->size = size;
+		if (decompress) {
+			dev_dbg(device, "f/w decompressing %s\n",
+				fw_priv->fw_name);
+			rc = decompress(device, fw_priv, size, buffer);
+			/* discard the superfluous original content */
+			vfree(buffer);
+			buffer = NULL;
+			if (rc) {
+				fw_free_paged_buf(fw_priv);
+				continue;
+			}
+		} else {
+			dev_dbg(device, "direct-loading %s\n",
+				fw_priv->fw_name);
+			if (!fw_priv->data)
+				fw_priv->data = buffer;
+			fw_priv->size = size;
+		}
 		fw_state_done(fw_priv);
 		break;
 	}
@@ -645,7 +768,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
-	ret = fw_get_filesystem_firmware(device, fw->priv);
+	ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
+#ifdef CONFIG_FW_LOADER_COMPRESS
+	if (ret == -ENOENT)
+		ret = fw_get_filesystem_firmware(device, fw->priv, ".xz",
+						 fw_decompress_xz);
+#endif
+
 	if (ret) {
 		if (!(opt_flags & FW_OPT_NO_WARN))
 			dev_warn(device,
-- 
2.16.4

Re: [PATCH v2.5 2/3] firmware: Add support for loading compressed files

[Luis Chamberlain]

Sorry for the late review... Ah!

On Tue, Jun 11, 2019 at 02:26:25PM +0200, Takashi Iwai wrote:
> @@ -354,7 +454,12 @@ module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
>  MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
>  
>  static int
> -fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
> +fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
> +			   const char *suffix,
> +			   int (*decompress)(struct device *dev,
> +					     struct fw_priv *fw_priv,
> +					     size_t in_size,
> +					     const void *in_buffer))

I *think* this could be cleaner, I'll elaborate below.

> @@ -645,7 +768,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>  	if (ret <= 0) /* error or already assigned */
>  		goto out;
>  
> -	ret = fw_get_filesystem_firmware(device, fw->priv);
> +	ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
> +#ifdef CONFIG_FW_LOADER_COMPRESS
> +	if (ret == -ENOENT)
> +		ret = fw_get_filesystem_firmware(device, fw->priv, ".xz",
> +						 fw_decompress_xz);
> +#endif

Hrm, and let more #ifdef'ery.

And so if someone wants to add bzip, we'd add yet-another if else on the
return value of this call... and yet more #ifdefs.

We already have a list of paths supported. It seems what we need instead
is a list of supported suffixes, and a respective structure which then
has its set of callbacks for posthandling.

This way, this could all be handled inside fw_get_filesystem_firmware()
neatly, and we can just strive towards avoiding #ifdef'ery.

Since I'm late to review, this could be done in the future, but I do
think something along these lines would make the code more maintainable
and extensible.

  Luis

Re: [PATCH v2.5 2/3] firmware: Add support for loading compressed files

[Takashi Iwai]

On Thu, 20 Jun 2019 01:26:47 +0200,
Luis Chamberlain wrote:
> 
> Sorry for the late review... Ah!

No problem, thanks for review.

> On Tue, Jun 11, 2019 at 02:26:25PM +0200, Takashi Iwai wrote:
> > @@ -354,7 +454,12 @@ module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
> >  MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
> >  
> >  static int
> > -fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
> > +fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
> > +			   const char *suffix,
> > +			   int (*decompress)(struct device *dev,
> > +					     struct fw_priv *fw_priv,
> > +					     size_t in_size,
> > +					     const void *in_buffer))
> 
> I *think* this could be cleaner, I'll elaborate below.
> 
> > @@ -645,7 +768,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> >  	if (ret <= 0) /* error or already assigned */
> >  		goto out;
> >  
> > -	ret = fw_get_filesystem_firmware(device, fw->priv);
> > +	ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
> > +#ifdef CONFIG_FW_LOADER_COMPRESS
> > +	if (ret == -ENOENT)
> > +		ret = fw_get_filesystem_firmware(device, fw->priv, ".xz",
> > +						 fw_decompress_xz);
> > +#endif
> 
> Hrm, and let more #ifdef'ery.
> 
> And so if someone wants to add bzip, we'd add yet-another if else on the
> return value of this call... and yet more #ifdefs.
> 
> We already have a list of paths supported. It seems what we need instead
> is a list of supported suffixes, and a respective structure which then
> has its set of callbacks for posthandling.
> 
> This way, this could all be handled inside fw_get_filesystem_firmware()
> neatly, and we can just strive towards avoiding #ifdef'ery.

Yes, I had similar idea.  Actually my plan for multiple compression
formats was:

- Move the decompression part into another file, e.g. decompress_xz.c
  and change in Makefile:
  firmware_class-$(CONFIG_FW_LOADER_COMPRESS_XZ) += decompress_xz.o
  
- Create a table of the extension and the decompression,

  static struct fw_decompression_table fw_decompressions[] = {
	{ "", NULL },
#ifdef CONFIG_FW_LOADER_COMPRESS_XZ
	{ ".xz", fw_decompress_xz },
#endif
#ifdef CONFIG_FW_LOADER_COMPRESS_BZIP2
	{ ".bz2", fw_decompress_bzip2 },
#endif
	.....
  };

and call it

	for (i = 0; i < ARRAY_SIZE(fw_decompressions); i++) {
		ret = fw_get_filesystem_firmware(device, fw->priv,
						 fw_decompressions[i].extesnion,
						 fw_decompressions[i].func);
		if (ret != -ENOENT)
			break;
	}


The patch was submitted in the current form just because it's simpler
for a single compression case.  But as you can see in the v2 patchset
change, it has already the decompression function pointer, so that the
code can be cleaned up / extended easily later if multiple formats are
supported like the above.


BTW, I took a look at other compression methods, and found that LZ4 is
a bit tough with the current API.  ZSTD should be relatively easy, as
it can get the whole decompressed size at first, so we'll be able to
do vmalloc().  For others, we may need a streaming decompression and
growing buffer per page.  But LZ4 decompression API doesn't seem
allowing the partial decompression-and-continue as I used for XZ, so
it'll be tricky.
GZIP and BZIP2 should work in a streaming mode like XZ, I suppose.


thanks,

Takashi

Re: [PATCH v2.5 2/3] firmware: Add support for loading compressed files

[Greg Kroah-Hartman]

On Thu, Jun 20, 2019 at 09:36:03AM +0200, Takashi Iwai wrote:
> On Thu, 20 Jun 2019 01:26:47 +0200,
> Luis Chamberlain wrote:
> > 
> > Sorry for the late review... Ah!
> 
> No problem, thanks for review.
> 
> > On Tue, Jun 11, 2019 at 02:26:25PM +0200, Takashi Iwai wrote:
> > > @@ -354,7 +454,12 @@ module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
> > >  MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
> > >  
> > >  static int
> > > -fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
> > > +fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
> > > +			   const char *suffix,
> > > +			   int (*decompress)(struct device *dev,
> > > +					     struct fw_priv *fw_priv,
> > > +					     size_t in_size,
> > > +					     const void *in_buffer))
> > 
> > I *think* this could be cleaner, I'll elaborate below.
> > 
> > > @@ -645,7 +768,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> > >  	if (ret <= 0) /* error or already assigned */
> > >  		goto out;
> > >  
> > > -	ret = fw_get_filesystem_firmware(device, fw->priv);
> > > +	ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
> > > +#ifdef CONFIG_FW_LOADER_COMPRESS
> > > +	if (ret == -ENOENT)
> > > +		ret = fw_get_filesystem_firmware(device, fw->priv, ".xz",
> > > +						 fw_decompress_xz);
> > > +#endif
> > 
> > Hrm, and let more #ifdef'ery.
> > 
> > And so if someone wants to add bzip, we'd add yet-another if else on the
> > return value of this call... and yet more #ifdefs.
> > 
> > We already have a list of paths supported. It seems what we need instead
> > is a list of supported suffixes, and a respective structure which then
> > has its set of callbacks for posthandling.
> > 
> > This way, this could all be handled inside fw_get_filesystem_firmware()
> > neatly, and we can just strive towards avoiding #ifdef'ery.
> 
> Yes, I had similar idea.  Actually my plan for multiple compression
> formats was:
> 
> - Move the decompression part into another file, e.g. decompress_xz.c
>   and change in Makefile:
>   firmware_class-$(CONFIG_FW_LOADER_COMPRESS_XZ) += decompress_xz.o
>   
> - Create a table of the extension and the decompression,
> 
>   static struct fw_decompression_table fw_decompressions[] = {
> 	{ "", NULL },
> #ifdef CONFIG_FW_LOADER_COMPRESS_XZ
> 	{ ".xz", fw_decompress_xz },
> #endif
> #ifdef CONFIG_FW_LOADER_COMPRESS_BZIP2
> 	{ ".bz2", fw_decompress_bzip2 },
> #endif
> 	.....
>   };

But why?  Why not just stick with one for now, we don't need a zillion
different formats to start with.  Let's just stick with .xz and that's
it.  There is no need to do anything else for the foreseeable future.

thanks,

greg k-h

Re: [PATCH v2.5 2/3] firmware: Add support for loading compressed files

[Takashi Iwai]

On Thu, 20 Jun 2019 10:10:03 +0200,
Greg Kroah-Hartman wrote:
> 
> On Thu, Jun 20, 2019 at 09:36:03AM +0200, Takashi Iwai wrote:
> > On Thu, 20 Jun 2019 01:26:47 +0200,
> > Luis Chamberlain wrote:
> > > 
> > > Sorry for the late review... Ah!
> > 
> > No problem, thanks for review.
> > 
> > > On Tue, Jun 11, 2019 at 02:26:25PM +0200, Takashi Iwai wrote:
> > > > @@ -354,7 +454,12 @@ module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
> > > >  MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
> > > >  
> > > >  static int
> > > > -fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
> > > > +fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
> > > > +			   const char *suffix,
> > > > +			   int (*decompress)(struct device *dev,
> > > > +					     struct fw_priv *fw_priv,
> > > > +					     size_t in_size,
> > > > +					     const void *in_buffer))
> > > 
> > > I *think* this could be cleaner, I'll elaborate below.
> > > 
> > > > @@ -645,7 +768,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> > > >  	if (ret <= 0) /* error or already assigned */
> > > >  		goto out;
> > > >  
> > > > -	ret = fw_get_filesystem_firmware(device, fw->priv);
> > > > +	ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
> > > > +#ifdef CONFIG_FW_LOADER_COMPRESS
> > > > +	if (ret == -ENOENT)
> > > > +		ret = fw_get_filesystem_firmware(device, fw->priv, ".xz",
> > > > +						 fw_decompress_xz);
> > > > +#endif
> > > 
> > > Hrm, and let more #ifdef'ery.
> > > 
> > > And so if someone wants to add bzip, we'd add yet-another if else on the
> > > return value of this call... and yet more #ifdefs.
> > > 
> > > We already have a list of paths supported. It seems what we need instead
> > > is a list of supported suffixes, and a respective structure which then
> > > has its set of callbacks for posthandling.
> > > 
> > > This way, this could all be handled inside fw_get_filesystem_firmware()
> > > neatly, and we can just strive towards avoiding #ifdef'ery.
> > 
> > Yes, I had similar idea.  Actually my plan for multiple compression
> > formats was:
> > 
> > - Move the decompression part into another file, e.g. decompress_xz.c
> >   and change in Makefile:
> >   firmware_class-$(CONFIG_FW_LOADER_COMPRESS_XZ) += decompress_xz.o
> >   
> > - Create a table of the extension and the decompression,
> > 
> >   static struct fw_decompression_table fw_decompressions[] = {
> > 	{ "", NULL },
> > #ifdef CONFIG_FW_LOADER_COMPRESS_XZ
> > 	{ ".xz", fw_decompress_xz },
> > #endif
> > #ifdef CONFIG_FW_LOADER_COMPRESS_BZIP2
> > 	{ ".bz2", fw_decompress_bzip2 },
> > #endif
> > 	.....
> >   };
> 
> But why?  Why not just stick with one for now, we don't need a zillion
> different formats to start with.  Let's just stick with .xz and that's
> it.  There is no need to do anything else for the foreseeable future.

Yeah, that's the reason I submitted the patch in the current form;
XZ format should be good enough and it's simpler for a single format,
after all.  The suggestion above is only for the case we need to
support multiple formats.

Maybe we may want to support ZSTD in future, as Fedora is moving
toward to that format.  Once when the time comes, we can revisit how
the things can be cleaned up.
(And, I heard Ubuntu switching to LZ4, but LZ4 is difficult, as
mentioned in the previous mail...)


thanks,

Takashi

[PATCH v2.5 3/3] selftests: firmware: Add compressed firmware tests

[Takashi Iwai]

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@suse.de>
---
 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

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

[Greg Kroah-Hartman]

On Tue, Jun 11, 2019 at 02:26:23PM +0200, Takashi Iwai wrote:
> [resubmitted with the missing patch]
> 
> Hi,
> 
> here are the rest and the main part of patches to add the support for
> loading the compressed firmware files.  The patch was slightly
> refactored for more easily enhancing for other compression formats (if
> anyone wants).  Also the selftest patch is included.  The
> functionality doesn't change from the previous patchset.

Looks great, thanks for doing this.  All now queued up.

greg k-h