Re: [PATCH] i2c: i2c-tegra: fix possible race condition after tx

[Vincent Palatin]

On Fri, Jun 3, 2011 at 18:01, Stephen Warren <swarren@xxxxxxxxxx> wrote:
>Tested-by: Stephen Warren <swarren@xxxxxxxxxx>
>
> (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.


-- 
Vincent
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/