[PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN

[PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN

[Alexey Korolev]

Hi all,

CFI driver in 2.6.24 kernel is broken. Not so intensive read/write operations cause incomplete writes which lead to kernel panics in JFFS2. 
We investigated the issue - it is caused by bug in FL_SHUTDOWN parsing code. Sometimes chip returns -EIO as if it is in FL_SHUTDOWN state when it should wait in FL_PONT (error in order of conditions).

The following patch fixes the problem. 
Could you please include it?

Signed-off-by: Alexey Korolev <akorolev@infradead.org>

diff -aurpp a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
--- a/drivers/mtd/chips/cfi_cmdset_0001.c	2008-02-11 08:51:11.000000000 +0300
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c	2008-04-04 12:46:24.000000000 +0400
@@ -729,14 +729,14 @@ static int chip_ready (struct map_info *
 		chip->state = FL_READY;
 		return 0;
 
+	case FL_SHUTDOWN:
+		/* The machine is rebooting now,so no one can get chip anymore */
+		return -EIO;
 	case FL_POINT:
 		/* Only if there's no operation suspended... */
 		if (mode == FL_READY && chip->oldstate == FL_READY)
 			return 0;
 
-	case FL_SHUTDOWN:
-		/* The machine is rebooting now,so no one can get chip anymore */
-		return -EIO;
 	default:
 	sleep:
 		set_current_state(TASK_UNINTERRUPTIBLE);

Thanks,
Alexey

Re: [PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN

[Jörn Engel]

On Fri, 4 April 2008 10:06:59 +0100, Alexey Korolev wrote:
> 
> CFI driver in 2.6.24 kernel is broken. Not so intensive read/write operations cause incomplete writes which lead to kernel panics in JFFS2. 
> We investigated the issue - it is caused by bug in FL_SHUTDOWN parsing code. Sometimes chip returns -EIO as if it is in FL_SHUTDOWN state when it should wait in FL_PONT (error in order of conditions).
> 
> The following patch fixes the problem. 
> Could you please include it?
> 
> Signed-off-by: Alexey Korolev <akorolev@infradead.org>
> 
> diff -aurpp a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> --- a/drivers/mtd/chips/cfi_cmdset_0001.c	2008-02-11 08:51:11.000000000 +0300
> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c	2008-04-04 12:46:24.000000000 +0400
> @@ -729,14 +729,14 @@ static int chip_ready (struct map_info *
>  		chip->state = FL_READY;
>  		return 0;
>  
> +	case FL_SHUTDOWN:
> +		/* The machine is rebooting now,so no one can get chip anymore */
> +		return -EIO;
>  	case FL_POINT:
>  		/* Only if there's no operation suspended... */
>  		if (mode == FL_READY && chip->oldstate == FL_READY)
>  			return 0;

Ouch!  I've been bitten by these before.  After I've carefully written
code that takes advantage of missing break; statements, a colleague of
mine reordered the code and missed those subtleties.

Could you add a comment like this where appropriate:
		/* fall through */

It might even have prevented this bug if Kevin had seen such a comment
before writing his patch.

Reviewed-By: Joern Engel <joern@logfs.org>

>  
> -	case FL_SHUTDOWN:
> -		/* The machine is rebooting now,so no one can get chip anymore */
> -		return -EIO;
>  	default:
>  	sleep:
>  		set_current_state(TASK_UNINTERRUPTIBLE);

Jörn

-- 
When in doubt, punt.  When somebody actually complains, go back and fix it...
The 90% solution is a good thing.
-- Rob Landley

Re: [PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN

[Alexey Korolev]

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

Hi Jörn

> 
> Ouch!  I've been bitten by these before.  After I've carefully written
> code that takes advantage of missing break; statements, a colleague of
> mine reordered the code and missed those subtleties.
> 
> Could you add a comment like this where appropriate:
> 		/* fall through */
No problem. See patch update in next message. 
> 
> It might even have prevented this bug if Kevin had seen such a comment
> before writing his patch.
> 
> Reviewed-By: Joern Engel <joern@logfs.org>
> 
> >  
> > -	case FL_SHUTDOWN:
> > -		/* The machine is rebooting now,so no one can get chip anymore */
> > -		return -EIO;
> >  	default:
> >  	sleep:
> >  		set_current_state(TASK_UNINTERRUPTIBLE);
> 
> 

Re: [PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN

[Alexey Korolev]

Hi 


 CFI driver in 2.6.24 kernel is broken. Not so intensive read/write operations cause incomplete writes which lead to kernel panics in JFFS2. 
 We investigated the issue - it is caused by bug in FL_SHUTDOWN parsing code. Sometimes chip returns -EIO as if it is in FL_SHUTDOWN state when it should wait in FL_PONT (error in order of conditions).

The following patch fixes the bug in state parsing code of CFI. 
Also I've added comments to notify developers if they want to add new case in future.
Please include it.
 
Signed-off-by: Alexey Korolev <akorolev@infradead.org>
Reviewed-By: Joern Engel <joern@logfs.org>

diff -aurpp a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
--- a/drivers/mtd/chips/cfi_cmdset_0001.c	2008-02-11 08:51:11.000000000 +0300
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c	2008-04-04 15:50:47.000000000 +0400
@@ -669,7 +669,7 @@ static int chip_ready (struct map_info *
 			/* Someone else might have been playing with it. */
 			return -EAGAIN;
 		}
-
+		/* Fall through */
 	case FL_READY:
 	case FL_CFI_QUERY:
 	case FL_JEDEC_QUERY:
@@ -729,14 +729,14 @@ static int chip_ready (struct map_info *
 		chip->state = FL_READY;
 		return 0;
 
+	case FL_SHUTDOWN:
+		/* The machine is rebooting now,so no one can get chip anymore */
+		return -EIO;
 	case FL_POINT:
 		/* Only if there's no operation suspended... */
 		if (mode == FL_READY && chip->oldstate == FL_READY)
 			return 0;
-
-	case FL_SHUTDOWN:
-		/* The machine is rebooting now,so no one can get chip anymore */
-		return -EIO;
+		/* Fall through */
 	default:
 	sleep:
 		set_current_state(TASK_UNINTERRUPTIBLE);
-------------
Thanks,
Alexey

Re: [PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN

[Kevin Hao]

Oh, my fault. Thanks for correction.

Thanks,
Kevin

On 4/4/08, Alexey Korolev <akorolev@infradead.org> wrote:
> Hi
>
>
>  CFI driver in 2.6.24 kernel is broken. Not so intensive read/write operations cause incomplete writes which lead to kernel panics in JFFS2.
>  We investigated the issue - it is caused by bug in FL_SHUTDOWN parsing code. Sometimes chip returns -EIO as if it is in FL_SHUTDOWN state when it should wait in FL_PONT (error in order of conditions).
>
> The following patch fixes the bug in state parsing code of CFI.
> Also I've added comments to notify developers if they want to add new case in future.
> Please include it.
>
> Signed-off-by: Alexey Korolev <akorolev@infradead.org>
> Reviewed-By: Joern Engel <joern@logfs.org>
>
> diff -aurpp a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> --- a/drivers/mtd/chips/cfi_cmdset_0001.c       2008-02-11 08:51:11.000000000 +0300
> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c       2008-04-04 15:50:47.000000000 +0400
> @@ -669,7 +669,7 @@ static int chip_ready (struct map_info *
>                        /* Someone else might have been playing with it. */
>                        return -EAGAIN;
>                }
> -
> +               /* Fall through */
>        case FL_READY:
>        case FL_CFI_QUERY:
>        case FL_JEDEC_QUERY:
> @@ -729,14 +729,14 @@ static int chip_ready (struct map_info *
>                chip->state = FL_READY;
>                return 0;
>
> +       case FL_SHUTDOWN:
> +               /* The machine is rebooting now,so no one can get chip anymore */
> +               return -EIO;
>        case FL_POINT:
>                /* Only if there's no operation suspended... */
>                if (mode == FL_READY && chip->oldstate == FL_READY)
>                        return 0;
> -
> -       case FL_SHUTDOWN:
> -               /* The machine is rebooting now,so no one can get chip anymore */
> -               return -EIO;
> +               /* Fall through */
>        default:
>        sleep:
>                set_current_state(TASK_UNINTERRUPTIBLE);
> -------------
> Thanks,
> Alexey
>
>

RE: [PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN

[Joakim Tjernlund]

> -----Original Message-----
> From: linux-mtd-bounces@lists.infradead.org [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf
> Of Alexey Korolev
> Sent: den 4 april 2008 14:22
> To: Jörn Engel
> Cc: nico@cam.org; haokexin@gmail.com; dwmw2@infradead.org; akpm@linux-foundation.org; linux-
> mtd@lists.infradead.org
> Subject: Re: [PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN
> 
> Hi
> 
> 
>  CFI driver in 2.6.24 kernel is broken. Not so intensive read/write operations cause incomplete writes
> which lead to kernel panics in JFFS2.
>  We investigated the issue - it is caused by bug in FL_SHUTDOWN parsing code. Sometimes chip returns -
> EIO as if it is in FL_SHUTDOWN state when it should wait in FL_PONT (error in order of conditions).
> 
> The following patch fixes the bug in state parsing code of CFI.
> Also I've added comments to notify developers if they want to add new case in future.
> Please include it.
> 
> Signed-off-by: Alexey Korolev <akorolev@infradead.org>
> Reviewed-By: Joern Engel <joern@logfs.org>

Acked-By: Joakim Tjernlund <Joakim.Tjenrlund@transmode.se>

Guys, I think you should sent this directly to Linus as David seems
unavailable ATM. It should really go into 2.6.25

 Jocke

Re: [PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN

[Andrew Morton]

On Thu, 10 Apr 2008 00:57:07 +0200
"Joakim Tjernlund" <Joakim.Tjernlund@transmode.se> wrote:

> > -----Original Message-----
> > From: linux-mtd-bounces@lists.infradead.org [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf
> > Of Alexey Korolev
> > Sent: den 4 april 2008 14:22
> > To: J__rn Engel
> > Cc: nico@cam.org; haokexin@gmail.com; dwmw2@infradead.org; akpm@linux-foundation.org; linux-
> > mtd@lists.infradead.org
> > Subject: Re: [PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN
> > 
> > Hi
> > 
> > 
> >  CFI driver in 2.6.24 kernel is broken. Not so intensive read/write operations cause incomplete writes
> > which lead to kernel panics in JFFS2.
> >  We investigated the issue - it is caused by bug in FL_SHUTDOWN parsing code. Sometimes chip returns -
> > EIO as if it is in FL_SHUTDOWN state when it should wait in FL_PONT (error in order of conditions).
> > 
> > The following patch fixes the bug in state parsing code of CFI.
> > Also I've added comments to notify developers if they want to add new case in future.
> > Please include it.
> > 
> > Signed-off-by: Alexey Korolev <akorolev@infradead.org>
> > Reviewed-By: Joern Engel <joern@logfs.org>
> 
> Acked-By: Joakim Tjernlund <Joakim.Tjenrlund@transmode.se>
> 
> Guys, I think you should sent this directly to Linus as David seems
> unavailable ATM. It should really go into 2.6.25
> 

I don't have a copy of the patch.

RE: [PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN

[Nicolas Pitre]

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

On Thu, 10 Apr 2008, Joakim Tjernlund wrote:

> > -----Original Message-----
> > From: linux-mtd-bounces@lists.infradead.org [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf
> > Of Alexey Korolev
> > Sent: den 4 april 2008 14:22
> > To: Jörn Engel
> > Cc: nico@cam.org; haokexin@gmail.com; dwmw2@infradead.org; akpm@linux-foundation.org; linux-
> > mtd@lists.infradead.org
> > Subject: Re: [PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN
> > 
> > Hi
> > 
> > 
> >  CFI driver in 2.6.24 kernel is broken. Not so intensive read/write operations cause incomplete writes
> > which lead to kernel panics in JFFS2.
> >  We investigated the issue - it is caused by bug in FL_SHUTDOWN parsing code. Sometimes chip returns -
> > EIO as if it is in FL_SHUTDOWN state when it should wait in FL_PONT (error in order of conditions).
> > 
> > The following patch fixes the bug in state parsing code of CFI.
> > Also I've added comments to notify developers if they want to add new case in future.
> > Please include it.
> > 
> > Signed-off-by: Alexey Korolev <akorolev@infradead.org>
> > Reviewed-By: Joern Engel <joern@logfs.org>
> 
> Acked-By: Joakim Tjernlund <Joakim.Tjenrlund@transmode.se>
> 
> Guys, I think you should sent this directly to Linus as David seems
> unavailable ATM. It should really go into 2.6.25

It is already in mainline.  See commit fb6d080c.


Nicolas

RE: [PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN

[Joakim Tjernlund]

> -----Original Message-----
> From: Nicolas Pitre [mailto:nico@cam.org]
> Sent: den 10 april 2008 01:02
> To: Joakim Tjernlund
> Cc: 'Alexey Korolev'; 'Jörn Engel'; haokexin@gmail.com; dwmw2@infradead.org; akpm@linux-
> foundation.org; linux-mtd@lists.infradead.org
> Subject: RE: [PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN
> 
> On Thu, 10 Apr 2008, Joakim Tjernlund wrote:
> 
> > > -----Original Message-----
> > > From: linux-mtd-bounces@lists.infradead.org [mailto:linux-mtd-bounces@lists.infradead.org] On
> Behalf
> > > Of Alexey Korolev
> > > Sent: den 4 april 2008 14:22
> > > To: Jörn Engel
> > > Cc: nico@cam.org; haokexin@gmail.com; dwmw2@infradead.org; akpm@linux-foundation.org; linux-
> > > mtd@lists.infradead.org
> > > Subject: Re: [PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN
> > >
> > > Hi
> > >
> > >
> > >  CFI driver in 2.6.24 kernel is broken. Not so intensive read/write operations cause incomplete
> writes
> > > which lead to kernel panics in JFFS2.
> > >  We investigated the issue - it is caused by bug in FL_SHUTDOWN parsing code. Sometimes chip
> returns -
> > > EIO as if it is in FL_SHUTDOWN state when it should wait in FL_PONT (error in order of
> conditions).
> > >
> > > The following patch fixes the bug in state parsing code of CFI.
> > > Also I've added comments to notify developers if they want to add new case in future.
> > > Please include it.
> > >
> > > Signed-off-by: Alexey Korolev <akorolev@infradead.org>
> > > Reviewed-By: Joern Engel <joern@logfs.org>
> >
> > Acked-By: Joakim Tjernlund <Joakim.Tjenrlund@transmode.se>
> >
> > Guys, I think you should sent this directly to Linus as David seems
> > unavailable ATM. It should really go into 2.6.25
> 
> It is already in mainline.  See commit fb6d080c.

Oh, good. Never saw any mail about it on the list, sorry for the noise.

     Jocke

RE: [PATCH] Fix of broken state in CFI driver caused by FL_SHUTDOWN

[Alexey Korolev]

Hi, 

> > >  CFI driver in 2.6.24 kernel is broken. Not so intensive read/write operations cause incomplete writes
> > > which lead to kernel panics in JFFS2.
> > >  We investigated the issue - it is caused by bug in FL_SHUTDOWN parsing code. Sometimes chip returns -
> > > EIO as if it is in FL_SHUTDOWN state when it should wait in FL_PONT (error in order of conditions).
> > > 
> > > The following patch fixes the bug in state parsing code of CFI.
> > > Also I've added comments to notify developers if they want to add new case in future.
> > > Please include it.
> > > 
> > > Signed-off-by: Alexey Korolev <akorolev@infradead.org>
> > > Reviewed-By: Joern Engel <joern@logfs.org>
> > 
> > Acked-By: Joakim Tjernlund <Joakim.Tjenrlund@transmode.se>
> > 
> > Guys, I think you should sent this directly to Linus as David seems
> > unavailable ATM. It should really go into 2.6.25
> 
> It is already in mainline.  See commit fb6d080c.
> 
Excelent, thank you very much!

> Nicolas
>