| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <542D4A51.9060907@ti.com>
Date: Thu, 2 Oct 2014 15:51:29 +0300
From: Roger Quadros <rogerq@...com>
To: Rostislav Lisovy <lisovy@...il.com>,
David Woodhouse <dwmw2@...radead.org>,
Brian Norris <computersforpeace@...il.com>,
Tony Lindgren <tony@...mide.com>,
<linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
CC: <michal.vokac@...ap.cz>, <sojkam1@....cvut.cz>,
Rostislav Lisovy <lisovy@...ica.cz>
Subject: Re: [PATCH v2 2/2] mtd: nand: omap: Synchronize access to the ECC
engine
On 10/02/2014 03:16 PM, Rostislav Lisovy wrote:
> The AM335x Technical Reference Manual (spruh73j.pdf) says
> "Because the ECC engine includes only one accumulation context,
> it can be allocated to only one chip-select at a time ... "
> (7.1.3.3.12.3). Since the commit 97a288ba2cfa ("ARM: omap2+:
> gpmc-nand: Use dynamic platform_device_alloc()") gpmc-nand
> driver supports multiple NAND flash devices connected to
> the single controller. Use mutexes to restrict access
> to the ECC engine for single read/write operation at a time.
>
> Tested with custom AM335x board using 2x NAND flash chips.
>
> Signed-off-by: Rostislav Lisovy <lisovy@...ica.cz>
> ---
> Changes since v1:
> * Since not all the read/write operations are performed by the
> omap_read(write)_page_bch() functions use the locks directly on
> those places that configure the ECC engine (take the lock) and
> read the result from the ECC engine (release the lock).
> This approach should cover read/write operations with all
> possible ECC modes. (Roger Quadros)
>
Don't you think this approach is racy?
IMHO the lock must be held across the entire page operation
i.e.
hold ecc lock
ecc.hwctl
chip->read/write_buf
ecc.calculate
ecc.correct
release ecc lock
else we risk simultaneous NAND operations on multiple chips
stomping on each other in between the entire sequence.
Then on further investigation isn't nand_get_device() already doing the same
thing as you are attempting here?
The chip->controller->lock is meant for serializing NAND controller access.
so instead of adding a new lock in the omap2 nand driver we need to ensure that
we are maintaining the same nand_hw_control (controller) structure across multiple NAND chips.
Let's move this controller structure out of omap_nand_info and keep it global to the driver
and make sure every NAND instance uses it.
cheers,
-roger
>
> drivers/mtd/nand/omap2.c | 28 +++++++++++++++++++++++++---
> 1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index f0dcdb6..898cb44 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -24,6 +24,7 @@
> #include <linux/slab.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> +#include <linux/mutex.h>
>
> #include <linux/mtd/nand_bch.h>
> #include <linux/platform_data/elm.h>
> @@ -144,6 +145,12 @@ static u_char bch8_vector[] = {0xf3, 0xdb, 0x14, 0x16, 0x8b, 0xd2, 0xbe, 0xcc,
> 0xac, 0x6b, 0xff, 0x99, 0x7b};
> static u_char bch4_vector[] = {0x00, 0x6b, 0x31, 0xdd, 0x41, 0xbc, 0x10};
>
> +/*
> + * Because the ECC engine includes only one accumulation context,
> + * it can be allocated to only one chip-select at a time
> + */
> +static DEFINE_MUTEX(omap_eccengine_lock);
> +
> struct omap_nand_info {
> struct nand_hw_control controller;
> struct omap_nand_platform_data *pdata;
> @@ -926,10 +933,13 @@ static int omap_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
> struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
> mtd);
> u32 val;
> + int ret = 0;
>
> val = readl(info->reg.gpmc_ecc_config);
> - if (((val >> ECC_CONFIG_CS_SHIFT) & CS_MASK) != info->gpmc_cs)
> - return -EINVAL;
> + if (((val >> ECC_CONFIG_CS_SHIFT) & CS_MASK) != info->gpmc_cs) {
> + ret = -EINVAL;
> + goto leave;
> + }
>
> /* read ecc result */
> val = readl(info->reg.gpmc_ecc1_result);
> @@ -938,7 +948,10 @@ static int omap_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
> /* P2048o, P1024o, P512o, P256o, P2048e, P1024e, P512e, P256e */
> *ecc_code++ = ((val >> 8) & 0x0f) | ((val >> 20) & 0xf0);
>
> - return 0;
> +leave:
> + /* Release the ECC engine */
> + mutex_unlock(&omap_eccengine_lock);
> + return ret;
> }
>
> /**
> @@ -954,6 +967,9 @@ static void omap_enable_hwecc(struct mtd_info *mtd, int mode)
> unsigned int dev_width = (chip->options & NAND_BUSWIDTH_16) ? 1 : 0;
> u32 val;
>
> + /* ECC Engine is shared among multiple NAND devices */
> + mutex_lock(&omap_eccengine_lock);
> +
> /* clear ecc and enable bits */
> val = ECCCLEAR | ECC1;
> writel(val, info->reg.gpmc_ecc_control);
> @@ -1132,6 +1148,9 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, int mode)
> return;
> }
>
> + /* ECC Engine is shared among multiple NAND devices */
> + mutex_lock(&omap_eccengine_lock);
> +
> writel(ECC1, info->reg.gpmc_ecc_control);
>
> /* Configure ecc size for BCH */
> @@ -1252,6 +1271,7 @@ static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd,
> ecc_code[25] = ((val >> 0) & 0xFF);
> break;
> default:
> + mutex_unlock(&omap_eccengine_lock);
> return -EINVAL;
> }
>
> @@ -1280,12 +1300,14 @@ static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd,
> case OMAP_ECC_BCH16_CODE_HW:
> break;
> default:
> + mutex_unlock(&omap_eccengine_lock);
> return -EINVAL;
> }
>
> ecc_calc += eccbytes;
> }
>
> + mutex_unlock(&omap_eccengine_lock);
> return 0;
> }
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists