| Message ID | 1297094831-10330-1-git-send-email-Joakim.Tjernlund@transmode.se |
|---|---|
| State | Accepted |
| Commit | ecf3fde07c8dcb92a1bf3fbdfe70905d85cd00e1 |
| Headers | show |
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
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
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?
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
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.
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
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!
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. */
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(-)