| Message ID | 1551189648-58073-1-git-send-email-liujian56@huawei.com |
|---|---|
| State | Superseded |
| Delegated to: | Vignesh R |
| Headers | show |
| Series | [v3] cfi: fix deadloop in cfi_cmdset_0002.c do_write_buffer | expand |
> -----Original Message----- > From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf > Of Liu Jian > Sent: Tuesday, February 26, 2019 11:01 PM > To: dwmw2@infradead.org; computersforpeace@gmail.com; > bbrezillon@kernel.org; marek.vasut@gmail.com; richard@nod.at; > joakim.tjernlund@infinera.com; ikegami@allied-telesis.co.jp; > keescook@chromium.org; vigneshr@ti.com > Cc: linux-mtd@lists.infradead.org; liujian56@huawei.com; > linux-kernel@vger.kernel.org > Subject: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c do_write_buffer > > In function do_write_buffer(), in the for loop, there is a case > chip_ready() returns 1 while chip_good() returns 0, so it never > break the loop. > To fix this, chip_good() is enough and it should timeout if it stay > bad for a while. > > Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check > correct value") > Signed-off-by: Yi Huaijie <yihuaijie@huawei.com> > Signed-off-by: Liu Jian <liujian56@huawei.com> > Reviewed-by: Tokunori Ikegami <ikegami_to@yahoo.co.jp> > --- > v2->v3: > Follow Vignesh's advice: > add one more check for check_good() even when time_after() returns true. > > drivers/mtd/chips/cfi_cmdset_0002.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c > b/drivers/mtd/chips/cfi_cmdset_0002.c > index 72428b6..3da2376 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -1876,7 +1876,7 @@ static int __xipram do_write_buffer(struct map_info > *map, struct flchip *chip, > continue; > } > > - if (time_after(jiffies, timeo) && !chip_ready(map, adr)) > + if (time_after(jiffies, timeo) && !chip_good(map, adr, > datum)) Just another idea to understand easily. unsigned long now = jiffies; if (chip_good(map, adr, datum)) { xip_enable(map, chip, adr); goto op_done; } if (time_after(now, timeo) { break; } Regards, Ikegami > break; > > if (chip_good(map, adr, datum)) { > -- > 2.7.4 > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
> -----Original Message----- > From: Tokunori Ikegami [mailto:ikegami.t@gmail.com] > Sent: Thursday, February 28, 2019 10:26 PM > To: liujian (CE) <liujian56@huawei.com>; dwmw2@infradead.org; > computersforpeace@gmail.com; bbrezillon@kernel.org; > marek.vasut@gmail.com; richard@nod.at; joakim.tjernlund@infinera.com; > ikegami@allied-telesis.co.jp; keescook@chromium.org; vigneshr@ti.com > Cc: linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: RE: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c do_write_buffer > > > > > -----Original Message----- > > From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On > > Behalf Of Liu Jian > > Sent: Tuesday, February 26, 2019 11:01 PM > > To: dwmw2@infradead.org; computersforpeace@gmail.com; > > bbrezillon@kernel.org; marek.vasut@gmail.com; richard@nod.at; > > joakim.tjernlund@infinera.com; ikegami@allied-telesis.co.jp; > > keescook@chromium.org; vigneshr@ti.com > > Cc: linux-mtd@lists.infradead.org; liujian56@huawei.com; > > linux-kernel@vger.kernel.org > > Subject: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c > > do_write_buffer > > > > In function do_write_buffer(), in the for loop, there is a case > > chip_ready() returns 1 while chip_good() returns 0, so it never break > > the loop. > > To fix this, chip_good() is enough and it should timeout if it stay > > bad for a while. > > > > Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to > > check correct value") > > Signed-off-by: Yi Huaijie <yihuaijie@huawei.com> > > Signed-off-by: Liu Jian <liujian56@huawei.com> > > Reviewed-by: Tokunori Ikegami <ikegami_to@yahoo.co.jp> > > --- > > v2->v3: > > Follow Vignesh's advice: > > add one more check for check_good() even when time_after() returns true. > > > > drivers/mtd/chips/cfi_cmdset_0002.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c > > b/drivers/mtd/chips/cfi_cmdset_0002.c > > index 72428b6..3da2376 100644 > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > > @@ -1876,7 +1876,7 @@ static int __xipram do_write_buffer(struct > > map_info *map, struct flchip *chip, > > continue; > > } > > > > - if (time_after(jiffies, timeo) && !chip_ready(map, adr)) > > + if (time_after(jiffies, timeo) && !chip_good(map, adr, > > datum)) > > Just another idea to understand easily. > > unsigned long now = jiffies; > > if (chip_good(map, adr, datum)) { > xip_enable(map, chip, adr); > goto op_done; > } > > if (time_after(now, timeo) { > break; > } > Thank you~. It is more easier to understand！ If there are no other comments, I will send new patch again ): Best Regards, Liujian > Regards, > Ikegami > > > break; > > > > if (chip_good(map, adr, datum)) { > > -- > > 2.7.4 > > > > > > ______________________________________________________ > > Linux MTD discussion mailing list > > http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Thu, 28 Feb 2019 15:12:15 +0000 "liujian (CE)" <liujian56@huawei.com> wrote: > > -----Original Message----- > > From: Tokunori Ikegami [mailto:ikegami.t@gmail.com] > > Sent: Thursday, February 28, 2019 10:26 PM > > To: liujian (CE) <liujian56@huawei.com>; dwmw2@infradead.org; > > computersforpeace@gmail.com; bbrezillon@kernel.org; > > marek.vasut@gmail.com; richard@nod.at; joakim.tjernlund@infinera.com; > > ikegami@allied-telesis.co.jp; keescook@chromium.org; vigneshr@ti.com > > Cc: linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.org > > Subject: RE: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c do_write_buffer > > > > > > > > > -----Original Message----- > > > From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On > > > Behalf Of Liu Jian > > > Sent: Tuesday, February 26, 2019 11:01 PM > > > To: dwmw2@infradead.org; computersforpeace@gmail.com; > > > bbrezillon@kernel.org; marek.vasut@gmail.com; richard@nod.at; > > > joakim.tjernlund@infinera.com; ikegami@allied-telesis.co.jp; > > > keescook@chromium.org; vigneshr@ti.com > > > Cc: linux-mtd@lists.infradead.org; liujian56@huawei.com; > > > linux-kernel@vger.kernel.org > > > Subject: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c > > > do_write_buffer > > > > > > In function do_write_buffer(), in the for loop, there is a case > > > chip_ready() returns 1 while chip_good() returns 0, so it never break > > > the loop. > > > To fix this, chip_good() is enough and it should timeout if it stay > > > bad for a while. > > > > > > Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to > > > check correct value") > > > Signed-off-by: Yi Huaijie <yihuaijie@huawei.com> > > > Signed-off-by: Liu Jian <liujian56@huawei.com> > > > Reviewed-by: Tokunori Ikegami <ikegami_to@yahoo.co.jp> > > > --- > > > v2->v3: > > > Follow Vignesh's advice: > > > add one more check for check_good() even when time_after() returns true. > > > > > > drivers/mtd/chips/cfi_cmdset_0002.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c > > > b/drivers/mtd/chips/cfi_cmdset_0002.c > > > index 72428b6..3da2376 100644 > > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > > > @@ -1876,7 +1876,7 @@ static int __xipram do_write_buffer(struct > > > map_info *map, struct flchip *chip, > > > continue; > > > } > > > > > > - if (time_after(jiffies, timeo) && !chip_ready(map, adr)) > > > + if (time_after(jiffies, timeo) && !chip_good(map, adr, > > > datum)) > > > > Just another idea to understand easily. > > > > unsigned long now = jiffies; > > > > if (chip_good(map, adr, datum)) { > > xip_enable(map, chip, adr); > > goto op_done; > > } > > > > if (time_after(now, timeo) { > > break; > > } > > > > Thank you~. It is more easier to understand！ > If there are no other comments, I will send new patch again ): 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.
> -----Original Message----- > From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf > Of Boris Brezillon > Sent: Friday, March 1, 2019 12:42 AM > To: liujian (CE) > 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; vigneshr@ti.com > Subject: Re: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c > do_write_buffer > > On Thu, 28 Feb 2019 15:12:15 +0000 > "liujian (CE)" <liujian56@huawei.com> wrote: > > > > -----Original Message----- > > > From: Tokunori Ikegami [mailto:ikegami.t@gmail.com] > > > Sent: Thursday, February 28, 2019 10:26 PM > > > To: liujian (CE) <liujian56@huawei.com>; dwmw2@infradead.org; > > > computersforpeace@gmail.com; bbrezillon@kernel.org; > > > marek.vasut@gmail.com; richard@nod.at; > joakim.tjernlund@infinera.com; > > > ikegami@allied-telesis.co.jp; keescook@chromium.org; vigneshr@ti.com > > > Cc: linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.org > > > Subject: RE: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c > do_write_buffer > > > > > > > > > > > > > -----Original Message----- > > > > From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On > > > > Behalf Of Liu Jian > > > > Sent: Tuesday, February 26, 2019 11:01 PM > > > > To: dwmw2@infradead.org; computersforpeace@gmail.com; > > > > bbrezillon@kernel.org; marek.vasut@gmail.com; richard@nod.at; > > > > joakim.tjernlund@infinera.com; ikegami@allied-telesis.co.jp; > > > > keescook@chromium.org; vigneshr@ti.com > > > > Cc: linux-mtd@lists.infradead.org; liujian56@huawei.com; > > > > linux-kernel@vger.kernel.org > > > > Subject: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c > > > > do_write_buffer > > > > > > > > In function do_write_buffer(), in the for loop, there is a case > > > > chip_ready() returns 1 while chip_good() returns 0, so it never break > > > > the loop. > > > > To fix this, chip_good() is enough and it should timeout if it stay > > > > bad for a while. > > > > > > > > Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to > > > > check correct value") > > > > Signed-off-by: Yi Huaijie <yihuaijie@huawei.com> > > > > Signed-off-by: Liu Jian <liujian56@huawei.com> > > > > Reviewed-by: Tokunori Ikegami <ikegami_to@yahoo.co.jp> > > > > --- > > > > v2->v3: > > > > Follow Vignesh's advice: > > > > add one more check for check_good() even when time_after() returns > true. > > > > > > > > drivers/mtd/chips/cfi_cmdset_0002.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c > > > > b/drivers/mtd/chips/cfi_cmdset_0002.c > > > > index 72428b6..3da2376 100644 > > > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > > > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > > > > @@ -1876,7 +1876,7 @@ static int __xipram do_write_buffer(struct > > > > map_info *map, struct flchip *chip, > > > > continue; > > > > } > > > > > > > > - if (time_after(jiffies, timeo) && !chip_ready(map, adr)) > > > > + if (time_after(jiffies, timeo) && !chip_good(map, adr, > > > > datum)) > > > > > > Just another idea to understand easily. > > > > > > unsigned long now = jiffies; > > > > > > if (chip_good(map, adr, datum)) { > > > xip_enable(map, chip, adr); > > > goto op_done; > > > } > > > > > > if (time_after(now, timeo) { > > > break; > > > } > > > > > > > Thank you~. It is more easier to understand！ > > If there are no other comments, I will send new patch again ): > > 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. 1. Original Vignesh-san suggestion if (chip_good(map, adr, datum)) { xip_enable(map, chip, adr); goto op_done; } if (time_after(jiffies, timeo)) { /* Test chip_good() if TRUE incorrectly again so write failure by time_after() can be avoided. */ if (chip_good(map, adr, datum)) { 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. */ if (time_after(jiffies, timeo) && !chip_good(map, adr)) 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; } if (time_after(now, timeo)) break; Note: Some brackets have been fixed from the previous mail. Regards, Ikegami > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
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
[...] >>>>> In function do_write_buffer(), in the for loop, there is a case >>>>> chip_ready() returns 1 while chip_good() returns 0, so it never break >>>>> the loop. >>>>> To fix this, chip_good() is enough and it should timeout if it stay >>>>> bad for a while. >>>>> >>>>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to >>>>> check correct value") >>>>> Signed-off-by: Yi Huaijie <yihuaijie@huawei.com> >>>>> Signed-off-by: Liu Jian <liujian56@huawei.com> >>>>> Reviewed-by: Tokunori Ikegami <ikegami_to@yahoo.co.jp> >>>>> --- >>>>> v2->v3: >>>>> Follow Vignesh's advice: >>>>> add one more check for check_good() even when time_after() returns >> true. >>>>> >>>>> drivers/mtd/chips/cfi_cmdset_0002.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c >>>>> b/drivers/mtd/chips/cfi_cmdset_0002.c >>>>> index 72428b6..3da2376 100644 >>>>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c >>>>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c >>>>> @@ -1876,7 +1876,7 @@ static int __xipram do_write_buffer(struct >>>>> map_info *map, struct flchip *chip, >>>>> continue; >>>>> } >>>>> >>>>> - if (time_after(jiffies, timeo) && !chip_ready(map, adr)) >>>>> + if (time_after(jiffies, timeo) && !chip_good(map, adr, >>>>> datum)) >>>> >>>> Just another idea to understand easily. >>>> >>>> unsigned long now = jiffies; >>>> >>>> if (chip_good(map, adr, datum)) { >>>> xip_enable(map, chip, adr); >>>> goto op_done; >>>> } >>>> >>>> if (time_after(now, timeo) { >>>> break; >>>> } >>>> >>> >>> Thank you~. It is more easier to understand！ >>> If there are no other comments, I will send new patch again ): >> >> 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. > As Boris explained above version 3 does not really follow my suggestion... Please see below > 1. Original Vignesh-san suggestion > > if (chip_good(map, adr, datum)) { > xip_enable(map, chip, adr); > goto op_done; > } > > if (time_after(jiffies, timeo)) { > /* Test chip_good() if TRUE incorrectly again so write failure by time_after() can be avoided. */ > if (chip_good(map, adr, datum)) { > xip_enable(map, chip, adr); > goto op_done; > } > break; > } > Right, here we check chip_good() once _even when time_after() is true_ to avoid _spurious_ timeout > 2. Liujian-san v3 patch > > /* Test chip_good() if FALSE correctly so write failure by time_after() can be avoided. */ > if (time_after(jiffies, timeo) && !chip_good(map, adr)) > break; > > if (chip_good(map, adr, datum)) { > xip_enable(map, chip, adr); > goto op_done; > } > This is a better version of 1 > 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; > } > What if thread gets pre-empted at this point and is re-scheduled exactly after timeo jiffies have elapsed? Below check would be true and exit loop > if (time_after(now, timeo)) > break; > So, code does not check for check chip_good() after timeoout has elapsed. But chip_good() might be true at this point. Therefore leading to spurious timeout. So this version is still not right. > Note: Some brackets have been fixed from the previous mail. >
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/
> [...] > >>>>> In function do_write_buffer(), in the for loop, there is a case > >>>>> chip_ready() returns 1 while chip_good() returns 0, so it never break > >>>>> the loop. > >>>>> To fix this, chip_good() is enough and it should timeout if it stay > >>>>> bad for a while. > >>>>> > >>>>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to > >>>>> check correct value") > >>>>> Signed-off-by: Yi Huaijie <yihuaijie@huawei.com> > >>>>> Signed-off-by: Liu Jian <liujian56@huawei.com> > >>>>> Reviewed-by: Tokunori Ikegami <ikegami_to@yahoo.co.jp> > >>>>> --- > >>>>> v2->v3: > >>>>> Follow Vignesh's advice: > >>>>> add one more check for check_good() even when time_after() returns > >> true. > >>>>> > >>>>> drivers/mtd/chips/cfi_cmdset_0002.c | 2 +- > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c > >>>>> b/drivers/mtd/chips/cfi_cmdset_0002.c > >>>>> index 72428b6..3da2376 100644 > >>>>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c > >>>>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > >>>>> @@ -1876,7 +1876,7 @@ static int __xipram do_write_buffer(struct > >>>>> map_info *map, struct flchip *chip, > >>>>> continue; > >>>>> } > >>>>> > >>>>> - if (time_after(jiffies, timeo) && !chip_ready(map, adr)) > >>>>> + if (time_after(jiffies, timeo) && !chip_good(map, adr, > >>>>> datum)) > >>>> > >>>> Just another idea to understand easily. > >>>> > >>>> unsigned long now = jiffies; > >>>> > >>>> if (chip_good(map, adr, datum)) { > >>>> xip_enable(map, chip, adr); > >>>> goto op_done; > >>>> } > >>>> > >>>> if (time_after(now, timeo) { > >>>> break; > >>>> } > >>>> > >>> > >>> Thank you~. It is more easier to understand！ > >>> If there are no other comments, I will send new patch again ): > >> > >> 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. > > > > As Boris explained above version 3 does not really follow my > suggestion... Please see below > > > 1. Original Vignesh-san suggestion > > > > if (chip_good(map, adr, datum)) { > > xip_enable(map, chip, adr); > > goto op_done; > > } > > > > if (time_after(jiffies, timeo)) { > > /* Test chip_good() if TRUE incorrectly again so write > failure by time_after() can be avoided. */ > > if (chip_good(map, adr, datum)) { > > xip_enable(map, chip, adr); > > goto op_done; > > } > > break; > > } > > > > > Right, here we check chip_good() once _even when time_after() is true_ > to avoid _spurious_ timeout > > > 2. Liujian-san v3 patch > > > > /* Test chip_good() if FALSE correctly so write failure by > time_after() can be avoided. */ > > if (time_after(jiffies, timeo) && !chip_good(map, adr)) > > break; > > > > if (chip_good(map, adr, datum)) { > > xip_enable(map, chip, adr); > > goto op_done; > > } > > > > This is a better version of 1 > > > 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; > > } > > > > What if thread gets pre-empted at this point and is re-scheduled exactly > after timeo jiffies have elapsed? Below check would be true and exit loop I think that the jiffies value now is save before chip_good() so below check would be false and not exit loop. Regards, Ikegami > > > if (time_after(now, timeo)) > > break; > > > > So, code does not check for check chip_good() after timeoout has > elapsed. But chip_good() might be true at this point. Therefore leading > to spurious timeout. So this version is still not right. > > > Note: Some brackets have been fixed from the previous mail. > > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Sat, 2 Mar 2019 01:59:41 +0900 "Tokunori Ikegami" <ikegami.t@gmail.com> wrote: > > [...] > > >>>>> In function do_write_buffer(), in the for loop, there is a > > >>>>> case chip_ready() returns 1 while chip_good() returns 0, so > > >>>>> it never break the loop. > > >>>>> To fix this, chip_good() is enough and it should timeout if > > >>>>> it stay bad for a while. > > >>>>> > > >>>>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write > > >>>>> buffer to check correct value") > > >>>>> Signed-off-by: Yi Huaijie <yihuaijie@huawei.com> > > >>>>> Signed-off-by: Liu Jian <liujian56@huawei.com> > > >>>>> Reviewed-by: Tokunori Ikegami <ikegami_to@yahoo.co.jp> > > >>>>> --- > > >>>>> v2->v3: > > >>>>> Follow Vignesh's advice: > > >>>>> add one more check for check_good() even when time_after() > > >>>>> returns > > >> true. > > >>>>> > > >>>>> drivers/mtd/chips/cfi_cmdset_0002.c | 2 +- > > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>>>> > > >>>>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c > > >>>>> b/drivers/mtd/chips/cfi_cmdset_0002.c > > >>>>> index 72428b6..3da2376 100644 > > >>>>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c > > >>>>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > > >>>>> @@ -1876,7 +1876,7 @@ static int __xipram > > >>>>> do_write_buffer(struct map_info *map, struct flchip *chip, > > >>>>> continue; > > >>>>> } > > >>>>> > > >>>>> - if (time_after(jiffies, timeo) > > >>>>> && !chip_ready(map, adr)) > > >>>>> + if (time_after(jiffies, timeo) > > >>>>> && !chip_good(map, adr, datum)) > > >>>> > > >>>> Just another idea to understand easily. > > >>>> > > >>>> unsigned long now = jiffies; > > >>>> > > >>>> if (chip_good(map, adr, datum)) { > > >>>> xip_enable(map, chip, adr); > > >>>> goto op_done; > > >>>> } > > >>>> > > >>>> if (time_after(now, timeo) { > > >>>> break; > > >>>> } > > >>>> > > >>> > > >>> Thank you~. It is more easier to understand！ > > >>> If there are no other comments, I will send new patch again ): > > >> > > >> 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. > > > > > > > As Boris explained above version 3 does not really follow my > > suggestion... Please see below > > > > > 1. Original Vignesh-san suggestion > > > > > > if (chip_good(map, adr, datum)) { > > > xip_enable(map, chip, adr); > > > goto op_done; > > > } > > > > > > if (time_after(jiffies, timeo)) { > > > /* Test chip_good() if TRUE incorrectly again so > > > write > > failure by time_after() can be avoided. */ > > > if (chip_good(map, adr, datum)) { > > > xip_enable(map, chip, adr); > > > goto op_done; > > > } > > > break; > > > } > > > > > > > > > Right, here we check chip_good() once _even when time_after() is > > true_ to avoid _spurious_ timeout > > > > > 2. Liujian-san v3 patch > > > > > > /* Test chip_good() if FALSE correctly so write failure > > > by > > time_after() can be avoided. */ > > > if (time_after(jiffies, timeo) && !chip_good(map, adr)) > > > break; > > > > > > if (chip_good(map, adr, datum)) { > > > xip_enable(map, chip, adr); > > > goto op_done; > > > } > > > > > > > This is a better version of 1 > > > > > 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; > > > } > > > > > > > What if thread gets pre-empted at this point and is re-scheduled > > exactly after timeo jiffies have elapsed? Below check would be true > > and exit loop > > I think that the jiffies value now is save before chip_good() so > below check would be false and not exit loop. True, I overlooked that part, and so Vignesh did. This proves one thing: code is not easier to follow with your version. IMO, if we want to make things clear, we should add a comment to Liujian's explaining why the extra test after time_after(jiffies, timeo) is needed.
> > > [...] > > > >>>>> In function do_write_buffer(), in the for loop, there is a > > > >>>>> case chip_ready() returns 1 while chip_good() returns 0, so > > > >>>>> it never break the loop. > > > >>>>> To fix this, chip_good() is enough and it should timeout if > > > >>>>> it stay bad for a while. > > > >>>>> > > > >>>>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write > > > >>>>> buffer to check correct value") > > > >>>>> Signed-off-by: Yi Huaijie <yihuaijie@huawei.com> > > > >>>>> Signed-off-by: Liu Jian <liujian56@huawei.com> > > > >>>>> Reviewed-by: Tokunori Ikegami <ikegami_to@yahoo.co.jp> > > > >>>>> --- > > > >>>>> v2->v3: > > > >>>>> Follow Vignesh's advice: > > > >>>>> add one more check for check_good() even when time_after() > > > >>>>> returns > > > >> true. > > > >>>>> > > > >>>>> drivers/mtd/chips/cfi_cmdset_0002.c | 2 +- > > > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > > > >>>>> > > > >>>>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c > > > >>>>> b/drivers/mtd/chips/cfi_cmdset_0002.c > > > >>>>> index 72428b6..3da2376 100644 > > > >>>>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c > > > >>>>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > > > >>>>> @@ -1876,7 +1876,7 @@ static int __xipram > > > >>>>> do_write_buffer(struct map_info *map, struct flchip *chip, > > > >>>>> continue; > > > >>>>> } > > > >>>>> > > > >>>>> - if (time_after(jiffies, timeo) > > > >>>>> && !chip_ready(map, adr)) > > > >>>>> + if (time_after(jiffies, timeo) > > > >>>>> && !chip_good(map, adr, datum)) > > > >>>> > > > >>>> Just another idea to understand easily. > > > >>>> > > > >>>> unsigned long now = jiffies; > > > >>>> > > > >>>> if (chip_good(map, adr, datum)) { > > > >>>> xip_enable(map, chip, adr); > > > >>>> goto op_done; > > > >>>> } > > > >>>> > > > >>>> if (time_after(now, timeo) { > > > >>>> break; > > > >>>> } > > > >>>> > > > >>> > > > >>> Thank you~. It is more easier to understand！ > > > >>> If there are no other comments, I will send new patch again ): > > > >> > > > >> 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. > > > > > > > > > > As Boris explained above version 3 does not really follow my > > > suggestion... Please see below > > > > > > > 1. Original Vignesh-san suggestion > > > > > > > > if (chip_good(map, adr, datum)) { > > > > xip_enable(map, chip, adr); > > > > goto op_done; > > > > } > > > > > > > > if (time_after(jiffies, timeo)) { > > > > /* Test chip_good() if TRUE incorrectly again so > > > > write > > > failure by time_after() can be avoided. */ > > > > if (chip_good(map, adr, datum)) { > > > > xip_enable(map, chip, adr); > > > > goto op_done; > > > > } > > > > break; > > > > } > > > > > > > > > > > > > Right, here we check chip_good() once _even when time_after() is > > > true_ to avoid _spurious_ timeout > > > > > > > 2. Liujian-san v3 patch > > > > > > > > /* Test chip_good() if FALSE correctly so write failure > > > > by > > > time_after() can be avoided. */ > > > > if (time_after(jiffies, timeo) && !chip_good(map, adr)) > > > > break; > > > > > > > > if (chip_good(map, adr, datum)) { > > > > xip_enable(map, chip, adr); > > > > goto op_done; > > > > } > > > > > > > > > > This is a better version of 1 > > > > > > > 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; > > > > } > > > > > > > > > > What if thread gets pre-empted at this point and is re-scheduled > > > exactly after timeo jiffies have elapsed? Below check would be true > > > and exit loop > > > > I think that the jiffies value now is save before chip_good() so > > below check would be false and not exit loop. > > True, I overlooked that part, and so Vignesh did. This proves one > thing: code is not easier to follow with your version. IMO, if we want > to make things clear, we should add a comment to Liujian's explaining > why the extra test after time_after(jiffies, timeo) is needed. I see so I am okay with the change of Liujian-san v3 patch. Also agree with the comment to be added. Regards, Ikegami > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
The "mtd: " prefix is still missing. Should be "mtd: cfi: ". If you send a new version, please fix that. Thanks, Boris On Tue, 26 Feb 2019 22:00:48 +0800 Liu Jian <liujian56@huawei.com> wrote: > In function do_write_buffer(), in the for loop, there is a case > chip_ready() returns 1 while chip_good() returns 0, so it never > break the loop. > To fix this, chip_good() is enough and it should timeout if it stay > bad for a while. > > Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value") > Signed-off-by: Yi Huaijie <yihuaijie@huawei.com> > Signed-off-by: Liu Jian <liujian56@huawei.com> > Reviewed-by: Tokunori Ikegami <ikegami_to@yahoo.co.jp> > --- > v2->v3: > Follow Vignesh's advice: > add one more check for check_good() even when time_after() returns true. > > drivers/mtd/chips/cfi_cmdset_0002.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c > index 72428b6..3da2376 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -1876,7 +1876,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, > continue; > } > > - if (time_after(jiffies, timeo) && !chip_ready(map, adr)) > + if (time_after(jiffies, timeo) && !chip_good(map, adr, datum)) > break; > > if (chip_good(map, adr, datum)) {
On 01-Mar-19 11:25 PM, Tokunori Ikegami wrote: >>>> [...] >>>>>>>>> In function do_write_buffer(), in the for loop, there is a >>>>>>>>> case chip_ready() returns 1 while chip_good() returns 0, so >>>>>>>>> it never break the loop. >>>>>>>>> To fix this, chip_good() is enough and it should timeout if >>>>>>>>> it stay bad for a while. >>>>>>>>> >>>>>>>>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write >>>>>>>>> buffer to check correct value") >>>>>>>>> Signed-off-by: Yi Huaijie <yihuaijie@huawei.com> >>>>>>>>> Signed-off-by: Liu Jian <liujian56@huawei.com> >>>>>>>>> Reviewed-by: Tokunori Ikegami <ikegami_to@yahoo.co.jp> >>>>>>>>> --- >>>>>>>>> v2->v3: >>>>>>>>> Follow Vignesh's advice: >>>>>>>>> add one more check for check_good() even when time_after() >>>>>>>>> returns >>>>>> true. >>>>>>>>> >>>>>>>>> drivers/mtd/chips/cfi_cmdset_0002.c | 2 +- >>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c >>>>>>>>> b/drivers/mtd/chips/cfi_cmdset_0002.c >>>>>>>>> index 72428b6..3da2376 100644 >>>>>>>>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c >>>>>>>>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c >>>>>>>>> @@ -1876,7 +1876,7 @@ static int __xipram >>>>>>>>> do_write_buffer(struct map_info *map, struct flchip *chip, >>>>>>>>> continue; >>>>>>>>> } >>>>>>>>> >>>>>>>>> - if (time_after(jiffies, timeo) >>>>>>>>> && !chip_ready(map, adr)) >>>>>>>>> + if (time_after(jiffies, timeo) >>>>>>>>> && !chip_good(map, adr, datum)) >>>>>>>> >>>>>>>> Just another idea to understand easily. >>>>>>>> >>>>>>>> unsigned long now = jiffies; >>>>>>>> >>>>>>>> if (chip_good(map, adr, datum)) { >>>>>>>> xip_enable(map, chip, adr); >>>>>>>> goto op_done; >>>>>>>> } >>>>>>>> >>>>>>>> if (time_after(now, timeo) { >>>>>>>> break; >>>>>>>> } >>>>>>>> >>>>>>> >>>>>>> Thank you~. It is more easier to understand！ >>>>>>> If there are no other comments, I will send new patch again ): >>>>>> >>>>>> 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. >>>>> >>>> >>>> As Boris explained above version 3 does not really follow my >>>> suggestion... Please see below >>>> >>>>> 1. Original Vignesh-san suggestion >>>>> >>>>> if (chip_good(map, adr, datum)) { >>>>> xip_enable(map, chip, adr); >>>>> goto op_done; >>>>> } >>>>> >>>>> if (time_after(jiffies, timeo)) { >>>>> /* Test chip_good() if TRUE incorrectly again so >>>>> write >>>> failure by time_after() can be avoided. */ >>>>> if (chip_good(map, adr, datum)) { >>>>> xip_enable(map, chip, adr); >>>>> goto op_done; >>>>> } >>>>> break; >>>>> } >>>>> >>>> >>>> >>>> Right, here we check chip_good() once _even when time_after() is >>>> true_ to avoid _spurious_ timeout >>>> >>>>> 2. Liujian-san v3 patch >>>>> >>>>> /* Test chip_good() if FALSE correctly so write failure >>>>> by >>>> time_after() can be avoided. */ >>>>> if (time_after(jiffies, timeo) && !chip_good(map, adr)) >>>>> break; >>>>> >>>>> if (chip_good(map, adr, datum)) { >>>>> xip_enable(map, chip, adr); >>>>> goto op_done; >>>>> } >>>>> >>>> >>>> This is a better version of 1 >>>> >>>>> 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; >>>>> } >>>>> >>>> >>>> What if thread gets pre-empted at this point and is re-scheduled >>>> exactly after timeo jiffies have elapsed? Below check would be true >>>> and exit loop >>> >>> I think that the jiffies value now is save before chip_good() so >>> below check would be false and not exit loop. >> Ok, I get it now. >> True, I overlooked that part, and so Vignesh did. This proves one >> thing: code is not easier to follow with your version. IMO, if we want >> to make things clear, we should add a comment to Liujian's explaining >> why the extra test after time_after(jiffies, timeo) is needed. > > I see so I am okay with the change of Liujian-san v3 patch. > Also agree with the comment to be added. > Right, I like the current patch from Liujian, because its more consistent with the existing code in this file. Liujian, Could you re-post with a comment added as suggested above? Regards Vignesh
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c index 72428b6..3da2376 100644 --- a/drivers/mtd/chips/cfi_cmdset_0002.c +++ b/drivers/mtd/chips/cfi_cmdset_0002.c @@ -1876,7 +1876,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, continue; } - if (time_after(jiffies, timeo) && !chip_ready(map, adr)) + if (time_after(jiffies, timeo) && !chip_good(map, adr, datum)) break; if (chip_good(map, adr, datum)) {
In function do_write_buffer(), in the for loop, there is a case chip_ready() returns 1 while chip_good() returns 0, so it never break the loop. To fix this, chip_good() is enough and it should timeout if it stay bad for a while. Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value") Signed-off-by: Yi Huaijie <yihuaijie@huawei.com> Signed-off-by: Liu Jian <liujian56@huawei.com> Reviewed-by: Tokunori Ikegami <ikegami_to@yahoo.co.jp> --- v2->v3: Follow Vignesh's advice: add one more check for check_good() even when time_after() returns true. drivers/mtd/chips/cfi_cmdset_0002.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)