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

["Tokunori Ikegami" <ikegami_to@xxxxxxxxxxx>]

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.

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/