Re: mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted.

[Boris Brezillon <boris.brezillon@collabora.com>]

On Thu, 14 Feb 2019 00:39:09 +0000
Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote:

> Hi All,
> 
> On 8/02/19 12:58 PM, Przemyslaw Sobon wrote:
> > Fixes: dfeae1073583(mtd: cfi_cmdset_0002: Change write buffer to
> >         check correct value)
> > 
> > There was an endless loop in CFI Flash driver when a value was written
> > incorrectly. In such case chip_ready returns true but chip_good returns
> > false and we never get out of the loop.
> > 
> > The solution was to break the loop in 2 cases, either device is ready or
> > device is not ready and timeout elapsed. The correctness of the write is
> > checked after the loop ended. That way we ensure the loop always ends.
> > 
> > Signed-off-by: Przemyslaw Sobon <psobon@amazon.com>  
> 
> Mark (cc'd) has done some testing here, and assuming he's happy with the 
> forgery.
> 
> Tested-by: Mark Tomlinson <Mark.Tomlinson@alliedtelesis.co.nz>

I'm a bit lost. Ikegami told us that checking for chip_ready() was not
enough and chip_good() could return true after a few tests even though
it initially returned false.

I'd really like to get that fixed, but it looks like you haven't reached
a consensus on what the appropriate fix is :-/.

> 
> > ---
> >   drivers/mtd/chips/cfi_cmdset_0002.c | 11 +++++++----
> >   1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index 72428b6bfc47..6cc31d2057e9 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -1879,15 +1879,18 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
> >   		if (time_after(jiffies, timeo) && !chip_ready(map, adr))
> >   			break;
> >   
> > -		if (chip_good(map, adr, datum)) {
> > -			xip_enable(map, chip, adr);
> > -			goto op_done;
> > -		}
> > +		if (chip_ready(map, adr))
> > +			break;
> >   
> >   		/* Latency issues. Drop the lock, wait a while and retry */
> >   		UDELAY(map, chip, adr, 1);
> >   	}
> >   
> > +	if (chip_good(map, adr, datum)) {
> > +		xip_enable(map, chip, adr);
> > +		goto op_done;
> > +	}
> > +
> >   	/*
> >   	 * Recovery from write-buffer programming failures requires
> >   	 * the write-to-buffer-reset sequence.  Since the last part
> >   
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/