RE: [PATCH] cfi: Fixup of write errors on XIP

RE: [PATCH] cfi: Fixup of write errors on XIP

[Korolev, Alexey]

Hi all,

Several days ago I sent this patch to the list. I wonder are there any
objections to it?
I guess this patch shouldn't break anything. It just fixes write errors
I saw in our tests. The patch has been verified on several
configurations. 
If you don't mind I will commit it by the end of this week.

Thanks,
Alexey 

 
> Here is fixup of issue I have seen on XIP configuration. 
> Sometimes I received write errors with message "buffer write error
> (status timeout)" on test  which did read write and erase on several
> volumes at the same time.
> 
> I investigated the issue. 
> The scenario of the issue is following:
 
> 1. do_write_buffer 
> 2. Waiting for write complete in xip_udelay
> 3. System Interrupt
> 4. Write suspend
> 5. Rescheduling
> 6. Block erasing by other process. ( This operation typically took
> rather long time )
> 7. Complete, rescheduling 
> 8. Return to write (write is not complete due to suspend ). 
> 9. Check timeout. Time is up.
> 10. Error.
>  
> I made small fixup for these issue. Please see patch below.


===================================================================
--- c/drivers/mtd/chips/cfi_cmdset_0001.c	2006-02-22
20:58:05.869203280 +0300
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c	2006-02-22
20:55:42.272033368 +0300
@@ -1571,6 +1571,7 @@
 	/* GO GO GO */
 	map_write(map, CMD(0xd0), cmd_adr);
 	chip->state = FL_WRITING;
+	chip->write_suspended = 0;
 
 	INVALIDATE_CACHE_UDELAY(map, chip, cmd_adr,
 				adr, len,
@@ -1592,6 +1593,12 @@
 			continue;
 		}
 
+		/* Somebody suspended write. We should reset timeo. */
+		if (chip->write_suspended) {
+			chip->write_suspended = 0;
+			timeo = jiffies + (HZ/2);
+		}
+		
 		status = map_read(map, cmd_adr);
 		if (map_word_andequal(map, status, status_OK,
status_OK))
 			break;
====================================================================

RE: [PATCH] cfi: Fixup of write errors on XIP

[Nicolas Pitre]

On Wed, 1 Mar 2006, Korolev, Alexey wrote:

> Hi all,
> 
> Several days ago I sent this patch to the list. I wonder are there any
> objections to it?

I do have some.

> I guess this patch shouldn't break anything. It just fixes write errors
> I saw in our tests. The patch has been verified on several
> configurations. 

It is not a proper fix.  Something else doesn't work as expected.

> If you don't mind I will commit it by the end of this week.

Please don't just yet.

> The scenario of the issue is following:
>  
> 1. do_write_buffer 
> 2. Waiting for write complete in xip_udelay
> 3. System Interrupt
> 4. Write suspend
> 5. Rescheduling
> 6. Block erasing by other process. ( This operation typically took
> rather long time )
> 7. Complete, rescheduling 
> 8. Return to write (write is not complete due to suspend ). 
> 9. Check timeout. Time is up.
> 10. Error.

This should not happen.  And if it does then the bug is in xip_udelay() 
and therefore should be fixed there.

The fact is, xip_udelay() should not return until either the flash 
status is 0x80 (done) or the delay expired.  The code looks like:

	...
	start = xip_curtime();
	...
	do {
		...
		if (xip_irqpending()...) {
			/* suspend code */
			...
			[here we substract the time waited so far]
			usec -= xip_elapsed_since(start);
			...
			/* schedule code */
			...
			/* Resume the write or erase operation */
			map_write(map, CMD(0xd0), adr);
			map_write(map, CMD(0x70), adr);
			chip->state = oldstate;
			start = xip_currtime();
		}
		...
		status = map_read(map, adr);
	} while (!map_word_andequal(map, status, OK, OK)
		 && xip_elapsed_since(start) < usec);

So, even if this gets rescheduled a long time after an erase occurred in 
another process, the "start" variable will get set to the new time when 
the write is resumed and the erase delay is therefore already accounted 
for.

What if you replace "usec -= xip_elapsed_since(start)" by
"usec -= xip_elapsed_since(start) / 2" instead?  This doesn't have to be 
exactly accurate -- in fact this is there just to avoid looping forever 
if the flash never return a success status.



Nicolas

Re: [PATCH] cfi: Fixup of write errors on XIP

[Alexey, Korolev]

Nicolas


> Please don't just yet.
>
> > The scenario of the issue is following:
> > 
> > 1. do_write_buffer
> > 2. Waiting for write complete in xip_udelay
> > 3. System Interrupt
> > 4. Write suspend
> > 5. Rescheduling
> > 6. Block erasing by other process. ( This operation typically took
> > rather long time )
> > 7. Complete, rescheduling
> > 8. Return to write (write is not complete due to suspend ).
> > 9. Check timeout. Time is up.
> > 10. Error.
>
> This should not happen.  And if it does then the bug is in xip_udelay()
> and therefore should be fixed there.
>
> The fact is, xip_udelay() should not return until either the flash
> status is 0x80 (done) or the delay expired.  The code looks like:
>
This is absolutelly correct.
But delay may expire sometimes before chip get ready even if chip has 
not been suspended.
Buffer programming time for chip may vary.
For example timeout has expired couple usecs before status get ready. 
(the such variations are absolutely ok).
You go up to do_write_buffer,  and get the described scenario if chips 
has been suspended at the very begging of waiting in xip_uddelay.
I thought about possibility to make fix in xip_udelay, but I didn't find 
a good solution here.

Thanks a lot,
Alexey

Re: [PATCH] cfi: Fixup of write errors on XIP

[Nicolas Pitre]

I'm back, sorry for the delay.

On Thu, 2 Mar 2006, Alexey, Korolev wrote:

