Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755215Ab1FBNc1 (ORCPT ); Thu, 2 Jun 2011 09:32:27 -0400 Received: from mail-iw0-f174.google.com ([209.85.214.174]:39891 "EHLO mail-iw0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752897Ab1FBNcY convert rfc822-to-8bit (ORCPT ); Thu, 2 Jun 2011 09:32:24 -0400 MIME-Version: 1.0 X-Originating-IP: [226.16.121.39] In-Reply-To: <1303251276-18768-1-git-send-email-vpalatin@chromium.org> References: <1303251276-18768-1-git-send-email-vpalatin@chromium.org> From: Vincent Palatin Date: Thu, 2 Jun 2011 09:32:03 -0400 X-Google-Sender-Auth: cCvRusQtZARUc3Cr00N-UpMLSwc Message-ID: Subject: Re: [PATCH] i2c: i2c-tegra: fix possible race condition after tx To: Jean Delvare , Ben Dooks , linux-i2c@vger.kernel.org Cc: Olof Johansson , linux-kernel@vger.kernel.org, Colin Cross , linux-tegra@vger.kernel.org, Vincent Palatin Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8005 Lines: 187 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 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 > --- > ?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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/