Hi Boris-san, > -----Original Message----- > From: linux-mtd [mailto:[email protected]] On Behalf > Of Boris Brezillon > Sent: Saturday, March 2, 2019 1:07 AM > To: Tokunori Ikegami > Cc: 'Tokunori Ikegami'; [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; 'liujian (CE)'; [email protected] > 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" <[email protected]> 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/