> Nicolas
> 
> 
> > Please don't just yet.
> > 
> > > The scenario of the issue is following:
> > > > 1. do_write_buffer
> > > 2. Waiting for write complete in xip_udelay
> > > 3. System Interrupt
> > > 4. Write suspend
> > > 5. Rescheduling
> > > 6. Block erasing by other process. ( This operation typically took
> > > rather long time )
> > > 7. Complete, rescheduling
> > > 8. Return to write (write is not complete due to suspend ).
> > > 9. Check timeout. Time is up.
> > > 10. Error.
> > 
> > This should not happen.  And if it does then the bug is in xip_udelay()
> > and therefore should be fixed there.
> > 
> > The fact is, xip_udelay() should not return until either the flash
> > status is 0x80 (done) or the delay expired.  The code looks like:
> > 
> This is absolutelly correct.
> But delay may expire sometimes before chip get ready even if chip has not been
> suspended.

So?

> Buffer programming time for chip may vary.
> For example timeout has expired couple usecs before status get ready. (the
> such variations are absolutely ok).

Agreed.

> You go up to do_write_buffer,  and get the described scenario if chips has
> been suspended at the very begging of waiting in xip_uddelay.

I still don't see the problem.

Let's suppose we enter xip_udelay(), and scheduling happens, and another 
or even multiple other threads take their time to erase the flash, say 
for more than 4 seconds.  Currently xip_udelay() remembers how much time 
it had remaining before being suspended and will report that time when 
it is scheduled back.  So xip_udelay() should never return before the 
estimated amount of time required to perform the write as actually been 
spent actively writing.

Do we agree so far?

Now xip_udelay() may return to do_write_buffer() either because the 
write has completed, or the timeout occurred.  Like you said the timeout 
is normal since the write might still have a few microseconds to go.  
But only at that point is the timeo variable initialized with
jiffies + (HZ/2).

Further down, though, is a call to UDELAY() going again into 
xip_udelay() but this time any suspended operation is not accounted for 
in the timeo variable.

But the thing is, if you look at get_chip(), you'll see that nothing can 
go and erase another flash sector when a write is suspended.  In other 
words, completion of a write operation always has priority on any erase 
attempt.  So the problem you're describing may not be due to any erase 
delay occurring in another thread.

The only possibility I can see is that xip_udelay() is interrupted so 
often that the UDELAY(map, chip, cmd_adr, 1) call never gets a chance to 
make any progress, even in the course of a half second wall clock time.

Do you have a high interrupt rate in your system?

If this is not the case that means there is a real bug somewhere and I'd 
prefer that the real bug be found and addressed instead of masking it 
out.


Nicolas

Re: [PATCH] cfi: Fixup of write errors on XIP

[Alexey, Korolev]

Hi,

>
> I'm back, sorry for the delay.
>
I'm sorry too. I just returned from vacations.

> But the thing is, if you look at get_chip(), you'll see that nothing can
> go and erase another flash sector when a write is suspended.  In other
> words, completion of a write operation always has priority on any erase
> attempt.  So the problem you're describing may not be due to any erase
> delay occurring in another thread.
>
Oh. Seems if XIP while write is happened get chip will not allow to erase?

> The only possibility I can see is that xip_udelay() is interrupted so
> often that the UDELAY(map, chip, cmd_adr, 1) call never gets a chance to
> make any progress, even in the course of a half second wall clock time.
>
> Do you have a high interrupt rate in your system?
>
Iterrupt rate is not high. It's about one per 10ms. I'm going to make 
some more investigations.
I would be happy to hear your suggestions.

Thanks a lot,
Alexey

Re: [PATCH] cfi: Fixup of write errors on XIP

[Nicolas Pitre]

On Tue, 21 Mar 2006, Alexey, Korolev wrote:

> > But the thing is, if you look at get_chip(), you'll see that nothing can
> > go and erase another flash sector when a write is suspended.  In other
> > words, completion of a write operation always has priority on any erase
> > attempt.  So the problem you're describing may not be due to any erase
> > delay occurring in another thread.
> > 
> Oh. Seems if XIP while write is happened get chip will not allow to erase?

An erase request will never proceed if a write operation is pending.  
And this is true whether you have XIP or not.  The rational is that 
writes are significantly faster than erases so they should complete 
before any erase is started.


Nicolas

Re: [PATCH] cfi: Fixup of write errors on XIP

[Alexey, Korolev]

Nicolas,

I've made some more investigations for the write errors issue on XIP.
The issue takes place when I attempt to write some data to one chip and 
erase data from another.

I collected a debug log describing the issue. Please see it below:
XIP udelay start waiting for WRITE
IRQ while WRITE
XIP udelay start waiting for ERASE
IRQ while ERASE
IRQ while ERASE
IRQ while ERASE
IRQ while ERASE
IRQ while ERASE (45 times)
...
WRITE 1 buffer write error (status timeout)
IRQ while ERASE
IRQ while ERASE
ERASE DONE

So there are two processes which have the same priority.
Rescheduling happens not so often. Once writing process has been 
switched to erasing process, next switch may not happen for very long 
time >1/2sec.
The problem here that cond_resched call doesn't switch processes often. 
(I mean if we have two processes of the same priority, cond_resched will 
switch active process with some low probability.)

Another problem here if I try to use several processes of the same 
priority.
In this case the probability to switch back to write procedure is much 
lower than before .
I made very simple test:

dd if=rnd of=/dev/mtd3 bs=1k count=16k&
flash_eraseall /dev/mtd10&
flash_eraseall /dev/mtd11&
where:
mtd3 is mapped to the first flash chip
mtd10, mtd11 are mapped to the second flash chips.
This case I was able to reproduce the "buffer write error (status 
timeout)" issue within first 20 seconds of test.

I'm afraid that this issue can be easily reproduced in case of system 
overload.
I think it's rather probable to face the situation on embedded platform 
when you have several high priority threads consuming 99% of CPU and 
writing thread (for example logging thread).

I found two possible ways for fixing this issue:
1. Which has been sent before.
Add lines in waiting cycle of do_write_buffer.
===============
--- c/drivers/mtd/chips/cfi_cmdset_0001.c 2006-02-22 20:58:05.869203280 
+0300
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c 2006-02-22 20:55:42.272033368 
+0300
@@ -1571,6 +1571,7 @@
/* GO GO GO */
map_write(map, CMD(0xd0), cmd_adr);
chip->state = FL_WRITING;
+ chip->write_suspended = 0;

INVALIDATE_CACHE_UDELAY(map, chip, cmd_adr,
adr, len,
@@ -1592,6 +1593,12 @@
continue;
}

