Re: Numonyx NOR and chip->mutex bug?

[Joakim Tjernlund <joakim.tjernlund@transmode.se>]

Michael Cashwell <mboards@prograde.net> wrote on 2011/02/06 16:49:53:
>
> On Feb 6, 2011, at 4:46 AM, Joakim Tjernlund wrote:
>
> >> It's not that doing that causes incorrect bits in the SR but that it disrupts up the 0xe8/count/data... sequence that must happen atomically. That later causes a command sequence error, which is what the status 0xb0 means.
> >>
> >> So how is an erase thread waking up and not seeing the chip->state to be something other than the FL_ERASING and going back to sleep?
> >
> > The only thing I can think of the the earlier discussion about dropping the lock.
>
> Right. Specifically about why an awakened erase thread that should not proceed (because the chip->state is wrong) is doing so anyway.
>
> 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.

>
> 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.

>
> > 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

>
> 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.

>
> Makes me head hurt!

You are not alone :)