Linux-mtd Archive on lore.kernel.org
 help / Atom feed
* [PATCH] mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted.
@ 2019-02-07 23:58 Przemyslaw Sobon
  2019-02-08 15:01 ` Tokunori Ikegami
  2019-02-14  0:39 ` Chris Packham
  0 siblings, 2 replies; 11+ messages in thread
From: Przemyslaw Sobon @ 2019-02-07 23:58 UTC (permalink / raw)
  To: bbrezillon, Joakim.Tjernlund, linux-mtd, chris.packham, fbettoni,
	ikegami, liujian56
  Cc: psobon

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


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted.
  2019-02-07 23:58 [PATCH] mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted Przemyslaw Sobon
@ 2019-02-08 15:01 ` Tokunori Ikegami
  2019-02-14  0:39 ` Chris Packham
  1 sibling, 0 replies; 11+ messages in thread
From: Tokunori Ikegami @ 2019-02-08 15:01 UTC (permalink / raw)
  To: Przemyslaw Sobon, bbrezillon, Joakim.Tjernlund, linux-mtd,
	chris.packham, fbettoni, liujian56
  Cc: ikegami_to

Hi Przemek-san,

I think that for the error case it should be done to retry at first.
It can be implemented separately but it is possible to be not enough.

Since the flash write error causes the user data corruption I think.
File systems and applications do not execute any recovery usually.
In the past I saw a similar write error actually and fixed as below.

  dfeae1073583d ("mtd: cfi_cmdset_0002: Change write buffer to check correct
value")

I am also seeing a similar flash write error for the word write case.
In the case the retry with the reset recovery does not work fully.
After the repeated retry with the reset the flash is not able to work.
There is a possibility for the buffer write also but sorry not sure.
Since there is a difference to execute the recovery command.

As Jocke-san mentioned I also think the chip_ready() does not work.
It is followed correctly basically the flash chip specification.
But actually it does not check the chip state correctly I think.
So for the flash write error cases I saw the chip_good() is necessary.

Regards,
Ikegami

> -----Original Message-----
> From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf
> Of Przemyslaw Sobon
> Sent: Friday, February 8, 2019 8:58 AM
> To: bbrezillon@kernel.org; Joakim.Tjernlund@infinera.com;
> linux-mtd@lists.infradead.org; chris.packham@alliedtelesis.co.nz;
> fbettoni@gmail.com; ikegami@allied-telesis.co.jp; liujian56@huawei.com
> Cc: psobon@amazon.com
> Subject: [PATCH] mtd: cfi: Fixed endless loop problem in CFI when value
> was written but corrupted.
> 
> 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>
> ---
>  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
> --
> 2.16.5
> 
> 
> ______________________________________________________
> 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/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted.
  2019-02-07 23:58 [PATCH] mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted Przemyslaw Sobon
  2019-02-08 15:01 ` Tokunori Ikegami
@ 2019-02-14  0:39 ` Chris Packham
  2019-02-19  8:00   ` Boris Brezillon
  1 sibling, 1 reply; 11+ messages in thread
From: Chris Packham @ 2019-02-14  0:39 UTC (permalink / raw)
  To: Przemyslaw Sobon, bbrezillon, Joakim.Tjernlund, linux-mtd,
	fbettoni, ikegami, liujian56, Mark Tomlinson

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>

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted.
  2019-02-14  0:39 ` Chris Packham
@ 2019-02-19  8:00   ` Boris Brezillon
  2019-02-19 20:02     ` Mark Tomlinson
  0 siblings, 1 reply; 11+ messages in thread
From: Boris Brezillon @ 2019-02-19  8:00 UTC (permalink / raw)
  To: Chris Packham
  Cc: Joakim.Tjernlund, ikegami, Mark Tomlinson, Przemyslaw Sobon,
	linux-mtd, liujian56, fbettoni

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/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted.
  2019-02-19  8:00   ` Boris Brezillon
@ 2019-02-19 20:02     ` Mark Tomlinson
  2019-02-20  8:03       ` Boris Brezillon
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Tomlinson @ 2019-02-19 20:02 UTC (permalink / raw)
  To: Boris Brezillon, Chris Packham
  Cc: Joakim.Tjernlund, ikegami, Przemyslaw Sobon, linux-mtd,
	liujian56, fbettoni


On 19/02/19 9:00 PM, Boris Brezillon wrote:
> 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 :-/.
I have done some further testing and this patch doesn't work 100%. It
appears at least some flash chips do not start toggling immediately, and
therefore chip_ready() can return true early. A timeout is reported,
even though that isn't what happened.

chip_good() makes an additional check over chip_ready() and is the call
I believe we should be using. I will submit a new patch which should fix
the infinite loop as well as not mis-reporting errors.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted.
  2019-02-19 20:02     ` Mark Tomlinson
@ 2019-02-20  8:03       ` Boris Brezillon
  2019-02-20 20:50         ` Mark Tomlinson
  0 siblings, 1 reply; 11+ messages in thread
From: Boris Brezillon @ 2019-02-20  8:03 UTC (permalink / raw)
  To: Mark Tomlinson
  Cc: Joakim.Tjernlund, ikegami, Przemyslaw Sobon, Chris Packham,
	linux-mtd, liujian56, fbettoni

On Tue, 19 Feb 2019 20:02:37 +0000
Mark Tomlinson <Mark.Tomlinson@alliedtelesis.co.nz> wrote:

> On 19/02/19 9:00 PM, Boris Brezillon wrote:
> > 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 :-/.  
> I have done some further testing and this patch doesn't work 100%. It
> appears at least some flash chips do not start toggling immediately, and
> therefore chip_ready() can return true early. A timeout is reported,
> even though that isn't what happened.
> 
> chip_good() makes an additional check over chip_ready() and is the call
> I believe we should be using. I will submit a new patch which should fix
> the infinite loop as well as not mis-reporting errors.

No, please, don't do that. We already have 3 versions of the same fix
floating around (one from Ikegami, one from Liu Jian and another one
from Przemyslaw). Can you please sync and submit a single patch that
all of you agree on?


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted.
  2019-02-20  8:03       ` Boris Brezillon
@ 2019-02-20 20:50         ` Mark Tomlinson
  0 siblings, 0 replies; 11+ messages in thread
From: Mark Tomlinson @ 2019-02-20 20:50 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Joakim.Tjernlund, ikegami, Przemyslaw Sobon, Chris Packham,
	linux-mtd, liujian56, fbettoni


On 20/02/19 9:03 PM, Boris Brezillon wrote:
> On Tue, 19 Feb 2019 20:02:37 +0000
> Mark Tomlinson <Mark.Tomlinson@alliedtelesis.co.nz> wrote:
>
>> On 19/02/19 9:00 PM, Boris Brezillon wrote:
>>> 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 :-/.
>> I have done some further testing and this patch doesn't work 100%. It
>> appears at least some flash chips do not start toggling immediately, and
>> therefore chip_ready() can return true early. A timeout is reported,
>> even though that isn't what happened.
>>
>> chip_good() makes an additional check over chip_ready() and is the call
>> I believe we should be using. I will submit a new patch which should fix
>> the infinite loop as well as not mis-reporting errors.
> No, please, don't do that. We already have 3 versions of the same fix
> floating around (one from Ikegami, one from Liu Jian and another one
> from Przemyslaw). Can you please sync and submit a single patch that
> all of you agree on?
>
Ikegami-san has pointed out Liu Jian's patch to me. That patch works fine
for me, so I won't be creating another one afterall. Hope that reduces the
number of possible patches.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted.
  2019-01-16  8:33 ` Joakim.Tjernlund
  2019-01-16  8:50   ` Joakim.Tjernlund
@ 2019-01-16  8:54   ` psobon
  1 sibling, 0 replies; 11+ messages in thread
From: psobon @ 2019-01-16  8:54 UTC (permalink / raw)
  To: linux-mtd

-----Original Message-----
From: Joakim Tjernlund <Joakim.Tjernlund@infinera.com> 
Sent: Wednesday, January 16, 2019 12:33 AM
To: linux-mtd at lists.infradead.org; computersforpeace at gmail.com; ikegami at allied-telesis.co.jp; Sobon, Przemyslaw <psobon@amazon.com>; dwmw2 at infradead.org; richard at nod.at; marek.vasut at gmail.com
Subject: Re: [PATCH] mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted.

> On Wed, 2019-01-16 at 00:32 +0000, Przemyslaw Sobon wrote:
> > 
> > 
> > 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>
> 
> 
> hmm, current code was introduced by Tokunori Ikegami <ikegami@allied-telesis.co.jp> to address another problem he had.
> See 
>    mtd: cfi_cmdset_0002: Change write buffer to check correct value and
>    mtd: cfi_cmdset_0002: Change erase functions to check chip good only
> 
> I wonder if you need to wrap an extra loop with retries around chip_good
> to adress the problem Tokunori had.
If we add "time_after" loop and write is complete but wrong value was written we would have
to wait specific amount of time anyway. Example: we try to write value 4 at address 0x100, the write itself
is done (chip is in ready state) after 10 us but value written was 3 (wrong value). In my proposal the loop
will end after 10 us and we would check if value written is correct and if not we would return error. The
execution time of the loop would be 10 us then. If we surround chip_good with a loop and consider above
situation we will wait whatever the loop is set to e.g. 1ms even though the write was done after 10 us.
This is because we always read value 3 and we retry until timeout elapses.
> 
> Tokunori, what do you think ?
> 
> Jocke
> > ---
> >  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
> > --
> > 2.16.5
> > 
> > 
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.
> > infradead.org%2Fmailman%2Flistinfo%2Flinux-mtd%2F&amp;data=02%7C01%7Cj
> > oakim.tjernlund%40infinera.com%7C35b64c743938427ffa7208d67b4a2a60%7C28
> > 5643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C636831955813341722&amp;sdata=
> > TgI7aw8Qv57MY%2B62KWS87kyfte2A8qQY5OFjQc9Vhwc%3D&amp;reserved=0
> 
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted.
  2019-01-16  8:33 ` Joakim.Tjernlund
@ 2019-01-16  8:50   ` Joakim.Tjernlund
  2019-01-16  8:54   ` psobon
  1 sibling, 0 replies; 11+ messages in thread
From: Joakim.Tjernlund @ 2019-01-16  8:50 UTC (permalink / raw)
  To: linux-mtd

On Wed, 2019-01-16 at 08:33 +0000, Joakim Tjernlund wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Wed, 2019-01-16 at 00:32 +0000, Przemyslaw Sobon wrote:
> > 
> > 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>
> 
> hmm, current code was introduced by Tokunori Ikegami <ikegami@allied-telesis.co.jp> to address another problem he had.

Seems like Tokunori Ikegami's email address is invalid now ...

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted.
  2019-01-16  0:32 [PATCH] " Przemyslaw Sobon
@ 2019-01-16  8:33 ` Joakim.Tjernlund
  2019-01-16  8:50   ` Joakim.Tjernlund
  2019-01-16  8:54   ` psobon
  0 siblings, 2 replies; 11+ messages in thread
From: Joakim.Tjernlund @ 2019-01-16  8:33 UTC (permalink / raw)
  To: linux-mtd

On Wed, 2019-01-16 at 00:32 +0000, Przemyslaw Sobon wrote:
> 
> 
> 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>


hmm, current code was introduced by Tokunori Ikegami <ikegami@allied-telesis.co.jp> to address another problem he had.
See 
   mtd: cfi_cmdset_0002: Change write buffer to check correct value
and
   mtd: cfi_cmdset_0002: Change erase functions to check chip good only

I wonder if you need to wrap an extra loop with retries around chip_good to adress the problem Tokunori had.

Tokunori, what do you think ?

Jocke
> ---
>  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
> --
> 2.16.5
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-mtd%2F&amp;data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C35b64c743938427ffa7208d67b4a2a60%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C636831955813341722&amp;sdata=TgI7aw8Qv57MY%2B62KWS87kyfte2A8qQY5OFjQc9Vhwc%3D&amp;reserved=0

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted.
@ 2019-01-16  0:32 " Przemyslaw Sobon
  2019-01-16  8:33 ` Joakim.Tjernlund
  0 siblings, 1 reply; 11+ messages in thread
From: Przemyslaw Sobon @ 2019-01-16  0:32 UTC (permalink / raw)
  To: David Woodhouse, Brian Norris, Marek Vasut, Richard Weinberger,
	linux-mtd
  Cc: psobon

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-07 23:58 [PATCH] mtd: cfi: Fixed endless loop problem in CFI when value was written but corrupted Przemyslaw Sobon
2019-02-08 15:01 ` Tokunori Ikegami
2019-02-14  0:39 ` Chris Packham
2019-02-19  8:00   ` Boris Brezillon
2019-02-19 20:02     ` Mark Tomlinson
2019-02-20  8:03       ` Boris Brezillon
2019-02-20 20:50         ` Mark Tomlinson
  -- strict thread matches above, loose matches on Subject: below --
2019-01-16  0:32 [PATCH] " Przemyslaw Sobon
2019-01-16  8:33 ` Joakim.Tjernlund
2019-01-16  8:50   ` Joakim.Tjernlund
2019-01-16  8:54   ` psobon

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org linux-mtd@archiver.kernel.org
	public-inbox-index linux-mtd


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/ public-inbox