[PATCH v7 0/5] mtd: cfi_cmdset_0002: Change write and erase functions

[PATCH v7 0/5] mtd: cfi_cmdset_0002: Change write and erase functions

[Tokunori Ikegami]

The changes are to make sure to check the operation status.
Actually the flash write and erase error behavior is caused on our products.
The flash is Macronix flash device MX29GL512FHT2I-11G used by our products.
The patch series was separated for changes of flash write and erase.
Since those were not depended each other at the time.
But by additional changes the changes are related more as same way currently.
So combine patch series for the flash write and erase changes as v6.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Reviewed-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
Cc: linux-mtd@lists.infradead.org
Cc: stable@vger.kernel.org

Tokunori Ikegami (5):
  mtd: cfi_cmdset_0002: Change write buffer to check correct value
  mtd: cfi_cmdset_0002: Change definition naming to retry write
    operation
  mtd: cfi_cmdset_0002: Change erase functions to retry for error
  mtd: cfi_cmdset_0002: Change erase functions to check chip good only
  mtd: cfi_cmdset_0002: Change erase one block to enable XIP once

 drivers/mtd/chips/cfi_cmdset_0002.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

-- 
2.16.1

[PATCH v7 1/5] mtd: cfi_cmdset_0002: Change write buffer to check correct value

[Tokunori Ikegami]

For the word write it is checked if the chip has the correct value.
But it is not checked for the write buffer as only checked if ready.
To make sure for the write buffer change to check the value.

It is enough as this patch is only checking the last written word.
Since it is described by data sheets to check the operation status.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Reviewed-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
Cc: linux-mtd@lists.infradead.org
Cc: stable@vger.kernel.org
---
 drivers/mtd/chips/cfi_cmdset_0002.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index e397b80e40cc..1d6be8c63d39 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1879,7 +1879,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
 		if (time_after(jiffies, timeo) && !chip_ready(map, adr))
 			break;
 
-		if (chip_ready(map, adr)) {
+		if (chip_good(map, adr, datum)) {
 			xip_enable(map, chip, adr);
 			goto op_done;
 		}
-- 
2.16.1

[PATCH v7 2/5] mtd: cfi_cmdset_0002: Change definition naming to retry write operation

[Tokunori Ikegami]

The definition can be used for other program and erase operations also.
So change the naming to MAX_RETRIES from MAX_WORD_RETRIES.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Reviewed-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
Cc: linux-mtd@lists.infradead.org
Cc: stable@vger.kernel.org
---
 drivers/mtd/chips/cfi_cmdset_0002.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 1d6be8c63d39..0b67f90415fd 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -42,7 +42,7 @@
 #define AMD_BOOTLOC_BUG
 #define FORCE_WORD_WRITE 0
 
-#define MAX_WORD_RETRIES 3
+#define MAX_RETRIES 3
 
 #define SST49LF004B		0x0060
 #define SST49LF040B		0x0050
@@ -1646,7 +1646,7 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 		map_write(map, CMD(0xF0), chip->start);
 		/* FIXME - should have reset delay before continuing */
 
-		if (++retry_cnt <= MAX_WORD_RETRIES)
+		if (++retry_cnt <= MAX_RETRIES)
 			goto retry;
 
 		ret = -EIO;
@@ -2105,7 +2105,7 @@ static int do_panic_write_oneword(struct map_info *map, struct flchip *chip,
 		map_write(map, CMD(0xF0), chip->start);
 		/* FIXME - should have reset delay before continuing */
 
-		if (++retry_cnt <= MAX_WORD_RETRIES)
+		if (++retry_cnt <= MAX_RETRIES)
 			goto retry;
 
 		ret = -EIO;
-- 
2.16.1

[PATCH v7 3/5] mtd: cfi_cmdset_0002: Change erase functions to retry for error

[Tokunori Ikegami]

For the word write functions it is retried for error.
But it is not implemented to retry for the erase functions.
To make sure for the erase functions change to retry as same.

This is needed to prevent the flash erase error caused only once.
It was caused by the error case of chip_good() in the do_erase_oneblock().
Also it was confirmed on the MACRONIX flash device MX29GL512FHT2I-11G.
But the error issue behavior is not able to reproduce at this moment.
The flash controller is parallel Flash interface integrated on BCM53003.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Reviewed-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
Cc: linux-mtd@lists.infradead.org
Cc: stable@vger.kernel.org
---
 drivers/mtd/chips/cfi_cmdset_0002.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 0b67f90415fd..e703900975d4 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -2240,6 +2240,7 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip)
 	unsigned long int adr;
 	DECLARE_WAITQUEUE(wait, current);
 	int ret = 0;
+	int retry_cnt = 0;
 
 	adr = cfi->addr_unlock1;
 
@@ -2257,6 +2258,7 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip)
 	ENABLE_VPP(map);
 	xip_disable(map, chip, adr);
 
