RE: [RESEND v4 01/11] mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()

["Tokunori Ikegami" <ikegami_to@xxxxxxxxxxx>]

Hi Vignesh-san,

Thanks for your advice.
Noted it.

Regards,
Ikegami

> -----Original Message-----
> From: Vignesh R [mailto:vigneshr@xxxxxx]
> Sent: Tuesday, February 5, 2019 8:58 PM
> To: Tokunori Ikegami; 'Boris Brezillon'
> Cc: 'Joakim Tjernlund'; 'Chris Packham'; linux-mtd@xxxxxxxxxxxxxxxxxxx;
> 'Felix Fietkau'
> Subject: Re: [RESEND v4 01/11] mtd: cfi_cmdset_0002: Use chip_good() to
> retry in do_write_oneword()
> 
> Hi,
> 
> On 04/02/19 11:54 PM, Tokunori Ikegami wrote:
> > Hi,
> >
> > Very sorry for many times resending the patch mails.
> > Since I could not send the mails correctly with the incorrect mail address.
> > Still I can not send correctly the mail to stable@xxxxxxxxxxxxxxx so I
> will
> > investigate this later.
> >
> 
> You don't have to explicitly send mail/Cc stable@xxxxxxxxxxxxxxx. Its just
> a tag.
> 
> From Documentation/process/stable-kernel-rules.rst[1]:
> Option 1
> ********
> 
> To have the patch automatically included in the stable tree, add the tag
> 
> . code-block:: none
> 
>      Cc: stable@xxxxxxxxxxxxxxx
> 
> in the sign-off area. Once the patch is merged it will be applied to
> the stable tree without anything else needing to be done by the author
> or subsystem maintainer.
> 
> Along with Fixes tag[2]:
> A Fixes: tag indicates that the patch fixes an issue in a previous commit.
> It
> is used to make it easy to determine where a bug originated, which can help
> review a bug fix. This tag also assists the stable kernel team in determining
> which stable kernel versions should receive your fix.
> 
> 
> [1]https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.
> html#for-all-other-submissions-choose-one-of-the-following-procedures
> [2]https://www.kernel.org/doc/html/latest/process/submitting-patches.h
> tml#describe-your-changes
> 
> Regards
> Vignesh
> 
> > Regards,
> > Ikegami
> >
> >> -----Original Message-----
> >> From: Tokunori Ikegami [mailto:ikegami_to@xxxxxxxxxxx]
> >> Sent: Tuesday, February 5, 2019 3:20 AM
> >> To: Boris Brezillon
> >> Cc: Tokunori Ikegami; Felix Fietkau; Chris Packham; Joakim Tjernlund;
> >> linux-mtd@xxxxxxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx
> >> Subject: [RESEND v4 01/11] mtd: cfi_cmdset_0002: Use chip_good() to retry
> >> in do_write_oneword()
> >>
> >> 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.
> >>
> >> Signed-off-by: Tokunori Ikegami <ikegami_to@xxxxxxxxxxx>
> >> Signed-off-by: Felix Fietkau <nbd@xxxxxxxx>
> >> Co-Developed-by: Hauke Mehrtens <hauke@xxxxxxxxxx>
> >> Co-Developed-by: Koen Vandeputte <koen.vandeputte@xxxxxxxxxxxx>
> >> Reported-by: Fabio Bettoni <fbettoni@xxxxxxxxx>
> >> Cc: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> >> Cc: Joakim Tjernlund <Joakim.Tjernlund@xxxxxxxxxxxx>
> >> Cc: linux-mtd@xxxxxxxxxxxxxxxxxxx
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> ---
> >> Changes since v3:
> >> - Update the commit message for the comments.
> >> - Drop the addition of blanks lines around xip_enable().
> >> - Delete unnecessary setting the ret variable to -EIO.
> >> - Change the email address of Tokunori Ikegami to ikegami_to@xxxxxxxxxxx.
> >>
> >> Changes since v2:
> >> - Just update the commit message for the comment.
> >>
> >> Changes since v1:
> >> - Just update the commit message.
> >>
> >> Background:
> >> This is required for OpenWrt Project to result the flash write issue
> as
> >> below patche.
> >>
> <https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=ddc11c3
> >> 932c7b7b7df7d5fbd48f207e77619eaa7>
> >>
> >> Also the original patch in OpenWRT is below.
> >>
> <https://github.com/openwrt/openwrt/blob/v18.06.0/target/linux/ar71xx/
> >> patches-4.9/403-mtd_fix_cfi_cmdset_0002_status_check.patch>
> >>
> >> The reason to use chip_good() is that just actually fix the issue.
> >> And also in the past I had fixed the erase function also as same way
> by
> >> the
> >> patch below.
> >>   <https://patchwork.ozlabs.org/patch/922656/>
> >>     Note: The reason for the patch for erase is same.
> >>
> >> In my understanding the chip_ready() is just checked the value twice
> from
> >> flash.
> >> So I think that sometimes incorrect value is read twice and it is depended
> >> on the flash device behavior but not sure..
> >>
> >> So change to use chip_good() instead of chip_ready().
> >>
> >>  drivers/mtd/chips/cfi_cmdset_0002.c | 18 ++++++++++--------
> >>  1 file changed, 10 insertions(+), 8 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 72428b6..91a491b
> >> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> >> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> >> @@ -1627,29 +1627,31 @@ static int __xipram do_write_oneword(struct
> >> map_info *map, struct flchip *chip,
> >>  			continue;
> >>  		}
> >>
> >> -		if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
> >> +		if (chip_good(map, adr, datum))
> >> +			break;
> >> +
> >> +		if (time_after(jiffies, timeo)){
> >>  			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))
> >> -			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/
> >
> 
> --
> Regards
> Vignesh


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