[PATCH] 2.4.27 - MTD cfi_cmdset_0002.c - Duplicate cleanup in error path

[PATCH] 2.4.27 - MTD cfi_cmdset_0002.c - Duplicate cleanup in error path

[bradgoodman.com]

Patch to 2.4.x: Corrects an obvious error where all of the cleanups are done
twice in the event of a chip programming error. This can result in
kernel BUG() getting called on subsequent programming attempts.


--- linux-2.4.22.prepatch/drivers/mtd/chips/cfi_cmdset_0002.c	Fri Jun 13 10:51:34 2003
+++ linux-2.4.22/drivers/mtd/chips/cfi_cmdset_0002.new	Thu Jul 15 14:44:30 2004
@@ -549,11 +549,6 @@
 			}
 		} else {
 			printk(KERN_WARNING "Waiting for write to complete timed out in do_write_oneword.");        
-			
-			chip->state = FL_READY;
-			wake_up(&chip->wq);
-			cfi_spin_unlock(chip->mutex);
-			DISABLE_VPP(map);
 			ret = -EIO;
 		}
 	}

Re: [PATCH] 2.4.27 - MTD cfi_cmdset_0002.c - Duplicate cleanup in error path

[Rainer Weikusat]

"bradgoodman.com" <bkgoodman@bradgoodman.com> writes:
> Patch to 2.4.x: Corrects an obvious error where all of the cleanups are done
> twice in the event of a chip programming error. This can result in
> kernel BUG() getting called on subsequent programming attempts.
>
>
> --- linux-2.4.22.prepatch/drivers/mtd/chips/cfi_cmdset_0002.c	Fri Jun 13 10:51:34 2003
> +++ linux-2.4.22/drivers/mtd/chips/cfi_cmdset_0002.new	Thu Jul 15 14:44:30 2004
> @@ -549,11 +549,6 @@
>  			}
>  		} else {
>  			printk(KERN_WARNING "Waiting for write to complete timed out in do_write_oneword.");        
> -			
> -			chip->state = FL_READY;
> -			wake_up(&chip->wq);
> -			cfi_spin_unlock(chip->mutex);
> -			DISABLE_VPP(map);
>  			ret = -EIO;
>  		}
>  	}

I suggest the following instead:

--------------------------
--- cfi_cmdset_0002.c.orig	2004-07-24 15:05:31.000000000 +0800
+++ cfi_cmdset_0002.c	2004-07-24 15:06:06.000000000 +0800
@@ -461,7 +461,6 @@
 	unsigned int dq6, dq5;	
 	struct cfi_private *cfi = map->fldrv_priv;
 	DECLARE_WAITQUEUE(wait, current);
-	int ret = 0;
 
  retry:
 	cfi_spin_lock(chip->mutex);
@@ -554,7 +553,7 @@
 			wake_up(&chip->wq);
 			cfi_spin_unlock(chip->mutex);
 			DISABLE_VPP(map);
-			ret = -EIO;
+			return -EIO;
 		}
 	}
 
@@ -563,7 +562,7 @@
 	wake_up(&chip->wq);
 	cfi_spin_unlock(chip->mutex);
 
-	return ret;
+	return 0;
 }
 
 static int cfi_amdstd_write (struct mtd_info *mtd, loff_t to , size_t len, size_t *retlen, const u_char *buf)
----------------------------

That way, it is consistent with the other low-level chip access
functions. But the algorithm is per se buggy, anyway, because except
if DQ5 was raised before, the chip is not 'ready' (for reading array
data), but still in programming mode and will remain there until the
'embedded programming algorithm' stops, because (according to the
docs) a reset command will not be accepted until DQ5 has been raised
and the opportunityto check for that is gone after the syscall
returned to the caller.

Re: [PATCH] 2.4.27 - MTD cfi_cmdset_0002.c - Duplicate cleanup in error path

[Marcelo Tosatti]

Applied Rainer's patch, its equivalent and I his
"consisteny with other algorithms" point is a good one.

Thanks guys!

On Sat, Jul 24, 2004 at 03:14:03PM +0800, Rainer Weikusat wrote:
> "bradgoodman.com" <bkgoodman@bradgoodman.com> writes:
> > Patch to 2.4.x: Corrects an obvious error where all of the cleanups are done
> > twice in the event of a chip programming error. This can result in
> > kernel BUG() getting called on subsequent programming attempts.
<snip>
> That way, it is consistent with the other low-level chip access
> functions. But the algorithm is per se buggy, anyway, because except
> if DQ5 was raised before, the chip is not 'ready' (for reading array
> data), but still in programming mode and will remain there until the
> 'embedded programming algorithm' stops, because (according to the
> docs) a reset command will not be accepted until DQ5 has been raised
> and the opportunityto check for that is gone after the syscall
> returned to the caller.

Re: [PATCH] 2.4.27 - MTD cfi_cmdset_0002.c - Duplicate cleanup in error path

[bkgoodman]

I noticed it in the 2.4.28-pre1 changelog.

I had concidered doing it has he described - I figured though as a
first-time submitter, the less I touched, the more comfortable people
would feel with it ;-)

Thanks,

-BKG

>
> Applied Rainer's patch, its equivalent and I his
> "consisteny with other algorithms" point is a good one.
>
> Thanks guys!
>
> On Sat, Jul 24, 2004 at 03:14:03PM +0800, Rainer Weikusat wrote:
>> "bradgoodman.com" <bkgoodman@bradgoodman.com> writes:
>> > Patch to 2.4.x: Corrects an obvious error where all of the cleanups
>> are done twice in the event of a chip programming error. This can
>> result in kernel BUG() getting called on subsequent programming
>> attempts.
> <snip>
>> That way, it is consistent with the other low-level chip access
>> functions. But the algorithm is per se buggy, anyway, because except
>> if DQ5 was raised before, the chip is not 'ready' (for reading array
>> data), but still in programming mode and will remain there until the
>> 'embedded programming algorithm' stops, because (according to the
>> docs) a reset command will not be accepted until DQ5 has been raised
>> and the opportunityto check for that is gone after the syscall
>> returned to the caller.