From: Stephen Warren <swarren@nvidia.com>
To: Vincent Palatin <vpalatin@chromium.org>
Cc: Jean Delvare <khali@linux-fr.org>,
Ben Dooks <ben-linux@fluff.org>,
"linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
Olof Johansson <olofj@chromium.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Colin Cross <ccross@android.com>,
"linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>
Subject: RE: [PATCH] i2c: i2c-tegra: fix possible race condition after tx
Date: Fri, 3 Jun 2011 15:33:02 -0700 [thread overview]
Message-ID: <74CDBE0F657A3D45AFBB94109FB122FF0498E1C896@HQMAIL01.nvidia.com> (raw)
In-Reply-To: <BANLkTin_ypRamBbdmE1unWyovVc-h_nSa5jDkNkfc2S+n4fwdg@mail.gmail.com>
Vincent Palatin wrote at Friday, June 03, 2011 4:19 PM:
> On Fri, Jun 3, 2011 at 18:01, Stephen Warren <swarren@nvidia.com> wrote:
> >Tested-by: Stephen Warren <swarren@nvidia.com>
> >
> > (using code based on 3.0-rc1, on Harmony, ran "speaker-test -c 2", and
> > then adjusted the volume a lot using alsamixer, thus causing quite a few
> > I2C transactions)
>
> Thanks for the testing and the review !
>
> >> > @@ -213,38 +213,41 @@ static int tegra_i2c_empty_rx_fifo(struct
> tegra_i2c_dev *i2c_dev)
> >> > u32 val;
> >> > int rx_fifo_avail;
> >> > u8 *buf = i2c_dev->msg_buf;
> >> > - size_t buf_remaining = i2c_dev->msg_buf_remaining;
> >
> > The old code read msg_buf_remaining once up front and did everything
> > based on that.
> >
> >> > int words_to_transfer;
> >> > + int bytes_to_transfer;
> >> >
> >> > val = i2c_readl(i2c_dev, I2C_FIFO_STATUS);
> >> > rx_fifo_avail = (val & I2C_FIFO_STATUS_RX_MASK) >>
> >> > I2C_FIFO_STATUS_RX_SHIFT;
> >> >
> >> > /* Rounds down to not include partial word at the end of buf
> */
> >> > - words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD;
> >> > + words_to_transfer = atomic_read(&i2c_dev->msg_buf_remaining)
> /
> >> > + BYTES_PER_FIFO_WORD;
> >
> > Whereas the new code reads msg_buf_remaining once here...
> >
> >> > if (words_to_transfer > rx_fifo_avail)
> >> > words_to_transfer = rx_fifo_avail;
> >> >
> >> > + atomic_sub(words_to_transfer * BYTES_PER_FIFO_WORD,
> >> > + &i2c_dev->msg_buf_remaining);
> >> > i2c_readsl(i2c_dev, buf, I2C_RX_FIFO, words_to_transfer);
> >> >
> >> > buf += words_to_transfer * BYTES_PER_FIFO_WORD;
> >> > - buf_remaining -= words_to_transfer * BYTES_PER_FIFO_WORD;
> >> > rx_fifo_avail -= words_to_transfer;
> >> >
> >> > /*
> >> > * If there is a partial word at the end of buf, handle it manually to
> >> > * prevent overwriting past the end of buf
> >> > */
> >> > - if (rx_fifo_avail > 0 && buf_remaining > 0) {
> >> > - BUG_ON(buf_remaining > 3);
> >> > + bytes_to_transfer = atomic_read(&i2c_dev->msg_buf_remaining);
> >
> > And again here...
> >
> >> > + if (rx_fifo_avail > 0 && bytes_to_transfer > 0) {
> >> > + BUG_ON(bytes_to_transfer > 3);
> >
> > That means that if msg_buf_remaining increases between those two reads,
> > this BUG_ON could trigger.
> >
> > I assume this isn't possible, because the I2C core only sends one
> > transaction to the I2C driver and doesn't send any more requests down
> > until the previous is complete. If so, then the new code seems fine, but
> > I did want to double-check this.
>
> The transfers are serialized in the i2c_transfer function of the core
> (which calls the tegra_i2c_xfer callback) and msg_buf_remaining can
> only increase when it is set at the beginning of tegra_i2c_xfer_msg.
> So yes we have at most one transaction and I don't think we can
> trigger this BUG_ON.
Great, that's what I figured. So, the change looks good to me, so
Acked-by: Stephen Warren <swarren@nvidia.com>
too!
--
nvpublic
prev parent reply other threads:[~2011-06-03 22:33 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-19 22:14 [PATCH] i2c: i2c-tegra: fix possible race condition after tx Vincent Palatin
2011-06-02 13:32 ` Vincent Palatin
2011-06-03 22:01 ` Stephen Warren
2011-06-03 22:18 ` Vincent Palatin
2011-06-03 22:33 ` Stephen Warren [this message]
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=74CDBE0F657A3D45AFBB94109FB122FF0498E1C896@HQMAIL01.nvidia.com \
--to=swarren@nvidia.com \
--cc=ben-linux@fluff.org \
--cc=ccross@android.com \
--cc=khali@linux-fr.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=olofj@chromium.org \
--cc=vpalatin@chromium.org \
/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).