[PATCH] mtd: fix race in cfi_cmdset_0001 driver

[PATCH] mtd: fix race in cfi_cmdset_0001 driver

[Joakim Tjernlund]

As inval_cache_and_wait_for_operation() drop and reclaim the lock
to invalidate the cache, some other thread may suspend the operation
before reaching the for(;;) loop. Therefore the loop must start with
checking the chip->state before reading status from the chip.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---

 David, I think this should go to Linus asap.
 Stefan and Michael, would be great if you could
 reply with a Acked-By: too.

 drivers/mtd/chips/cfi_cmdset_0001.c |   43 ++++++++++++++++++-----------------
 1 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
index e89f2d0..9772a62 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -1245,10 +1245,32 @@ static int inval_cache_and_wait_for_operation(
 	sleep_time = chip_op_time / 2;
 
 	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);
+			mutex_unlock(&chip->mutex);
+			schedule();
+			remove_wait_queue(&chip->wq, &wait);
+			mutex_lock(&chip->mutex);
+			continue;
+		}
+
 		status = map_read(map, cmd_adr);
 		if (map_word_andequal(map, status, status_OK, status_OK))
 			break;
 
+		if (chip->erase_suspended && chip_state == FL_ERASING)  {
+			/* Erase suspend occured while sleep: reset timeout */
+			timeo = reset_timeo;
+			chip->erase_suspended = 0;
+		}
+		if (chip->write_suspended && chip_state == FL_WRITING)  {
+			/* Write suspend occured while sleep: reset timeout */
+			timeo = reset_timeo;
+			chip->write_suspended = 0;
+		}
 		if (!timeo) {
 			map_write(map, CMD(0x70), cmd_adr);
 			chip->state = FL_STATUS;
@@ -1272,27 +1294,6 @@ static int inval_cache_and_wait_for_operation(
 			timeo--;
 		}
 		mutex_lock(&chip->mutex);
-
-		while (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);
-			mutex_unlock(&chip->mutex);
-			schedule();
-			remove_wait_queue(&chip->wq, &wait);
-			mutex_lock(&chip->mutex);
-		}
-		if (chip->erase_suspended && chip_state == FL_ERASING)  {
-			/* Erase suspend occured while sleep: reset timeout */
-			timeo = reset_timeo;
-			chip->erase_suspended = 0;
-		}
-		if (chip->write_suspended && chip_state == FL_WRITING)  {
-			/* Write suspend occured while sleep: reset timeout */
-			timeo = reset_timeo;
-			chip->write_suspended = 0;
-		}
 	}
 
 	/* Done and happy. */
-- 
1.7.3.4

Re: [PATCH] mtd: fix race in cfi_cmdset_0001 driver

[Stefan Bigler]

Am 02/07/2011 05:07 PM, schrieb Joakim Tjernlund:
> As inval_cache_and_wait_for_operation() drop and reclaim the lock
> to invalidate the cache, some other thread may suspend the operation
> before reaching the for(;;) loop. Therefore the loop must start with
> checking the chip->state before reading status from the chip.
>
> Signed-off-by: Joakim Tjernlund<Joakim.Tjernlund@transmode.se>
> ---
>
>   David, I think this should go to Linus asap.
>   Stefan and Michael, would be great if you could
>   reply with a Acked-By: too.
>
>   drivers/mtd/chips/cfi_cmdset_0001.c |   43 ++++++++++++++++++-----------------
>   1 files changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> index e89f2d0..9772a62 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> @@ -1245,10 +1245,32 @@ static int inval_cache_and_wait_for_operation(
>   	sleep_time = chip_op_time / 2;
>
>   	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);
> +			mutex_unlock(&chip->mutex);
> +			schedule();
> +			remove_wait_queue(&chip->wq,&wait);
> +			mutex_lock(&chip->mutex);
> +			continue;
> +		}
> +
>   		status = map_read(map, cmd_adr);
>   		if (map_word_andequal(map, status, status_OK, status_OK))
>   			break;
>
> +		if (chip->erase_suspended&&  chip_state == FL_ERASING)  {
> +			/* Erase suspend occured while sleep: reset timeout */
> +			timeo = reset_timeo;
> +			chip->erase_suspended = 0;
> +		}
> +		if (chip->write_suspended&&  chip_state == FL_WRITING)  {
> +			/* Write suspend occured while sleep: reset timeout */
> +			timeo = reset_timeo;
> +			chip->write_suspended = 0;
> +		}
>   		if (!timeo) {
>   			map_write(map, CMD(0x70), cmd_adr);
>   			chip->state = FL_STATUS;
> @@ -1272,27 +1294,6 @@ static int inval_cache_and_wait_for_operation(
>   			timeo--;
>   		}
>   		mutex_lock(&chip->mutex);
> -
> -		while (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);
> -			mutex_unlock(&chip->mutex);
> -			schedule();
> -			remove_wait_queue(&chip->wq,&wait);
> -			mutex_lock(&chip->mutex);
> -		}
> -		if (chip->erase_suspended&&  chip_state == FL_ERASING)  {
> -			/* Erase suspend occured while sleep: reset timeout */
> -			timeo = reset_timeo;
> -			chip->erase_suspended = 0;
> -		}
> -		if (chip->write_suspended&&  chip_state == FL_WRITING)  {
> -			/* Write suspend occured while sleep: reset timeout */
> -			timeo = reset_timeo;
> -			chip->write_suspended = 0;
> -		}
>   	}
>
>   	/* Done and happy. */

