Numonyx NOR and chip->mutex bug?
Joakim Tjernlund
joakim.tjernlund at transmode.se
Mon Feb 7 10:01:30 EST 2011
Stefan Bigler <stefan.bigler at keymile.com> wrote on 2011/02/07 15:47:00:
>
> Hi
>
> I run the test on my side with the patches proposed with and without
> barrier().
> Both worked without problems.
Thanks for testing.
>
> My workspace has now the following patches:
>
> 1) clear status before suspend
> ------------------------------
> + /* numonyx data sheet clearly says to always reset the status bits
> + before resuming a suspended erase. not doing so results in
> + an ambiguous mixture of status bits once the command ends. */
> + debug_map_write(map, CMD(0x50), adr);
> + debug_map_write(map, CMD(0xd0), adr);
The 0xd0 seems should not be there, its already in place?
it does seem like a good idea to add a Clear Status to be sure you
won't end up with error bits set just before resume.
This is a separate patch though. Does your test case work without this patch
and only my patch applied?
>
>
> 2) compare chip>state and chip_state before touching the chip
> ------------------------------------------------------------
I plan to submit this one shortly, if you and Michael could Ack.
it when I do, that would be great.
>
> diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> index e89f2d0..73e29a3 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> @@ -1243,12 +1243,34 @@ static int inval_cache_and_wait_for_operation(
> timeo = 500000;
> reset_timeo = timeo;
> sleep_time = chip_op_time / 2;
> -
> + barrier(); /* make sure chip->state gets reloaded */
> 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;
> - }
> }
>
> 3) Fix for errata regarding Flexlock Write Timing
> -------------------------------------------------
> This patch is not tested because the code is not called in my
> test cases.
Same here.
>
> + int ofs_factor = cfi->interleave * cfi->device_type;
>
>
> + /* address numonyx errata regarding Flexlock Write Timing.
> + before doing a 0x60 lock/unlock sequence on a sector
> + its current lock state needs to be read and the result
> + discarded. */
> + debug_map_write(map, CMD(0x90), adr+(2*ofs_factor));
> + chip->state = FL_JEDEC_QUERY;
> + (void) cfi_read_query(map, adr+(2*ofs_factor));
>
>
> What are your plans?
> Can I do some more tests?
>
>
> Regards Stefan
>
> Attached you find the small script creating a ubivolume write to it and delete it afterwards.
>
> dd if=foofoo of=testfile bs=1024 count=1024
> date
> echo time ubimkvol /dev/ubi0 -s 6MiB -N test1
> time ubimkvol /dev/ubi0 -s 6MiB -N test1
> date
> echo time cp testfile /dev/`cat /proc/mtd | grep test1 | sed 's/: .*//' | sed 's/mtd/mtdblock/'`
> time cp testfile /dev/`cat /proc/mtd | grep test1 | sed 's/: .*//' | sed 's/mtd/mtdblock/'`
> date
> echo time ubirmvol /dev/ubi0 -N test1
> time ubirmvol /dev/ubi0 -N test1
>
>
> Am 02/06/2011 10:20 PM, schrieb Joakim Tjernlund:
> > Michael Cashwell<mboards at prograde.net> wrote on 2011/02/06 22:13:40:
> >> On Feb 6, 2011, at 12:29 PM, Joakim Tjernlund wrote:
> >>
> >>> Michael Cashwell<mboards at prograde.net> wrote on 2011/02/06 16:49:53:
> >>>
> >>>> That's clearly what's happening in Stefan's trace when thread 465 writes 0xe8 and the next write is 0x70 by thread 209. Such a sequence is absolutely illegal (for the flash) and the latter thread is the problem. If we could get a stack trace for that map_write 0x70 I think we'd find the
thread
> > awoke and touched the chip without verifying the state first. The question is why.
> >>> Without my patch it is clear that you do end up with this problem. The first time one enter the for(;;) loop the driver reads out status from the chip before checking chip->state. This of course assumes that dropping the lock earlier may cause a schedule. So far Stefans tests indicates this to
> > be true.
> >> Yes, it was your patch and his log that lead me down that path!
> >>
> >>>> One last idea.
> >>>>
> >>>> The whole for(;;) loop in inval_cache_and_wait_for_operation() looks odd to me. Continuing with your idea of moving the chip->state while loop first, I see other problems. It seems to me that anywhere we drop and retake the chip mutex the very next thing needs to be the state check loop. Any
> > break in holding that mutex means we must go back to the top and check state again.
> >>>> I don't think the code as written does that. I have a completely reordered version of this function. (It didn't fix my issue but I think mine is something else.) On Monday I'll send that to you so you can consider it.
> >>> Yes, it is a bit odd. In addition to my patch one could move the erase suspend tests before the if(!timeo) test.
> >> Precisely. I suspect you may well already have my reordered version. :-)
> > hehe, lets se(the barrier() is probably useless):
> >
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> > index e89f2d0..73e29a3 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> > @@ -1243,12 +1243,34 @@ static int inval_cache_and_wait_for_operation(
> > timeo = 500000;
> > reset_timeo = timeo;
> > sleep_time = chip_op_time / 2;
> > -
> > + barrier(); /* make sure chip->state gets reloaded */
> > 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. */
> >
> >>>>> Oh, one more thing, possibly one needs to add cpu_relax() or similar to force gcc to reload chip->state in the while loop?
> >>>> I was also wondering about possible gcc optimization issues. I'm on 4.5.2 and that worked for me with the 2003 flash part. The same binaries fail with the 2008 parts, so, I don't know.
> >>> Very recent gcc, I am 3.4.6 but recently I began testing a little with 4.5.2. I do think I will wait for 4.5.3
> >> I tried 4.5.1 but it failed for other reasons. I submitted bug reports to gnu and a fix appeared (finally) in 4.5.2. It's been good so far but I'm always mindful of that issue.
> >>
> >> Staying current is a two edge sword. In general, later gccs have better code analysis and warnings which are valuable even if we ship using an older version.
> > Precisely, but later gcc is worse optimizing trivial code.
> >
> >>>> Keep in mind that chip->state is not a hardware access. It's just another struct member. And I think that the rules are that any function call counts as a sequence point and gcc isn't allowed to think it knows what the result is and must reload it.
> >>>>
> >>>> Lastly, consider the direction of the behavior. If we're in the state-check while loop then we got there because the two things were NOT equal. If an optimization error is causing a stale value to be compared wouldn't the observed behavior be that it remains not equal? (If it's not reloaded
> > then how could it change?)
> >>>> I'd expect an optimization error like that to get us stuck in the while loop, not exit from it prematurely.
> >>> Yes, all true. I wonder though if the mutex lock/unlock counts as a reload point? These are usually some inline asm. If not one could possibly argue that the first chip->state access, before entering the while body is using an old value.
> >> Yes, how inlines interact with sequence points has never been entirely clear to me. Especially since the compiler is free to inline something I didn't tell it to and to ignore me telling to inline if it wants to.
> >>
> >> I *think* the rules are semantic. If it's written (preprocessor aside) to look like a function call then it counts as a sequence point even if it ends up being inlined. But that's all quite beyond anything I can say for sure!
> > That makes sense too.
> >
> >>>> Makes me head hurt!
> >>> You are not alone :)
> >> So collectively maybe we can make it hurt less. That's my theory, anyway, and I'm sticking to it.
> > :)
> >
>
More information about the linux-mtd
mailing list