Re-entrancy of flash erase/write

Re-entrancy of flash erase/write

[Robert Kaiser]

Hi,

maybe I just don't know where to look, but..

Am I right in assuming that it is not possible to erase
and/or write different blocks of the same flash chip
concurrently ?

If so, where is the lock in the MTD code that would
prevent concurrent execution of erase/write on the
same chip by multiple processes ?

Thanks for any insights!

Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

Re: Re-entrancy of flash erase/write

[David Woodhouse]

rob@sysgo.de said:
>  Am I right in assuming that it is not possible to erase and/or write
> different blocks of the same flash chip concurrently ?

No - on some chips you can. Not most though.

> If so, where is the lock in the MTD code that would prevent concurrent
> execution of erase/write on the same chip by multiple processes ? 

In the chip drivers themselves. Grep for TASK_UNINTERRUPTIBLE in 
cfi_cmdset_0001.c, for example.


--
dwmw2

Re: Re-entrancy of flash erase/write

[Robert Kaiser]

On Fri, 22 Feb 2002, David Woodhouse wrote:

> 
> rob@sysgo.de said:
> >  Am I right in assuming that it is not possible to erase and/or write
> > different blocks of the same flash chip concurrently ?
> 
> No - on some chips you can. Not most though.
> 
> > If so, where is the lock in the MTD code that would prevent concurrent
> > execution of erase/write on the same chip by multiple processes ? 
> 
> In the chip drivers themselves. Grep for TASK_UNINTERRUPTIBLE in 
> cfi_cmdset_0001.c, for example.


OK, thanks!

The reason I'm asking is that I have observed some strange behaviour
while testing my new concat_erase function and I'm wondering wether
it is a bug in my code or a general conceptual problem with MTD:

In my test, I was repeatedly extracting a large tar archive unto a
JFFS2-mounted device, deleting all the files again, then doing
another tar extract. What happened was that at some point, JFFS's
garbage collector started erasing blocks while tar was continuing
to write to flash. In that situation, it ocassionally happened that
VPP was turned off by one of the threads while the other one was
still busy writing/erasing, causing that operation to fail.
I was able to fix this by introducing a counter in my map driver's
set_vpp() function and doing the actual disable/enable of VPP only
when the counter reaches zero. Now I'm wondering if this is a flaw
in MTD or in my new code.

Any ideas ?


Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

Re: Re-entrancy of flash erase/write

[Robert Kaiser]

On Fri, 22 Feb 2002, Robert Kaiser wrote:

> 
> On Fri, 22 Feb 2002, David Woodhouse wrote:
> 
> > 
> > rob@sysgo.de said:
> > > .., where is the lock in the MTD code that would prevent concurrent
> > > execution of erase/write on the same chip by multiple processes ? 
> > 
> > In the chip drivers themselves. Grep for TASK_UNINTERRUPTIBLE in 
> > cfi_cmdset_0001.c, for example.
> 
> .. What happened was that at some point, JFFS's
> garbage collector started erasing blocks while tar was continuing
> to write to flash. In that situation, it ocassionally happened that
> VPP was turned off by one of the threads while the other one was
> still busy writing/erasing, causing that operation to fail.
> I was able to fix this by introducing a counter in my map driver's
> set_vpp() function and doing the actual disable/enable of VPP only
> when the counter reaches zero. Now I'm wondering if this is a flaw
> in MTD or in my new code.

I looked into this a little further: it seems that the erase
functions in cfi_cmdset_000?.c and amd_flash.c temporarily release
the chip->mutex while VPP is on. Won't that allow other threads
to get hold of the mutex and fiddle with VPP, possibly turning it
off ?

That would explain the behavior I'm observing here. Only why doensn't
anyone else see this problem ?

A proper solution IMHO would be to use a counter (similar to what I
now have in my mapping driver) in the ENABLE/DISABLE_VPP() macros.

Any ideas ?


Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

Re: Re-entrancy of flash erase/write

[Adam Wozniak]

On Mon, 25 Feb 2002, Robert Kaiser wrote:
> I looked into this a little further: it seems that the erase
> functions in cfi_cmdset_000?.c and amd_flash.c temporarily release
> the chip->mutex while VPP is on. Won't that allow other threads
> to get hold of the mutex and fiddle with VPP, possibly turning it
> off ?
>
> That would explain the behavior I'm observing here. Only why doensn't
> anyone else see this problem ?
>
> A proper solution IMHO would be to use a counter (similar to what I
> now have in my mapping driver) in the ENABLE/DISABLE_VPP() macros.

If I recall correctly, last time I dug into this:

ENABLE/DISABLE_VPP  call the map_info set_vpp function
For the physmap_map map_info, there is no set_vpp, it is NULL

So for people who use physmap_map anyway, they won't see the behavior
you are describing.  I suspect it is the same for other maps.

--Adam
-- 
Adam Wozniak (KG6GZR)   COM DEV Broadband - Digital and Software Systems
awozniak@comdev.cc      805 Aerovista Place, San Luis Obispo, CA 93401
                        http://www.comdev.cc
                        Voice: (805) 544-1089       Fax: (805) 544-2055

