[PATCH for 5.2.y] mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()

[PATCH for 5.2.y] mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()

[Tokunori Ikegami]

As reported by the OpenWRT team, write requests sometimes fail on some
platforms.
Currently to check the state chip_ready() is used correctly as described by
the flash memory S29GL256P11TFI01 datasheet.
Also chip_good() is used to check if the write is succeeded and it was
implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve error
checking").
But actually the write failure is caused on some platforms and also it can
be fixed by using chip_good() to check the state and retry instead.
Also it seems that it is caused after repeated about 1,000 times to retry
the write one word with the reset command.
By using chip_good() to check the state to be done it can be reduced the
retry with reset.
It is depended on the actual flash chip behavior so the root cause is
unknown.

Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: linux-mtd@lists.infradead.org
Cc: stable@vger.kernel.org
Reported-by: Fabio Bettoni <fbettoni@gmail.com>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
[vigneshr@ti.com: Fix a checkpatch warning]
Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
---
 drivers/mtd/chips/cfi_cmdset_0002.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index c8fa5906bdf9..ed3e640eb03a 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1628,29 +1628,35 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 			continue;
 		}
 
-		if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
+		/*
+		 * We check "time_after" and "!chip_good" before checking
+		 * "chip_good" to avoid the failure due to scheduling.
+		 */
+		if (time_after(jiffies, timeo) && !chip_good(map, adr, datum)) {
 			xip_enable(map, chip, adr);
 			printk(KERN_WARNING "MTD %s(): software timeout\n", __func__);
 			xip_disable(map, chip, adr);
+			ret = -EIO;
 			break;
 		}
 
-		if (chip_ready(map, adr))
+		if (chip_good(map, adr, datum))
 			break;
 
 		/* Latency issues. Drop the lock, wait a while and retry */
 		UDELAY(map, chip, adr, 1);
 	}
+
 	/* Did we succeed? */
-	if (!chip_good(map, adr, datum)) {
+	if (ret) {
 		/* reset on all failures. */
 		map_write(map, CMD(0xF0), chip->start);
 		/* FIXME - should have reset delay before continuing */
 
-		if (++retry_cnt <= MAX_RETRIES)
+		if (++retry_cnt <= MAX_RETRIES) {
+			ret = 0;
 			goto retry;
-
-		ret = -EIO;
+		}
 	}
 	xip_enable(map, chip, adr);
  op_done:
-- 
2.11.0


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

Re: [PATCH for 5.2.y] mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()

[Greg KH]

On Mon, Sep 23, 2019 at 11:32:51AM +0900, Tokunori Ikegami wrote:
> As reported by the OpenWRT team, write requests sometimes fail on some
> platforms.
> Currently to check the state chip_ready() is used correctly as described by
> the flash memory S29GL256P11TFI01 datasheet.
> Also chip_good() is used to check if the write is succeeded and it was
> implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve error
> checking").
> But actually the write failure is caused on some platforms and also it can
> be fixed by using chip_good() to check the state and retry instead.
> Also it seems that it is caused after repeated about 1,000 times to retry
> the write one word with the reset command.
> By using chip_good() to check the state to be done it can be reduced the
> retry with reset.
> It is depended on the actual flash chip behavior so the root cause is
> unknown.
> 
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> Cc: linux-mtd@lists.infradead.org
> Cc: stable@vger.kernel.org
> Reported-by: Fabio Bettoni <fbettoni@gmail.com>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
> [vigneshr@ti.com: Fix a checkpatch warning]
> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)

What is the git commit id of this patch in Linus's tree?

thanks,

greg k-h

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

[PATCH for 5.2.y] mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()

[Tokunori Ikegami]

As reported by the OpenWRT team, write requests sometimes fail on some
platforms.
Currently to check the state chip_ready() is used correctly as described by
the flash memory S29GL256P11TFI01 datasheet.
Also chip_good() is used to check if the write is succeeded and it was
implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve error
checking").
But actually the write failure is caused on some platforms and also it can
be fixed by using chip_good() to check the state and retry instead.
Also it seems that it is caused after repeated about 1,000 times to retry
the write one word with the reset command.
By using chip_good() to check the state to be done it can be reduced the
retry with reset.
It is depended on the actual flash chip behavior so the root cause is
unknown.

Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: linux-mtd@lists.infradead.org
Cc: stable@vger.kernel.org
Reported-by: Fabio Bettoni <fbettoni@gmail.com>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
[vigneshr@ti.com: Fix a checkpatch warning]
Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
---
 drivers/mtd/chips/cfi_cmdset_0002.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)
 mode change 100644 => 100755 drivers/mtd/chips/cfi_cmdset_0002.c

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
old mode 100644
new mode 100755
index c8fa5906bdf9..ed3e640eb03a
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1628,29 +1628,35 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 			continue;
 		}
 
