[PATCH 0/3] mtd: fix AMD/Intel flash bugs

[PATCH 0/3] mtd: fix AMD/Intel flash bugs

[Joakim Tjernlund]

This fixes a bug which allows read/write to a erase block currently
erasing.
Also works around a chip bug in Micron 28F00AP30 chips.

Joakim Tjernlund (3):
  cfi_cmdset_0001: Do not allow read/write to suspend erase block.
  cfi_cmdset_0001: Workaround Micron Erase suspend bug.
  cfi_cmdset_0002: Do not allow read/write to suspend erase block.

 drivers/mtd/chips/cfi_cmdset_0001.c | 33 ++++++++++++++++++++++++++++-----
 drivers/mtd/chips/cfi_cmdset_0002.c |  9 ++++++---
 include/linux/mtd/flashchip.h       |  1 +
 3 files changed, 35 insertions(+), 8 deletions(-)

-- 
2.13.6

[PATCH 1/3] cfi_cmdset_0001: Do not allow read/write to suspend erase block.

[Joakim Tjernlund]

From: Joakim Tjernlund <joakim.tjernlund@transmode.se>

Currently it is possible to read and/or write to suspend EB's.
Writing /dev/mtdX or /dev/mtdblockX from several processes may
break the flash state machine.

Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
Cc: <stable@vger.kernel.org>
---
 drivers/mtd/chips/cfi_cmdset_0001.c | 16 +++++++++++-----
 include/linux/mtd/flashchip.h       |  1 +
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
index 60d5d19e347f..b59872304ae7 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -849,21 +849,25 @@ static int chip_ready (struct map_info *map, struct flchip *chip, unsigned long
 		     (mode == FL_WRITING && (cfip->SuspendCmdSupport & 1))))
 			goto sleep;
 
+		/* Do not allow suspend iff read/write to EB address */
+		if ((adr & chip->in_progress_block_mask) ==
+		    chip->in_progress_block_addr)
+			goto sleep;
 
 		/* Erase suspend */
-		map_write(map, CMD(0xB0), adr);
+		map_write(map, CMD(0xB0), chip->in_progress_block_addr);
 
 		/* If the flash has finished erasing, then 'erase suspend'
 		 * appears to make some (28F320) flash devices switch to
 		 * 'read' mode.  Make sure that we switch to 'read status'
 		 * mode so we get the right data. --rmk
 		 */
