Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 17 Feb 2021 14:22:41 +0100
Message-ID: <s5hczwy4yn2.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Luis Chamberlain <mcgrof@kernel.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC 1/4] firmware: Add the support for ZSTD-compressed firmware files
In-Reply-To: <20210217131644.GM4332@42.do-not-panic.com>
References: <20210127154939.13288-1-tiwai@suse.de>
        <20210127154939.13288-2-tiwai@suse.de>
        <20210217131644.GM4332@42.do-not-panic.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 17 Feb 2021 14:16:44 +0100,
Luis Chamberlain wrote:
> 
> On Wed, Jan 27, 2021 at 04:49:36PM +0100, Takashi Iwai wrote:
> > Due to the popular demands on ZSTD, here is a patch to add a support
> > of ZSTD-compressed firmware files via the direct firmware loader.
> > It's just like XZ-compressed file support, providing a decompressor
> > with ZSTD.  Since ZSTD API can give the decompression size beforehand,
> > the code is even simpler than XZ.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/base/firmware_loader/Kconfig | 21 ++++++--
> >  drivers/base/firmware_loader/main.c  | 74 ++++++++++++++++++++++++++--
> >  2 files changed, 87 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig
> > index 5b24f3959255..f5307978927c 100644
> > --- a/drivers/base/firmware_loader/Kconfig
> > +++ b/drivers/base/firmware_loader/Kconfig
> > @@ -157,17 +157,28 @@ config FW_LOADER_USER_HELPER_FALLBACK
> >  
> >  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).
> > +if FW_LOADER_COMPRESS
> > +config FW_LOADER_COMPRESS_XZ
> > +	bool "Enable XZ-compressed firmware support"
> > +	select FW_LOADER_PAGED_BUF
> > +	select XZ_DEC
> > +	help
> > +	  This option adds the support for XZ-compressed files.
> > +	  The files have to be compressed with either none or crc32
> > +	  integrity check type (pass "-C crc32" option to xz command).
> > +
> > +config FW_LOADER_COMPRESS_ZSTD
> > +	bool "Enable ZSTD-compressed firmware support"
> > +	select ZSTD_DECOMPRESS
> > +	help
> > +	  This option adds the support for ZSTD-compressed files.
> > +endif # FW_LOADER_COMPRESS
> >  
> >  config FW_CACHE
> >  	bool "Enable firmware caching during suspend"
> > diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> > index 78355095e00d..71332ed4959d 100644
> > --- a/drivers/base/firmware_loader/main.c
> > +++ b/drivers/base/firmware_loader/main.c
> > @@ -34,6 +34,7 @@
> >  #include <linux/syscore_ops.h>
> >  #include <linux/reboot.h>
> >  #include <linux/security.h>
> > +#include <linux/zstd.h>
> >  #include <linux/xz.h>
> >  
> >  #include <generated/utsrelease.h>
> > @@ -362,10 +363,72 @@ int fw_map_paged_buf(struct fw_priv *fw_priv)
> >  }
> >  #endif
> >  
> > +/*
> > + * ZSTD-compressed firmware support
> > + */
> > +#ifdef CONFIG_FW_LOADER_COMPRESS_ZSTD
> > +static int fw_decompress_zstd(struct device *dev, struct fw_priv *fw_priv,
> > +			      size_t in_size, const void *in_buffer)
> > +{
> > +	size_t len, out_size, workspace_size;
> > +	void *workspace, *out_buf;
> > +	ZSTD_DCtx *ctx;
> > +	int err;
> > +
> > +	if (fw_priv->data) {
> > +		out_size = fw_priv->allocated_size;
> > +		out_buf = fw_priv->data;
> > +	} else {
> > +		out_size = ZSTD_findDecompressedSize(in_buffer, in_size);
> > +		if (out_size == ZSTD_CONTENTSIZE_UNKNOWN ||
> > +		    out_size == ZSTD_CONTENTSIZE_ERROR) {
> > +			dev_dbg(dev, "%s: invalid decompression size\n", __func__);
> > +			return -EINVAL;
> > +		}
> > +		out_buf = vzalloc(out_size);
> > +		if (!out_buf)
> > +			return -ENOMEM;
> > +	}
> > +
> > +	workspace_size = ZSTD_DCtxWorkspaceBound();
> > +	workspace = kvzalloc(workspace_size, GFP_KERNEL);
> > +	if (!workspace) {
> > +		err = -ENOMEM;
> > +		goto error;
> > +	}
> > +
> > +	ctx = ZSTD_initDCtx(workspace, workspace_size);
> > +	if (!ctx) {
> > +		dev_dbg(dev, "%s: failed to initialize context\n", __func__);
> > +		err = -EINVAL;
> > +		goto error;
> > +	}
> > +
> > +	len = ZSTD_decompressDCtx(ctx, out_buf, out_size, in_buffer, in_size);
> > +	if (ZSTD_isError(len)) {
> > +		dev_dbg(dev, "%s: failed to decompress: %d\n", __func__,
> > +			ZSTD_getErrorCode(len));
> > +		err = -EINVAL;
> > +		goto error;
> > +	}
> > +
> > +	fw_priv->size = len;
> > +	if (!fw_priv->data)
> > +		fw_priv->data = out_buf;
> > +	err = 0;
> > +
> > + error:
> > +	kvfree(workspace);
> > +	if (!fw_priv->data)
> > +		vfree(out_buf);
> > +	return err;
> > +}
> > +#endif /* CONFIG_FW_LOADER_COMPRESS_ZSTD */
> > +
> >  /*
> >   * XZ-compressed firmware support
> >   */
> > -#ifdef CONFIG_FW_LOADER_COMPRESS
> > +#ifdef CONFIG_FW_LOADER_COMPRESS_XZ
> >  /* show an error and return the standard error code */
> >  static int fw_decompress_xz_error(struct device *dev, enum xz_ret xz_ret)
> >  {
> > @@ -459,7 +522,7 @@ static int fw_decompress_xz(struct device *dev, struct fw_priv *fw_priv,
> >  	else
> >  		return fw_decompress_xz_pages(dev, fw_priv, in_size, in_buffer);
> >  }
> > -#endif /* CONFIG_FW_LOADER_COMPRESS */
> > +#endif /* CONFIG_FW_LOADER_COMPRESS_XZ */
> >  
> >  /* direct firmware loading support */
> >  static char fw_path_para[256];
> > @@ -814,7 +877,12 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> >  	if (!(opt_flags & FW_OPT_PARTIAL))
> >  		nondirect = true;
> >  
> > -#ifdef CONFIG_FW_LOADER_COMPRESS
> > +#ifdef CONFIG_FW_LOADER_COMPRESS_ZSTD
> > +	if (ret == -ENOENT && nondirect)
> > +		ret = fw_get_filesystem_firmware(device, fw->priv, ".zst",
> > +						 fw_decompress_zstd);
> > +#endif
> > +#ifdef CONFIG_FW_LOADER_COMPRESS_XZ
> >  	if (ret == -ENOENT && nondirect)
> >  		ret = fw_get_filesystem_firmware(device, fw->priv, ".xz",
> >  						 fw_decompress_xz);
> 
> Since this is growing, it would be nicer if we avoid the #ifdef mess and
> simply have code iterate over supported compressions, and this is
> observed here in one line.

If it ever grows further, yeah, but I guess this won't happen so
often :)  As of now, I chose the open-code as there are only two.


thanks,

Takashi