+ /* Somebody suspended write. We should reset timeo. */
+ if (chip->write_suspended) {
+ chip->write_suspended = 0;
+ timeo = jiffies + (HZ/2);
+ }
+
status = map_read(map, cmd_adr);
if (map_word_andequal(map, status, status_OK, status_OK))
break;
=============

2. Fixup in xip_udelay function.
xip_udelay already check's the status. So this function will not wait 
more than required.
=============
--- a/drivers/mtd/chips/cfi_cmdset_0001.c 2006-02-09 04:02:07.000000000 
+0300
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c 2006-03-28 17:35:02.747532640 
+0400
@@ -913,6 +913,7 @@
struct cfi_pri_intelext *cfip = cfi->cmdset_priv;
map_word status, OK = CMD(0x80);
unsigned long suspended, start = xip_currtime();
+ int exit_timeo = max(usec,1000000);
flstate_t oldstate, newstate;

do {
@@ -933,7 +934,7 @@
*/
map_write(map, CMD(0xb0), adr);
map_write(map, CMD(0x70), adr);
- usec -= xip_elapsed_since(start);
+ exit_timeo -= xip_elapsed_since(start);
suspended = xip_currtime();
do {
if (xip_elapsed_since(suspended) > 100000) {
@@ -1004,7 +1005,7 @@
}
status = map_read(map, adr);
} while (!map_word_andequal(map, status, OK, OK)
- && xip_elapsed_since(start) < usec);
+ && xip_elapsed_since(start) < exit_timeo);
}

#define UDELAY(map, chip, adr, usec) xip_udelay(map, chip, adr, usec)
=============

I'd like to know what solution do you prefer? If you have another it 
would be interesting to look at too.


Thanks,
Alexey

PS I'd like to note that the issue of "buffer write error (status 
timeout)" may seriously affect on file systemы because this case MTD 
reports "a lie" to upper levels. MTD successfully writes data to flash 
but it reports that write error has occurred.

Re: [PATCH] cfi: Fixup of write errors on XIP

[Nicolas Pitre]

On Tue, 28 Mar 2006, Alexey, Korolev wrote:

> Nicolas,
> 
> I've made some more investigations for the write errors issue on XIP.
> The issue takes place when I attempt to write some data to one chip and erase
> data from another.

Ahhhh !

> I'm afraid that this issue can be easily reproduced in case of system
> overload.

... and more than one flash chip.

> I found two possible ways for fixing this issue:
> 1. Which has been sent before.
> Add lines in waiting cycle of do_write_buffer.
> 
> 2. Fixup in xip_udelay function.
> xip_udelay already check's the status. So this function will not wait more
> than required.

I think both solutions, although they fix this particular problem, are 
not addressing the fundamental issue which is that the current code 
structure to cope with XIP and non-XIP is becoming a total mess.  In 
fact the whole timeout/error handling should be factored out of every 
functions into a sincle subfunction, actually one for XIP and one for 
non-XIP.  This way the non-XIP case would not have to care about chip 
state changing since that cannot happen, and the timeout treshold would 
be more accurately computed as well.


Nicolas

Re: [PATCH] cfi: Fixup of write errors on XIP

[Nicolas Pitre]

On Wed, 29 Mar 2006, Nicolas Pitre wrote:

> I think both solutions, although they fix this particular problem, are 
> not addressing the fundamental issue which is that the current code 
> structure to cope with XIP and non-XIP is becoming a total mess.  In 
> fact the whole timeout/error handling should be factored out of every 
> functions into a sincle subfunction, actually one for XIP and one for 
> non-XIP.  This way the non-XIP case would not have to care about chip 
                         ^^^^^^^
                         [I mean the XIP case of course]  

> state changing since that cannot happen, and the timeout treshold would 
> be more accurately computed as well.

Nicolas

Re: [PATCH] cfi: Fixup of write errors on XIP

[Nicolas Pitre]

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

On Wed, 29 Mar 2006, Nicolas Pitre wrote:

> On Tue, 28 Mar 2006, Alexey, Korolev wrote:
> 
> > I've made some more investigations for the write errors issue on XIP.
> > The issue takes place when I attempt to write some data to one chip and erase
> > data from another.
> > 
> > I found two possible ways for fixing this issue:
> > 1. Which has been sent before.
> > Add lines in waiting cycle of do_write_buffer.
> > 
> > 2. Fixup in xip_udelay function.
> > xip_udelay already check's the status. So this function will not wait more
> > than required.
> 
> I think both solutions, although they fix this particular problem, are 
> not addressing the fundamental issue which is that the current code 
> structure to cope with XIP and non-XIP is becoming a total mess.  In 
> fact the whole timeout/error handling should be factored out of every 
> functions into a sincle subfunction, actually one for XIP and one for 
> non-XIP.  This way the XIP case would not have to care about chip 
> state changing since that cannot happen, and the timeout treshold 
> would be more accurately computed as well.

Please find attached a patch for what I mean.

Could you test it and confirm the problem is actually gone?


Nicolas

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

Index: drivers/mtd/chips/cfi_cmdset_0001.c
===================================================================
RCS file: /home/cvs/mtd/drivers/mtd/chips/cfi_cmdset_0001.c,v
retrieving revision 1.190
diff -u -r1.190 cfi_cmdset_0001.c
--- drivers/mtd/chips/cfi_cmdset_0001.c	29 Mar 2006 22:31:38 -0000	1.190
+++ drivers/mtd/chips/cfi_cmdset_0001.c	29 Mar 2006 22:38:34 -0000
@@ -406,7 +406,7 @@
 	for (i=0; i< cfi->numchips; i++) {
 		cfi->chips[i].word_write_time = 1<<cfi->cfiq->WordWriteTimeoutTyp;
 		cfi->chips[i].buffer_write_time = 1<<cfi->cfiq->BufWriteTimeoutTyp;
-		cfi->chips[i].erase_time = 1<<cfi->cfiq->BlockEraseTimeoutTyp;
+		cfi->chips[i].erase_time = 1000<<cfi->cfiq->BlockEraseTimeoutTyp;
 		cfi->chips[i].ref_point_counter = 0;
 	}
 