-		map_write(map, CMD(0x70), adr);
+		map_write(map, CMD(0x70), chip->in_progress_block_addr);
 		chip->oldstate = FL_ERASING;
 		chip->state = FL_ERASE_SUSPENDING;
 		chip->erase_suspended = 1;
 		for (;;) {
-			status = map_read(map, adr);
+			status = map_read(map, chip->in_progress_block_addr);
 			if (map_word_andequal(map, status, status_OK, status_OK))
 			        break;
 
@@ -1059,8 +1063,8 @@ static void put_chip(struct map_info *map, struct flchip *chip, unsigned long ad
 		   sending the 0x70 (Read Status) command to an erasing
 		   chip and expecting it to be ignored, that's what we
 		   do. */
-		map_write(map, CMD(0xd0), adr);
-		map_write(map, CMD(0x70), adr);
+		map_write(map, CMD(0xd0), chip->in_progress_block_addr);
+		map_write(map, CMD(0x70), chip->in_progress_block_addr);
 		chip->oldstate = FL_READY;
 		chip->state = FL_ERASING;
 		break;
@@ -1951,6 +1955,8 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
 	map_write(map, CMD(0xD0), adr);
 	chip->state = FL_ERASING;
 	chip->erase_suspended = 0;
+	chip->in_progress_block_addr = adr;
+	chip->in_progress_block_mask = ~(len - 1);
 
 	ret = INVAL_CACHE_AND_WAIT(map, chip, adr,
 				   adr, len,
diff --git a/include/linux/mtd/flashchip.h b/include/linux/mtd/flashchip.h
index b63fa457febd..3529683f691e 100644
--- a/include/linux/mtd/flashchip.h
+++ b/include/linux/mtd/flashchip.h
@@ -85,6 +85,7 @@ struct flchip {
 	unsigned int write_suspended:1;
 	unsigned int erase_suspended:1;
 	unsigned long in_progress_block_addr;
+	unsigned long in_progress_block_mask;
 
 	struct mutex mutex;
 	wait_queue_head_t wq; /* Wait on here when we're waiting for the chip
-- 
2.13.6

[PATCH 2/3] cfi_cmdset_0001: Workaround Micron Erase suspend bug.

[Joakim Tjernlund]

From: Joakim Tjernlund <joakim.tjernlund@transmode.se>

Some Micron chips does not work well wrt Erase suspend for
boot blocks. This avoids the issue by not allowing Erase suspend
for the boot blocks for the 28F00AP30(1GBit) chip.

Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
Cc: <stable@vger.kernel.org>
---
 drivers/mtd/chips/cfi_cmdset_0001.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
index b59872304ae7..64ae65dab877 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -45,6 +45,7 @@
 #define I82802AB	0x00ad
 #define I82802AC	0x00ac
 #define PF38F4476	0x881c
+#define M28F00AP30	0x8963
 /* STMicroelectronics chips */
 #define M50LPW080       0x002F
 #define M50FLW080A	0x0080
@@ -375,6 +376,17 @@ static void cfi_fixup_major_minor(struct cfi_private *cfi,
 		extp->MinorVersion = '1';
 }
 
+static int cfi_is_micron_28F00AP30(struct cfi_private *cfi, struct flchip *chip)
+{
+	/*
+	 * Micron(was Numonyx) 1Gbit bottom boot are buggy w.r.t
+	 * Erase Supend for their small Erase Blocks(0x8000)
+	 */
+	if (cfi->mfr == CFI_MFR_INTEL && cfi->id == M28F00AP30)
+		return 1;
+	return 0;
+}
+
 static inline struct cfi_pri_intelext *
 read_pri_intelext(struct map_info *map, __u16 adr)
 {
@@ -854,6 +866,11 @@ static int chip_ready (struct map_info *map, struct flchip *chip, unsigned long
 		    chip->in_progress_block_addr)
 			goto sleep;
 
+		/* do not suspend small EBs, buggy Micron Chips */
+		if (cfi_is_micron_28F00AP30(cfi, chip) &&
+		    (chip->in_progress_block_mask == ~(0x8000-1)))
+			goto sleep;
+
 		/* Erase suspend */
 		map_write(map, CMD(0xB0), chip->in_progress_block_addr);
 
-- 
2.13.6

[PATCH 3/3] cfi_cmdset_0002: Do not allow read/write to suspend erase block.

[Joakim Tjernlund]

Currently it is possible to read and/or write to suspend EB's.
Writing /dev/mtdX or /dev/mtdblockX from several processes may
break the flash state machine.

Taken from cfi_cmdset_0001 driver.

Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
Cc: <stable@vger.kernel.org>
---
 drivers/mtd/chips/cfi_cmdset_0002.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 56aa6b75213d..d524a64ed754 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -816,9 +816,10 @@ static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr
 		    (mode == FL_WRITING && (cfip->EraseSuspend & 0x2))))
 			goto sleep;
 
-		/* We could check to see if we're trying to access the sector
-		 * that is currently being erased. However, no user will try
-		 * anything like that so we just wait for the timeout. */
+		/* Do not allow suspend iff read/write to EB address */
+		if ((adr & chip->in_progress_block_mask) ==
+		    chip->in_progress_block_addr)
+			goto sleep;
 
 		/* Erase suspend */
 		/* It's harmless to issue the Erase-Suspend and Erase-Resume
@@ -2267,6 +2268,7 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip)
 	chip->state = FL_ERASING;
 	chip->erase_suspended = 0;
 	chip->in_progress_block_addr = adr;
+	chip->in_progress_block_mask = ~(map->size - 1);
 
 	INVALIDATE_CACHE_UDELAY(map, chip,
 				adr, map->size,
@@ -2356,6 +2358,7 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
 	chip->state = FL_ERASING;
 	chip->erase_suspended = 0;
 	chip->in_progress_block_addr = adr;
+	chip->in_progress_block_mask = ~(len - 1);
 
 	INVALIDATE_CACHE_UDELAY(map, chip,
 				adr, len,
-- 
2.13.6

Re: [PATCH 0/3] mtd: fix AMD/Intel flash bugs

[Joakim Tjernlund]

On Thu, 2018-03-01 at 14:39 +0100, Joakim Tjernlund wrote:
> This fixes a bug which allows read/write to a erase block currently
> erasing.
> Also works around a chip bug in Micron 28F00AP30 chips.
> 
> Joakim Tjernlund (3):
>   cfi_cmdset_0001: Do not allow read/write to suspend erase block.
>   cfi_cmdset_0001: Workaround Micron Erase suspend bug.
>   cfi_cmdset_0002: Do not allow read/write to suspend erase block.
> 
>  drivers/mtd/chips/cfi_cmdset_0001.c | 33 ++++++++++++++++++++++++++++-----
>  drivers/mtd/chips/cfi_cmdset_0002.c |  9 ++++++---
>  include/linux/mtd/flashchip.h       |  1 +
>  3 files changed, 35 insertions(+), 8 deletions(-)
> 

Ping?
This is a fairly serious bug as it confuses the flash state machine,
making further flash access fail.

 Jocke

Re: [PATCH 0/3] mtd: fix AMD/Intel flash bugs

[Andrea Adami]

On Sun, Mar 11, 2018 at 5:06 PM, Joakim Tjernlund
<Joakim.Tjernlund@infinera.com> wrote:
> On Thu, 2018-03-01 at 14:39 +0100, Joakim Tjernlund wrote:
>> This fixes a bug which allows read/write to a erase block currently
>> erasing.
>> Also works around a chip bug in Micron 28F00AP30 chips.
>>
>> Joakim Tjernlund (3):
>>   cfi_cmdset_0001: Do not allow read/write to suspend erase block.
>>   cfi_cmdset_0001: Workaround Micron Erase suspend bug.
>>   cfi_cmdset_0002: Do not allow read/write to suspend erase block.
>>
>>  drivers/mtd/chips/cfi_cmdset_0001.c | 33 ++++++++++++++++++++++++++++-----
>>  drivers/mtd/chips/cfi_cmdset_0002.c |  9 ++++++---
>>  include/linux/mtd/flashchip.h       |  1 +
>>  3 files changed, 35 insertions(+), 8 deletions(-)
>>
>
> Ping?
> This is a fairly serious bug as it confuses the flash state machine,
> making further flash access fail.
>

Hello,

I don't undertsand exactly from the titles what is happening.
Do you want to disable erase and wite suspend like in this other 2016
patch found around?

https://patchwork.kernel.org/patch/9380029/

See, I am user of the Sharp LH28F640BF NOR, very similar to the Intel
Startaflash, 4-Plane Page Mode Dual Work.
On these chips erase suspend works just fine.
What has been disabled is dual work: simultaneous read-while-erase and
read-while-program because there has never been real support for
n-planes in kernel.

What is exactly the issue?
Regards
Andrea

>  Jocke
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/3] mtd: fix AMD/Intel flash bugs

[Joakim Tjernlund]

On Mon, 2018-03-12 at 10:09 +0100, Andrea Adami wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Sun, Mar 11, 2018 at 5:06 PM, Joakim Tjernlund
> <Joakim.Tjernlund@infinera.com> wrote:
> > On Thu, 2018-03-01 at 14:39 +0100, Joakim Tjernlund wrote:
> > > This fixes a bug which allows read/write to a erase block currently
> > > erasing.
> > > Also works around a chip bug in Micron 28F00AP30 chips.
> > > 
> > > Joakim Tjernlund (3):
> > >   cfi_cmdset_0001: Do not allow read/write to suspend erase block.
> > >   cfi_cmdset_0001: Workaround Micron Erase suspend bug.
> > >   cfi_cmdset_0002: Do not allow read/write to suspend erase block.
> > > 
> > >  drivers/mtd/chips/cfi_cmdset_0001.c | 33 ++++++++++++++++++++++++++++-----
> > >  drivers/mtd/chips/cfi_cmdset_0002.c |  9 ++++++---
> > >  include/linux/mtd/flashchip.h       |  1 +
> > >  3 files changed, 35 insertions(+), 8 deletions(-)
> > > 
> > 
> > Ping?
> > This is a fairly serious bug as it confuses the flash state machine,
> > making further flash access fail.
> > 
> 
> Hello,
> 
> I don't undertsand exactly from the titles what is happening.
> Do you want to disable erase and wite suspend like in this other 2016
> patch found around?

Not quite, the first part is about preventing read/write to a block that is
being erased. This can happen if two apps opens the same /dev/mtd device.

Then there is a bug fix for erase suspend not working in the smaller boot block
sections, that part is similar to the patch you reference but much less invasive

> https://patchwork.kernel.org/patch/9380029/
> 
> See, I am user of the Sharp LH28F640BF NOR, very similar to the Intel
> Startaflash, 4-Plane Page Mode Dual Work.
> On these chips erase suspend works just fine.
> What has been disabled is dual work: simultaneous read-while-erase and
> read-while-program because there has never been real support for
> n-planes in kernel.
> 
> What is exactly the issue?
> Regards
> Andrea
> 
> >  Jocke
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/3] mtd: fix AMD/Intel flash bugs

[Boris Brezillon]

On Sun, 11 Mar 2018 16:06:03 +0000
Joakim Tjernlund <Joakim.Tjernlund@infinera.com> wrote:

> On Thu, 2018-03-01 at 14:39 +0100, Joakim Tjernlund wrote:
> > This fixes a bug which allows read/write to a erase block currently
> > erasing.
> > Also works around a chip bug in Micron 28F00AP30 chips.
> > 
> > Joakim Tjernlund (3):
> >   cfi_cmdset_0001: Do not allow read/write to suspend erase block.
> >   cfi_cmdset_0001: Workaround Micron Erase suspend bug.
> >   cfi_cmdset_0002: Do not allow read/write to suspend erase block.
> > 
> >  drivers/mtd/chips/cfi_cmdset_0001.c | 33 ++++++++++++++++++++++++++++-----
> >  drivers/mtd/chips/cfi_cmdset_0002.c |  9 ++++++---
> >  include/linux/mtd/flashchip.h       |  1 +
> >  3 files changed, 35 insertions(+), 8 deletions(-)
> >   
> 
> Ping?

You didn't Cc the MTD maintainers, so that's not surprising you don't
get a reply from us :P.

> This is a fairly serious bug as it confuses the flash state machine,
> making further flash access fail.

I'll try to have a look.

> 
>  Jocke
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/3] mtd: fix AMD/Intel flash bugs

[Joakim Tjernlund]

On Thu, 2018-03-15 at 16:54 +0100, Boris Brezillon wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Sun, 11 Mar 2018 16:06:03 +0000
> Joakim Tjernlund <Joakim.Tjernlund@infinera.com> wrote:
> 
> > On Thu, 2018-03-01 at 14:39 +0100, Joakim Tjernlund wrote:
> > > This fixes a bug which allows read/write to a erase block currently
> > > erasing.
> > > Also works around a chip bug in Micron 28F00AP30 chips.
> > > 
> > > Joakim Tjernlund (3):
> > >   cfi_cmdset_0001: Do not allow read/write to suspend erase block.
> > >   cfi_cmdset_0001: Workaround Micron Erase suspend bug.
> > >   cfi_cmdset_0002: Do not allow read/write to suspend erase block.
> > > 
> > >  drivers/mtd/chips/cfi_cmdset_0001.c | 33 ++++++++++++++++++++++++++++-----
> > >  drivers/mtd/chips/cfi_cmdset_0002.c |  9 ++++++---
> > >  include/linux/mtd/flashchip.h       |  1 +
> > >  3 files changed, 35 insertions(+), 8 deletions(-)
> > > 
> > 
> > Ping?
> 
> You didn't Cc the MTD maintainers, so that's not surprising you don't
> get a reply from us :P.

I find it more surprising that the MTD maintainers doesn't read the MTD list ? I figured
this was a requirement.

> 
> > This is a fairly serious bug as it confuses the flash state machine,
> > making further flash access fail.
> 
> I'll try to have a look.

Thanks

Re: [PATCH 0/3] mtd: fix AMD/Intel flash bugs

[Boris Brezillon]

On Thu, 15 Mar 2018 17:55:57 +0000
Joakim Tjernlund <Joakim.Tjernlund@infinera.com> wrote:

> On Thu, 2018-03-15 at 16:54 +0100, Boris Brezillon wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > 
> > 
> > On Sun, 11 Mar 2018 16:06:03 +0000
> > Joakim Tjernlund <Joakim.Tjernlund@infinera.com> wrote:
> >   
> > > On Thu, 2018-03-01 at 14:39 +0100, Joakim Tjernlund wrote:  
> > > > This fixes a bug which allows read/write to a erase block currently
> > > > erasing.
> > > > Also works around a chip bug in Micron 28F00AP30 chips.
> > > > 
> > > > Joakim Tjernlund (3):
> > > >   cfi_cmdset_0001: Do not allow read/write to suspend erase block.
> > > >   cfi_cmdset_0001: Workaround Micron Erase suspend bug.
> > > >   cfi_cmdset_0002: Do not allow read/write to suspend erase block.
> > > > 
> > > >  drivers/mtd/chips/cfi_cmdset_0001.c | 33 ++++++++++++++++++++++++++++-----
> > > >  drivers/mtd/chips/cfi_cmdset_0002.c |  9 ++++++---
> > > >  include/linux/mtd/flashchip.h       |  1 +
> > > >  3 files changed, 35 insertions(+), 8 deletions(-)
> > > >   
> > > 
> > > Ping?  
> > 
> > You didn't Cc the MTD maintainers, so that's not surprising you don't
> > get a reply from us :P.  
> 
> I find it more surprising that the MTD maintainers doesn't read the MTD list ?

I do read the ML occasionally, though I tend to rely on what's in my
inbox, so there might be a bigger latency if you don't Cc me. I guess
the process differ for each maintainer, but it's a good practice to Cc
maintainers when you send a patch.

> I figured this was a requirement.
> 
> >   
> > > This is a fairly serious bug as it confuses the flash state machine,
> > > making further flash access fail.  
> > 
> > I'll try to have a look.  
> 
> Thanks


-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/3] cfi_cmdset_0001: Workaround Micron Erase suspend bug.

[Richard Weinberger]

Joakim,

On Thu, Mar 1, 2018 at 2:39 PM, Joakim Tjernlund
<joakim.tjernlund@infinera.com> wrote:
> From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
>
> Some Micron chips does not work well wrt Erase suspend for
> boot blocks. This avoids the issue by not allowing Erase suspend
> for the boot blocks for the 28F00AP30(1GBit) chip.

Is this bug documented somewhere?

> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/mtd/chips/cfi_cmdset_0001.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> index b59872304ae7..64ae65dab877 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> @@ -45,6 +45,7 @@
>  #define I82802AB       0x00ad
>  #define I82802AC       0x00ac
>  #define PF38F4476      0x881c
> +#define M28F00AP30     0x8963
>  /* STMicroelectronics chips */
>  #define M50LPW080       0x002F
>  #define M50FLW080A     0x0080
> @@ -375,6 +376,17 @@ static void cfi_fixup_major_minor(struct cfi_private *cfi,
>                 extp->MinorVersion = '1';
>  }
>
> +static int cfi_is_micron_28F00AP30(struct cfi_private *cfi, struct flchip *chip)
> +{
> +       /*
> +        * Micron(was Numonyx) 1Gbit bottom boot are buggy w.r.t
> +        * Erase Supend for their small Erase Blocks(0x8000)
> +        */
> +       if (cfi->mfr == CFI_MFR_INTEL && cfi->id == M28F00AP30)
> +               return 1;
> +       return 0;
> +}
> +
>  static inline struct cfi_pri_intelext *
>  read_pri_intelext(struct map_info *map, __u16 adr)
>  {
> @@ -854,6 +866,11 @@ static int chip_ready (struct map_info *map, struct flchip *chip, unsigned long
>                     chip->in_progress_block_addr)
>                         goto sleep;
>
> +               /* do not suspend small EBs, buggy Micron Chips */
> +               if (cfi_is_micron_28F00AP30(cfi, chip) &&
> +                   (chip->in_progress_block_mask == ~(0x8000-1)))
> +                       goto sleep;
> +
>                 /* Erase suspend */
>                 map_write(map, CMD(0xB0), chip->in_progress_block_addr);
>
> --
> 2.13.6
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



-- 
Thanks,
//richard

Re: [PATCH 2/3] cfi_cmdset_0001: Workaround Micron Erase suspend bug.

[Joakim Tjernlund]

On Wed, 2018-03-21 at 00:06 +0100, Richard Weinberger wrote:
> 
> Joakim,
> 
> On Thu, Mar 1, 2018 at 2:39 PM, Joakim Tjernlund
> <joakim.tjernlund@infinera.com> wrote:
> > From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> > 
> > Some Micron chips does not work well wrt Erase suspend for
> > boot blocks. This avoids the issue by not allowing Erase suspend
> > for the boot blocks for the 28F00AP30(1GBit) chip.
> 
> Is this bug documented somewhere?

One would hope but no. We got confirmation of the problem from the supplier though
but nothing written. 

 Jocke

Re: [PATCH 1/3] cfi_cmdset_0001: Do not allow read/write to suspend erase block.

[Richard Weinberger]

On Thu, Mar 1, 2018 at 2:39 PM, Joakim Tjernlund
<joakim.tjernlund@infinera.com> wrote:
> From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
>
> Currently it is possible to read and/or write to suspend EB's.
> Writing /dev/mtdX or /dev/mtdblockX from several processes may
> break the flash state machine.
>
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/mtd/chips/cfi_cmdset_0001.c | 16 +++++++++++-----
>  include/linux/mtd/flashchip.h       |  1 +
>  2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> index 60d5d19e347f..b59872304ae7 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> @@ -849,21 +849,25 @@ static int chip_ready (struct map_info *map, struct flchip *chip, unsigned long
>                      (mode == FL_WRITING && (cfip->SuspendCmdSupport & 1))))
>                         goto sleep;
>
> +               /* Do not allow suspend iff read/write to EB address */
> +               if ((adr & chip->in_progress_block_mask) ==
> +                   chip->in_progress_block_addr)
> +                       goto sleep;
>
>                 /* Erase suspend */
> -               map_write(map, CMD(0xB0), adr);
> +               map_write(map, CMD(0xB0), chip->in_progress_block_addr);
>
>                 /* If the flash has finished erasing, then 'erase suspend'
>                  * appears to make some (28F320) flash devices switch to
>                  * 'read' mode.  Make sure that we switch to 'read status'
>                  * mode so we get the right data. --rmk
>                  */
> -               map_write(map, CMD(0x70), adr);
> +               map_write(map, CMD(0x70), chip->in_progress_block_addr);

Why do you change to chip->in_progress_block_addr here?

>                 chip->oldstate = FL_ERASING;
>                 chip->state = FL_ERASE_SUSPENDING;
>                 chip->erase_suspended = 1;
>                 for (;;) {
> -                       status = map_read(map, adr);
> +                       status = map_read(map, chip->in_progress_block_addr);
>                         if (map_word_andequal(map, status, status_OK, status_OK))
>                                 break;
>
> @@ -1059,8 +1063,8 @@ static void put_chip(struct map_info *map, struct flchip *chip, unsigned long ad
>                    sending the 0x70 (Read Status) command to an erasing
>                    chip and expecting it to be ignored, that's what we
>                    do. */
> -               map_write(map, CMD(0xd0), adr);
> -               map_write(map, CMD(0x70), adr);
> +               map_write(map, CMD(0xd0), chip->in_progress_block_addr);
> +               map_write(map, CMD(0x70), chip->in_progress_block_addr);
>                 chip->oldstate = FL_READY;
>                 chip->state = FL_ERASING;
>                 break;
> @@ -1951,6 +1955,8 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
>         map_write(map, CMD(0xD0), adr);
>         chip->state = FL_ERASING;
>         chip->erase_suspended = 0;
> +       chip->in_progress_block_addr = adr;
> +       chip->in_progress_block_mask = ~(len - 1);
>
>         ret = INVAL_CACHE_AND_WAIT(map, chip, adr,
>                                    adr, len,
> diff --git a/include/linux/mtd/flashchip.h b/include/linux/mtd/flashchip.h
> index b63fa457febd..3529683f691e 100644
> --- a/include/linux/mtd/flashchip.h
> +++ b/include/linux/mtd/flashchip.h
> @@ -85,6 +85,7 @@ struct flchip {
>         unsigned int write_suspended:1;
>         unsigned int erase_suspended:1;
>         unsigned long in_progress_block_addr;
> +       unsigned long in_progress_block_mask;
>
>         struct mutex mutex;
>         wait_queue_head_t wq; /* Wait on here when we're waiting for the chip
> --
> 2.13.6
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



-- 
Thanks,
//richard

Re: [PATCH 3/3] cfi_cmdset_0002: Do not allow read/write to suspend erase block.

[Richard Weinberger]

Joakim,

On Thu, Mar 1, 2018 at 2:39 PM, Joakim Tjernlund
<joakim.tjernlund@infinera.com> wrote:
> Currently it is possible to read and/or write to suspend EB's.
> Writing /dev/mtdX or /dev/mtdblockX from several processes may
> break the flash state machine.
>
> Taken from cfi_cmdset_0001 driver.

Does cfi_cmdset_0020 also need fixing?

> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 56aa6b75213d..d524a64ed754 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -816,9 +816,10 @@ static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr
>                     (mode == FL_WRITING && (cfip->EraseSuspend & 0x2))))
>                         goto sleep;
>
> -               /* We could check to see if we're trying to access the sector
> -                * that is currently being erased. However, no user will try
> -                * anything like that so we just wait for the timeout. */

:-)

> +               /* Do not allow suspend iff read/write to EB address */
> +               if ((adr & chip->in_progress_block_mask) ==
> +                   chip->in_progress_block_addr)
> +                       goto sleep;
>
>                 /* Erase suspend */
>                 /* It's harmless to issue the Erase-Suspend and Erase-Resume
> @@ -2267,6 +2268,7 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip)
>         chip->state = FL_ERASING;
>         chip->erase_suspended = 0;
>         chip->in_progress_block_addr = adr;
> +       chip->in_progress_block_mask = ~(map->size - 1);
>
>         INVALIDATE_CACHE_UDELAY(map, chip,
>                                 adr, map->size,
> @@ -2356,6 +2358,7 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
>         chip->state = FL_ERASING;
>         chip->erase_suspended = 0;
>         chip->in_progress_block_addr = adr;
> +       chip->in_progress_block_mask = ~(len - 1);
>
>         INVALIDATE_CACHE_UDELAY(map, chip,
>                                 adr, len,

Reviewed-by: Richard Weinberger <richard@nod.at>

-- 
Thanks,
//richard

Re: [PATCH 1/3] cfi_cmdset_0001: Do not allow read/write to suspend erase block.

[Joakim Tjernlund]

On Thu, 2018-03-22 at 15:14 +0100, Richard Weinberger wrote:
> 
> On Thu, Mar 1, 2018 at 2:39 PM, Joakim Tjernlund
> <joakim.tjernlund@infinera.com> wrote:
> > From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> > 
> > Currently it is possible to read and/or write to suspend EB's.
> > Writing /dev/mtdX or /dev/mtdblockX from several processes may
> > break the flash state machine.
> > 
> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > Cc: <stable@vger.kernel.org>
> > ---
> >  drivers/mtd/chips/cfi_cmdset_0001.c | 16 +++++++++++-----
> >  include/linux/mtd/flashchip.h       |  1 +
> >  2 files changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> > index 60d5d19e347f..b59872304ae7 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> > @@ -849,21 +849,25 @@ static int chip_ready (struct map_info *map, struct flchip *chip, unsigned long
> >                      (mode == FL_WRITING && (cfip->SuspendCmdSupport & 1))))
> >                         goto sleep;
> > 
> > +               /* Do not allow suspend iff read/write to EB address */
> > +               if ((adr & chip->in_progress_block_mask) ==
> > +                   chip->in_progress_block_addr)
> > +                       goto sleep;
> > 
> >                 /* Erase suspend */
> > -               map_write(map, CMD(0xB0), adr);
> > +               map_write(map, CMD(0xB0), chip->in_progress_block_addr);
> > 
> >                 /* If the flash has finished erasing, then 'erase suspend'
> >                  * appears to make some (28F320) flash devices switch to
> >                  * 'read' mode.  Make sure that we switch to 'read status'
> >                  * mode so we get the right data. --rmk
> >                  */
> > -               map_write(map, CMD(0x70), adr);
> > +               map_write(map, CMD(0x70), chip->in_progress_block_addr);
> 
> Why do you change to chip->in_progress_block_addr here?

To be consistent, in_progress_block_addr is the block in progress. adr will work too
but it looks odd to mix adr and in_progress_block_addr so change it for clarity.

> 
> >                 chip->oldstate = FL_ERASING;
> >                 chip->state = FL_ERASE_SUSPENDING;
> >                 chip->erase_suspended = 1;
> >                 for (;;) {
> > -                       status = map_read(map, adr);
> > +                       status = map_read(map, chip->in_progress_block_addr);
> >                         if (map_word_andequal(map, status, status_OK, status_OK))
> >                                 break;
> > 
> > @@ -1059,8 +1063,8 @@ static void put_chip(struct map_info *map, struct flchip *chip, unsigned long ad
> >                    sending the 0x70 (Read Status) command to an erasing
> >                    chip and expecting it to be ignored, that's what we
> >                    do. */
> > -               map_write(map, CMD(0xd0), adr);
> > -               map_write(map, CMD(0x70), adr);
> > +               map_write(map, CMD(0xd0), chip->in_progress_block_addr);
> > +               map_write(map, CMD(0x70), chip->in_progress_block_addr);
> >                 chip->oldstate = FL_READY;
> >                 chip->state = FL_ERASING;
> >                 break;
> > @@ -1951,6 +1955,8 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
> >         map_write(map, CMD(0xD0), adr);
> >         chip->state = FL_ERASING;
> >         chip->erase_suspended = 0;
> > +       chip->in_progress_block_addr = adr;
> > +       chip->in_progress_block_mask = ~(len - 1);
> > 
> >         ret = INVAL_CACHE_AND_WAIT(map, chip, adr,
> >                                    adr, len,
> > diff --git a/include/linux/mtd/flashchip.h b/include/linux/mtd/flashchip.h
> > index b63fa457febd..3529683f691e 100644
> > --- a/include/linux/mtd/flashchip.h
> > +++ b/include/linux/mtd/flashchip.h
> > @@ -85,6 +85,7 @@ struct flchip {
> >         unsigned int write_suspended:1;
> >         unsigned int erase_suspended:1;
> >         unsigned long in_progress_block_addr;
> > +       unsigned long in_progress_block_mask;
> > 
> >         struct mutex mutex;
> >         wait_queue_head_t wq; /* Wait on here when we're waiting for the chip
> > --
> > 2.13.6
> > 
> > 
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
> 
> 
> --
> Thanks,
> //richard

Re: [PATCH 3/3] cfi_cmdset_0002: Do not allow read/write to suspend erase block.

[Joakim Tjernlund]

On Thu, 2018-03-22 at 15:21 +0100, Richard Weinberger wrote:
> 
> Joakim,
> 
> On Thu, Mar 1, 2018 at 2:39 PM, Joakim Tjernlund
> <joakim.tjernlund@infinera.com> wrote:
> > Currently it is possible to read and/or write to suspend EB's.
> > Writing /dev/mtdX or /dev/mtdblockX from several processes may
> > break the flash state machine.
> > 
> > Taken from cfi_cmdset_0001 driver.
> 
> Does cfi_cmdset_0020 also need fixing?

Dunno, but my guess is yes.

 Jocke

Re: [PATCH 0/3] mtd: fix AMD/Intel flash bugs

[Joakim Tjernlund]

On Thu, 2018-03-15 at 18:55 +0100, Joakim Tjernlund wrote:
> On Thu, 2018-03-15 at 16:54 +0100, Boris Brezillon wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > 
> > 
> > On Sun, 11 Mar 2018 16:06:03 +0000
> > Joakim Tjernlund <Joakim.Tjernlund@infinera.com> wrote:
> > 
> > > On Thu, 2018-03-01 at 14:39 +0100, Joakim Tjernlund wrote:
> > > > This fixes a bug which allows read/write to a erase block currently
> > > > erasing.
> > > > Also works around a chip bug in Micron 28F00AP30 chips.
> > > > 
> > > > Joakim Tjernlund (3):
> > > >   cfi_cmdset_0001: Do not allow read/write to suspend erase block.
> > > >   cfi_cmdset_0001: Workaround Micron Erase suspend bug.
> > > >   cfi_cmdset_0002: Do not allow read/write to suspend erase block.
> > > > 
> > > >  drivers/mtd/chips/cfi_cmdset_0001.c | 33 ++++++++++++++++++++++++++++-----
> > > >  drivers/mtd/chips/cfi_cmdset_0002.c |  9 ++++++---
> > > >  include/linux/mtd/flashchip.h       |  1 +
> > > >  3 files changed, 35 insertions(+), 8 deletions(-)
> > > > 
> > > 
> > > Ping?
> > 
> > You didn't Cc the MTD maintainers, so that's not surprising you don't
> > get a reply from us :P.
> 
> I find it more surprising that the MTD maintainers doesn't read the MTD list ? I figured
> this was a requirement.
> 
> > 
> > > This is a fairly serious bug as it confuses the flash state machine,
> > > making further flash access fail.
> > 
> > I'll try to have a look.
> 
> Thanks

Ping? 

Re: [PATCH 0/3] mtd: fix AMD/Intel flash bugs

[Boris Brezillon]

On Thu,  1 Mar 2018 14:39:38 +0100
Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:

> This fixes a bug which allows read/write to a erase block currently
> erasing.
> Also works around a chip bug in Micron 28F00AP30 chips.
> 
> Joakim Tjernlund (3):
>   cfi_cmdset_0001: Do not allow read/write to suspend erase block.
>   cfi_cmdset_0001: Workaround Micron Erase suspend bug.
>   cfi_cmdset_0002: Do not allow read/write to suspend erase block.
> 

All these patches have a Cc-stable tag but no Fixes tag. Can you add
a proper Fixes to each of them?

>  drivers/mtd/chips/cfi_cmdset_0001.c | 33 ++++++++++++++++++++++++++++-----
>  drivers/mtd/chips/cfi_cmdset_0002.c |  9 ++++++---
>  include/linux/mtd/flashchip.h       |  1 +
>  3 files changed, 35 insertions(+), 8 deletions(-)
> 

Re: [PATCH 0/3] mtd: fix AMD/Intel flash bugs

[Joakim Tjernlund]

On Fri, 2018-04-20 at 21:05 +0200, Boris Brezillon wrote:
> 
> On Thu,  1 Mar 2018 14:39:38 +0100
> Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:
> 
> > This fixes a bug which allows read/write to a erase block currently
> > erasing.
> > Also works around a chip bug in Micron 28F00AP30 chips.
> > 
> > Joakim Tjernlund (3):
> >   cfi_cmdset_0001: Do not allow read/write to suspend erase block.
> >   cfi_cmdset_0001: Workaround Micron Erase suspend bug.
> >   cfi_cmdset_0002: Do not allow read/write to suspend erase block.
> > 
> 
> All these patches have a Cc-stable tag but no Fixes tag. Can you add
> a proper Fixes to each of them?

I don't think I can, the code is ancient in this area.

Re: [PATCH 1/3] cfi_cmdset_0001: Do not allow read/write to suspend erase block.

[Boris Brezillon]

On Thu,  1 Mar 2018 14:39:39 +0100
Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:

> From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> 
> Currently it is possible to read and/or write to suspend EB's.
> Writing /dev/mtdX or /dev/mtdblockX from several processes may
> break the flash state machine.
> 
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> Cc: <stable@vger.kernel.org>

Applied the patch series to mtd/master after changing the subject
prefix for "mtd: cfi: cmdset_xxx: ".

I'll send a fixes PR to Linus later this week.

Thanks,

Boris

> ---
>  drivers/mtd/chips/cfi_cmdset_0001.c | 16 +++++++++++-----
>  include/linux/mtd/flashchip.h       |  1 +
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> index 60d5d19e347f..b59872304ae7 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> @@ -849,21 +849,25 @@ static int chip_ready (struct map_info *map, struct flchip *chip, unsigned long
>  		     (mode == FL_WRITING && (cfip->SuspendCmdSupport & 1))))
>  			goto sleep;
>  
> +		/* Do not allow suspend iff read/write to EB address */
> +		if ((adr & chip->in_progress_block_mask) ==
> +		    chip->in_progress_block_addr)
> +			goto sleep;
>  
>  		/* Erase suspend */
> -		map_write(map, CMD(0xB0), adr);
> +		map_write(map, CMD(0xB0), chip->in_progress_block_addr);
>  
>  		/* If the flash has finished erasing, then 'erase suspend'
>  		 * appears to make some (28F320) flash devices switch to
>  		 * 'read' mode.  Make sure that we switch to 'read status'
>  		 * mode so we get the right data. --rmk
>  		 */
> -		map_write(map, CMD(0x70), adr);
> +		map_write(map, CMD(0x70), chip->in_progress_block_addr);
>  		chip->oldstate = FL_ERASING;
>  		chip->state = FL_ERASE_SUSPENDING;
>  		chip->erase_suspended = 1;
>  		for (;;) {
> -			status = map_read(map, adr);
> +			status = map_read(map, chip->in_progress_block_addr);
>  			if (map_word_andequal(map, status, status_OK, status_OK))
>  			        break;
>  
> @@ -1059,8 +1063,8 @@ static void put_chip(struct map_info *map, struct flchip *chip, unsigned long ad
>  		   sending the 0x70 (Read Status) command to an erasing
>  		   chip and expecting it to be ignored, that's what we
>  		   do. */
> -		map_write(map, CMD(0xd0), adr);
> -		map_write(map, CMD(0x70), adr);
> +		map_write(map, CMD(0xd0), chip->in_progress_block_addr);
> +		map_write(map, CMD(0x70), chip->in_progress_block_addr);
>  		chip->oldstate = FL_READY;
>  		chip->state = FL_ERASING;
>  		break;
> @@ -1951,6 +1955,8 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
>  	map_write(map, CMD(0xD0), adr);
>  	chip->state = FL_ERASING;
>  	chip->erase_suspended = 0;
> +	chip->in_progress_block_addr = adr;
> +	chip->in_progress_block_mask = ~(len - 1);
>  
>  	ret = INVAL_CACHE_AND_WAIT(map, chip, adr,
>  				   adr, len,
> diff --git a/include/linux/mtd/flashchip.h b/include/linux/mtd/flashchip.h
> index b63fa457febd..3529683f691e 100644
> --- a/include/linux/mtd/flashchip.h
> +++ b/include/linux/mtd/flashchip.h
> @@ -85,6 +85,7 @@ struct flchip {
>  	unsigned int write_suspended:1;
>  	unsigned int erase_suspended:1;
>  	unsigned long in_progress_block_addr;
> +	unsigned long in_progress_block_mask;
>  
>  	struct mutex mutex;
>  	wait_queue_head_t wq; /* Wait on here when we're waiting for the chip