[PATCH] enable erase-suspend-program for CFI cmdset_0002

[PATCH] enable erase-suspend-program for CFI cmdset_0002

[Norbert van Bolhuis]

erase-suspend for writing is required to avoid blocking applications that wish
to write some data (to a NOR block other than the one being erased).
Particularly, it solves some huge delays that an application (which writes to a
UBIFS) will experience if UBI attaches to empty NOR flash. In this case the
UBI background thread will erase a lot of blocks and the application can be blocked
for minutes because of the "MTD/CFI chip lock".
This feature has been disabled for years. Maybe this was because the old code
turned it on for erase-suspend read-only chips also (cfip->EraseSuspend & 0x1).
This is wrong and corrected now.
I tested this patch and it seems to work fine.

Signed-off-by: Norbert van Bolhuis <nvbolhuis@aimvalley.nl>
---
 drivers/mtd/chips/cfi_cmdset_0002.c |   16 +++-------------
 1 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 94bb61e..1d49e18 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -490,10 +490,6 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
 	}
 #endif
 
-	/* FIXME: erase-suspend-program is broken.  See
-	   http://lists.infradead.org/pipermail/linux-mtd/2003-December/009001.html */
-	printk(KERN_NOTICE "cfi_cmdset_0002: Disabling erase-suspend-program due to code brokenness.\n");
-
 	__module_get(THIS_MODULE);
 	return mtd;
 
@@ -589,15 +585,9 @@ static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr
 		return 0;
 
 	case FL_ERASING:
