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/
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2019-03-01 16:54 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-26 14:00 [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c do_write_buffer Liu Jian
2019-02-28 14:25 ` Tokunori Ikegami
2019-02-28 15:12 ` liujian (CE)
2019-02-28 15:42 ` Boris Brezillon
2019-03-01 14:51 ` Tokunori Ikegami
2019-03-01 16:07 ` Boris Brezillon
2019-03-01 16:54 ` Tokunori Ikegami [this message]
2019-03-01 16:47 ` Vignesh Raghavendra
2019-03-01 16:59 ` Tokunori Ikegami
2019-03-01 17:43 ` Boris Brezillon
2019-03-01 17:55 ` Tokunori Ikegami
2019-03-02 8:57 ` Vignesh Raghavendra
2019-03-01 19:56 ` Boris Brezillon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='000301d4d04f$76c2aad0$64480070$@gmail.com' \
--to=ikegami.t@gmail.com \
--cc=bbrezillon@kernel.org \
--cc=boris.brezillon@collabora.com \
--cc=computersforpeace@gmail.com \
--cc=dwmw2@infradead.org \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=liujian56@huawei.com \
--cc=marek.vasut@gmail.com \
--cc=richard@nod.at \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).