| Message ID | 20190917175048.12895-1-ikegami.t@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Vignesh R |
| Headers | show |
| Series | [for,5.2.y] mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword() | expand |
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
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
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
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
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:
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