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

[Takashi Iwai <tiwai@suse.de>]

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