Re: BUG in mtd/chips/cfi_cmdset_0002.c for 64bit width flashes (linuxppc_2_4_devel)

Re: BUG in mtd/chips/cfi_cmdset_0002.c for 64bit width flashes (linuxppc_2_4_devel)

[Gary Thomas]

On Fri, 2004-02-06 at 06:53, Steffen Rumler wrote:
> Hi,
> 
> We have found the following bug in mtd/chips/cfi_cmdset_0002.c
> for 64bit bus width.
> 
> The routine do_write_oneword() uses the DQ6 algorithm in order
> to detect the end of programming phase (see bitkeeper: linuxppc_2_4_devel)
> 
> 
> oldstatus = cfi_read(map, adr);
> status = cfi_read(map, adr);
> 
> while( (status & dq6) != (oldstatus & dq6) &&
>          (status & dq5) != dq5 &&
>          !time_after(jiffies, timeo) ) {
> 
>      if (need_resched()) {
>          cfi_spin_unlock(chip->mutex);
>          yield();
>          cfi_spin_lock(chip->mutex);
>      } else
>          udelay(1);
> 
>      oldstatus = cfi_read( map, adr );
>      status = cfi_read( map, adr );
> }
> 
> There are two contiguous calls of cfi_read() to check for the DQ6 toggling.
> 
> But for 64bit one cfi_read() results in two flash accesses, one for
> the upper 32bit and the other for lower 32bit. In this way the DQ6 bits toggle
> for the two accesses related to one cfi_read(). The first access will be
> compared with the third and the second with the fourth.
> The end detection is broken, the body of the while loop will never be executed.
> 
> I suggest to switch to the alternative DQ7 algorithm.

You should probably discuss this with the folks that support the MTD
layer (cc'd)

-- 
Gary Thomas <gary@mlbassoc.com>
MLB Associates

Re: BUG in mtd/chips/cfi_cmdset_0002.c for 64bit width flashes (linuxppc_2_4_devel)

[Eric W. Biederman]

Gary Thomas <gary@mlbassoc.com> writes:

> On Fri, 2004-02-06 at 06:53, Steffen Rumler wrote:
> > Hi,
> > 
> > We have found the following bug in mtd/chips/cfi_cmdset_0002.c
> > for 64bit bus width.
> > 
> > The routine do_write_oneword() uses the DQ6 algorithm in order
> > to detect the end of programming phase (see bitkeeper: linuxppc_2_4_devel)
> > 
> > 
> > oldstatus = cfi_read(map, adr);
> > status = cfi_read(map, adr);
> > 
> > while( (status & dq6) != (oldstatus & dq6) &&
> >          (status & dq5) != dq5 &&
> >          !time_after(jiffies, timeo) ) {
> > 
> >      if (need_resched()) {
> >          cfi_spin_unlock(chip->mutex);
> >          yield();
> >          cfi_spin_lock(chip->mutex);
> >      } else
> >          udelay(1);
> > 
> >      oldstatus = cfi_read( map, adr );
> >      status = cfi_read( map, adr );
> > }
> > 
> > There are two contiguous calls of cfi_read() to check for the DQ6 toggling.
> > 
> > But for 64bit one cfi_read() results in two flash accesses, one for
> > the upper 32bit and the other for lower 32bit. 

Possibly there are 2 accesses on the bus but not to a single
flash chip.  If there is a problem it is with your map
driver, not generating 64bit bus transactions. 

My dimm memory says you need to use the FPU to generate a 64bit
access on PPC.

> > In this way the DQ6 bits toggle
> > for the two accesses related to one cfi_read(). The first access will be
> > compared with the third and the second with the fourth.
> > The end detection is broken, the body of the while loop will never be
> executed.
> 
> > 
> > I suggest to switch to the alternative DQ7 algorithm.

I suggest you use an uptodate version of cfi_cmdset_0002 before
complaining.  The logic that looks at dq5 in that loop was removed a
long time ago.  Of course the last merge with 2.4.x may have
happened even longer ago.

Eric