+ retry:
 	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
 	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
 	cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
@@ -2310,6 +2312,9 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip)
 		map_write(map, CMD(0xF0), chip->start);
 		/* FIXME - should have reset delay before continuing */
 
+		if (++retry_cnt <= MAX_RETRIES)
+			goto retry;
+
 		ret = -EIO;
 	}
 
@@ -2329,6 +2334,7 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
 	unsigned long timeo = jiffies + HZ;
 	DECLARE_WAITQUEUE(wait, current);
 	int ret = 0;
+	int retry_cnt = 0;
 
 	adr += chip->start;
 
@@ -2346,6 +2352,7 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
 	ENABLE_VPP(map);
 	xip_disable(map, chip, adr);
 
+ retry:
 	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
 	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
 	cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
@@ -2402,6 +2409,9 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
 		map_write(map, CMD(0xF0), chip->start);
 		/* FIXME - should have reset delay before continuing */
 
+		if (++retry_cnt <= MAX_RETRIES)
+			goto retry;
+
 		ret = -EIO;
 	}
 
-- 
2.16.1

[PATCH v7 4/5] mtd: cfi_cmdset_0002: Change erase functions to check chip good only

[Tokunori Ikegami]

Currently the functions use to check both chip ready and good.
But the chip ready is not enough to check the operation status.
So change this to check the chip good instead of this.
About the retry functions to make sure the error handling remain it.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Reviewed-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
Cc: linux-mtd@lists.infradead.org
Cc: stable@vger.kernel.org
---
 drivers/mtd/chips/cfi_cmdset_0002.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index e703900975d4..6adda4dc2007 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -2294,12 +2294,13 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip)
 			chip->erase_suspended = 0;
 		}
 
-		if (chip_ready(map, adr))
+		if (chip_good(map, adr, map_word_ff(map)))
 			break;
 
 		if (time_after(jiffies, timeo)) {
 			printk(KERN_WARNING "MTD %s(): software timeout\n",
 			       __func__);
+			ret = -EIO;
 			break;
 		}
 
@@ -2307,15 +2308,15 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip)
 		UDELAY(map, chip, adr, 1000000/HZ);
 	}
 	/* Did we succeed? */
-	if (!chip_good(map, adr, map_word_ff(map))) {
+	if (ret) {
 		/* reset on all failures. */
 		map_write(map, CMD(0xF0), chip->start);
 		/* FIXME - should have reset delay before continuing */
 
-		if (++retry_cnt <= MAX_RETRIES)
+		if (++retry_cnt <= MAX_RETRIES) {
+			ret = 0;
 			goto retry;
-
-		ret = -EIO;
+		}
 	}
 
 	chip->state = FL_READY;
@@ -2388,7 +2389,7 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
 			chip->erase_suspended = 0;
 		}
 
-		if (chip_ready(map, adr)) {
+		if (chip_good(map, adr, map_word_ff(map))) {
 			xip_enable(map, chip, adr);
 			break;
 		}
@@ -2397,6 +2398,7 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
 			xip_enable(map, chip, adr);
 			printk(KERN_WARNING "MTD %s(): software timeout\n",
 			       __func__);
+			ret = -EIO;
 			break;
 		}
 
@@ -2404,15 +2406,15 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
 		UDELAY(map, chip, adr, 1000000/HZ);
 	}
 	/* Did we succeed? */