-		if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */
-			goto sleep;
-
-		if (!(   mode == FL_READY
-		      || mode == FL_POINT
-		      || !cfip
-		      || (mode == FL_WRITING && (cfip->EraseSuspend & 0x2))
-		      || (mode == FL_WRITING && (cfip->EraseSuspend & 0x1)
-		    )))
+		if (!cfip || !(cfip->EraseSuspend & (0x1|0x2)) ||
+		    !(mode == FL_READY || mode == FL_POINT ||
+		    (mode == FL_WRITING && (cfip->EraseSuspend & 0x2))))
 			goto sleep;
 
 		/* We could check to see if we're trying to access the sector
-- 
1.5.2.2

Re: [PATCH] enable erase-suspend-program for CFI cmdset_0002

[Nicolas Pitre]

On Thu, 19 Nov 2009, Norbert van Bolhuis wrote:

> 
> erase-suspend for writing is required to avoid blocking applications that wish
> to write some data (to a NOR block other than the one being erased).
> Particularly, it solves some huge delays that an application (which writes to a
> UBIFS) will experience if UBI attaches to empty NOR flash. In this case the
> UBI background thread will erase a lot of blocks and the application can be blocked
> for minutes because of the "MTD/CFI chip lock".
> This feature has been disabled for years. Maybe this was because the old code
> turned it on for erase-suspend read-only chips also (cfip->EraseSuspend & 0x1).
> This is wrong and corrected now.
> I tested this patch and it seems to work fine.
> 
> Signed-off-by: Norbert van Bolhuis <nvbolhuis@aimvalley.nl>

FYI: I have no experience with non-Intel parts and no good knowledge of 
the cmdset_0002 code.  So I can't review this.

> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c |   16 +++-------------
>  1 files changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 94bb61e..1d49e18 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -490,10 +490,6 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
>  	}
>  #endif
>  
> -	/* FIXME: erase-suspend-program is broken.  See
> -	   http://lists.infradead.org/pipermail/linux-mtd/2003-December/009001.html */
> -	printk(KERN_NOTICE "cfi_cmdset_0002: Disabling erase-suspend-program due to code brokenness.\n");
> -
>  	__module_get(THIS_MODULE);
>  	return mtd;
>  
> @@ -589,15 +585,9 @@ static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr
>  		return 0;
>  
>  	case FL_ERASING:
> -		if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */
> -			goto sleep;
> -
> -		if (!(   mode == FL_READY
> -		      || mode == FL_POINT
> -		      || !cfip
> -		      || (mode == FL_WRITING && (cfip->EraseSuspend & 0x2))
> -		      || (mode == FL_WRITING && (cfip->EraseSuspend & 0x1)
> -		    )))
> +		if (!cfip || !(cfip->EraseSuspend & (0x1|0x2)) ||
> +		    !(mode == FL_READY || mode == FL_POINT ||
> +		    (mode == FL_WRITING && (cfip->EraseSuspend & 0x2))))
>  			goto sleep;
>  
>  		/* We could check to see if we're trying to access the sector
> -- 
> 1.5.2.2
> 

Re: [PATCH] enable erase-suspend-program for CFI cmdset_0002

[Norbert van Bolhuis]

Nicolas Pitre wrote:
> On Thu, 19 Nov 2009, Norbert van Bolhuis wrote:
> 
>> erase-suspend for writing is required to avoid blocking applications that wish
>> to write some data (to a NOR block other than the one being erased).
>> Particularly, it solves some huge delays that an application (which writes to a
>> UBIFS) will experience if UBI attaches to empty NOR flash. In this case the
>> UBI background thread will erase a lot of blocks and the application can be blocked
>> for minutes because of the "MTD/CFI chip lock".
>> This feature has been disabled for years. Maybe this was because the old code
>> turned it on for erase-suspend read-only chips also (cfip->EraseSuspend & 0x1).
>> This is wrong and corrected now.
>> I tested this patch and it seems to work fine.
>>
>> Signed-off-by: Norbert van Bolhuis <nvbolhuis@aimvalley.nl>
> 
> FYI: I have no experience with non-Intel parts and no good knowledge of 
> the cmdset_0002 code.  So I can't review this.
> 

OK.

so, who's approving/reviewing patches for cmdset_0002. Nobody ?

Re: [PATCH] enable erase-suspend-program for CFI cmdset_0002

[Artem Bityutskiy]

On Tue, 2009-11-24 at 10:07 +0100, Norbert van Bolhuis wrote:
> Nicolas Pitre wrote:
> > On Thu, 19 Nov 2009, Norbert van Bolhuis wrote:
> > 
> >> erase-suspend for writing is required to avoid blocking applications that wish
> >> to write some data (to a NOR block other than the one being erased).
> >> Particularly, it solves some huge delays that an application (which writes to a
> >> UBIFS) will experience if UBI attaches to empty NOR flash. In this case the
> >> UBI background thread will erase a lot of blocks and the application can be blocked
> >> for minutes because of the "MTD/CFI chip lock".
> >> This feature has been disabled for years. Maybe this was because the old code
> >> turned it on for erase-suspend read-only chips also (cfip->EraseSuspend & 0x1).
> >> This is wrong and corrected now.
> >> I tested this patch and it seems to work fine.
> >>
> >> Signed-off-by: Norbert van Bolhuis <nvbolhuis@aimvalley.nl>
> > 
> > FYI: I have no experience with non-Intel parts and no good knowledge of 
> > the cmdset_0002 code.  So I can't review this.
> > 
> 
> OK.
> 
> so, who's approving/reviewing patches for cmdset_0002. Nobody ?

I think we can just take it, since you tested it and confirm it works
and solves a real problem. I'll put it to my l2 tree later, do not have
time right now.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH] enable erase-suspend-program for CFI cmdset_0002

[Artem Bityutskiy]

On Thu, 2009-11-19 at 12:01 +0100, Norbert van Bolhuis wrote:
> erase-suspend for writing is required to avoid blocking applications that wish
> to write some data (to a NOR block other than the one being erased).
> Particularly, it solves some huge delays that an application (which writes to a
> UBIFS) will experience if UBI attaches to empty NOR flash. In this case the
> UBI background thread will erase a lot of blocks and the application can be blocked
> for minutes because of the "MTD/CFI chip lock".
> This feature has been disabled for years. Maybe this was because the old code
> turned it on for erase-suspend read-only chips also (cfip->EraseSuspend & 0x1).
> This is wrong and corrected now.
> I tested this patch and it seems to work fine.
> 
> Signed-off-by: Norbert van Bolhuis <nvbolhuis@aimvalley.nl>

I did not follow the patch conversation too closely - but should you be
the patch author or Joakim? If you took it as it is - then it is Joakim.
If you modified it, then at least Joakim's name should be in the commit
log.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH] enable erase-suspend-program for CFI cmdset_0002

[Norbert van Bolhuis]

Artem Bityutskiy wrote:

> I did not follow the patch conversation too closely - but should you be
> the patch author or Joakim? If you took it as it is - then it is Joakim.
> If you modified it, then at least Joakim's name should be in the commit
> log.
> 

I also removed the "printk(ERR_NOTICE ...", so I guess I modified it.

you want me to re-submit the patch with CC or Acked-by Joakim ?

Re: [PATCH] enable erase-suspend-program for CFI cmdset_0002

[Artem Bityutskiy]

On Tue, 2009-11-24 at 16:12 +0100, Norbert van Bolhuis wrote:
> Artem Bityutskiy wrote:
> 
> > I did not follow the patch conversation too closely - but should you be
> > the patch author or Joakim? If you took it as it is - then it is Joakim.
> > If you modified it, then at least Joakim's name should be in the commit
> > log.
> > 
> 
> I also removed the "printk(ERR_NOTICE ...", so I guess I modified it.
> 
> you want me to re-submit the patch with CC or Acked-by Joakim ?

If you just modify slightly, I can just make Joakim the author, which
sounds fair. And put a not that you tweaked it - this is the common
practice. Is this ok with you?

No need to re-send, I can do these things myself.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH] enable erase-suspend-program for CFI cmdset_0002

[Norbert van Bolhuis]

Artem Bityutskiy wrote:

> 
> If you just modify slightly, I can just make Joakim the author, which
> sounds fair. And put a not that you tweaked it - this is the common
> practice. Is this ok with you?
> 

yes.

> No need to re-send, I can do these things myself.
> 

ok, good.

Re: [PATCH] enable erase-suspend-program for CFI cmdset_0002

[Artem Bityutskiy]

On Tue, 2009-11-24 at 16:20 +0100, Norbert van Bolhuis wrote:
> Artem Bityutskiy wrote:
> 
> > 
> > If you just modify slightly, I can just make Joakim the author, which
> > sounds fair. And put a not that you tweaked it - this is the common
> > practice. Is this ok with you?
> > 
> 
> yes.
> 
> > No need to re-send, I can do these things myself.
> > 
> 
> ok, good.

Pushed the patch to my l2 tree:

http://git.infradead.org/users/dedekind/l2-mtd-2.6.git/commit/6c8793f0c4eae1ce45f654a0fb6cf0b94d7061d6

Thanks!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)