@@ -895,26 +895,33 @@
 
 /*
  * When a delay is required for the flash operation to complete, the
- * xip_udelay() function is polling for both the given timeout and pending
- * (but still masked) hardware interrupts.  Whenever there is an interrupt
- * pending then the flash erase or write operation is suspended, array mode
- * restored and interrupts unmasked.  Task scheduling might also happen at that
- * point.  The CPU eventually returns from the interrupt or the call to
- * schedule() and the suspended flash operation is resumed for the remaining
- * of the delay period.
+ * xip_wait_for_operation() function is polling for both the given timeout
+ * and pending (but still masked) hardware interrupts.  Whenever there is an
+ * interrupt pending then the flash erase or write operation is suspended,
+ * array mode restored and interrupts unmasked.  Task scheduling might also
+ * happen at that point.  The CPU eventually returns from the interrupt or
+ * the call to schedule() and the suspended flash operation is resumed for
+ * the remaining of the delay period.
  *
  * Warning: this function _will_ fool interrupt latency tracing tools.
  */
 
-static void __xipram xip_udelay(struct map_info *map, struct flchip *chip,
-				unsigned long adr, int usec)
+static int __xipram xip_wait_for_operation(
+		struct map_info *map, struct flchip *chip,
+		unsigned long adr, int *chip_op_time )
 {
 	struct cfi_private *cfi = map->fldrv_priv;
 	struct cfi_pri_intelext *cfip = cfi->cmdset_priv;
 	map_word status, OK = CMD(0x80);
-	unsigned long suspended, start = xip_currtime();
+	unsigned long usec, suspended, start, done;
 	flstate_t oldstate, newstate;
 
+       	start = xip_currtime();
+	usec = *chip_op_time * 8;
+	if (usec == 0)
+		usec = 500000;
+	done = 0;
+
 	do {
 		cpu_relax();
 		if (xip_irqpending() && cfip &&
@@ -931,9 +938,9 @@
 			 * we resume the whole thing at once).  Yes, it
 			 * can happen!
 			 */
+			usec -= done;
 			map_write(map, CMD(0xb0), adr);
 			map_write(map, CMD(0x70), adr);
-			usec -= xip_elapsed_since(start);
 			suspended = xip_currtime();
 			do {
 				if (xip_elapsed_since(suspended) > 100000) {
@@ -943,7 +950,7 @@
 					 * This is a critical error but there
 					 * is not much we can do here.
 					 */
-					return;
+					return -EIO;
 				}
 				status = map_read(map, adr);
 			} while (!map_word_andequal(map, status, OK, OK));
@@ -1003,65 +1010,106 @@
 			xip_cpu_idle();
 		}
 		status = map_read(map, adr);
+		done = xip_elapsed_since(start);
 	} while (!map_word_andequal(map, status, OK, OK)
-		 && xip_elapsed_since(start) < usec);
-}
+		 && done < usec);
 
-#define UDELAY(map, chip, adr, usec)  xip_udelay(map, chip, adr, usec)
+	return (done >= usec) ? -ETIME : 0;
+}
 
 /*
  * The INVALIDATE_CACHED_RANGE() macro is normally used in parallel while
  * the flash is actively programming or erasing since we have to poll for
  * the operation to complete anyway.  We can't do that in a generic way with
  * a XIP setup so do it before the actual flash operation in this case
- * and stub it out from INVALIDATE_CACHE_UDELAY.
+ * and stub it out from INVAL_CACHE_AND_WAIT.
  */
 #define XIP_INVAL_CACHED_RANGE(map, from, size)  \
 	INVALIDATE_CACHED_RANGE(map, from, size)
 
-#define INVALIDATE_CACHE_UDELAY(map, chip, cmd_adr, adr, len, usec)  \
-	UDELAY(map, chip, cmd_adr, usec)
-
-/*
- * Extra notes:
- *
- * Activating this XIP support changes the way the code works a bit.  For
- * example the code to suspend the current process when concurrent access
- * happens is never executed because xip_udelay() will always return with the
- * same chip state as it was entered with.  This is why there is no care for
- * the presence of add_wait_queue() or schedule() calls from within a couple
- * xip_disable()'d  areas of code, like in do_erase_oneblock for example.
- * The queueing and scheduling are always happening within xip_udelay().
- *
- * Similarly, get_chip() and put_chip() just happen to always be executed
- * with chip->state set to FL_READY (or FL_XIP_WHILE_*) where flash state
- * is in array mode, therefore never executing many cases therein and not
- * causing any problem with XIP.
- */
+#define INVAL_CACHE_AND_WAIT(map, chip, cmd_adr, inval_adr, inval_len, p_usec) \
+	xip_wait_for_operation(map, chip, cmd_adr, p_usec)
 
 #else
 
 #define xip_disable(map, chip, adr)
 #define xip_enable(map, chip, adr)
 #define XIP_INVAL_CACHED_RANGE(x...)
+#define INVAL_CACHE_AND_WAIT inval_cache_and_wait_for_operation
+
+static int inval_cache_and_wait_for_operation(
+		struct map_info *map, struct flchip *chip,
+		unsigned long cmd_adr, unsigned long inval_adr, int inval_len,
+		int *chip_op_time )
+{
+	struct cfi_private *cfi = map->fldrv_priv;
+	map_word status, status_OK = CMD(0x80);
+	int z, chip_state = chip->state;
+	unsigned long timeo;
+
+	spin_unlock(chip->mutex);
+	if (inval_len)
+		INVALIDATE_CACHED_RANGE(map, inval_adr, inval_len);
+	if (*chip_op_time)
+		cfi_udelay(*chip_op_time);
+	spin_lock(chip->mutex);
+
+	timeo = *chip_op_time * 8 * HZ / 1000000;
+	if (timeo < HZ/2)
+		timeo = HZ/2;
+	timeo += jiffies;
+
+	z = 0;
+	for (;;) {
+		if (chip->state != chip_state) {
+			/* Someone's suspended the operation: sleep */
+			DECLARE_WAITQUEUE(wait, current);
+
+			set_current_state(TASK_UNINTERRUPTIBLE);
+			add_wait_queue(&chip->wq, &wait);
+			spin_unlock(chip->mutex);
+			schedule();
+			remove_wait_queue(&chip->wq, &wait);
+			timeo = jiffies + (HZ / 2); /* FIXME */
+			spin_lock(chip->mutex);
+			continue;
+		}
+
+		status = map_read(map, cmd_adr);
+		if (map_word_andequal(map, status, status_OK, status_OK))
+			break;
 
-#define UDELAY(map, chip, adr, usec)  \
-do {  \
-	spin_unlock(chip->mutex);  \
-	cfi_udelay(usec);  \
-	spin_lock(chip->mutex);  \
-} while (0)
-
-#define INVALIDATE_CACHE_UDELAY(map, chip, cmd_adr, adr, len, usec)  \
-do {  \
-	spin_unlock(chip->mutex);  \
-	INVALIDATE_CACHED_RANGE(map, adr, len);  \
-	cfi_udelay(usec);  \
-	spin_lock(chip->mutex);  \
-} while (0)
+		/* OK Still waiting */
+		if (time_after(jiffies, timeo)) {
+			map_write(map, CMD(0x70), cmd_adr);
+			chip->state = FL_STATUS;
+			return -ETIME;
+		}
+
+		/* Latency issues. Drop the lock, wait a while and retry */
+		z++;
+		spin_unlock(chip->mutex);
+		cfi_udelay(1);
+		spin_lock(chip->mutex);
+	}
+
+	if (!z) {
+		if (!--(*chip_op_time))
+			*chip_op_time = 1;
+	} else if (z > 1)
+		*chip_op_time += z/2;
+
+	/* Done and happy. */
+ 	chip->state = FL_STATUS;
+	return 0;
+}
 
 #endif
 
