Vincent Palatin wrote at Friday, June 03, 2011 4:19 PM:

On Fri, Jun 3, 2011 at 18:01, Stephen Warren [off-list ref] wrote:

quoted

Tested-by: Stephen Warren <redacted>

(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 !

quoted

quoted

quoted

@@ -213,38 +213,41 @@ static int tegra_i2c_empty_rx_fifo(struct

tegra_i2c_dev *i2c_dev)

quoted

quoted

quoted

       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.

quoted

quoted

       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

*/

quoted

quoted

quoted

-       words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD;
+       words_to_transfer = atomic_read(&i2c_dev->msg_buf_remaining)

/

quoted

quoted

quoted

+               BYTES_PER_FIFO_WORD;

Whereas the new code reads msg_buf_remaining once here...

quoted

quoted

       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...

quoted

quoted

+       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 <redacted>

too!

-- 
nvpublic