RE: [PATCH v2 01/10] mtd: cfi_cmdset_0002: Change do_write_oneword() to use chip_good()

[IKEGAMI Tokunori <ikegami@xxxxxxxxxxxxxxxxxxxx>]

> -----Original Message-----
> From: stable-owner@xxxxxxxxxxxxxxx
> [mailto:stable-owner@xxxxxxxxxxxxxxx] On Behalf Of Chris Packham
> Sent: Thursday, October 25, 2018 6:24 AM
> To: IKEGAMI Tokunori; boris.brezillon@xxxxxxxxxxxxxxxxxx
> Cc: Hauke Mehrtens; Koen Vandeputte; Fabio Bettoni; Joakim Tjernlund;
> linux-mtd@xxxxxxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 01/10] mtd: cfi_cmdset_0002: Change
> do_write_oneword() to use chip_good()
> 
> On 20/10/18 5:55 AM, Tokunori Ikegami wrote:
> > 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>
> >
> > So change to use chip_good() instead of chip_ready().
> >
> > 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().
> 
> I think much of this is commentary for readers of the mailing list not
> something that should be in the final commit (i.e. it should be after
> the '---'). The commit message should really reflect what the problem
> was and how this change fixes it.

[Ikegami] Yes fixed as so.

> 
> I think you've got a better handle on what chip_good() does than I do so
> I won't attempt to provide a commit message here.
> 
> > Signed-off-by: Tokunori Ikegami <ikegami@xxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Hauke Mehrtens <hauke@xxxxxxxxxx>
> > Signed-off-by: Koen Vandeputte <koen.vandeputte@xxxxxxxxxxxx>
> > Signed-off-by: Fabio Bettoni <fbettoni@xxxxxxxxx>
> > Co-Developed-by: Hauke Mehrtens <hauke@xxxxxxxxxx>
> > Co-Developed-by: Koen Vandeputte <koen.vandeputte@xxxxxxxxxxxx>
> > Co-Developed-by: Fabio Bettoni <fbettoni@xxxxxxxxx>
> > Reported-by: Fabio Bettoni <fbettoni@xxxxxxxxx>
> > Cc: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> > Cc: Joakim Tjernlund <Joakim.Tjernlund@xxxxxxxxxxxx>
> > Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > Cc: linux-mtd@xxxxxxxxxxxxxxxxxxx
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> > Changes since v1:
> > - Just update the commit message.
> >
> >   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 72428b6bfc47..251c9e1675bd 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -1627,31 +1627,37 @@ 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:
> >   	if (mode == FL_OTP_WRITE)
> >   		otp_exit(map, chip, adr, map_bankwidth(map));
> >


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