-	if (!chip_good(map, adr, map_word_ff(map))) {
+	if (ret) {
 		/* reset on all failures. */
 		map_write(map, CMD(0xF0), chip->start);
 		/* FIXME - should have reset delay before continuing */
 
-		if (++retry_cnt <= MAX_RETRIES)
+		if (++retry_cnt <= MAX_RETRIES) {
+			ret = 0;
 			goto retry;
-
-		ret = -EIO;
+		}
 	}
 
 	chip->state = FL_READY;
-- 
2.16.1

[PATCH v7 5/5] mtd: cfi_cmdset_0002: Change erase one block to enable XIP once

[Tokunori Ikegami]

To enable XIP it is executed both normal and error cases.
This call can be moved after the for loop as same with erase chip.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Reviewed-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
Cc: linux-mtd@lists.infradead.org
Cc: stable@vger.kernel.org
---
 drivers/mtd/chips/cfi_cmdset_0002.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 6adda4dc2007..6c681cbf2d37 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -2389,13 +2389,10 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
 			chip->erase_suspended = 0;
 		}
 
-		if (chip_good(map, adr, map_word_ff(map))) {
-			xip_enable(map, chip, adr);
+		if (chip_good(map, adr, map_word_ff(map)))
 			break;
-		}
 
 		if (time_after(jiffies, timeo)) {
-			xip_enable(map, chip, adr);
 			printk(KERN_WARNING "MTD %s(): software timeout\n",
 			       __func__);
 			ret = -EIO;
@@ -2418,6 +2415,7 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
 	}
 
 	chip->state = FL_READY;
+	xip_enable(map, chip, adr);
 	DISABLE_VPP(map);
 	put_chip(map, chip, adr);
 	mutex_unlock(&chip->mutex);
-- 
2.16.1

RE: [PATCH v7 0/5] mtd: cfi_cmdset_0002: Change write and erase functions

[IKEGAMI Tokunori]

Hi,

Sorry changelogs have not been described by the v7 patches so let me send mail about this.
There is no change from the v6 patches but just changed the commit messages to change and add CC lines as below.

  1. Change
    - Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
    + Reviewed-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>

  2. Add
    + Cc: stable@vger.kernel.org

Regards,
Ikegami

> -----Original Message-----
> From: Tokunori Ikegami [mailto:ikegami@allied-telesis.co.jp]
> Sent: Monday, May 28, 2018 8:28 AM
> To: Boris Brezillon
> Cc: IKEGAMI Tokunori; PACKHAM Chris; Brian Norris; David Woodhouse; Boris
> Brezillon; Marek Vasut; Richard Weinberger; Cyrille Pitchen;
> linux-mtd@lists.infradead.org; stable@vger.kernel.org
> Subject: [PATCH v7 0/5] mtd: cfi_cmdset_0002: Change write and erase
> functions
> 
> The changes are to make sure to check the operation status.
> Actually the flash write and erase error behavior is caused on our products.
> The flash is Macronix flash device MX29GL512FHT2I-11G used by our products.
> The patch series was separated for changes of flash write and erase.
> Since those were not depended each other at the time.
> But by additional changes the changes are related more as same way currently.
> So combine patch series for the flash write and erase changes as v6.
> 
> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Reviewed-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Marek Vasut <marek.vasut@gmail.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
> Cc: linux-mtd@lists.infradead.org
> Cc: stable@vger.kernel.org
> 
> Tokunori Ikegami (5):
>   mtd: cfi_cmdset_0002: Change write buffer to check correct value
>   mtd: cfi_cmdset_0002: Change definition naming to retry write
>     operation
>   mtd: cfi_cmdset_0002: Change erase functions to retry for error
>   mtd: cfi_cmdset_0002: Change erase functions to check chip good only
>   mtd: cfi_cmdset_0002: Change erase one block to enable XIP once
> 
>  drivers/mtd/chips/cfi_cmdset_0002.c | 36
> +++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 13 deletions(-)
> 
> --
> 2.16.1

Re: [PATCH v7 5/5] mtd: cfi_cmdset_0002: Change erase one block to enable XIP once

[Boris Brezillon]

On Mon, 28 May 2018 08:28:01 +0900
Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:

> To enable XIP it is executed both normal and error cases.
> This call can be moved after the for loop as same with erase chip.
> 
> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Reviewed-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Marek Vasut <marek.vasut@gmail.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
> Cc: linux-mtd@lists.infradead.org
> Cc: stable@vger.kernel.org

Is this really a bug fix? Doesn't look like a bug fix to me.

