Hi Ben,
can this fix for tegra I2C go in 3.1 ?
It has approved and tested by Stephen.
Thanks,
--
Vincent
On Fri, Jun 3, 2011 at 15:33, Stephen Warren <swarren@xxxxxxxxxx> wrote:
> Vincent Palatin wrote at Friday, June 03, 2011 4:19 PM:
>> 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.
>
> Great, that's what I figured. So, the change looks good to me, so
>
> Acked-by: Stephen Warren <swarren@xxxxxxxxxx>
>
> too!
>
> --
> nvpublic
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
