| Message ID | 200911191101.nAJB1wwH000784@linpc062.aimsys.nl |
|---|---|
| State | Accepted |
| Commit | 2695eab964efaa382168e0351705967bd9deb7ea |
| Headers | show |
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 >
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 ?
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.
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.
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 ?
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.
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.
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!
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
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(-)