Messages in this thread | | | Date | Fri, 1 Mar 2019 17:07:15 +0100 | From | Boris Brezillon <> | 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.
> break; >
See now why your version is not correct?
Regards,
Boris
| |