[PATCH v2 1/2] mtd: nand: omap: Do not use global variables

[PATCH v2 1/2] mtd: nand: omap: Do not use global variables

[Rostislav Lisovy]

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.
Remove global variable to make the code thread-safe.

Signed-off-by: Rostislav Lisovy <lisovy@merica.cz>
---
Changes since v1:
* Do not dynamically allocate the 'struct nand_ecclayout' but rather
  include it in the 'struct omap_nand_info' (Roger Quadros)


 drivers/mtd/nand/omap2.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 3b357e9..f0dcdb6 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -144,9 +144,6 @@ 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};
 
-/* oob info generated runtime depending on ecc algorithm and layout selected */
-static struct nand_ecclayout omap_oobinfo;
-
 struct omap_nand_info {
 	struct nand_hw_control		controller;
 	struct omap_nand_platform_data	*pdata;
@@ -168,6 +165,8 @@ struct omap_nand_info {
 	u_char				*buf;
 	int					buf_len;
 	struct gpmc_nand_regs		reg;
+	/* generated on runtime depending on ecc algorithm and layout selected */
+	struct nand_ecclayout		oobinfo;
 	/* fields specific for BCHx_HW ECC scheme */
 	struct device			*elm_dev;
 	struct device_node		*of_node;
@@ -1840,7 +1839,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 	}
 
 	/* populate MTD interface based on ECC scheme */
-	ecclayout		= &omap_oobinfo;
+	ecclayout		= &info->oobinfo;
 	switch (info->ecc_opt) {
 	case OMAP_ECC_HAM1_CODE_SW:
 		nand_chip->ecc.mode = NAND_ECC_SOFT;
-- 
1.9.1

[PATCH v2 2/2] mtd: nand: omap: Synchronize access to the ECC engine

[Rostislav Lisovy]

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@merica.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)


 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;
 }
 
-- 
1.9.1

Re: [PATCH v2 2/2] mtd: nand: omap: Synchronize access to the ECC engine

[Roger Quadros]

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@merica.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;
>  }
>  
> 

Re: [PATCH v2 2/2] mtd: nand: omap: Synchronize access to the ECC engine

[Rostislav Lisovy]

Hello Roger;
Thanks for the review.

On Čt, 2014-10-02 at 15:51 +0300, Roger Quadros wrote:
> Don't you think this approach is racy?
> 
> IMHO the lock must be held across the entire page operation

I still think it is done in this way.

> i.e.
> hold ecc lock
> ecc.hwctl
> chip->read/write_buf
> ecc.calculate
> ecc.correct
> release ecc lock

According to my understanding of the code 'ecc.correct' does not access
the ECC engine directly, it gets the '*read_ecc' and '*calc_ecc' from
the 'ecc.calculate'.

Once again the work flow you described + my locking approach:
* ecc.hwctl       <-- mutex_lock() just before accessing the ECC engine
* chip->read/write_buf
* ecc.calculate   <-- mutex_unlock() just after reading from ECC engine
* ecc.correct

> 
> 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.

Ok; I will take a look at it. It looks like the appropriate lock to use.
I am just a bit unsure if the 'spinlock' is the correct synchronization
primitive here.

Best regards;
Rostislav

Re: [PATCH v2 1/2] mtd: nand: omap: Do not use global variables

[Roger Quadros]

On 10/02/2014 03:16 PM, Rostislav Lisovy wrote:
> 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.
> Remove global variable to make the code thread-safe.
> 
> Signed-off-by: Rostislav Lisovy <lisovy@merica.cz>

Acked-by: Roger Quadros <rogerq@ti.com>

cheers,
-roger

[PATCH v3 2/2] mtd: nand: omap: Synchronize the access to the ECC engine

[Rostislav Lisovy]

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 global 'struct nand_hw_control' among multiple NAND
instances to synchronize the access to the single ECC Engine.

Tested with custom AM335x board using 2x NAND flash chips.

Signed-off-by: Rostislav Lisovy <lisovy@merica.cz>
---
Changes since v2:
* Do not use custom locks. Use global 'struct nand_hw_control'
  among multiple NAND instances and it will do the work for us
  (Roger Quadros)

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)


 drivers/mtd/nand/omap2.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index f0dcdb6..dfae46f 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -144,8 +144,13 @@ 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};
 
