From: Bitan Biswas <bbiswas@nvidia.com> To: Dmitry Osipenko <digetx@gmail.com>, Laxman Dewangan <ldewangan@nvidia.com>, Thierry Reding <treding@nvidia.com>, Jonathan Hunter <jonathanh@nvidia.com>, linux-i2c@vger.kernel.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org, Peter Rosin <peda@axentia.se>, Wolfram Sang <wsa@the-dreams.de> Cc: Shardar Mohammed <smohammed@nvidia.com>, Sowjanya Komatineni <skomatineni@nvidia.com>, Mantravadi Karthik <mkarthik@nvidia.com> Subject: Re: [PATCH V5 6/7] i2c: tegra: fix PIO rx/tx residual transfer check Date: Thu, 13 Jun 2019 02:59:25 -0700 [thread overview] Message-ID: <3899af9b-07b0-8a76-e343-82871d3eb19a@nvidia.com> (raw) In-Reply-To: <b6b24358-36a0-af98-1b29-9a622baa9600@gmail.com> On 6/12/19 6:55 AM, Dmitry Osipenko wrote: > 11.06.2019 13:51, Bitan Biswas пишет: >> Fix expression for residual bytes(less than word) transfer >> in I2C PIO mode RX/TX. >> >> Signed-off-by: Bitan Biswas <bbiswas@nvidia.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >> index 4dfb4c1..0596c12 100644 >> --- a/drivers/i2c/busses/i2c-tegra.c >> +++ b/drivers/i2c/busses/i2c-tegra.c >> @@ -514,7 +514,8 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) >> * 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) { >> + if (rx_fifo_avail > 0 && >> + (buf_remaining > 0 && buf_remaining < BYTES_PER_FIFO_WORD)) { > > The buf_remaining >= BYTES_PER_FIFO_WORD is not possible to happen > because there are three possible cases: > > 1) buf_remaining > rx_fifo_avail * 4: > > In this case rx_fifo_avail = 0 > > 2) buf_remaining < rx_fifo_avail * 4; > > In this case buf_remaining is always < 4 because > words_to_transfer is a buf_remaining rounded down to 4 > and then divided by 4. Hence: > > buf_remaining -= (buf_remaining / 4) * 4 always results > into buf_remaining < 4. > > 3) buf_remaining == rx_fifo_avail * 4: > > In this case rx_fifo_avail = 0 and buf_remaining = 0. > > Case 2 should never happen and means that something gone wrong. > Yes I now agree with you. The first condition "rx_fifo_avail > 0" failure will take care and prevent need for additional checks. >> BUG_ON(buf_remaining > 3); >> val = i2c_readl(i2c_dev, I2C_RX_FIFO); >> val = cpu_to_le32(val); >> @@ -557,11 +558,10 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev) >> words_to_transfer = tx_fifo_avail; >> >> /* >> - * Update state before writing to FIFO. If this casues us >> + * Update state before writing to FIFO. If this causes us >> * to finish writing all bytes (AKA buf_remaining goes to 0) we >> * have a potential for an interrupt (PACKET_XFER_COMPLETE is >> - * not maskable). We need to make sure that the isr sees >> - * buf_remaining as 0 and doesn't call us back re-entrantly. >> + * not maskable). >> */ >> buf_remaining -= words_to_transfer * BYTES_PER_FIFO_WORD; >> tx_fifo_avail -= words_to_transfer; >> @@ -580,7 +580,8 @@ 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) { >> + if (tx_fifo_avail > 0 && >> + (buf_remaining > 0 && buf_remaining < BYTES_PER_FIFO_WORD)) { >> BUG_ON(buf_remaining > 3); >> memcpy(&val, buf, buf_remaining); >> val = le32_to_cpu(val); >> > > Same as for RX. > Yes shall discard this patch from the next update. -Thanks, Bitan
WARNING: multiple messages have this Message-ID (diff)
From: Bitan Biswas <bbiswas@nvidia.com> To: Dmitry Osipenko <digetx@gmail.com>, Laxman Dewangan <ldewangan@nvidia.com>, Thierry Reding <treding@nvidia.com>, Jonathan Hunter <jonathanh@nvidia.com>, <linux-i2c@vger.kernel.org>, <linux-tegra@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Peter Rosin <peda@axentia.se>, Wolfram Sang <wsa@the-dreams.de> Cc: Shardar Mohammed <smohammed@nvidia.com>, Sowjanya Komatineni <skomatineni@nvidia.com>, Mantravadi Karthik <mkarthik@nvidia.com> Subject: Re: [PATCH V5 6/7] i2c: tegra: fix PIO rx/tx residual transfer check Date: Thu, 13 Jun 2019 02:59:25 -0700 [thread overview] Message-ID: <3899af9b-07b0-8a76-e343-82871d3eb19a@nvidia.com> (raw) In-Reply-To: <b6b24358-36a0-af98-1b29-9a622baa9600@gmail.com> On 6/12/19 6:55 AM, Dmitry Osipenko wrote: > 11.06.2019 13:51, Bitan Biswas пишет: >> Fix expression for residual bytes(less than word) transfer >> in I2C PIO mode RX/TX. >> >> Signed-off-by: Bitan Biswas <bbiswas@nvidia.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >> index 4dfb4c1..0596c12 100644 >> --- a/drivers/i2c/busses/i2c-tegra.c >> +++ b/drivers/i2c/busses/i2c-tegra.c >> @@ -514,7 +514,8 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) >> * 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) { >> + if (rx_fifo_avail > 0 && >> + (buf_remaining > 0 && buf_remaining < BYTES_PER_FIFO_WORD)) { > > The buf_remaining >= BYTES_PER_FIFO_WORD is not possible to happen > because there are three possible cases: > > 1) buf_remaining > rx_fifo_avail * 4: > > In this case rx_fifo_avail = 0 > > 2) buf_remaining < rx_fifo_avail * 4; > > In this case buf_remaining is always < 4 because > words_to_transfer is a buf_remaining rounded down to 4 > and then divided by 4. Hence: > > buf_remaining -= (buf_remaining / 4) * 4 always results > into buf_remaining < 4. > > 3) buf_remaining == rx_fifo_avail * 4: > > In this case rx_fifo_avail = 0 and buf_remaining = 0. > > Case 2 should never happen and means that something gone wrong. > Yes I now agree with you. The first condition "rx_fifo_avail > 0" failure will take care and prevent need for additional checks. >> BUG_ON(buf_remaining > 3); >> val = i2c_readl(i2c_dev, I2C_RX_FIFO); >> val = cpu_to_le32(val); >> @@ -557,11 +558,10 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev) >> words_to_transfer = tx_fifo_avail; >> >> /* >> - * Update state before writing to FIFO. If this casues us >> + * Update state before writing to FIFO. If this causes us >> * to finish writing all bytes (AKA buf_remaining goes to 0) we >> * have a potential for an interrupt (PACKET_XFER_COMPLETE is >> - * not maskable). We need to make sure that the isr sees >> - * buf_remaining as 0 and doesn't call us back re-entrantly. >> + * not maskable). >> */ >> buf_remaining -= words_to_transfer * BYTES_PER_FIFO_WORD; >> tx_fifo_avail -= words_to_transfer; >> @@ -580,7 +580,8 @@ 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) { >> + if (tx_fifo_avail > 0 && >> + (buf_remaining > 0 && buf_remaining < BYTES_PER_FIFO_WORD)) { >> BUG_ON(buf_remaining > 3); >> memcpy(&val, buf, buf_remaining); >> val = le32_to_cpu(val); >> > > Same as for RX. > Yes shall discard this patch from the next update. -Thanks, Bitan
next prev parent reply other threads:[~2019-06-13 9:59 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-11 10:51 [PATCH V5 1/7] i2c: tegra: clean up macros Bitan Biswas 2019-06-11 10:51 ` Bitan Biswas 2019-06-11 10:51 ` [PATCH V5 2/7] i2c: tegra: remove unnecessary variable init Bitan Biswas 2019-06-11 10:51 ` Bitan Biswas 2019-06-12 10:21 ` Wolfram Sang 2019-06-11 10:51 ` [PATCH V5 3/7] i2c: tegra: fix alignment and spacing violations Bitan Biswas 2019-06-11 10:51 ` Bitan Biswas 2019-06-12 10:21 ` Wolfram Sang 2019-06-11 10:51 ` [PATCH V5 4/7] i2c: tegra: add spinlock definition comment Bitan Biswas 2019-06-11 10:51 ` Bitan Biswas 2019-06-12 10:21 ` Wolfram Sang 2019-06-11 10:51 ` [PATCH V5 5/7] i2c: tegra: fix msleep warning Bitan Biswas 2019-06-11 10:51 ` Bitan Biswas 2019-06-12 10:21 ` Wolfram Sang 2019-06-11 10:51 ` [PATCH V5 6/7] i2c: tegra: fix PIO rx/tx residual transfer check Bitan Biswas 2019-06-11 10:51 ` Bitan Biswas 2019-06-12 10:24 ` Wolfram Sang 2019-06-13 11:43 ` Bitan Biswas 2019-06-13 11:43 ` Bitan Biswas 2019-06-13 11:52 ` Laxman Dewangan 2019-06-13 11:52 ` Laxman Dewangan 2019-06-13 13:13 ` Wolfram Sang 2019-06-12 13:55 ` Dmitry Osipenko 2019-06-13 9:59 ` Bitan Biswas [this message] 2019-06-13 9:59 ` Bitan Biswas 2019-06-12 14:30 ` Dmitry Osipenko 2019-06-13 11:30 ` Bitan Biswas 2019-06-13 11:30 ` Bitan Biswas 2019-06-13 12:28 ` Dmitry Osipenko 2019-06-14 9:50 ` Bitan Biswas 2019-06-14 9:50 ` Bitan Biswas 2019-06-14 13:02 ` Dmitry Osipenko 2019-06-18 5:21 ` Bitan Biswas 2019-06-18 5:21 ` Bitan Biswas 2019-06-11 10:51 ` [PATCH V5 7/7] i2c: tegra: remove BUG, BUG_ON Bitan Biswas 2019-06-11 10:51 ` Bitan Biswas 2019-06-11 11:38 ` Dmitry Osipenko 2019-06-12 10:21 ` [PATCH V5 1/7] i2c: tegra: clean up macros Wolfram Sang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3899af9b-07b0-8a76-e343-82871d3eb19a@nvidia.com \ --to=bbiswas@nvidia.com \ --cc=digetx@gmail.com \ --cc=jonathanh@nvidia.com \ --cc=ldewangan@nvidia.com \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-tegra@vger.kernel.org \ --cc=mkarthik@nvidia.com \ --cc=peda@axentia.se \ --cc=skomatineni@nvidia.com \ --cc=smohammed@nvidia.com \ --cc=treding@nvidia.com \ --cc=wsa@the-dreams.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.