[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 4 Apr 2018 21:52:34 +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 6/8] mtd: diskonchip: Allocate rs control per instance
On Wed, 28 Mar 2018 22:51:44 +0200
Thomas Gleixner <tglx@...utronix.de> wrote:
> The reed solomon library is moving the on stack decoder buffers into the rs
> control structure. That would break the DoC driver because multiple
> instances share the same control structure and can operate in parallel. At
> least in theory....
>
> Instantiate a rs control instance per DoC device to avoid that. The per
> instance buffer is fine as the operation on a single DoC instance is
> serialized by the MTD/NAND core.
>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
> drivers/mtd/nand/diskonchip.c | 65 ++++++++++++++++++++----------------------
> 1 file changed, 31 insertions(+), 34 deletions(-)
>
> --- a/drivers/mtd/nand/diskonchip.c
> +++ b/drivers/mtd/nand/diskonchip.c
> @@ -68,6 +68,7 @@ struct doc_priv {
> int curchip;
> int mh0_page;
> int mh1_page;
> + struct rs_control *rs_decoder;
> struct mtd_info *nextdoc;
>
> /* Handle the last stage of initialization (BBT scan, partitioning) */
> @@ -125,9 +126,6 @@ MODULE_PARM_DESC(doc_config_location, "P
> /* Number of symbols */
> #define NN 1023
>
> -/* the Reed Solomon control structure */
> -static struct rs_control *rs_decoder;
> -
> /*
> * The HW decoder in the DoC ASIC's provides us a error syndrome,
> * which we must convert to a standard syndrome usable by the generic
> @@ -933,7 +931,7 @@ static int doc200x_correct_data(struct m
> calc_ecc[i] = ReadDOC_(docptr, DoC_ECCSyndrome0 + i);
> }
>
> - ret = doc_ecc_decode(rs_decoder, dat, calc_ecc);
> + ret = doc_ecc_decode(doc->rs_decoder, dat, calc_ecc);
> if (ret > 0)
> printk(KERN_ERR "doc200x_correct_data corrected %d errors\n", ret);
> }
> @@ -1561,8 +1559,25 @@ static int __init doc_probe(unsigned lon
> goto fail;
> }
>
> +
> + /*
> + * Allocate a RS codec instance
> + *
> + * Symbolsize is 10 (bits)
> + * Primitve polynomial is x^10+x^3+1
> + * First consecutive root is 510
> + * Primitve element to generate roots = 1
> + * Generator polinomial degree = 4
> + */
> + doc = (struct doc_priv *) (nand + 1);
> + doc->rs_decoder = init_rs(10, 0x409, FCR, 1, NROOTS);
> + if (!doc->rs_decoder) {
> + pr_err("DiskOnChip: Could not create a RS codec\n");
> + ret = -ENOMEM;
> + goto fail_nand;
> + }
> +
> mtd = nand_to_mtd(nand);
> - doc = (struct doc_priv *) (nand + 1);
> nand->bbt_td = (struct nand_bbt_descr *) (doc + 1);
> nand->bbt_md = nand->bbt_td + 1;
>
> @@ -1612,19 +1627,24 @@ static int __init doc_probe(unsigned lon
> haven't yet added it. This is handled without incident by
> mtd_device_unregister, as far as I can tell. */
> nand_release(mtd);
> - kfree(nand);
> - goto fail;
> + goto fail_rs;
> }
>
> /* Success! */
> doclist = mtd;
> return 0;
>
> - notfound:
> +fail_rs:
> + free_rs(doc->rs_decoder);
> +fail_nand:
> + kfree(nand);
> + goto fail;
> +
> +notfound:
> /* Put back the contents of the DOCControl register, in case it's not
> actually a DiskOnChip. */
> WriteDOC(save_control, virtadr, DOCControl);
> - fail:
> +fail:
Can we simplify a bit the exit path (in other words, avoid the 'goto
fail' in the 'fail_nand' section) by initializing nand and doc to NULL
and then adding something like that to the 'fail' path:
if (doc && doc->rs_decoder)
free_rs(doc->rs_decoder);
kfree(nand);
> iounmap(virtadr);
>
> error_ioremap:
> @@ -1647,6 +1667,7 @@ static void release_nanddoc(void)
> nand_release(mtd);
> iounmap(doc->virtadr);
> release_mem_region(doc->physadr, DOC_IOREMAP_LEN);
> + kfree(doc->rs_decoder);
You mean free_rs() not kfree(), right?
> kfree(nand);
> }
> }
> @@ -1655,26 +1676,11 @@ static int __init init_nanddoc(void)
> {
> int i, ret = 0;
>
> - /* We could create the decoder on demand, if memory is a concern.
> - * This way we have it handy, if an error happens
> - *
> - * Symbolsize is 10 (bits)
> - * Primitve polynomial is x^10+x^3+1
> - * first consecutive root is 510
> - * primitve element to generate roots = 1
> - * generator polinomial degree = 4
> - */
> - rs_decoder = init_rs(10, 0x409, FCR, 1, NROOTS);
> - if (!rs_decoder) {
> - printk(KERN_ERR "DiskOnChip: Could not create a RS decoder\n");
> - return -ENOMEM;
> - }
> -
> if (doc_config_location) {
> printk(KERN_INFO "Using configured DiskOnChip probe address 0x%lx\n", doc_config_location);
> ret = doc_probe(doc_config_location);
> if (ret < 0)
> - goto outerr;
> + return ret;
> } else {
> for (i = 0; (doc_locations[i] != 0xffffffff); i++) {
> doc_probe(doc_locations[i]);
> @@ -1685,23 +1691,14 @@ static int __init init_nanddoc(void)
> if (!doclist) {
> printk(KERN_INFO "No valid DiskOnChip devices found\n");
> ret = -ENODEV;
> - goto outerr;
> }
> return 0;
> - outerr:
> - free_rs(rs_decoder);
> - return ret;
> }
>
> static void __exit cleanup_nanddoc(void)
> {
> /* Cleanup the nand/DoC resources */
> release_nanddoc();
> -
> - /* Free the reed solomon resources */
> - if (rs_decoder) {
> - free_rs(rs_decoder);
> - }
> }
>
> module_init(init_nanddoc);
>
>
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.