+/* Shared among all NAND instances to synchronize the access to the ECC Engine */
+struct nand_hw_control omap_gpmc_controller = {
+	.lock = __SPIN_LOCK_UNLOCKED(omap_gpmc_controller.lock),
+	.wq = __WAIT_QUEUE_HEAD_INITIALIZER(omap_gpmc_controller.wq),
+};
+
 struct omap_nand_info {
-	struct nand_hw_control		controller;
 	struct omap_nand_platform_data	*pdata;
 	struct mtd_info			mtd;
 	struct nand_chip		nand;
@@ -1685,9 +1690,6 @@ static int omap_nand_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, info);
 
-	spin_lock_init(&info->controller.lock);
-	init_waitqueue_head(&info->controller.wq);
-
 	info->pdev		= pdev;
 	info->gpmc_cs		= pdata->cs;
 	info->reg		= pdata->reg;
@@ -1707,7 +1709,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 
 	info->phys_base = res->start;
 
-	nand_chip->controller = &info->controller;
+	nand_chip->controller = &omap_gpmc_controller;
 
 	nand_chip->IO_ADDR_W = nand_chip->IO_ADDR_R;
 	nand_chip->cmd_ctrl  = omap_hwcontrol;
-- 
1.9.1

Re: [PATCH v3 2/2] mtd: nand: omap: Synchronize the access to the ECC engine

[Rostislav Lisovy]

On Thu, 2014-10-02 at 16:16 +0200, 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 global 'struct nand_hw_control' among multiple NAND
> instances to synchronize the access to the single ECC Engine.
> 
> Tested with custom AM335x board using 2x NAND flash chips.
> 
> Signed-off-by: Rostislav Lisovy <lisovy@merica.cz>


Just a kind appeal if someone is willing to review this.

Best regards;
Rostislav

Re: [PATCH v3 2/2] mtd: nand: omap: Synchronize the access to the ECC engine

[Frans Klaver]

On Thu, Oct 2, 2014 at 4:16 PM, Rostislav Lisovy <lisovy@gmail.com> wrote:
>
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index f0dcdb6..dfae46f 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -144,8 +144,13 @@ 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};
>
> +/* Shared among all NAND instances to synchronize the access to the ECC Engine */
> +struct nand_hw_control omap_gpmc_controller = {
> +       .lock = __SPIN_LOCK_UNLOCKED(omap_gpmc_controller.lock),
> +       .wq = __WAIT_QUEUE_HEAD_INITIALIZER(omap_gpmc_controller.wq),
> +};
> +

Any reason not to make this one static?

Thanks,
Frans

Re: [PATCH v3 2/2] mtd: nand: omap: Synchronize the access to the ECC engine

[Roger Quadros]

Hi Rostislav,

Sorry to miss this one.

On 10/29/2014 11:24 AM, Frans Klaver wrote:
> On Thu, Oct 2, 2014 at 4:16 PM, Rostislav Lisovy <lisovy@gmail.com> wrote:
>>
>> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
>> index f0dcdb6..dfae46f 100644
>> --- a/drivers/mtd/nand/omap2.c
>> +++ b/drivers/mtd/nand/omap2.c
>> @@ -144,8 +144,13 @@ 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};
>>
>> +/* Shared among all NAND instances to synchronize the access to the ECC Engine */
>> +struct nand_hw_control omap_gpmc_controller = {
>> +       .lock = __SPIN_LOCK_UNLOCKED(omap_gpmc_controller.lock),
>> +       .wq = __WAIT_QUEUE_HEAD_INITIALIZER(omap_gpmc_controller.wq),
>> +};
>> +
> 
> Any reason not to make this one static?
> 
Right. Apart from that, this patch looks good for me.
So after that change you can add my

Acked-by: Roger Quadros <rogerq@ti.com>

cheers,
-roger

Re: [PATCH v2 1/2] mtd: nand: omap: Do not use global variables

[Brian Norris]

On Thu, Oct 02, 2014 at 02:16:12PM +0200, Rostislav Lisovy wrote:
> 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.
> Remove global variable to make the code thread-safe.
> 
> Signed-off-by: Rostislav Lisovy <lisovy@merica.cz>
> ---
> Changes since v1:
> * Do not dynamically allocate the 'struct nand_ecclayout' but rather
>   include it in the 'struct omap_nand_info' (Roger Quadros)

Pushed to l2-mtd.git with some small changes to the comment.

Thanks,
Brian