Also, every time you add Cc stable you should try to find the commit
that introduced the bug. Sometime it's not possible because the bug
existed before git was in use, but most of the time you'll find the
offending commit using git blame.

A fixes tag should be formatted like that:

Fixes: <commit-id> ("commit subject")

> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 6adda4dc2007..6c681cbf2d37 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -2389,13 +2389,10 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
>  			chip->erase_suspended = 0;
>  		}
>  
> -		if (chip_good(map, adr, map_word_ff(map))) {
> -			xip_enable(map, chip, adr);
> +		if (chip_good(map, adr, map_word_ff(map)))
>  			break;
> -		}
>  
>  		if (time_after(jiffies, timeo)) {
> -			xip_enable(map, chip, adr);
>  			printk(KERN_WARNING "MTD %s(): software timeout\n",
>  			       __func__);
>  			ret = -EIO;
> @@ -2418,6 +2415,7 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
>  	}
>  
>  	chip->state = FL_READY;
> +	xip_enable(map, chip, adr);
>  	DISABLE_VPP(map);
>  	put_chip(map, chip, adr);
>  	mutex_unlock(&chip->mutex);

RE: [PATCH v7 5/5] mtd: cfi_cmdset_0002: Change erase one block to enable XIP once

[IKEGAMI Tokunori]

Hi Boris-san,

Thanks for your reviewing and advices.

> Is this really a bug fix? Doesn't look like a bug fix to me.

  No as you mentioned it is not a bug fix but just a refactoring to reduce xip_enable() line.

> Also, every time you add Cc stable you should try to find the commit
> that introduced the bug. Sometime it's not possible because the bug
> existed before git was in use, but most of the time you'll find the
> offending commit using git blame.
> 
> A fixes tag should be formatted like that:
> 
> Fixes: <commit-id> ("commit subject")

  Okay I will do that in future.

  This is just FYI.
  I have just confirmed that the xip_enable() line itself was implemented by the commit 02b15e343aeef.
  For this patch it is not a bug fix so I will not add the Fixes line into the commit message.
  But if needed it please let me know that.

