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 V7] i2c: tegra: remove BUG, BUG_ON Date: Mon, 17 Jun 2019 11:41:00 -0700 [thread overview] Message-ID: <43a3fae8-dd3e-c7d3-42a7-493210e601e2@nvidia.com> (raw) In-Reply-To: <5a8ad23f-33c8-5140-cef8-f9cef70764b1@gmail.com> On 6/17/19 5:13 AM, Dmitry Osipenko wrote: > 17.06.2019 8:09, Bitan Biswas пишет: >> Remove BUG, BUG_ON as it makes system usable: >> - Remove redundant BUG_ON calls or replace with WARN_ON_ONCE >> as needed. >> - Remove BUG() and mask Rx interrupt similar as Tx >> for message fully sent case. >> - Add caller error handling and WARN_ON_ONCE check for non-zero >> rx_fifo_avail in tegra_i2c_empty_rx_fifo() after all processing. > > The commit message should describe motivation of the change and not the change itself, > unless it's some additional information which is required for better understanding of > the code. > > In yours case it could be something like that: > > The usage of BUG() macro is generally discouraged in kernel, unless > it's a problem that results in a physical damage or loss of data. > This patch removes unnecessary BUG() macros and replaces the rest > with a warnings. I shall update as per above comments. > >> Signed-off-by: Bitan Biswas <bbiswas@nvidia.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 45 ++++++++++++++++++++++++++++++++++-------- >> 1 file changed, 37 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >> index 4dfb4c1..b155b61 100644 >> --- a/drivers/i2c/busses/i2c-tegra.c >> +++ b/drivers/i2c/busses/i2c-tegra.c >> @@ -73,6 +73,7 @@ >> #define I2C_ERR_NO_ACK BIT(0) >> #define I2C_ERR_ARBITRATION_LOST BIT(1) >> #define I2C_ERR_UNKNOWN_INTERRUPT BIT(2) >> +#define I2C_ERR_RX_BUFFER_OVERFLOW BIT(3) >> >> #define PACKET_HEADER0_HEADER_SIZE_SHIFT 28 >> #define PACKET_HEADER0_PACKET_ID_SHIFT 16 >> @@ -515,7 +516,11 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) >> * prevent overwriting past the end of buf >> */ >> if (rx_fifo_avail > 0 && buf_remaining > 0) { >> - BUG_ON(buf_remaining > 3); >> + /* >> + * buf_remaining > 3 check not needed as rx_fifo_avail == 0 >> + * when (words_to_transfer was > rx_fifo_avail) earlier >> + * in this function. >> + */ >> val = i2c_readl(i2c_dev, I2C_RX_FIFO); >> val = cpu_to_le32(val); >> memcpy(buf, &val, buf_remaining); >> @@ -523,7 +528,15 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) >> rx_fifo_avail--; >> } >> >> - BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0); >> + if ((!(i2c_dev->msg_buf_remaining)) && > > The RX FIFO shall be drained completely no matter what. > > Hence why the "i2c_dev->msg_buf_remaining" checking is needed here? I moved the part of below condition in Patch V6 to function tegra_i2c_empty_rx_fifo: >> + err_val = tegra_i2c_empty_rx_fifo(i2c_dev); >> + if ((!(i2c_dev->msg_buf_remaining)) && > Let's move this check into tegra_i2c_empty_rx_fifo() and return -EINVAL for that case. > This will make code to look cleaner. Is above condition not needed? > > Secondly, in the future please don't add parens where they are not needed. In this > case parens around !i2c_dev->msg_buf_remaining are not needed at all. > I shall look out for similar unnecessary parentheses and update the patch. >> + WARN_ON_ONCE(rx_fifo_avail)) >> + return -EINVAL; >> + >> + /* >> + * buf_remaining > 0 at this point can only have rx_fifo_avail == 0 > > The rx_fifo_avail is always 0 at this point, including the case of buf_remaining == 0. > It will be better if you'll add a comment for the WARN_ON_ONCE(rx_fifo_avail) above, > saying that RX FIFO must be fully drained, and then just drop this comment. > OK. >> + * as this corresponds to (words_to_transfer was > rx_fifo_avail) >> + * case earlier in this function. >> + */ >> i2c_dev->msg_buf_remaining = buf_remaining; >> i2c_dev->msg_buf = buf; > > [snip] >
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 V7] i2c: tegra: remove BUG, BUG_ON Date: Mon, 17 Jun 2019 11:41:00 -0700 [thread overview] Message-ID: <43a3fae8-dd3e-c7d3-42a7-493210e601e2@nvidia.com> (raw) In-Reply-To: <5a8ad23f-33c8-5140-cef8-f9cef70764b1@gmail.com> On 6/17/19 5:13 AM, Dmitry Osipenko wrote: > 17.06.2019 8:09, Bitan Biswas пишет: >> Remove BUG, BUG_ON as it makes system usable: >> - Remove redundant BUG_ON calls or replace with WARN_ON_ONCE >> as needed. >> - Remove BUG() and mask Rx interrupt similar as Tx >> for message fully sent case. >> - Add caller error handling and WARN_ON_ONCE check for non-zero >> rx_fifo_avail in tegra_i2c_empty_rx_fifo() after all processing. > > The commit message should describe motivation of the change and not the change itself, > unless it's some additional information which is required for better understanding of > the code. > > In yours case it could be something like that: > > The usage of BUG() macro is generally discouraged in kernel, unless > it's a problem that results in a physical damage or loss of data. > This patch removes unnecessary BUG() macros and replaces the rest > with a warnings. I shall update as per above comments. > >> Signed-off-by: Bitan Biswas <bbiswas@nvidia.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 45 ++++++++++++++++++++++++++++++++++-------- >> 1 file changed, 37 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >> index 4dfb4c1..b155b61 100644 >> --- a/drivers/i2c/busses/i2c-tegra.c >> +++ b/drivers/i2c/busses/i2c-tegra.c >> @@ -73,6 +73,7 @@ >> #define I2C_ERR_NO_ACK BIT(0) >> #define I2C_ERR_ARBITRATION_LOST BIT(1) >> #define I2C_ERR_UNKNOWN_INTERRUPT BIT(2) >> +#define I2C_ERR_RX_BUFFER_OVERFLOW BIT(3) >> >> #define PACKET_HEADER0_HEADER_SIZE_SHIFT 28 >> #define PACKET_HEADER0_PACKET_ID_SHIFT 16 >> @@ -515,7 +516,11 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) >> * prevent overwriting past the end of buf >> */ >> if (rx_fifo_avail > 0 && buf_remaining > 0) { >> - BUG_ON(buf_remaining > 3); >> + /* >> + * buf_remaining > 3 check not needed as rx_fifo_avail == 0 >> + * when (words_to_transfer was > rx_fifo_avail) earlier >> + * in this function. >> + */ >> val = i2c_readl(i2c_dev, I2C_RX_FIFO); >> val = cpu_to_le32(val); >> memcpy(buf, &val, buf_remaining); >> @@ -523,7 +528,15 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) >> rx_fifo_avail--; >> } >> >> - BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0); >> + if ((!(i2c_dev->msg_buf_remaining)) && > > The RX FIFO shall be drained completely no matter what. > > Hence why the "i2c_dev->msg_buf_remaining" checking is needed here? I moved the part of below condition in Patch V6 to function tegra_i2c_empty_rx_fifo: >> + err_val = tegra_i2c_empty_rx_fifo(i2c_dev); >> + if ((!(i2c_dev->msg_buf_remaining)) && > Let's move this check into tegra_i2c_empty_rx_fifo() and return -EINVAL for that case. > This will make code to look cleaner. Is above condition not needed? > > Secondly, in the future please don't add parens where they are not needed. In this > case parens around !i2c_dev->msg_buf_remaining are not needed at all. > I shall look out for similar unnecessary parentheses and update the patch. >> + WARN_ON_ONCE(rx_fifo_avail)) >> + return -EINVAL; >> + >> + /* >> + * buf_remaining > 0 at this point can only have rx_fifo_avail == 0 > > The rx_fifo_avail is always 0 at this point, including the case of buf_remaining == 0. > It will be better if you'll add a comment for the WARN_ON_ONCE(rx_fifo_avail) above, > saying that RX FIFO must be fully drained, and then just drop this comment. > OK. >> + * as this corresponds to (words_to_transfer was > rx_fifo_avail) >> + * case earlier in this function. >> + */ >> i2c_dev->msg_buf_remaining = buf_remaining; >> i2c_dev->msg_buf = buf; > > [snip] >
next prev parent reply other threads:[~2019-06-17 18:41 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-17 5:09 [PATCH V7] i2c: tegra: remove BUG, BUG_ON Bitan Biswas 2019-06-17 5:09 ` Bitan Biswas 2019-06-17 12:13 ` Dmitry Osipenko 2019-06-17 18:41 ` Bitan Biswas [this message] 2019-06-17 18:41 ` Bitan Biswas 2019-06-17 19:28 ` Dmitry Osipenko 2019-06-18 4:29 ` Bitan Biswas 2019-06-18 4:29 ` Bitan Biswas
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=43a3fae8-dd3e-c7d3-42a7-493210e601e2@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.