Re: Re-entrancy of flash erase/write

[Robert Kaiser]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 779 bytes --]



On Mon, 25 Feb 2002, Adam Wozniak wrote:

> ENABLE/DISABLE_VPP  call the map_info set_vpp function
> For the physmap_map map_info, there is no set_vpp, it is NULL
> 
> So for people who use physmap_map anyway, they won't see the behavior
> you are describing.  I suspect it is the same for other maps.


Hmm, so you are suggesting that this problem has not been noticed
because so few people have set_vpp() functions these days ?

If that is the case, may I suggest the attached patch. It solved the
problem for me.

[David, OK to apply this ?]

Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1174 bytes --]

Index: map.h
===================================================================
RCS file: /home/cvs/mtd/include/linux/mtd/map.h,v
retrieving revision 1.27
diff -u -p -r1.27 map.h
--- map.h	2002/02/21 08:26:59	1.27
+++ map.h	2002/02/25 16:51:36
@@ -47,6 +47,7 @@ struct map_info {
 	void (*copy_to)(struct map_info *, unsigned long, const void *, ssize_t);
 
 	void (*set_vpp)(struct map_info *, int);
+	int  vpp_counter;
 	/* We put these two here rather than a single void *map_priv, 
 	   because we want mappers to be able to have quickly-accessible
 	   cache for the 'currently-mapped page' without the _extra_
@@ -90,7 +91,7 @@ static inline void map_destroy(struct mt
 	kfree(mtd);
 }
 
-#define ENABLE_VPP(map) do { if(map->set_vpp) map->set_vpp(map, 1); } while(0)
-#define DISABLE_VPP(map) do { if(map->set_vpp) map->set_vpp(map, 0); } while(0)
+#define ENABLE_VPP(map) do { if(map->set_vpp) if(++map->vpp_counter == 1) map->set_vpp(map, 1); } while(0)
+#define DISABLE_VPP(map) do { if(map->set_vpp) { if(--map->vpp_counter == 0) map->set_vpp(map, 0); else if(map->vpp_counter < 0) BUG();} } while(0)
 
 #endif /* __LINUX_MTD_MAP_H__ */

Re: Re-entrancy of flash erase/write

[Adam Wozniak]

On Mon, 25 Feb 2002, Robert Kaiser wrote:
> On Mon, 25 Feb 2002, Adam Wozniak wrote:
>
> > ENABLE/DISABLE_VPP  call the map_info set_vpp function
> > For the physmap_map map_info, there is no set_vpp, it is NULL
> >
> > So for people who use physmap_map anyway, they won't see the behavior
> > you are describing.  I suspect it is the same for other maps.
>
> Hmm, so you are suggesting that this problem has not been noticed
> because so few people have set_vpp() functions these days ?

It explains why many people may not see the problem you're describing.
I have no way to test that theory, however. =)

--Adam
-- 
Adam Wozniak (KG6GZR)   COM DEV Broadband - Digital and Software Systems
awozniak@comdev.cc      805 Aerovista Place, San Luis Obispo, CA 93401
                        http://www.comdev.cc
                        Voice: (805) 544-1089       Fax: (805) 544-2055

Re: Re-entrancy of flash erase/write

[David Woodhouse]

rob@sysgo.de said:
>  [David, OK to apply this ?] 

I'd rather it was done in the map driver's set_vpp function itself. 
Sometimes you have more than one map driver, because you have different 
regions of flash - but they all share the same Vpp control. In which case 
you need to do the counting in the map driver anyway. 


>  I looked into this a little further: it seems that the erase
> functions in cfi_cmdset_000?.c and amd_flash.c temporarily release the
> chip->mutex while VPP is on. Won't that allow other threads to get
> hold of the mutex and fiddle with VPP, possibly turning it off ? 

I see the possibility, but surely that would only _actually_ happen if we 
started to support writes during erase suspend? Otherwise, the only thing 
that should happen during an erase is a _read_, which doesn't set and clear 
vpp.


--
dwmw2

Re: Re-entrancy of flash erase/write

[Robert Kaiser]

On Mon, 25 Feb 2002, David Woodhouse wrote:

> 
> rob@sysgo.de said:
> >  [David, OK to apply this ?] 
> 
> I'd rather it was done in the map driver's set_vpp function itself. 
> Sometimes you have more than one map driver, because you have different 
> regions of flash - but they all share the same Vpp control. In which case 
> you need to do the counting in the map driver anyway. 

Agreed. I'll modify the dilnetpc driver then.

> 
> 
> >  I looked into this a little further: it seems that the erase
> > functions in cfi_cmdset_000?.c and amd_flash.c temporarily release the
> > chip->mutex while VPP is on. Won't that allow other threads to get
> > hold of the mutex and fiddle with VPP, possibly turning it off ? 
> 
> I see the possibility, but surely that would only _actually_ happen if we 
> started to support writes during erase suspend? Otherwise, the only thing 
> that should happen during an erase is a _read_, which doesn't set and clear 
> vpp.

What mechanism is there to avoid writes during erase suspend ? I have
seen the problem in situations where the JFFS2 GC thread was erasing
while another process (tar in my case) was writing to a *different
partition* in the same chip. (Maybe that is a hint ?) Introducing the 
set_vpp counter reliably solved this.

Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

