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