From: Michael Cashwell <mboards@prograde.net>
To: Joakim Tjernlund <joakim.tjernlund@transmode.se>
Cc: linux-mtd@lists.infradead.org,
Holger brunck <holger.brunck@keymile.com>,
stefan.bigler@keymile.com
Subject: Re: Numonyx NOR and chip->mutex bug?
Date: Fri, 4 Feb 2011 12:09:01 -0500 [thread overview]
Message-ID: <541E19B8-D428-4F59-B6BB-A3BD8F455AE4@prograde.net> (raw)
In-Reply-To: <OF5A6BB547.3462D591-ONC125782D.00475578-C125782D.0047EC74@transmode.se>
On Feb 4, 2011, at 8:05 AM, Joakim Tjernlund wrote:
> Stefan,
>
> I think the bug lies in
> static int inval_cache_and_wait_for_operation(
> struct map_info *map, struct flchip *chip,
> unsigned long cmd_adr, unsigned long inval_adr, int inval_len,
> unsigned int chip_op_time, unsigned int chip_op_time_max)
> {
> struct cfi_private *cfi = map->fldrv_priv;
> map_word status, status_OK = CMD(0x80);
> int chip_state = chip->state;
> unsigned int timeo, sleep_time, reset_timeo;
>
> mutex_unlock(&chip->mutex);
> if (inval_len)
> INVALIDATE_CACHED_RANGE(map, inval_adr, inval_len);
> mutex_lock(&chip->mutex);
>
> Here we drop the lock and take it again. Someone may suspend the
> erase and do a read/write instead and put the chip some other state.
That's where I get confused. It's true that another thread could do that but wouldn't it have to bracket its operation with calls to get_chip() and put_chip()? Those actually do the suspend and resume. I'm certainly seeing erases being suspended to do a buffered write and then being resumed.
A properly done suspend/resume should leave the hardware is the same state it was in before hand. So if get_chip/put_chip are not doing that then that's where the problem is.
Originally I thought a udelay(20) was needed after the resume command was sent. What I've most recently seen is that just doing one map_read() of status (and discarding the result) is enough to avoid my error:
switch(chip->oldstate) {
case FL_ERASING:
chip->state = chip->oldstate;
/* What if one interleaved chip has finished and the
other hasn't? The old code would leave the finished
one in READY mode. That's bad, and caused -EROFS
errors to be returned from do_erase_oneblock because
that's the only bit it checked for at the time.
As the state machine appears to explicitly allow
sending the 0x70 (Read Status) command to an erasing
chip and expecting it to be ignored, that's what we
do. */
map_write(map, CMD(0xd0), adr);
map_write(map, CMD(0x70), adr);
(void) map_read(map, adr); // <<<<----- added
chip->oldstate = FL_READY;
chip->state = FL_ERASING;
break;
I'm still not sure why. My guess is that it's timing (since udelay(20) in the same spot also works for me) but being timing-sensitive is making it hard to pin down. Adding printk()s also often makes it not happen.
-Mike
next prev parent reply other threads:[~2011-02-04 17:08 UTC|newest]
Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-01-25 18:14 Numonyx NOR and chip->mutex bug? Michael Cashwell
2011-01-25 18:56 ` Joakim Tjernlund
2011-01-25 22:03 ` Michael Cashwell
2011-01-25 23:09 ` Joakim Tjernlund
2011-02-02 16:20 ` Michael Cashwell
2011-02-02 17:37 ` Stefan Bigler
2011-02-02 20:12 ` Joakim Tjernlund
2011-02-02 21:19 ` Michael Cashwell
2011-02-03 8:11 ` Joakim Tjernlund
2011-02-03 9:50 ` Joakim Tjernlund
2011-02-03 15:24 ` Michael Cashwell
2011-02-03 16:38 ` Stefan Bigler
2011-02-03 23:18 ` Stefan Bigler
2011-02-04 10:47 ` Joakim Tjernlund
2011-02-04 11:04 ` Stefan Bigler
2011-02-04 12:26 ` Joakim Tjernlund
2011-02-04 12:35 ` Joakim Tjernlund
2011-02-04 12:42 ` Joakim Tjernlund
2011-02-04 13:05 ` Joakim Tjernlund
2011-02-04 13:25 ` Joakim Tjernlund
2011-02-04 16:45 ` Stefan Bigler
2011-02-04 16:55 ` Joakim Tjernlund
2011-02-04 17:09 ` Michael Cashwell [this message]
[not found] ` <OF42EF <F66AF016-8A2B-4116-BE49-CE05B91BE50F@prograde.net>
[not found] ` <OF42EF<F66AF016-8A2B-4116-BE49-CE05B91BE50F@prograde.net>
2011-02-05 10:29 ` Joakim Tjernlund
2011-02-05 19:19 ` Michael Cashwell
2011-02-05 21:47 ` Michael Cashwell
2011-02-06 9:46 ` Joakim Tjernlund
2011-02-06 15:49 ` Michael Cashwell
2011-02-06 17:29 ` Joakim Tjernlund
2011-02-06 21:13 ` Michael Cashwell
[not found] ` <OF2C1ABD39 <4D5005E4.1040506@keymile.com>
[not found] ` <OFFF2C6D34.91E5D6C3-ONC1257 <96BD3889-E8AD-408D-8275-ED1A5FD55F1B@prograde.net>
[not found] ` <OF2C1ABD39<4D5005E4.1040506@keymile.com>
[not found] ` <OFFF2C6D34.91E5D6C3-ONC1257<96BD3889-E8AD-408D-8275-ED1A5FD55F1B@prograde.net>
[not found] ` <OF9738D658.F46<4C117A67-5057-4ACD-8EBE-04E9C782570C@prograde.net>
[not found] ` <OF6D40AC82.1008D3DD-ONC1 <4D53E660.4020305@users.sourceforge.net>
2011-02-06 21:20 ` Joakim Tjernlund
2011-02-07 14:47 ` Stefan Bigler
2011-02-07 15:01 ` Joakim Tjernlund
2011-02-07 15:46 ` Michael Cashwell
2011-02-07 15:52 ` Stefan Bigler
2011-02-07 16:22 ` Joakim Tjernlund
2011-02-07 16:46 ` Michael Cashwell
2011-02-07 17:08 ` Stefan Bigler
2011-02-07 19:04 ` Michael Cashwell
2011-02-09 19:52 ` Michael Cashwell
2011-02-09 20:13 ` Joakim Tjernlund
2011-02-09 21:59 ` Michael Cashwell
2011-02-10 13:21 ` Anders Grafström
2011-02-10 14:04 ` Joakim Tjernlund
2011-02-10 15:04 ` Joakim Tjernlund
2011-02-10 15:59 ` Michael Cashwell
2011-02-10 16:05 ` Joakim Tjernlund
2011-02-10 16:41 ` Michael Cashwell
2011-02-10 16:46 ` Joakim Tjernlund
2011-02-10 17:02 ` Joakim Tjernlund
2011-02-10 17:10 ` Michael Cashwell
2011-02-10 17:20 ` Joakim Tjernlund
2011-02-10 17:47 ` Joakim Tjernlund
2011-02-10 18:26 ` Joakim Tjernlund
2011-02-11 18:03 ` Michael Cashwell
2011-02-12 10:47 ` Joakim Tjernlund
2011-02-14 15:59 ` Michael Cashwell
2011-02-14 15:44 ` Michael Cashwell
2011-02-10 16:43 ` Michael Cashwell
2011-02-10 17:54 ` Anders Grafström
2011-02-11 15:05 ` Michael Cashwell
2011-02-11 15:39 ` Joakim Tjernlund
2011-02-14 16:15 ` Michael Cashwell
2011-02-11 17:00 ` Joakim Tjernlund
2011-02-10 15:43 ` Michael Cashwell
2011-02-10 15:51 ` Joakim Tjernlund
2011-02-24 10:50 ` Joakim Tjernlund
2011-02-24 11:36 ` Joakim Tjernlund
2011-02-24 14:28 ` Michael Cashwell
2011-02-10 14:53 ` Joakim Tjernlund
2011-02-06 9:40 ` Joakim Tjernlund
2011-02-06 14:55 ` Michael Cashwell
2011-02-07 15:10 ` Michael Cashwell
2011-02-07 15:48 ` Joakim Tjernlund
2011-02-03 13:24 ` Michael Cashwell
2011-02-03 14:01 ` Joakim Tjernlund
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=541E19B8-D428-4F59-B6BB-A3BD8F455AE4@prograde.net \
--to=mboards@prograde.net \
--cc=holger.brunck@keymile.com \
--cc=joakim.tjernlund@transmode.se \
--cc=linux-mtd@lists.infradead.org \
--cc=stefan.bigler@keymile.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.