Regards,
Ikegami

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> Sent: Wednesday, May 30, 2018 3:58 AM
> To: IKEGAMI Tokunori
> Cc: PACKHAM Chris; Brian Norris; David Woodhouse; Boris Brezillon; Marek
> Vasut; Richard Weinberger; Cyrille Pitchen;
> linux-mtd@lists.infradead.org; stable@vger.kernel.org
> Subject: Re: [PATCH v7 5/5] mtd: cfi_cmdset_0002: Change erase one block
> to enable XIP once
> 
> On Mon, 28 May 2018 08:28:01 +0900
> Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:
> 
> > To enable XIP it is executed both normal and error cases.
> > This call can be moved after the for loop as same with erase chip.
> >
> > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > Reviewed-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > Cc: Brian Norris <computersforpeace@gmail.com>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Cc: Marek Vasut <marek.vasut@gmail.com>
> > Cc: Richard Weinberger <richard@nod.at>
> > Cc: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
> > Cc: linux-mtd@lists.infradead.org
> > Cc: stable@vger.kernel.org
> 
> Is this really a bug fix? Doesn't look like a bug fix to me.
> 
> Also, every time you add Cc stable you should try to find the commit
> that introduced the bug. Sometime it's not possible because the bug
> existed before git was in use, but most of the time you'll find the
> offending commit using git blame.
> 
> A fixes tag should be formatted like that:
> 
> Fixes: <commit-id> ("commit subject")
> 
> > ---
> >  drivers/mtd/chips/cfi_cmdset_0002.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index 6adda4dc2007..6c681cbf2d37 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -2389,13 +2389,10 @@ static int __xipram do_erase_oneblock(struct
> map_info *map, struct flchip *chip,
> >  			chip->erase_suspended = 0;
> >  		}
> >
> > -		if (chip_good(map, adr, map_word_ff(map))) {
> > -			xip_enable(map, chip, adr);
> > +		if (chip_good(map, adr, map_word_ff(map)))
> >  			break;
> > -		}
> >
> >  		if (time_after(jiffies, timeo)) {
> > -			xip_enable(map, chip, adr);
> >  			printk(KERN_WARNING "MTD %s(): software
> timeout\n",
> >  			       __func__);
> >  			ret = -EIO;
> > @@ -2418,6 +2415,7 @@ static int __xipram do_erase_oneblock(struct
> map_info *map, struct flchip *chip,
> >  	}
> >
> >  	chip->state = FL_READY;
> > +	xip_enable(map, chip, adr);
> >  	DISABLE_VPP(map);
> >  	put_chip(map, chip, adr);
> >  	mutex_unlock(&chip->mutex);

Re: [PATCH v7 5/5] mtd: cfi_cmdset_0002: Change erase one block to enable XIP once

[Boris Brezillon]

Hello,

On Tue, 29 May 2018 23:31:41 +0000
IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote:

> Hi Boris-san,
> 
> Thanks for your reviewing and advices.
> 
> > Is this really a bug fix? Doesn't look like a bug fix to me.  
> 
>   No as you mentioned it is not a bug fix but just a refactoring to reduce xip_enable() line.

Then you should drop the Cc: stable tag.

> 
> > Also, every time you add Cc stable you should try to find the commit
> > that introduced the bug. Sometime it's not possible because the bug
> > existed before git was in use, but most of the time you'll find the
> > offending commit using git blame.
> > 
> > A fixes tag should be formatted like that:
> > 
> > Fixes: <commit-id> ("commit subject")  
> 
>   Okay I will do that in future.
> 
>   This is just FYI.
>   I have just confirmed that the xip_enable() line itself was implemented by the commit 02b15e343aeef.
>   For this patch it is not a bug fix so I will not add the Fixes line into the commit message.

Right.

>   But if needed it please let me know that.

I checked the first patch and it seems it's one of these situation
where the code predates git, so no need to specify a Fixes tag.

Thanks,

Boris

RE: [PATCH v7 5/5] mtd: cfi_cmdset_0002: Change erase one block to enable XIP once

[IKEGAMI Tokunori]

Hi Boris-san,

I have just sent v8 version patch series and it changed the 5/5 patch only.

> Then you should drop the Cc: stable tag.

  Yes just dropped it from 5/5 patch only by the v8 version patch series.

> >   I have just confirmed that the xip_enable() line itself was implemented
> by the commit 02b15e343aeef.
> >   For this patch it is not a bug fix so I will not add the Fixes line
> into the commit message.
> 
> Right.

  I see.

> >   But if needed it please let me know that.
> 
> I checked the first patch and it seems it's one of these situation
> where the code predates git, so no need to specify a Fixes tag.

  I see so I will not change other 1/5 to 4/5 patches by the v8 version patch series.

Regards,
Ikegami

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> Sent: Wednesday, May 30, 2018 5:49 PM
> To: IKEGAMI Tokunori
> Cc: PACKHAM Chris; Brian Norris; David Woodhouse; Boris Brezillon; Marek
> Vasut; Richard Weinberger; Cyrille Pitchen;
> linux-mtd@lists.infradead.org; stable@vger.kernel.org
> Subject: Re: [PATCH v7 5/5] mtd: cfi_cmdset_0002: Change erase one block
> to enable XIP once
> 
> Hello,
> 
> On Tue, 29 May 2018 23:31:41 +0000
> IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote:
> 
> > Hi Boris-san,
> >
> > Thanks for your reviewing and advices.
> >
> > > Is this really a bug fix? Doesn't look like a bug fix to me.
> >
> >   No as you mentioned it is not a bug fix but just a refactoring to reduce
> xip_enable() line.
> 
> Then you should drop the Cc: stable tag.
> 
> >
> > > Also, every time you add Cc stable you should try to find the commit
> > > that introduced the bug. Sometime it's not possible because the bug
> > > existed before git was in use, but most of the time you'll find the
> > > offending commit using git blame.
> > >
> > > A fixes tag should be formatted like that:
> > >
> > > Fixes: <commit-id> ("commit subject")
> >
> >   Okay I will do that in future.
> >
> >   This is just FYI.
> >   I have just confirmed that the xip_enable() line itself was implemented
> by the commit 02b15e343aeef.
> >   For this patch it is not a bug fix so I will not add the Fixes line
> into the commit message.
> 
> Right.
> 
> >   But if needed it please let me know that.
> 
> I checked the first patch and it seems it's one of these situation
> where the code predates git, so no need to specify a Fixes tag.
> 
> Thanks,
> 
> Boris