+#define WAIT_TIMEOUT(map, chip, adr, udelay) \
+	({ int __udelay = (udelay); \
+	   INVAL_CACHE_AND_WAIT(map, chip, adr, 0, 0, &__udelay); })
+
 static int do_point_onechip (struct map_info *map, struct flchip *chip, loff_t adr, size_t len)
 {
 	unsigned long cmd_addr;
@@ -1251,14 +1299,11 @@
 				     unsigned long adr, map_word datum, int mode)
 {
 	struct cfi_private *cfi = map->fldrv_priv;
-	map_word status, status_OK, write_cmd;
-	unsigned long timeo;
-	int z, ret=0;
+	map_word status, write_cmd;
+	int ret=0;
 
 	adr += chip->start;
 
-	/* Let's determine those according to the interleave only once */
-	status_OK = CMD(0x80);
 	switch (mode) {
 	case FL_WRITING:
 		write_cmd = (cfi->cfiq->P_ID != 0x0200) ? CMD(0x40) : CMD(0x41);
@@ -1284,57 +1329,17 @@
 	map_write(map, datum, adr);
 	chip->state = mode;
 
-	INVALIDATE_CACHE_UDELAY(map, chip, adr,
-				adr, map_bankwidth(map),
-				chip->word_write_time);
-
-	timeo = jiffies + (HZ/2);
-	z = 0;
-	for (;;) {
-		if (chip->state != mode) {
-			/* Someone's suspended the write. Sleep */
-			DECLARE_WAITQUEUE(wait, current);
-
-			set_current_state(TASK_UNINTERRUPTIBLE);
-			add_wait_queue(&chip->wq, &wait);
-			spin_unlock(chip->mutex);
-			schedule();
-			remove_wait_queue(&chip->wq, &wait);
-			timeo = jiffies + (HZ / 2); /* FIXME */
-			spin_lock(chip->mutex);
-			continue;
-		}
-
-		status = map_read(map, adr);
-		if (map_word_andequal(map, status, status_OK, status_OK))
-			break;
-
-		/* OK Still waiting */
-		if (time_after(jiffies, timeo)) {
-			map_write(map, CMD(0x70), adr);
-			chip->state = FL_STATUS;
-			xip_enable(map, chip, adr);
-			printk(KERN_ERR "%s: word write error (status timeout)\n", map->name);
-			ret = -EIO;
-			goto out;
-		}
-
-		/* Latency issues. Drop the lock, wait a while and retry */
-		z++;
-		UDELAY(map, chip, adr, 1);
-	}
-	if (!z) {
-		chip->word_write_time--;
-		if (!chip->word_write_time)
-			chip->word_write_time = 1;
+	ret = INVAL_CACHE_AND_WAIT(map, chip, adr,
+				   adr, map_bankwidth(map),
+				   &chip->word_write_time);
+	if (ret) {
+		xip_enable(map, chip, adr);
+		printk(KERN_ERR "%s: word write error (status timeout)\n", map->name);
+		goto out;
 	}
-	if (z > 1)
-		chip->word_write_time++;
-
-	/* Done and happy. */
-	chip->state = FL_STATUS;
 
 	/* check for errors */
+	status = map_read(map, adr);
 	if (map_word_bitsset(map, status, CMD(0x1a))) {
 		unsigned long chipstatus = MERGESTATUS(status);
 
@@ -1451,9 +1456,9 @@
 				    unsigned long *pvec_seek, int len)
 {
 	struct cfi_private *cfi = map->fldrv_priv;
-	map_word status, status_OK, write_cmd, datum;
-	unsigned long cmd_adr, timeo;
-	int wbufsize, z, ret=0, word_gap, words;
+	map_word status, write_cmd, datum;
+	unsigned long cmd_adr;
+	int ret, wbufsize, word_gap, words;
 	const struct kvec *vec;
 	unsigned long vec_seek;
 
@@ -1462,7 +1467,6 @@
 	cmd_adr = adr & ~(wbufsize-1);
 
 	/* Let's determine this according to the interleave only once */
-	status_OK = CMD(0x80);
 	write_cmd = (cfi->cfiq->P_ID != 0x0200) ? CMD(0xe8) : CMD(0xe9);
 
 	spin_lock(chip->mutex);
@@ -1494,32 +1498,20 @@
 	}
 
 	chip->state = FL_WRITING_TO_BUFFER;
-
-	z = 0;
-	for (;;) {
-		map_write(map, write_cmd, cmd_adr);
-
+	map_write(map, write_cmd, cmd_adr);
+	ret = WAIT_TIMEOUT(map, chip, cmd_adr, 0);
+	if (ret) {
+		/* Argh. Not ready for write to buffer */
+		map_word Xstatus = map_read(map, cmd_adr);
+		map_write(map, CMD(0x70), cmd_adr);
+		chip->state = FL_STATUS;
 		status = map_read(map, cmd_adr);
-		if (map_word_andequal(map, status, status_OK, status_OK))
-			break;
-
-		UDELAY(map, chip, cmd_adr, 1);
-
-		if (++z > 20) {
-			/* Argh. Not ready for write to buffer */
-			map_word Xstatus;
-			map_write(map, CMD(0x70), cmd_adr);
-			chip->state = FL_STATUS;
-			Xstatus = map_read(map, cmd_adr);
-			/* Odd. Clear status bits */
-			map_write(map, CMD(0x50), cmd_adr);
-			map_write(map, CMD(0x70), cmd_adr);
-			xip_enable(map, chip, cmd_adr);
-			printk(KERN_ERR "%s: Chip not ready for buffer write. status = %lx, Xstatus = %lx\n",
-			       map->name, status.x[0], Xstatus.x[0]);
-			ret = -EIO;
-			goto out;
-		}
+		map_write(map, CMD(0x50), cmd_adr);
+		map_write(map, CMD(0x70), cmd_adr);
+		xip_enable(map, chip, cmd_adr);
+		printk(KERN_ERR "%s: Chip not ready for buffer write. Xstatus = %lx, status = %lx\n",
+				map->name, Xstatus.x[0], status.x[0]);
+		goto out;
 	}
 
 	/* Figure out the number of words to write */
@@ -1574,56 +1566,19 @@
 	map_write(map, CMD(0xd0), cmd_adr);
 	chip->state = FL_WRITING;
 
-	INVALIDATE_CACHE_UDELAY(map, chip, cmd_adr,
-				adr, len,
-				chip->buffer_write_time);
-
-	timeo = jiffies + (HZ/2);
-	z = 0;
-	for (;;) {
-		if (chip->state != FL_WRITING) {
-			/* Someone's suspended the write. Sleep */
-			DECLARE_WAITQUEUE(wait, current);
-			set_current_state(TASK_UNINTERRUPTIBLE);
-			add_wait_queue(&chip->wq, &wait);
-			spin_unlock(chip->mutex);
-			schedule();
-			remove_wait_queue(&chip->wq, &wait);
-			timeo = jiffies + (HZ / 2); /* FIXME */
-			spin_lock(chip->mutex);
-			continue;
-		}
-
-		status = map_read(map, cmd_adr);
-		if (map_word_andequal(map, status, status_OK, status_OK))
-			break;
-
-		/* OK Still waiting */
-		if (time_after(jiffies, timeo)) {
-			map_write(map, CMD(0x70), cmd_adr);
-			chip->state = FL_STATUS;
-			xip_enable(map, chip, cmd_adr);
-			printk(KERN_ERR "%s: buffer write error (status timeout)\n", map->name);
-			ret = -EIO;
-			goto out;
-		}
-
-		/* Latency issues. Drop the lock, wait a while and retry */
-		z++;
-		UDELAY(map, chip, cmd_adr, 1);
-	}
-	if (!z) {
-		chip->buffer_write_time--;
-		if (!chip->buffer_write_time)
-			chip->buffer_write_time = 1;
+	ret = INVAL_CACHE_AND_WAIT(map, chip, cmd_adr,
+				   adr, len,
+				   &chip->buffer_write_time);
+	if (ret) {
+		map_write(map, CMD(0x70), cmd_adr);
+		chip->state = FL_STATUS;
+		xip_enable(map, chip, cmd_adr);
+		printk(KERN_ERR "%s: buffer write error (status timeout)\n", map->name);
+		goto out;
 	}
-	if (z > 1)
-		chip->buffer_write_time++;
-
-	/* Done and happy. */
- 	chip->state = FL_STATUS;
 
 	/* check for errors */
+	status = map_read(map, cmd_adr);
 	if (map_word_bitsset(map, status, CMD(0x1a))) {
 		unsigned long chipstatus = MERGESTATUS(status);
 
@@ -1719,17 +1674,12 @@
 				      unsigned long adr, int len, void *thunk)
 {
 	struct cfi_private *cfi = map->fldrv_priv;
-	map_word status, status_OK;
-	unsigned long timeo;
+	map_word status;
 	int retries = 3;
-	DECLARE_WAITQUEUE(wait, current);
-	int ret = 0;
+	int ret;
 
 	adr += chip->start;
 
-	/* Let's determine this according to the interleave only once */
-	status_OK = CMD(0x80);
-
  retry:
 	spin_lock(chip->mutex);
 	ret = get_chip(map, chip, adr, FL_ERASING);
@@ -1751,48 +1701,15 @@
 	chip->state = FL_ERASING;
 	chip->erase_suspended = 0;
 
-	INVALIDATE_CACHE_UDELAY(map, chip, adr,
-				adr, len,
-				chip->erase_time*1000/2);
-
-	/* FIXME. Use a timer to check this, and return immediately. */
-	/* Once the state machine's known to be working I'll do that */
-
-	timeo = jiffies + (HZ*20);
-	for (;;) {
-		if (chip->state != FL_ERASING) {
-			/* Someone's suspended the erase. Sleep */
-			set_current_state(TASK_UNINTERRUPTIBLE);
-			add_wait_queue(&chip->wq, &wait);
-			spin_unlock(chip->mutex);
-			schedule();
-			remove_wait_queue(&chip->wq, &wait);
-			spin_lock(chip->mutex);
-			continue;
-		}
-		if (chip->erase_suspended) {
-			/* This erase was suspended and resumed.
-			   Adjust the timeout */
-			timeo = jiffies + (HZ*20); /* FIXME */
-			chip->erase_suspended = 0;
-		}
-
-		status = map_read(map, adr);
-		if (map_word_andequal(map, status, status_OK, status_OK))
-			break;
-
-		/* OK Still waiting */
-		if (time_after(jiffies, timeo)) {
-			map_write(map, CMD(0x70), adr);
-			chip->state = FL_STATUS;
-			xip_enable(map, chip, adr);
-			printk(KERN_ERR "%s: block erase error: (status timeout)\n", map->name);
-			ret = -EIO;
-			goto out;
-		}
-
-		/* Latency issues. Drop the lock, wait a while and retry */
-		UDELAY(map, chip, adr, 1000000/HZ);
+	ret = INVAL_CACHE_AND_WAIT(map, chip, adr,
+				   adr, len,
+				   &chip->erase_time);
+	if (ret) {
+		map_write(map, CMD(0x70), adr);
+		chip->state = FL_STATUS;
+		xip_enable(map, chip, adr);
+		printk(KERN_ERR "%s: block erase error: (status timeout)\n", map->name);
+		goto out;
 	}
 
 	/* We've broken this before. It doesn't hurt to be safe */
@@ -1821,7 +1738,6 @@
 			ret = -EIO;
 		} else if (chipstatus & 0x20 && retries--) {
 			printk(KERN_DEBUG "block erase failed at 0x%08lx: status 0x%lx. Retrying...\n", adr, chipstatus);
-			timeo = jiffies + HZ;
 			put_chip(map, chip, adr);
 			spin_unlock(chip->mutex);
 			goto retry;
@@ -1927,15 +1843,11 @@
 {
 	struct cfi_private *cfi = map->fldrv_priv;
 	struct cfi_pri_intelext *extp = cfi->cmdset_priv;
-	map_word status, status_OK;
-	unsigned long timeo = jiffies + HZ;
+	int udelay;
 	int ret;
 
 	adr += chip->start;
 
-	/* Let's determine this according to the interleave only once */
-	status_OK = CMD(0x80);
-
 	spin_lock(chip->mutex);
 	ret = get_chip(map, chip, adr, FL_LOCKING);
 	if (ret) {
@@ -1960,41 +1872,21 @@
 	 * If Instant Individual Block Locking supported then no need
 	 * to delay.
 	 */
+	udelay = (!extp || !(extp->FeatureSupport & (1 << 5))) ? 1000000/HZ : 0;
 
-	if (!extp || !(extp->FeatureSupport & (1 << 5)))
-		UDELAY(map, chip, adr, 1000000/HZ);
-
-	/* FIXME. Use a timer to check this, and return immediately. */
-	/* Once the state machine's known to be working I'll do that */
-
-	timeo = jiffies + (HZ*20);
-	for (;;) {
-
-		status = map_read(map, adr);
-		if (map_word_andequal(map, status, status_OK, status_OK))
-			break;
-
-		/* OK Still waiting */
-		if (time_after(jiffies, timeo)) {
-			map_write(map, CMD(0x70), adr);
-			chip->state = FL_STATUS;
-			xip_enable(map, chip, adr);
-			printk(KERN_ERR "%s: block unlock error: (status timeout)\n", map->name);
-			put_chip(map, chip, adr);
-			spin_unlock(chip->mutex);
-			return -EIO;
-		}
-
-		/* Latency issues. Drop the lock, wait a while and retry */
-		UDELAY(map, chip, adr, 1);
+	ret = WAIT_TIMEOUT(map, chip, adr, udelay);
+	if (ret) {
+		map_write(map, CMD(0x70), adr);
+		chip->state = FL_STATUS;
+		xip_enable(map, chip, adr);
+		printk(KERN_ERR "%s: block unlock error: (status timeout)\n", map->name);
+		goto out;
 	}
 
-	/* Done and happy. */
-	chip->state = FL_STATUS;
 	xip_enable(map, chip, adr);
-	put_chip(map, chip, adr);
+out:	put_chip(map, chip, adr);
 	spin_unlock(chip->mutex);
-	return 0;
+	return ret;
 }
 
 static int cfi_intelext_lock(struct mtd_info *mtd, loff_t ofs, size_t len)

Re: [PATCH] cfi: Fixup of write errors on XIP

[Alexey, Korolev]

Nicolas

Thank you very much for assistance.

>>I think both solutions, although they fix this particular problem, are 
>>not addressing the fundamental issue which is that the current code 
>>structure to cope with XIP and non-XIP is becoming a total mess.  In 
>>fact the whole timeout/error handling should be factored out of every 
>>functions into a sincle subfunction, actually one for XIP and one for 
>>non-XIP.  This way the XIP case would not have to care about chip 
>>state changing since that cannot happen, and the timeout treshold 
>>would be more accurately computed as well.
>>    
>>
It's correct. I had to spent some time to find out the solution which 
doesn't break anything and doesn't change the architecure.
IMO your fix is much better than I expected.

>Please find attached a patch for what I mean.
>
>Could you test it and confirm the problem is actually gone?
>  
>
This code definetely will not cause the write error issue on XIP.
For just a case I made tests. No write errors were faced after 3 hours 
of testing.

I have one comment to the patch:

>+
>+	if (!z) {
>+		if (!--(*chip_op_time))
>+			*chip_op_time = 1;
>+	} else if (z > 1)
>+		*chip_op_time += z/2;
>
This change may negativelly affect on write performance in case of SND 
kernel configuration.
The process of write timeout increasing if faster than process of write 
timeout decreasing.
If we consider write time as a random value with mean = 
chip->buffer_write_time and unknown variance.
It means that average buffer write timeout will be greater than 
necessary timeout.
For example: consider the process : 1 write: average time + 10us; 2 
write: average time - 10us; 3 write average time time + 10us etc.

To check it I made additional performance test on station with P30 chip.
MTD RAW write performance degradation was 3% in comaprision to kernel 
witout the patch.
I think it would be better to left    *chip_op_time ++  as it was before.

Thanks,
Alexey

Re: [PATCH] cfi: Fixup of write errors on XIP

[Nicolas Pitre]

On Thu, 30 Mar 2006, Alexey, Korolev wrote:

> > Please find attached a patch for what I mean.
> > 
> > Could you test it and confirm the problem is actually gone?
> >  
> This code definetely will not cause the write error issue on XIP.
> For just a case I made tests. No write errors were faced after 3 hours of
> testing.

Good.  It also constitute a nice cleanup, as shown by diffstat:

 1 files changed, 156 insertions(+), 264 deletions(-)

> I have one comment to the patch:
> 
> > +
> > +	if (!z) {
> > +		if (!--(*chip_op_time))
> > +			*chip_op_time = 1;
> > +	} else if (z > 1)
> > +		*chip_op_time += z/2;
> > 
> This change may negativelly affect on write performance in case of SND kernel
> configuration.
> The process of write timeout increasing if faster than process of write
> timeout decreasing.
> If we consider write time as a random value with mean =
> chip->buffer_write_time and unknown variance.
> It means that average buffer write timeout will be greater than necessary
> timeout.

True.  I wanted for the delay to converge faster, especially in the 
erase case which is much larger.  I'll commit the patch with the 
original increment and think of a solution for better convergence in a 
separate patch.

Thanks for testing.


Nicolas

[PATCH] cfi: Fixup of write errors on XIP

[Korolev, Alexey]

Hi all,

Several days ago I sent this patch to the list. I wonder are there any
objections to it?
I guess this patch shouldn't break anything. It just fixes write errors
I saw in our tests. The patch has been verified on several
configurations. 
If you don't mind I will commit it by the end of this week.

Thanks,
Alexey 

 
> Here is fixup of issue I have seen on XIP configuration. 
> Sometimes I received write errors with message "buffer write error
> (status timeout)" on test  which did read write and erase on several
> volumes at the same time.
> 
> I investigated the issue. 
> The scenario of the issue is following:
 
> 1. do_write_buffer 
> 2. Waiting for write complete in xip_udelay
> 3. System Interrupt
> 4. Write suspend
> 5. Rescheduling
> 6. Block erasing by other process. ( This operation typically took
> rather long time )
> 7. Complete, rescheduling 
> 8. Return to write (write is not complete due to suspend ). 
> 9. Check timeout. Time is up.
> 10. Error.
>  
> I made small fixup for these issue. Please see patch below.


===================================================================
--- c/drivers/mtd/chips/cfi_cmdset_0001.c	2006-02-22
20:58:05.869203280 +0300
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c	2006-02-22
20:55:42.272033368 +0300
@@ -1571,6 +1571,7 @@
 	/* GO GO GO */
 	map_write(map, CMD(0xd0), cmd_adr);
 	chip->state = FL_WRITING;
+	chip->write_suspended = 0;
 
 	INVALIDATE_CACHE_UDELAY(map, chip, cmd_adr,
 				adr, len,
@@ -1592,6 +1593,12 @@
 			continue;
 		}
 
+		/* Somebody suspended write. We should reset timeo. */
+		if (chip->write_suspended) {
+			chip->write_suspended = 0;
+			timeo = jiffies + (HZ/2);
+		}
+		
 		status = map_read(map, cmd_adr);
 		if (map_word_andequal(map, status, status_OK,
status_OK))
 			break;
====================================================================

[PATCH] cfi: Fixup of write errors on XIP

[Korolev, Alexey]

Hi all,
 
Here is fixup of issue I have seen on XIP configuration. 
Sometimes I received write errors with message "buffer write error
(status timeout)" on test  which did read write and erase on several
volumes at the same time.
 
I investigated the issue. 
The scenario of the issue is following:
 
1. do_write_buffer 
2. Waiting for write complete in xip_udelay
3. System Interrupt
4. Write suspend
5. Rescheduling
6. Block erasing by other process. ( This operation typically took
rather long time )
7. Complete, rescheduling 
8. Return to write (write is not complete due to suspend ). 
9. Check timeout. Time is up.
10. Error.
 
I made small fixup for these issue. Please see patch below.

Thanks,
Alexey

===================================================================
--- c/drivers/mtd/chips/cfi_cmdset_0001.c	2006-02-22
20:58:05.869203280 +0300
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c	2006-02-22
20:55:42.272033368 +0300
@@ -1571,6 +1571,7 @@
 	/* GO GO GO */
 	map_write(map, CMD(0xd0), cmd_adr);
 	chip->state = FL_WRITING;
+	chip->write_suspended = 0;
 
 	INVALIDATE_CACHE_UDELAY(map, chip, cmd_adr,
 				adr, len,
@@ -1592,6 +1593,12 @@
 			continue;
 		}
 
+		/* Somebody suspended write. We should reset timeo. */
+		if (chip->write_suspended) {
+			chip->write_suspended = 0;
+			timeo = jiffies + (HZ/2);
+		}
+		
 		status = map_read(map, cmd_adr);
 		if (map_word_andequal(map, status, status_OK,
status_OK))
 			break;
====================================================================

Re: [PATCH] cfi: Fixup of write errors on XIP

[Alexey, Korolev]

[-- Attachment #1: Type: text/plain, Size: 981 bytes --]

Hi all.

Several days ago I sent this patch to the list. I wonder has anybody 
complains about it?
I my opinion the patch shouldn't break anything. It just fixes up write 
errors I saw in our tests.
If you don't mind I will commit it.

Thanks,
Alexey

>  
> Here is fixup of issue I have seen on XIP configuration.
> Sometimes I received write errors with message "buffer write error
> (status timeout)" on test  which did read write and erase on several
> volumes at the same time.
>  
> I investigated the issue.
> The scenario of the issue is following:
>  
> 1. do_write_buffer
> 2. Waiting for write complete in xip_udelay
> 3. System Interrupt
> 4. Write suspend
> 5. Rescheduling
> 6. Block erasing by other process. ( This operation typically took
> rather long time )
> 7. Complete, rescheduling
> 8. Return to write (write is not complete due to suspend ).
> 9. Check timeout. Time is up.
> 10. Error.
>  
> I made small fixup for these issue. Please see patch below.
>


[-- Attachment #2: XIP_wr_errors_fixup.diff --]
[-- Type: text/plain, Size: 665 bytes --]

--- c/drivers/mtd/chips/cfi_cmdset_0001.c	2006-02-22 20:58:05.869203280 +0300
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c	2006-02-22 20:55:42.272033368 +0300
@@ -1571,6 +1571,7 @@
 	/* GO GO GO */
 	map_write(map, CMD(0xd0), cmd_adr);
 	chip->state = FL_WRITING;
+	chip->write_suspended = 0;
 
 	INVALIDATE_CACHE_UDELAY(map, chip, cmd_adr,
 				adr, len,
@@ -1592,6 +1593,12 @@
 			continue;
 		}
 
+		/* Somebody suspended write. We should reset timeo. */
+		if (chip->write_suspended) {
+			chip->write_suspended = 0;
+			timeo = jiffies + (HZ/2);
+		}
+		
 		status = map_read(map, cmd_adr);
 		if (map_word_andequal(map, status, status_OK, status_OK))
 			break;