Messages in this thread | | | From | "Tokunori Ikegami" <> | Subject | RE: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c do_write_buffer | Date | Sat, 2 Mar 2019 01:54:16 +0900 |
| |
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/
| |