Acked-by: Stefan Bigler<stefan.bigler@keymile.com>

Thanks!

Stefan

Re: [PATCH] mtd: fix race in cfi_cmdset_0001 driver

[Michael Cashwell]

On Feb 7, 2011, at 11:07 AM, Joakim Tjernlund wrote:

> As inval_cache_and_wait_for_operation() drop and reclaim the lock
> to invalidate the cache, some other thread may suspend the operation
> before reaching the for(;;) loop. Therefore the loop must start with
> checking the chip->state before reading status from the chip.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
> 
> David, I think this should go to Linus asap.
> Stefan and Michael, would be great if you could
> reply with a Acked-By: too.
> 
> drivers/mtd/chips/cfi_cmdset_0001.c |   43 ++++++++++++++++++-----------------
> 1 files changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> index e89f2d0..9772a62 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> @@ -1245,10 +1245,32 @@ static int inval_cache_and_wait_for_operation(
> 	sleep_time = chip_op_time / 2;
> 
> 	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);
> +			mutex_unlock(&chip->mutex);
> +			schedule();
> +			remove_wait_queue(&chip->wq, &wait);
> +			mutex_lock(&chip->mutex);
> +			continue;
> +		}
> +
> 		status = map_read(map, cmd_adr);
> 		if (map_word_andequal(map, status, status_OK, status_OK))
> 			break;
> 
> +		if (chip->erase_suspended && chip_state == FL_ERASING)  {
> +			/* Erase suspend occured while sleep: reset timeout */
> +			timeo = reset_timeo;
> +			chip->erase_suspended = 0;
> +		}
> +		if (chip->write_suspended && chip_state == FL_WRITING)  {
> +			/* Write suspend occured while sleep: reset timeout */
> +			timeo = reset_timeo;
> +			chip->write_suspended = 0;
> +		}
> 		if (!timeo) {
> 			map_write(map, CMD(0x70), cmd_adr);
> 			chip->state = FL_STATUS;
> @@ -1272,27 +1294,6 @@ static int inval_cache_and_wait_for_operation(
> 			timeo--;
> 		}
> 		mutex_lock(&chip->mutex);
> -
> -		while (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);
> -			mutex_unlock(&chip->mutex);
> -			schedule();
> -			remove_wait_queue(&chip->wq, &wait);
> -			mutex_lock(&chip->mutex);
> -		}
> -		if (chip->erase_suspended && chip_state == FL_ERASING)  {
> -			/* Erase suspend occured while sleep: reset timeout */
> -			timeo = reset_timeo;
> -			chip->erase_suspended = 0;
> -		}
> -		if (chip->write_suspended && chip_state == FL_WRITING)  {
> -			/* Write suspend occured while sleep: reset timeout */
> -			timeo = reset_timeo;
> -			chip->write_suspended = 0;
> -		}
> 	}
> 
> 	/* Done and happy. */

Acked-by: Michael Cashwell <mboards@prograde.net>

-Mike

