[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180421101449.50419733@bbrezillon>
Date: Sat, 21 Apr 2018 10:14:49 +0200
From: Boris Brezillon <boris.brezillon@...tlin.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: LKML <linux-kernel@...r.kernel.org>, Kees Cook <keescook@...omium.org>,
Segher Boessenkool <segher@...nel.crashing.org>, Kernel Hardening
<kernel-hardening@...ts.openwall.com>, Andrew Morton
<akpm@...uxfoundation.org>, Boris Brezillon
<boris.brezillon@...e-electrons.com>, Richard Weinberger <richard@....at>,
David Woodhouse <dwmw2@...radead.org>, Alasdair Kergon <agk@...hat.com>,
Mike Snitzer <snitzer@...hat.com>, Anton Vorontsov <anton@...msg.org>,
Colin Cross <ccross@...roid.com>, Tony Luck <tony.luck@...el.com>
Subject: Re: [patch V2 5/8] rslib: Split rs control struct
On Thu, 19 Apr 2018 12:04:46 +0200
Thomas Gleixner <tglx@...utronix.de> wrote:
> From: Thomas Gleixner <tglx@...utronix.de>
>
> The decoder library uses variable length arrays on stack. To get rid of
> them it would be simple to allocate fixed length arrays on stack, but those
> might become rather large. The other solution is to allocate the buffers in
> the rs control structure, but this cannot be done as long as the structure
> can be shared by several users. Sharing is desired because the RS polynom
> tables are large and initialization is time consuming.
>
> To solve this split the codec information out of the control structure and
> have a pointer to a shared codec in it. Instantiate the control structure
> for each user, create a new codec if no shareable is avaiable yet. Adjust
> all affected usage sites to the new scheme.
>
> This allows to add per instance decoder buffers to the control structure
> later on.
>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Cc: Boris Brezillon <boris.brezillon@...e-electrons.com>
> Cc: Tony Luck <tony.luck@...el.com>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Segher Boessenkool <segher@...nel.crashing.org>
> Cc: Kernel Hardening <kernel-hardening@...ts.openwall.com>
> Cc: Richard Weinberger <richard@....at>
> Cc: Mike Snitzer <snitzer@...hat.com>
> Cc: Anton Vorontsov <anton@...msg.org>
> Cc: Colin Cross <ccross@...roid.com>
> Cc: Andrew Morton <akpm@...uxfoundation.org>
> Cc: David Woodhouse <dwmw2@...radead.org>
> Cc: Alasdair Kergon <agk@...hat.com>
>
> ---
> drivers/mtd/nand/raw/cafe_nand.c | 7 +
> drivers/mtd/nand/raw/diskonchip.c | 7 +
For NAND relate stuff:
Acked-by: Boris Brezillon <boris.brezillon@...tlin.com>
Don't know which release this patch series is targeting, but if it's
4.18 I'll need an immutable branch just in case other patches touch the
same files.
> include/linux/rslib.h | 18 +++--
> lib/reed_solomon/decode_rs.c | 1
> lib/reed_solomon/encode_rs.c | 1
> lib/reed_solomon/reed_solomon.c | 144 ++++++++++++++++++++++------------------
> 6 files changed, 104 insertions(+), 74 deletions(-)
>
> --- a/drivers/mtd/nand/raw/cafe_nand.c
> +++ b/drivers/mtd/nand/raw/cafe_nand.c
> @@ -394,12 +394,13 @@ static int cafe_nand_read_page(struct mt
>
> for (i=0; i<8; i+=2) {
> uint32_t tmp = cafe_readl(cafe, NAND_ECC_SYN01 + (i*2));
> - syn[i] = cafe->rs->index_of[tmp & 0xfff];
> - syn[i+1] = cafe->rs->index_of[(tmp >> 16) & 0xfff];
> +
> + syn[i] = cafe->rs->codec->index_of[tmp & 0xfff];
> + syn[i+1] = cafe->rs->codec->index_of[(tmp >> 16) & 0xfff];
> }
>
> n = decode_rs16(cafe->rs, NULL, NULL, 1367, syn, 0, pos, 0,
> - pat);
> + pat);
>
> for (i = 0; i < n; i++) {
> int p = pos[i];
> --- a/drivers/mtd/nand/raw/diskonchip.c
> +++ b/drivers/mtd/nand/raw/diskonchip.c
> @@ -142,6 +142,7 @@ static int doc_ecc_decode(struct rs_cont
> int i, j, nerr, errpos[8];
> uint8_t parity;
> uint16_t ds[4], s[5], tmp, errval[8], syn[4];
> + struct rs_codec *cd = rs->codec;
>
> memset(syn, 0, sizeof(syn));
> /* Convert the ecc bytes into words */
> @@ -162,15 +163,15 @@ static int doc_ecc_decode(struct rs_cont
> for (j = 1; j < NROOTS; j++) {
> if (ds[j] == 0)
> continue;
> - tmp = rs->index_of[ds[j]];
> + tmp = cd->index_of[ds[j]];
> for (i = 0; i < NROOTS; i++)
> - s[i] ^= rs->alpha_to[rs_modnn(rs, tmp + (FCR + i) * j)];
> + s[i] ^= cd->alpha_to[rs_modnn(cd, tmp + (FCR + i) * j)];
> }
>
> /* Calc syn[i] = s[i] / alpha^(v + i) */
> for (i = 0; i < NROOTS; i++) {
> if (s[i])
> - syn[i] = rs_modnn(rs, rs->index_of[s[i]] + (NN - FCR - i));
> + syn[i] = rs_modnn(cd, cd->index_of[s[i]] + (NN - FCR - i));
> }
> /* Call the decoder library */
> nerr = decode_rs16(rs, NULL, NULL, 1019, syn, 0, errpos, 0, errval);
> --- a/include/linux/rslib.h
> +++ b/include/linux/rslib.h
> @@ -13,7 +13,7 @@
> #include <linux/list.h>
>
> /**
> - * struct rs_control - rs control structure
> + * struct rs_codec - rs codec data
> *
> * @mm: Bits per symbol
> * @nn: Symbols per block (= (1<<mm)-1)
> @@ -27,9 +27,9 @@
> * @gfpoly: The primitive generator polynominal
> * @gffunc: Function to generate the field, if non-canonical representation
> * @users: Users of this structure
> - * @list: List entry for the rs control list
> + * @list: List entry for the rs codec list
> */
> -struct rs_control {
> +struct rs_codec {
> int mm;
> int nn;
> uint16_t *alpha_to;
> @@ -45,6 +45,14 @@ struct rs_control {
> struct list_head list;
> };
>
> +/**
> + * struct rs_control - rs control structure per instance
> + * @codec: The codec used for this instance
> + */
> +struct rs_control {
> + struct rs_codec *codec;
> +};
> +
> /* General purpose RS codec, 8-bit data width, symbol width 1-15 bit */
> #ifdef CONFIG_REED_SOLOMON_ENC8
> int encode_rs8(struct rs_control *rs, uint8_t *data, int len, uint16_t *par,
> @@ -78,7 +86,7 @@ void free_rs(struct rs_control *rs);
>
> /** modulo replacement for galois field arithmetics
> *
> - * @rs: the rs control structure
> + * @rs: Pointer to the RS codec
> * @x: the value to reduce
> *
> * where
> @@ -88,7 +96,7 @@ void free_rs(struct rs_control *rs);
> * Simple arithmetic modulo would return a wrong result for values
> * >= 3 * rs->nn
> */
> -static inline int rs_modnn(struct rs_control *rs, int x)
> +static inline int rs_modnn(struct rs_codec *rs, int x)
> {
> while (x >= rs->nn) {
> x -= rs->nn;
> --- a/lib/reed_solomon/decode_rs.c
> +++ b/lib/reed_solomon/decode_rs.c
> @@ -10,6 +10,7 @@
> * Generic data width independent code which is included by the wrappers.
> */
> {
> + struct rs_codec *rs = rsc->codec;
> int deg_lambda, el, deg_omega;
> int i, j, r, k, pad;
> int nn = rs->nn;
> --- a/lib/reed_solomon/encode_rs.c
> +++ b/lib/reed_solomon/encode_rs.c
> @@ -10,6 +10,7 @@
> * Generic data width independent code which is included by the wrappers.
> */
> {
> + struct rs_codec *rs = rsc->codec;
> int i, j, pad;
> int nn = rs->nn;
> int nroots = rs->nroots;
> --- a/lib/reed_solomon/reed_solomon.c
> +++ b/lib/reed_solomon/reed_solomon.c
> @@ -11,22 +11,23 @@
> *
> * The generic Reed Solomon library provides runtime configurable
> * encoding / decoding of RS codes.
> - * Each user must call init_rs to get a pointer to a rs_control
> - * structure for the given rs parameters. This structure is either
> - * generated or a already available matching control structure is used.
> - * If a structure is generated then the polynomial arrays for
> - * fast encoding / decoding are built. This can take some time so
> - * make sure not to call this function from a time critical path.
> - * Usually a module / driver should initialize the necessary
> - * rs_control structure on module / driver init and release it
> - * on exit.
> - * The encoding puts the calculated syndrome into a given syndrome
> - * buffer.
> - * The decoding is a two step process. The first step calculates
> - * the syndrome over the received (data + syndrome) and calls the
> - * second stage, which does the decoding / error correction itself.
> - * Many hw encoders provide a syndrome calculation over the received
> - * data + syndrome and can call the second stage directly.
> + *
> + * Each user must call init_rs to get a pointer to a rs_control structure
> + * for the given rs parameters. The control struct is unique per instance.
> + * It points to a codec which can be shared by multiple control structures.
> + * If a codec is newly allocated then the polynomial arrays for fast
> + * encoding / decoding are built. This can take some time so make sure not
> + * to call this function from a time critical path. Usually a module /
> + * driver should initialize the necessary rs_control structure on module /
> + * driver init and release it on exit.
> + *
> + * The encoding puts the calculated syndrome into a given syndrome buffer.
> + *
> + * The decoding is a two step process. The first step calculates the
> + * syndrome over the received (data + syndrome) and calls the second stage,
> + * which does the decoding / error correction itself. Many hw encoders
> + * provide a syndrome calculation over the received data + syndrome and can
> + * call the second stage directly.
> */
> #include <linux/errno.h>
> #include <linux/kernel.h>
> @@ -36,13 +37,13 @@
> #include <linux/slab.h>
> #include <linux/mutex.h>
>
> -/* This list holds all currently allocated rs control structures */
> -static LIST_HEAD (rslist);
> +/* This list holds all currently allocated rs codec structures */
> +static LIST_HEAD(codec_list);
> /* Protection for the list */
> static DEFINE_MUTEX(rslistlock);
>
> /**
> - * rs_init - Initialize a Reed-Solomon codec
> + * codec_init - Initialize a Reed-Solomon codec
> * @symsize: symbol size, bits (1-8)
> * @gfpoly: Field generator polynomial coefficients
> * @gffunc: Field generator function
> @@ -50,18 +51,17 @@ static DEFINE_MUTEX(rslistlock);
> * @prim: primitive element to generate polynomial roots
> * @nroots: RS code generator polynomial degree (number of roots)
> *
> - * Allocate a control structure and the polynom arrays for faster
> + * Allocate a codec structure and the polynom arrays for faster
> * en/decoding. Fill the arrays according to the given parameters.
> */
> -static struct rs_control *rs_init(int symsize, int gfpoly, int (*gffunc)(int),
> - int fcr, int prim, int nroots)
> +static struct rs_codec *codec_init(int symsize, int gfpoly, int (*gffunc)(int),
> + int fcr, int prim, int nroots)
> {
> - struct rs_control *rs;
> int i, j, sr, root, iprim;
> + struct rs_codec *rs;
>
> - /* Allocate the control structure */
> - rs = kmalloc(sizeof (struct rs_control), GFP_KERNEL);
> - if (rs == NULL)
> + rs = kmalloc(sizeof(*rs), GFP_KERNEL);
> + if (!rs)
> return NULL;
>
> INIT_LIST_HEAD(&rs->list);
> @@ -138,6 +138,9 @@ static struct rs_control *rs_init(int sy
> /* convert rs->genpoly[] to index form for quicker encoding */
> for (i = 0; i <= nroots; i++)
> rs->genpoly[i] = rs->index_of[rs->genpoly[i]];
> +
> + rs->users = 1;
> + list_add(&rs->list, &codec_list);
> return rs;
>
> /* Error exit */
> @@ -154,26 +157,36 @@ static struct rs_control *rs_init(int sy
>
>
> /**
> - * free_rs - Free the rs control structure, if it is no longer used
> - * @rs: the control structure which is not longer used by the
> + * free_rs - Free the rs control structure
> + * @rs: The control structure which is not longer used by the
> * caller
> + *
> + * Free the control structure. If @rs is the last user of the associated
> + * codec, free the codec as well.
> */
> void free_rs(struct rs_control *rs)
> {
> + struct rs_codec *cd;
> +
> + if (!rs)
> + return;
> +
> + cd = rs->codec;
> mutex_lock(&rslistlock);
> - rs->users--;
> - if(!rs->users) {
> - list_del(&rs->list);
> - kfree(rs->alpha_to);
> - kfree(rs->index_of);
> - kfree(rs->genpoly);
> - kfree(rs);
> + cd->users--;
> + if(!cd->users) {
> + list_del(&cd->list);
> + kfree(cd->alpha_to);
> + kfree(cd->index_of);
> + kfree(cd->genpoly);
> + kfree(cd);
> }
> mutex_unlock(&rslistlock);
> + kfree(rs);
> }
>
> /**
> - * init_rs_internal - Find a matching or allocate a new rs control structure
> + * init_rs_internal - Allocate rs control, find a matching codec or allocate a new one
> * @symsize: the symbol size (number of bits)
> * @gfpoly: the extended Galois field generator polynomial coefficients,
> * with the 0th coefficient in the low order bit. The polynomial
> @@ -190,8 +203,8 @@ static struct rs_control *init_rs_intern
> int (*gffunc)(int), int fcr,
> int prim, int nroots)
> {
> - struct list_head *tmp;
> - struct rs_control *rs;
> + struct list_head *tmp;
> + struct rs_control *rs;
>
> /* Sanity checks */
> if (symsize < 1)
> @@ -203,33 +216,39 @@ static struct rs_control *init_rs_intern
> if (nroots < 0 || nroots >= (1<<symsize))
> return NULL;
>
> + rs = kzalloc(sizeof(*rs), GFP_KERNEL);
> + if (!rs)
> + return NULL;
> +
> mutex_lock(&rslistlock);
>
> /* Walk through the list and look for a matching entry */
> - list_for_each(tmp, &rslist) {
> - rs = list_entry(tmp, struct rs_control, list);
> - if (symsize != rs->mm)
> + list_for_each(tmp, &codec_list) {
> + struct rs_codec *cd = list_entry(tmp, struct rs_codec, list);
> +
> + if (symsize != cd->mm)
> continue;
> - if (gfpoly != rs->gfpoly)
> + if (gfpoly != cd->gfpoly)
> continue;
> - if (gffunc != rs->gffunc)
> + if (gffunc != cd->gffunc)
> continue;
> - if (fcr != rs->fcr)
> + if (fcr != cd->fcr)
> continue;
> - if (prim != rs->prim)
> + if (prim != cd->prim)
> continue;
> - if (nroots != rs->nroots)
> + if (nroots != cd->nroots)
> continue;
> /* We have a matching one already */
> - rs->users++;
> + cd->users++;
> + rs->codec = cd;
> goto out;
> }
>
> /* Create a new one */
> - rs = rs_init(symsize, gfpoly, gffunc, fcr, prim, nroots);
> - if (rs) {
> - rs->users = 1;
> - list_add(&rs->list, &rslist);
> + rs->codec = codec_init(symsize, gfpoly, gffunc, fcr, prim, nroots);
> + if (!rs->codec) {
> + kfree(rs);
> + rs = NULL;
> }
> out:
> mutex_unlock(&rslistlock);
> @@ -237,7 +256,7 @@ static struct rs_control *init_rs_intern
> }
>
> /**
> - * init_rs - Find a matching or allocate a new rs control structure
> + * init_rs - Create a RS control struct and initialize it
> * @symsize: the symbol size (number of bits)
> * @gfpoly: the extended Galois field generator polynomial coefficients,
> * with the 0th coefficient in the low order bit. The polynomial
> @@ -254,9 +273,8 @@ struct rs_control *init_rs(int symsize,
> }
>
> /**
> - * init_rs_non_canonical - Find a matching or allocate a new rs control
> - * structure, for fields with non-canonical
> - * representation
> + * init_rs_non_canonical - Allocate rs control struct for fields with
> + * non-canonical representation
> * @symsize: the symbol size (number of bits)
> * @gffunc: pointer to function to generate the next field element,
> * or the multiplicative identity element if given 0. Used
> @@ -275,7 +293,7 @@ struct rs_control *init_rs_non_canonical
> #ifdef CONFIG_REED_SOLOMON_ENC8
> /**
> * encode_rs8 - Calculate the parity for data values (8bit data width)
> - * @rs: the rs control structure
> + * @rsc: the rs control structure
> * @data: data field of a given type
> * @len: data length
> * @par: parity data, must be initialized by caller (usually all 0)
> @@ -285,7 +303,7 @@ struct rs_control *init_rs_non_canonical
> * symbol size > 8. The calling code must take care of encoding of the
> * syndrome result for storage itself.
> */
> -int encode_rs8(struct rs_control *rs, uint8_t *data, int len, uint16_t *par,
> +int encode_rs8(struct rs_control *rsc, uint8_t *data, int len, uint16_t *par,
> uint16_t invmsk)
> {
> #include "encode_rs.c"
> @@ -296,7 +314,7 @@ EXPORT_SYMBOL_GPL(encode_rs8);
> #ifdef CONFIG_REED_SOLOMON_DEC8
> /**
> * decode_rs8 - Decode codeword (8bit data width)
> - * @rs: the rs control structure
> + * @rsc: the rs control structure
> * @data: data field of a given type
> * @par: received parity data field
> * @len: data length
> @@ -311,7 +329,7 @@ EXPORT_SYMBOL_GPL(encode_rs8);
> * syndrome result and the received parity before calling this code.
> * Returns the number of corrected bits or -EBADMSG for uncorrectable errors.
> */
> -int decode_rs8(struct rs_control *rs, uint8_t *data, uint16_t *par, int len,
> +int decode_rs8(struct rs_control *rsc, uint8_t *data, uint16_t *par, int len,
> uint16_t *s, int no_eras, int *eras_pos, uint16_t invmsk,
> uint16_t *corr)
> {
> @@ -323,7 +341,7 @@ EXPORT_SYMBOL_GPL(decode_rs8);
> #ifdef CONFIG_REED_SOLOMON_ENC16
> /**
> * encode_rs16 - Calculate the parity for data values (16bit data width)
> - * @rs: the rs control structure
> + * @rsc: the rs control structure
> * @data: data field of a given type
> * @len: data length
> * @par: parity data, must be initialized by caller (usually all 0)
> @@ -331,7 +349,7 @@ EXPORT_SYMBOL_GPL(decode_rs8);
> *
> * Each field in the data array contains up to symbol size bits of valid data.
> */
> -int encode_rs16(struct rs_control *rs, uint16_t *data, int len, uint16_t *par,
> +int encode_rs16(struct rs_control *rsc, uint16_t *data, int len, uint16_t *par,
> uint16_t invmsk)
> {
> #include "encode_rs.c"
> @@ -342,7 +360,7 @@ EXPORT_SYMBOL_GPL(encode_rs16);
> #ifdef CONFIG_REED_SOLOMON_DEC16
> /**
> * decode_rs16 - Decode codeword (16bit data width)
> - * @rs: the rs control structure
> + * @rsc: the rs control structure
> * @data: data field of a given type
> * @par: received parity data field
> * @len: data length
> @@ -355,7 +373,7 @@ EXPORT_SYMBOL_GPL(encode_rs16);
> * Each field in the data array contains up to symbol size bits of valid data.
> * Returns the number of corrected bits or -EBADMSG for uncorrectable errors.
> */
> -int decode_rs16(struct rs_control *rs, uint16_t *data, uint16_t *par, int len,
> +int decode_rs16(struct rs_control *rsc, uint16_t *data, uint16_t *par, int len,
> uint16_t *s, int no_eras, int *eras_pos, uint16_t invmsk,
> uint16_t *corr)
> {
>
>
>
>
>
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
