From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753446AbdK2Bo5 (ORCPT ); Tue, 28 Nov 2017 20:44:57 -0500 Received: from mail-ua0-f194.google.com ([209.85.217.194]:33781 "EHLO mail-ua0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752359AbdK2Boz (ORCPT ); Tue, 28 Nov 2017 20:44:55 -0500 X-Google-Smtp-Source: AGs4zMZhzT6Ds3x0iMMDX5rr32BPFJUs7zmCQc81Bpl5JkraAWE0L+R1DQdSgDRVSJ2BmT6FTjq1SP0Z17EPoWwSMiU= MIME-Version: 1.0 In-Reply-To: References: From: Kees Cook Date: Tue, 28 Nov 2017 17:44:52 -0800 X-Google-Sender-Auth: siGbS6i5FP_9GITEKbTbtb7QRmc Message-ID: Subject: Re: [PATCH] pstore: add lz4hc and 842 compression support To: Geliang Tang Cc: Anton Vorontsov , Colin Cross , Tony Luck , LKML Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Sorry for the delay on this review! On Mon, Aug 28, 2017 at 6:56 AM, Geliang Tang wrote: > Currently, pstore has supported three compression algorithms: zlib, > lzo and lz4. This patch added two more compression algorithms: lz4hc > and 842. > > Signed-off-by: Geliang Tang > --- > fs/pstore/Kconfig | 14 ++++++ > fs/pstore/platform.c | 128 +++++++++++++++++++++++++++++++++++++++++++++------ > 2 files changed, 127 insertions(+), 15 deletions(-) > > diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig > index b42e5bd..e288596 100644 > --- a/fs/pstore/Kconfig > +++ b/fs/pstore/Kconfig > @@ -39,6 +39,20 @@ config PSTORE_LZ4_COMPRESS > select LZ4_DECOMPRESS > help > This option enables LZ4 compression algorithm support. > + > +config PSTORE_LZ4HC_COMPRESS > + bool "LZ4HC" > + select LZ4HC_COMPRESS > + select LZ4_DECOMPRESS > + help > + This option enables LZ4 high compression mode algorithm. > + > +config PSTORE_842_COMPRESS > + bool "842" > + select 842_COMPRESS > + select 842_DECOMPRESS > + help > + This option enables 842 compression algorithm support. > endchoice I'm fine with these short descriptions. checkpatch's warning for "choice" options doesn't always make sense. :) > > config PSTORE_CONSOLE > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c > index 2b21d18..252c320 100644 > --- a/fs/pstore/platform.c > +++ b/fs/pstore/platform.c > @@ -34,9 +34,12 @@ > #ifdef CONFIG_PSTORE_LZO_COMPRESS > #include > #endif > -#ifdef CONFIG_PSTORE_LZ4_COMPRESS > +#if defined(CONFIG_PSTORE_LZ4_COMPRESS) || defined(CONFIG_PSTORE_LZ4HC_COMPRESS) > #include > #endif > +#ifdef CONFIG_PSTORE_842_COMPRESS > +#include > +#endif > #include > #include > #include > @@ -337,20 +340,7 @@ static const struct pstore_zbackend backend_lzo = { > }; > #endif > > -#ifdef CONFIG_PSTORE_LZ4_COMPRESS > -static int compress_lz4(const void *in, void *out, size_t inlen, size_t outlen) > -{ > - int ret; > - > - ret = LZ4_compress_default(in, out, inlen, outlen, workspace); > - if (!ret) { > - pr_err("LZ4_compress_default error; compression failed!\n"); > - return -EIO; > - } > - > - return ret; > -} > - > +#if defined(CONFIG_PSTORE_LZ4_COMPRESS) || defined(CONFIG_PSTORE_LZ4HC_COMPRESS) > static int decompress_lz4(void *in, void *out, size_t inlen, size_t outlen) > { > int ret; > @@ -392,6 +382,21 @@ static void free_lz4(void) > big_oops_buf = NULL; > big_oops_buf_sz = 0; > } > +#endif > + > +#ifdef CONFIG_PSTORE_LZ4_COMPRESS > +static int compress_lz4(const void *in, void *out, size_t inlen, size_t outlen) > +{ > + int ret; > + > + ret = LZ4_compress_default(in, out, inlen, outlen, workspace); > + if (!ret) { > + pr_err("LZ4_compress_default error; compression failed!\n"); > + return -EIO; > + } > + > + return ret; Other compress/decompress return outlen, rather than ret. Is "ret" the outlen here? > +} > > static const struct pstore_zbackend backend_lz4 = { > .compress = compress_lz4, > @@ -402,6 +407,95 @@ static const struct pstore_zbackend backend_lz4 = { > }; > #endif > > +#ifdef CONFIG_PSTORE_LZ4HC_COMPRESS > +static int compress_lz4hc(const void *in, void *out, > + size_t inlen, size_t outlen) > +{ > + int ret; > + > + ret = LZ4_compress_HC(in, out, inlen, outlen, > + LZ4HC_DEFAULT_CLEVEL, workspace); > + if (!ret) { > + pr_err("LZ4_compress_HC error; compression failed!\n"); > + return -EIO; > + } > + > + return ret; And here? > +} > + > +static const struct pstore_zbackend backend_lz4hc = { > + .compress = compress_lz4hc, > + .decompress = decompress_lz4, > + .allocate = allocate_lz4, > + .free = free_lz4, > + .name = "lz4hc", > +}; > +#endif > + > +#ifdef CONFIG_PSTORE_842_COMPRESS > +static int compress_842(const void *in, void *out, size_t inlen, size_t outlen) > +{ > + int ret; > + unsigned int len; > + > + ret = sw842_compress(in, inlen, out, &len, workspace); > + if (!ret) { > + pr_err("sw842_compress error; compression failed!\n"); > + return -EIO; > + } > + outlen = len; This has no effect. What was intended? > + > + return ret; > +} > + > +static int decompress_842(void *in, void *out, size_t inlen, size_t outlen) > +{ > + int ret; > + unsigned int len; > + > + ret = sw842_decompress(in, inlen, out, &len); > + if (ret < 0) { > + pr_err("sw842_decompress error, ret = %d!\n", ret); > + return -EIO; > + } > + outlen = len; Same. > + > + return ret; > +} > + > +static void allocate_842(void) > +{ > + big_oops_buf_sz = psinfo->bufsize * 2; > + big_oops_buf = kmalloc(big_oops_buf_sz, GFP_KERNEL); > + if (big_oops_buf) { > + workspace = kmalloc(SW842_MEM_COMPRESS, GFP_KERNEL); > + if (!workspace) { > + kfree(big_oops_buf); > + big_oops_buf = NULL; > + } > + } else { > + pr_err("No memory for uncompressed data; skipping compression\n"); > + workspace = NULL; > + } > +} > + > +static void free_842(void) > +{ > + kfree(workspace); > + kfree(big_oops_buf); > + big_oops_buf = NULL; > + big_oops_buf_sz = 0; > +} > + > +static const struct pstore_zbackend backend_842 = { > + .compress = compress_842, > + .decompress = decompress_842, > + .allocate = allocate_842, > + .free = free_842, > + .name = "842", > +}; > +#endif > + > static const struct pstore_zbackend *zbackend = > #if defined(CONFIG_PSTORE_ZLIB_COMPRESS) > &backend_zlib; > @@ -409,6 +503,10 @@ static const struct pstore_zbackend *zbackend = > &backend_lzo; > #elif defined(CONFIG_PSTORE_LZ4_COMPRESS) > &backend_lz4; > +#elif defined(CONFIG_PSTORE_LZ4HC_COMPRESS) > + &backend_lz4hc; > +#elif defined(CONFIG_PSTORE_842_COMPRESS) > + &backend_842; > #else > NULL; > #endif > -- > 2.9.3 > Otherwise this all looks good. Thanks! -Kees -- Kees Cook Pixel Security