* [PATCH] i2c: i2c-tegra: fix possible race condition after tx
@ 2011-04-19 22:14 Vincent Palatin
2011-06-02 13:32 ` Vincent Palatin
0 siblings, 1 reply; 5+ messages in thread
From: Vincent Palatin @ 2011-04-19 22:14 UTC (permalink / raw)
To: Jean Delvare, Ben Dooks, linux-i2c
Cc: Olof Johansson, linux-kernel, Colin Cross, linux-tegra, Vincent Palatin
In tegra_i2c_fill_tx_fifo, once we have finished pushing all the bytes
to the I2C hardware controller, the interrupt might happen before we
have updated i2c_dev->msg_buf_remaining at the end of the function.
Then, in tegra_i2c_isr, we will call again tegra_i2c_fill_tx_fifo
triggering weird behaviour.
Of course, this is unlikely since the I2C bus is slow and thus the ISR
is called several hundreds of microseconds after the last register
write.
Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
---
drivers/i2c/busses/i2c-tegra.c | 54 ++++++++++++++++++++++-----------------
1 files changed, 30 insertions(+), 24 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index b4ab39b..c1b119b 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -125,7 +125,7 @@ struct tegra_i2c_dev {
struct completion msg_complete;
int msg_err;
u8 *msg_buf;
- size_t msg_buf_remaining;
+ atomic_t msg_buf_remaining;
int msg_read;
unsigned long bus_clk_rate;
bool is_suspended;
@@ -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;
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;
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);
+ if (rx_fifo_avail > 0 && bytes_to_transfer > 0) {
+ BUG_ON(bytes_to_transfer > 3);
+ atomic_set(&i2c_dev->msg_buf_remaining, 0);
val = i2c_readl(i2c_dev, I2C_RX_FIFO);
- memcpy(buf, &val, buf_remaining);
- buf_remaining = 0;
+ memcpy(buf, &val, bytes_to_transfer);
rx_fifo_avail--;
}
- BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0);
- i2c_dev->msg_buf_remaining = buf_remaining;
+ BUG_ON(rx_fifo_avail > 0 &&
+ atomic_read(&i2c_dev->msg_buf_remaining) > 0);
i2c_dev->msg_buf = buf;
return 0;
}
@@ -254,22 +257,24 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
u32 val;
int tx_fifo_avail;
u8 *buf = i2c_dev->msg_buf;
- size_t buf_remaining = i2c_dev->msg_buf_remaining;
int words_to_transfer;
+ int bytes_to_transfer;
val = i2c_readl(i2c_dev, I2C_FIFO_STATUS);
tx_fifo_avail = (val & I2C_FIFO_STATUS_TX_MASK) >>
I2C_FIFO_STATUS_TX_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;
if (words_to_transfer > tx_fifo_avail)
words_to_transfer = tx_fifo_avail;
+ atomic_sub(words_to_transfer * BYTES_PER_FIFO_WORD,
+ &i2c_dev->msg_buf_remaining);
i2c_writesl(i2c_dev, buf, I2C_TX_FIFO, words_to_transfer);
buf += words_to_transfer * BYTES_PER_FIFO_WORD;
- buf_remaining -= words_to_transfer * BYTES_PER_FIFO_WORD;
tx_fifo_avail -= words_to_transfer;
/*
@@ -277,16 +282,17 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
* prevent reading past the end of buf, which could cross a page
* boundary and fault.
*/
- if (tx_fifo_avail > 0 && buf_remaining > 0) {
- BUG_ON(buf_remaining > 3);
- memcpy(&val, buf, buf_remaining);
+ bytes_to_transfer = atomic_read(&i2c_dev->msg_buf_remaining);
+ if (tx_fifo_avail > 0 && bytes_to_transfer > 0) {
+ BUG_ON(bytes_to_transfer > 3);
+ memcpy(&val, buf, bytes_to_transfer);
+ atomic_set(&i2c_dev->msg_buf_remaining, 0);
i2c_writel(i2c_dev, val, I2C_TX_FIFO);
- buf_remaining = 0;
tx_fifo_avail--;
}
- BUG_ON(tx_fifo_avail > 0 && buf_remaining > 0);
- i2c_dev->msg_buf_remaining = buf_remaining;
+ BUG_ON(tx_fifo_avail > 0 &&
+ atomic_read(&i2c_dev->msg_buf_remaining) > 0);
i2c_dev->msg_buf = buf;
return 0;
}
@@ -364,21 +370,21 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
}
if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
- if (i2c_dev->msg_buf_remaining)
+ if (atomic_read(&i2c_dev->msg_buf_remaining))
tegra_i2c_empty_rx_fifo(i2c_dev);
else
BUG();
}
if (!i2c_dev->msg_read && (status & I2C_INT_TX_FIFO_DATA_REQ)) {
- if (i2c_dev->msg_buf_remaining)
+ if (atomic_read(&i2c_dev->msg_buf_remaining))
tegra_i2c_fill_tx_fifo(i2c_dev);
else
tegra_i2c_mask_irq(i2c_dev, I2C_INT_TX_FIFO_DATA_REQ);
}
if ((status & I2C_INT_PACKET_XFER_COMPLETE) &&
- !i2c_dev->msg_buf_remaining)
+ !atomic_read(&i2c_dev->msg_buf_remaining))
complete(&i2c_dev->msg_complete);
i2c_writel(i2c_dev, status, I2C_INT_STATUS);
@@ -408,7 +414,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
return -EINVAL;
i2c_dev->msg_buf = msg->buf;
- i2c_dev->msg_buf_remaining = msg->len;
+ atomic_set(&i2c_dev->msg_buf_remaining, msg->len);
i2c_dev->msg_err = I2C_ERR_NONE;
i2c_dev->msg_read = (msg->flags & I2C_M_RD);
INIT_COMPLETION(i2c_dev->msg_complete);
@@ -440,7 +446,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
if (msg->flags & I2C_M_RD)
int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
- else if (i2c_dev->msg_buf_remaining)
+ else if (atomic_read(&i2c_dev->msg_buf_remaining))
int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
tegra_i2c_unmask_irq(i2c_dev, int_mask);
dev_dbg(i2c_dev->dev, "unmasked irq: %02x\n",
--
1.7.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: i2c-tegra: fix possible race condition after tx
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
0 siblings, 1 reply; 5+ messages in thread
From: Vincent Palatin @ 2011-06-02 13:32 UTC (permalink / raw)
To: Jean Delvare, Ben Dooks, linux-i2c
Cc: Olof Johansson, linux-kernel, Colin Cross, linux-tegra, Vincent Palatin
Anyone has a comment on that patch ?
The I2C driver has been reworked but this issue seems to still exist
--
Vincent
On Tue, Apr 19, 2011 at 18:14, Vincent Palatin <vpalatin@chromium.org> wrote:
> In tegra_i2c_fill_tx_fifo, once we have finished pushing all the bytes
> to the I2C hardware controller, the interrupt might happen before we
> have updated i2c_dev->msg_buf_remaining at the end of the function.
> Then, in tegra_i2c_isr, we will call again tegra_i2c_fill_tx_fifo
> triggering weird behaviour.
> Of course, this is unlikely since the I2C bus is slow and thus the ISR
> is called several hundreds of microseconds after the last register
> write.
>
> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> ---
> drivers/i2c/busses/i2c-tegra.c | 54 ++++++++++++++++++++++-----------------
> 1 files changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index b4ab39b..c1b119b 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -125,7 +125,7 @@ struct tegra_i2c_dev {
> struct completion msg_complete;
> int msg_err;
> u8 *msg_buf;
> - size_t msg_buf_remaining;
> + atomic_t msg_buf_remaining;
> int msg_read;
> unsigned long bus_clk_rate;
> bool is_suspended;
> @@ -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;
> 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;
> 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);
> + if (rx_fifo_avail > 0 && bytes_to_transfer > 0) {
> + BUG_ON(bytes_to_transfer > 3);
> + atomic_set(&i2c_dev->msg_buf_remaining, 0);
> val = i2c_readl(i2c_dev, I2C_RX_FIFO);
> - memcpy(buf, &val, buf_remaining);
> - buf_remaining = 0;
> + memcpy(buf, &val, bytes_to_transfer);
> rx_fifo_avail--;
> }
>
> - BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0);
> - i2c_dev->msg_buf_remaining = buf_remaining;
> + BUG_ON(rx_fifo_avail > 0 &&
> + atomic_read(&i2c_dev->msg_buf_remaining) > 0);
> i2c_dev->msg_buf = buf;
> return 0;
> }
> @@ -254,22 +257,24 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
> u32 val;
> int tx_fifo_avail;
> u8 *buf = i2c_dev->msg_buf;
> - size_t buf_remaining = i2c_dev->msg_buf_remaining;
> int words_to_transfer;
> + int bytes_to_transfer;
>
> val = i2c_readl(i2c_dev, I2C_FIFO_STATUS);
> tx_fifo_avail = (val & I2C_FIFO_STATUS_TX_MASK) >>
> I2C_FIFO_STATUS_TX_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;
> if (words_to_transfer > tx_fifo_avail)
> words_to_transfer = tx_fifo_avail;
>
> + atomic_sub(words_to_transfer * BYTES_PER_FIFO_WORD,
> + &i2c_dev->msg_buf_remaining);
> i2c_writesl(i2c_dev, buf, I2C_TX_FIFO, words_to_transfer);
>
> buf += words_to_transfer * BYTES_PER_FIFO_WORD;
> - buf_remaining -= words_to_transfer * BYTES_PER_FIFO_WORD;
> tx_fifo_avail -= words_to_transfer;
>
> /*
> @@ -277,16 +282,17 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
> * prevent reading past the end of buf, which could cross a page
> * boundary and fault.
> */
> - if (tx_fifo_avail > 0 && buf_remaining > 0) {
> - BUG_ON(buf_remaining > 3);
> - memcpy(&val, buf, buf_remaining);
> + bytes_to_transfer = atomic_read(&i2c_dev->msg_buf_remaining);
> + if (tx_fifo_avail > 0 && bytes_to_transfer > 0) {
> + BUG_ON(bytes_to_transfer > 3);
> + memcpy(&val, buf, bytes_to_transfer);
> + atomic_set(&i2c_dev->msg_buf_remaining, 0);
> i2c_writel(i2c_dev, val, I2C_TX_FIFO);
> - buf_remaining = 0;
> tx_fifo_avail--;
> }
>
> - BUG_ON(tx_fifo_avail > 0 && buf_remaining > 0);
> - i2c_dev->msg_buf_remaining = buf_remaining;
> + BUG_ON(tx_fifo_avail > 0 &&
> + atomic_read(&i2c_dev->msg_buf_remaining) > 0);
> i2c_dev->msg_buf = buf;
> return 0;
> }
> @@ -364,21 +370,21 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
> }
>
> if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
> - if (i2c_dev->msg_buf_remaining)
> + if (atomic_read(&i2c_dev->msg_buf_remaining))
> tegra_i2c_empty_rx_fifo(i2c_dev);
> else
> BUG();
> }
>
> if (!i2c_dev->msg_read && (status & I2C_INT_TX_FIFO_DATA_REQ)) {
> - if (i2c_dev->msg_buf_remaining)
> + if (atomic_read(&i2c_dev->msg_buf_remaining))
> tegra_i2c_fill_tx_fifo(i2c_dev);
> else
> tegra_i2c_mask_irq(i2c_dev, I2C_INT_TX_FIFO_DATA_REQ);
> }
>
> if ((status & I2C_INT_PACKET_XFER_COMPLETE) &&
> - !i2c_dev->msg_buf_remaining)
> + !atomic_read(&i2c_dev->msg_buf_remaining))
> complete(&i2c_dev->msg_complete);
>
> i2c_writel(i2c_dev, status, I2C_INT_STATUS);
> @@ -408,7 +414,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
> return -EINVAL;
>
> i2c_dev->msg_buf = msg->buf;
> - i2c_dev->msg_buf_remaining = msg->len;
> + atomic_set(&i2c_dev->msg_buf_remaining, msg->len);
> i2c_dev->msg_err = I2C_ERR_NONE;
> i2c_dev->msg_read = (msg->flags & I2C_M_RD);
> INIT_COMPLETION(i2c_dev->msg_complete);
> @@ -440,7 +446,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
> int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
> if (msg->flags & I2C_M_RD)
> int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
> - else if (i2c_dev->msg_buf_remaining)
> + else if (atomic_read(&i2c_dev->msg_buf_remaining))
> int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
> tegra_i2c_unmask_irq(i2c_dev, int_mask);
> dev_dbg(i2c_dev->dev, "unmasked irq: %02x\n",
> --
> 1.7.3.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] i2c: i2c-tegra: fix possible race condition after tx
2011-06-02 13:32 ` Vincent Palatin
@ 2011-06-03 22:01 ` Stephen Warren
2011-06-03 22:18 ` Vincent Palatin
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Warren @ 2011-06-03 22:01 UTC (permalink / raw)
To: Vincent Palatin, Jean Delvare, Ben Dooks, linux-i2c
Cc: Olof Johansson, linux-kernel, Colin Cross, linux-tegra
Vincent Palatin wrote at Thursday, June 02, 2011 7:32 AM:
> Anyone has a comment on that patch ?
> The I2C driver has been reworked but this issue seems to still exist
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)
One question inline below though.
> On Tue, Apr 19, 2011 at 18:14, Vincent Palatin <vpalatin@chromium.org> wrote:
> > In tegra_i2c_fill_tx_fifo, once we have finished pushing all the bytes
> > to the I2C hardware controller, the interrupt might happen before we
> > have updated i2c_dev->msg_buf_remaining at the end of the function.
> > Then, in tegra_i2c_isr, we will call again tegra_i2c_fill_tx_fifo
> > triggering weird behaviour.
> > Of course, this is unlikely since the I2C bus is slow and thus the ISR
> > is called several hundreds of microseconds after the last register
> > write.
> >
> > Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> > ---
> > drivers/i2c/busses/i2c-tegra.c | 54 ++++++++++++++++++++++-----------------
> > 1 files changed, 30 insertions(+), 24 deletions(-)
...
> > @@ -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.
Thanks.
> > + atomic_set(&i2c_dev->msg_buf_remaining, 0);
> > val = i2c_readl(i2c_dev, I2C_RX_FIFO);
> > - memcpy(buf, &val, buf_remaining);
> > - buf_remaining = 0;
> > + memcpy(buf, &val, bytes_to_transfer);
> > rx_fifo_avail--;
> > }
> >
> > - BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0);
> > - i2c_dev->msg_buf_remaining = buf_remaining;
> > + BUG_ON(rx_fifo_avail > 0 &&
> > + atomic_read(&i2c_dev->msg_buf_remaining) > 0);
> > i2c_dev->msg_buf = buf;
> > return 0;
> > }
--
nvpublic
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: i2c-tegra: fix possible race condition after tx
2011-06-03 22:01 ` Stephen Warren
@ 2011-06-03 22:18 ` Vincent Palatin
2011-06-03 22:33 ` Stephen Warren
0 siblings, 1 reply; 5+ messages in thread
From: Vincent Palatin @ 2011-06-03 22:18 UTC (permalink / raw)
To: Stephen Warren
Cc: Jean Delvare, Ben Dooks, linux-i2c, Olof Johansson, linux-kernel,
Colin Cross, linux-tegra
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.
--
Vincent
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] i2c: i2c-tegra: fix possible race condition after tx
2011-06-03 22:18 ` Vincent Palatin
@ 2011-06-03 22:33 ` Stephen Warren
0 siblings, 0 replies; 5+ messages in thread
From: Stephen Warren @ 2011-06-03 22:33 UTC (permalink / raw)
To: Vincent Palatin
Cc: Jean Delvare, Ben Dooks, linux-i2c, Olof Johansson, linux-kernel,
Colin Cross, linux-tegra
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-06-03 22:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 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).