Re: Re-entrancy of flash erase/write

[David Woodhouse]

rob@sysgo.de said:
>  What mechanism is there to avoid writes during erase suspend ? I have
> seen the problem in situations where the JFFS2 GC thread was erasing
> while another process (tar in my case) was writing to a *different
> partition* in the same chip. (Maybe that is a hint ?) Introducing the
> set_vpp counter reliably solved this.

All the partition code does is add an offset and call into the original 
underlying flash driver. So it just looks like two concurrent calls to the 
one driver, and the state machine ought to deal with that correctly.

--
dwmw2

RE: Re-entrancy of flash erase/write

[Joakim Tjernlund]

> I see the possibility, but surely that would only _actually_ happen if we 
> started to support writes during erase suspend? Otherwise, the only thing 
> that should happen during an erase is a _read_, which doesn't set and clear 
> vpp.
The patch I sent a week or 2 ago support that for cmdset0001.c

 Jocke

Re: Re-entrancy of flash erase/write

[David Woodhouse]

joakim.tjernlund@lumentis.se said:
> The patch I sent a week or 2 ago support that for cmdset0001.c

Ah, sorry. Resend (or pointer)?

--
dwmw2

RE: Re-entrancy of flash erase/write

[Joakim Tjernlund]

> 
> joakim.tjernlund@lumentis.se said:
> > The patch I sent a week or 2 ago support that for cmdset0001.c
> 
> Ah, sorry. Resend (or pointer)?
pointer, http://lists.infradead.org/pipermail/linux-mtd/2002-February/004117.html

But there is one problem, I did this against 1.91 so I don't know if it applies 
against latest CVS. Has anybody tried?

 Jocke

Re: Re-entrancy of flash erase/write

[David Woodhouse]

joakim.tjernlund@lumentis.se said:
>  But there is one problem, I did this against 1.91 so I don't know if
> it applies  against latest CVS. Has anybody tried? 

passion /home/dwmw2/working/mtd $ patch -p0 --dry-run < ~/Mail/lists/mtd/368
missing header for unified diff at line 89 of patch
patching file `drivers/mtd/chips/cfi_cmdset_0001.c'
Hunk #1 FAILED at 4.
Hunk #2 FAILED at 61.
Hunk #3 succeeded at 156 (offset 3 lines).
Hunk #5 succeeded at 233 with fuzz 2 (offset 3 lines).
Hunk #6 succeeded at 284 (offset 4 lines).
Hunk #7 succeeded at 302 (offset 3 lines).
Hunk #8 succeeded at 337 (offset 4 lines).
Hunk #9 FAILED at 565.
Hunk #10 succeeded at 579 (offset 3 lines).
Hunk #11 succeeded at 606 (offset 4 lines).
Hunk #12 succeeded at 715 (offset 3 lines).
Hunk #13 FAILED at 865.
Hunk #14 succeeded at 889 (offset 10 lines).
Hunk #15 succeeded at 905 (offset 3 lines).
Hunk #16 succeeded at 1077 (offset 12 lines).
Hunk #17 succeeded at 1248 (offset 3 lines).
Hunk #18 FAILED at 1275.
5 out of 18 hunks FAILED -- saving rejects to drivers/mtd/chips/cfi_cmdset_0001.c.rej


--
dwmw2

RE: Re-entrancy of flash erase/write

[Robert Kaiser]

On Mon, 25 Feb 2002, Joakim Tjernlund wrote:

> > I see the possibility, but surely that would only _actually_ happen if we 
> > started to support writes during erase suspend? Otherwise, the only thing 
> > that should happen during an erase is a _read_, which doesn't set and clear 
> > vpp.
> The patch I sent a week or 2 ago support that for cmdset0001.c

Bingo! I have been using that version.

Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

RE: Re-entrancy of flash erase/write

[Joakim Tjernlund]

> On Mon, 25 Feb 2002, Joakim Tjernlund wrote:
> 
> > > I see the possibility, but surely that would only _actually_ happen if we 
> > > started to support writes during erase suspend? Otherwise, the only thing 
> > > that should happen during an erase is a _read_, which doesn't set and clear 
> > > vpp.
> > The patch I sent a week or 2 ago support that for cmdset0001.c
> 
> Bingo! I have been using that version.
> 
> Rob
Besides the VPP problem you had, is it stable for you? It is for me but I would
like confirmation from others.

RE: Re-entrancy of flash erase/write

[Jonas Holmberg]

> Am I right in assuming that it is not possible to erase
> and/or write different blocks of the same flash chip
> concurrently ?

Some chips allow it and some don't. Those that allow it require all simultaneous erases to be started within a very short timeout. None of the chip drivers use that feature I think.

> If so, where is the lock in the MTD code that would
> prevent concurrent execution of erase/write on the
> same chip by multiple processes ?

I haven't got the code in front of me, but I think it is in the beginning of the erase_oneblock-functions (in cfi_cmdset_0002.c at least). There's a status variable in the flashchip struct and processes are put on a waitqueue if the chip is busy.

/Jonas