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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
>  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
--
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


[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux