RE: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c do_write_buffer

From: "Tokunori Ikegami" <ikegami.t@gmail.com>
To: "'Boris Brezillon'" <boris.brezillon@collabora.com>
Cc: "'Tokunori Ikegami'" <ikegami.t@gmail.com>,
	<keescook@chromium.org>, <bbrezillon@kernel.org>,
	<richard@nod.at>, <linux-kernel@vger.kernel.org>,
	<marek.vasut@gmail.com>, <linux-mtd@lists.infradead.org>,
	<computersforpeace@gmail.com>, <dwmw2@infradead.org>,
	"'liujian \(CE\)'" <liujian56@huawei.com>, <vigneshr@ti.com>
Subject: RE: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c do_write_buffer
Date: Sat, 2 Mar 2019 01:54:16 +0900	[thread overview]
Message-ID: <000301d4d04f$76c2aad0$64480070$@gmail.com> (raw)
In-Reply-To: <20190301170715.68d89e84@collabora.com>

Hi Boris-san,

> -----Original Message-----
> From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf
> Of Boris Brezillon
> Sent: Saturday, March 2, 2019 1:07 AM
> To: Tokunori Ikegami
> Cc: 'Tokunori Ikegami'; keescook@chromium.org; bbrezillon@kernel.org;
> ikegami@allied-telesis.co.jp; richard@nod.at;
> linux-kernel@vger.kernel.org; marek.vasut@gmail.com;
> linux-mtd@lists.infradead.org; computersforpeace@gmail.com;
> dwmw2@infradead.org; 'liujian (CE)'; vigneshr@ti.com
> Subject: Re: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c
> do_write_buffer
> 
> Hi Ikegami,
> 
> On Fri, 1 Mar 2019 23:51:16 +0900
> "Tokunori Ikegami" <ikegami_to@yahoo.co.jp> wrote:
> 
> > > Except this version no longer does what Vignesh suggested. See how you
> > > no longer test if chip_good() is true if time_after() returns true.
> So,
> > > imagine the thread entering this function is preempted just after the
> > > first chip_good() test, and resumed a few ms later. time_after() will
> > > return true, but chip_good() might also return true, and you ignore
> it.
> >
> > I think that the following 3 versions will be worked for time_after()
> as a same result and follow the Vignesh-san suggestion.
> 
> Let me show you how they are different:
> 
> >
> > 1. Original Vignesh-san suggestion
> >
> > 	if (chip_good(map, adr, datum)) {
> > 		xip_enable(map, chip, adr);
> > 		goto op_done;
> > 	}
> 
> --> thread preempted here
> ==> chip_good() test becomes valid here
> --> thread resumed here, but timeout has expired
> 
> >
> > 	if (time_after(jiffies, timeo)) {
> 
> you enter this branch
> 
> > 		/* Test chip_good() if TRUE incorrectly again so write
> failure by time_after() can be avoided. */
> > 		if (chip_good(map, adr, datum)) {
> 
> chip_good() returns true
> 
> > 			xip_enable(map, chip, adr);
> > 			goto op_done;
> > 		}
> > 		break;
> > 	}
> >
> > 2. Liujian-san v3 patch
> >
> > 	/* Test chip_good() if FALSE correctly so write failure by
> time_after() can be avoided. */
> 
> --> thread preempted here
> ==> chip_good() test becomes valid here
> --> thread resumed here, but timeout has expired
> 
> > 	if (time_after(jiffies, timeo) && !chip_good(map, adr))
> 
> You do not enter this branch because the chip_good() test is done once
> more in case of timeout.
> 
> > 		break;
> >
> > 	if (chip_good(map, adr, datum)) {
> > 		xip_enable(map, chip, adr);
> > 		goto op_done;
> > 	}
> >
> > 3. My idea
> >
> > 	/* Save current jiffies value before chip_good() to avoid write
> failure by time_after() without testing chip_good() again. */
> > 	unsigned long now = jiffies;
> >
> > 	if (chip_good(map, adr, datum)) {
> > 		xip_enable(map, chip, adr);
> > 		goto op_done;
> > 	}
> >
> 
> --> thread preempted here
> ==> chip_good() test becomes valid here
> --> thread resumed here, but timeout has expired
> 
> > 	if (time_after(now, timeo))
> 
> You do enter this branch, and erroneously report a failure.

I do not think that it is not entered here since the value timeo is compare
with the saved value now before the chip_bood() by time_after().

> 
> > 		break;
> >
> 
> See now why your version is not correct?
> 
> Regards,
> 
> Boris
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/