Re: [PATCH] mtd: fix race in cfi_cmdset_0001 driver

[Artem Bityutskiy]

On Mon, 2011-02-07 at 17:07 +0100, Joakim Tjernlund wrote:
> As inval_cache_and_wait_for_operation() drop and reclaim the lock
> to invalidate the cache, some other thread may suspend the operation
> before reaching the for(;;) loop. Therefore the loop must start with
> checking the chip->state before reading status from the chip.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>

Am I right that this patch should be ignored and a new patch will be
created?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH] mtd: fix race in cfi_cmdset_0001 driver

[Joakim Tjernlund]

Artem Bityutskiy <dedekind1@gmail.com> wrote on 2011/02/11 15:02:16:
>
> On Mon, 2011-02-07 at 17:07 +0100, Joakim Tjernlund wrote:
> > As inval_cache_and_wait_for_operation() drop and reclaim the lock
> > to invalidate the cache, some other thread may suspend the operation
> > before reaching the for(;;) loop. Therefore the loop must start with
> > checking the chip->state before reading status from the chip.
> >
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
>
> Am I right that this patch should be ignored and a new patch will be
> created?

No, this is the real thing. Should go to Linus ASAP.

There is a hunt for another flash HW related bug hunt ongoing too
that may result in another patch touching other areas.

 Jocke

Re: [PATCH] mtd: fix race in cfi_cmdset_0001 driver

[Artem Bityutskiy]

On Fri, 2011-02-11 at 15:46 +0100, Joakim Tjernlund wrote:
> Artem Bityutskiy <dedekind1@gmail.com> wrote on 2011/02/11 15:02:16:
> >
> > On Mon, 2011-02-07 at 17:07 +0100, Joakim Tjernlund wrote:
> > > As inval_cache_and_wait_for_operation() drop and reclaim the lock
> > > to invalidate the cache, some other thread may suspend the operation
> > > before reaching the for(;;) loop. Therefore the loop must start with
> > > checking the chip->state before reading status from the chip.
> > >
> > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> >
> > Am I right that this patch should be ignored and a new patch will be
> > created?
> 
> No, this is the real thing. Should go to Linus ASAP.
> 
> There is a hunt for another flash HW related bug hunt ongoing too
> that may result in another patch touching other areas.

OK, all I can do is pushing to the l2 tree, the Linus story is up to
dwmw2 as usually, you should bug him. All I can do for you is letting
him know about this in the mtd chat.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH] mtd: fix race in cfi_cmdset_0001 driver

[Joakim Tjernlund]

Artem Bityutskiy <dedekind1@gmail.com> wrote on 2011/02/11 15:50:54:
>
> On Fri, 2011-02-11 at 15:46 +0100, Joakim Tjernlund wrote:
> > Artem Bityutskiy <dedekind1@gmail.com> wrote on 2011/02/11 15:02:16:
> > >
> > > On Mon, 2011-02-07 at 17:07 +0100, Joakim Tjernlund wrote:
> > > > As inval_cache_and_wait_for_operation() drop and reclaim the lock
> > > > to invalidate the cache, some other thread may suspend the operation
> > > > before reaching the for(;;) loop. Therefore the loop must start with
> > > > checking the chip->state before reading status from the chip.
> > > >
> > > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > >
> > > Am I right that this patch should be ignored and a new patch will be
> > > created?
> >
> > No, this is the real thing. Should go to Linus ASAP.
> >
> > There is a hunt for another flash HW related bug hunt ongoing too
> > that may result in another patch touching other areas.
>
> OK, all I can do is pushing to the l2 tree, the Linus story is up to
> dwmw2 as usually, you should bug him. All I can do for you is letting
> him know about this in the mtd chat.

Please do, I have mailed him directly too but he seems very busy.

 Jocke

Re: [PATCH] mtd: fix race in cfi_cmdset_0001 driver

[Artem Bityutskiy]

On Mon, 2011-02-07 at 17:07 +0100, Joakim Tjernlund wrote:
> As inval_cache_and_wait_for_operation() drop and reclaim the lock
> to invalidate the cache, some other thread may suspend the operation
> before reaching the for(;;) loop. Therefore the loop must start with
> checking the chip->state before reading status from the chip.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>

Pushed to l2-mtd-2.6.git, thanks!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)