-		if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
+		/*
+		 * We check "time_after" and "!chip_good" before checking
+		 * "chip_good" to avoid the failure due to scheduling.
+		 */
+		if (time_after(jiffies, timeo) && !chip_good(map, adr, datum)) {
 			xip_enable(map, chip, adr);
 			printk(KERN_WARNING "MTD %s(): software timeout\n", __func__);
 			xip_disable(map, chip, adr);
+			ret = -EIO;
 			break;
 		}
 
-		if (chip_ready(map, adr))
+		if (chip_good(map, adr, datum))
 			break;
 
 		/* Latency issues. Drop the lock, wait a while and retry */
 		UDELAY(map, chip, adr, 1);
 	}
+
 	/* Did we succeed? */
-	if (!chip_good(map, adr, datum)) {
+	if (ret) {
 		/* reset on all failures. */
 		map_write(map, CMD(0xF0), chip->start);
 		/* FIXME - should have reset delay before continuing */
 
-		if (++retry_cnt <= MAX_RETRIES)
+		if (++retry_cnt <= MAX_RETRIES) {
+			ret = 0;
 			goto retry;
-
-		ret = -EIO;
+		}
 	}
 	xip_enable(map, chip, adr);
  op_done:
-- 
2.11.0


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

Re: [PATCH for 5.2.y] mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()

[Greg KH]

On Wed, Sep 18, 2019 at 02:50:48AM +0900, Tokunori Ikegami wrote:
> As reported by the OpenWRT team, write requests sometimes fail on some
> platforms.
> Currently to check the state chip_ready() is used correctly as described by
> the flash memory S29GL256P11TFI01 datasheet.
> Also chip_good() is used to check if the write is succeeded and it was
> implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve error
> checking").
> But actually the write failure is caused on some platforms and also it can
> be fixed by using chip_good() to check the state and retry instead.
> Also it seems that it is caused after repeated about 1,000 times to retry
> the write one word with the reset command.
> By using chip_good() to check the state to be done it can be reduced the
> retry with reset.
> It is depended on the actual flash chip behavior so the root cause is
> unknown.
> 
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> Cc: linux-mtd@lists.infradead.org
> Cc: stable@vger.kernel.org
> Reported-by: Fabio Bettoni <fbettoni@gmail.com>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
> [vigneshr@ti.com: Fix a checkpatch warning]
> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>  mode change 100644 => 100755 drivers/mtd/chips/cfi_cmdset_0002.c

You changed the file to be executable???  That's not ok :(

Also, what is the git commit id of this patch in Linus's tree?  I can't
seem to find it there.

thanks,

greg k-h

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

Re: [PATCH for 5.2.y] mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()

[Tokunori Ikegami]

On 2019/09/18 3:11, Greg KH wrote:
> On Wed, Sep 18, 2019 at 02:50:48AM +0900, Tokunori Ikegami wrote:
>> As reported by the OpenWRT team, write requests sometimes fail on some
>> platforms.
>> Currently to check the state chip_ready() is used correctly as described by
>> the flash memory S29GL256P11TFI01 datasheet.
>> Also chip_good() is used to check if the write is succeeded and it was
>> implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve error
>> checking").
>> But actually the write failure is caused on some platforms and also it can
>> be fixed by using chip_good() to check the state and retry instead.
>> Also it seems that it is caused after repeated about 1,000 times to retry
>> the write one word with the reset command.
>> By using chip_good() to check the state to be done it can be reduced the
>> retry with reset.
>> It is depended on the actual flash chip behavior so the root cause is
>> unknown.
>>
>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
>> Cc: linux-mtd@lists.infradead.org
>> Cc: stable@vger.kernel.org
>> Reported-by: Fabio Bettoni <fbettoni@gmail.com>
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
>> [vigneshr@ti.com: Fix a checkpatch warning]
>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>> ---
>>   drivers/mtd/chips/cfi_cmdset_0002.c | 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>   mode change 100644 => 100755 drivers/mtd/chips/cfi_cmdset_0002.c
> You changed the file to be executable???  That's not ok :(

Very sorry for this.
I missed it to fix to not be executable since it was changed to be 
executable on my local environment.
Anyway I will do fix it.

>
> Also, what is the git commit id of this patch in Linus's tree?  I can't
> seem to find it there.

Actually it has not been pulled in Linus's tree.
But it has been merged into 
git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next for 
v5.4-rc1 as the git commit id 37c673ade35c.
So I thought as that it is okay to send the patches for the stable trees.
But should I wait to be pulled the patch in Linus's tree at first?

>
> thanks,
>
> greg k-h

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

Re: [PATCH for 5.2.y] mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()

[Greg KH]

On Wed, Sep 18, 2019 at 07:32:39AM +0900, Tokunori Ikegami wrote:
> 
> On 2019/09/18 3:11, Greg KH wrote:
> > On Wed, Sep 18, 2019 at 02:50:48AM +0900, Tokunori Ikegami wrote:
> > > As reported by the OpenWRT team, write requests sometimes fail on some
> > > platforms.
> > > Currently to check the state chip_ready() is used correctly as described by
> > > the flash memory S29GL256P11TFI01 datasheet.
> > > Also chip_good() is used to check if the write is succeeded and it was
> > > implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve error
> > > checking").
> > > But actually the write failure is caused on some platforms and also it can
> > > be fixed by using chip_good() to check the state and retry instead.
> > > Also it seems that it is caused after repeated about 1,000 times to retry
> > > the write one word with the reset command.
> > > By using chip_good() to check the state to be done it can be reduced the
> > > retry with reset.
> > > It is depended on the actual flash chip behavior so the root cause is
> > > unknown.
> > > 
> > > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > > Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> > > Cc: linux-mtd@lists.infradead.org
> > > Cc: stable@vger.kernel.org
> > > Reported-by: Fabio Bettoni <fbettoni@gmail.com>
> > > Signed-off-by: Felix Fietkau <nbd@nbd.name>
> > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> > > Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
> > > [vigneshr@ti.com: Fix a checkpatch warning]
> > > Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> > > ---
> > >   drivers/mtd/chips/cfi_cmdset_0002.c | 18 ++++++++++++------
> > >   1 file changed, 12 insertions(+), 6 deletions(-)
> > >   mode change 100644 => 100755 drivers/mtd/chips/cfi_cmdset_0002.c
> > You changed the file to be executable???  That's not ok :(
> 
> Very sorry for this.
> I missed it to fix to not be executable since it was changed to be
> executable on my local environment.
> Anyway I will do fix it.

Please do, we can not take these patches as-is at all.

> > Also, what is the git commit id of this patch in Linus's tree?  I can't
> > seem to find it there.
> 
> Actually it has not been pulled in Linus's tree.
> But it has been merged into
> git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next for
> v5.4-rc1 as the git commit id 37c673ade35c.
> So I thought as that it is okay to send the patches for the stable trees.
> But should I wait to be pulled the patch in Linus's tree at first?

Yes, you have to wait, please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

thanks,

greg k-h

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

Re: [PATCH for 5.2.y] mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()

[Tokunori Ikegami]

Hi,

On 2019/09/18 14:52, Greg KH wrote:
> On Wed, Sep 18, 2019 at 07:32:39AM +0900, Tokunori Ikegami wrote:
>> On 2019/09/18 3:11, Greg KH wrote:
>>> On Wed, Sep 18, 2019 at 02:50:48AM +0900, Tokunori Ikegami wrote:
>>>> As reported by the OpenWRT team, write requests sometimes fail on some
>>>> platforms.
>>>> Currently to check the state chip_ready() is used correctly as described by
>>>> the flash memory S29GL256P11TFI01 datasheet.
>>>> Also chip_good() is used to check if the write is succeeded and it was
>>>> implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve error
>>>> checking").
>>>> But actually the write failure is caused on some platforms and also it can
>>>> be fixed by using chip_good() to check the state and retry instead.
>>>> Also it seems that it is caused after repeated about 1,000 times to retry
>>>> the write one word with the reset command.
>>>> By using chip_good() to check the state to be done it can be reduced the
>>>> retry with reset.
>>>> It is depended on the actual flash chip behavior so the root cause is
>>>> unknown.
>>>>
>>>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>> Cc: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
>>>> Cc: linux-mtd@lists.infradead.org
>>>> Cc: stable@vger.kernel.org
>>>> Reported-by: Fabio Bettoni <fbettoni@gmail.com>
>>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>>>> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
>>>> [vigneshr@ti.com: Fix a checkpatch warning]
>>>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>>>> ---
>>>>    drivers/mtd/chips/cfi_cmdset_0002.c | 18 ++++++++++++------
>>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>>    mode change 100644 => 100755 drivers/mtd/chips/cfi_cmdset_0002.c
>>> You changed the file to be executable???  That's not ok :(
>> Very sorry for this.
>> I missed it to fix to not be executable since it was changed to be
>> executable on my local environment.
>> Anyway I will do fix it.
> Please do, we can not take these patches as-is at all.

I see.

>
>>> Also, what is the git commit id of this patch in Linus's tree?  I can't
>>> seem to find it there.
>> Actually it has not been pulled in Linus's tree.
>> But it has been merged into
>> git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next for
>> v5.4-rc1 as the git commit id 37c673ade35c.
>> So I thought as that it is okay to send the patches for the stable trees.
>> But should I wait to be pulled the patch in Linus's tree at first?
> Yes, you have to wait, please read:
>      https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.

Thank you for your advice.
I have just read the rule as described this so I will wait it to be 
existed in Linus's tree.

Regards,
Ikegami

>
> thanks,
>
> greg k-h

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