Re: [PATCH V9] i2c: tegra: remove BUG() macro
On Tue, Jun 18, 2019 at 04:09:42AM -0700, Bitan Biswas wrote: > 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 warning. > > Signed-off-by: Bitan Biswas Applied to for-next, thanks! signature.asc Description: PGP signature
Re: [PATCH V9] i2c: tegra: remove BUG() macro
On Tue, Jun 18, 2019 at 04:09:42AM -0700, Bitan Biswas wrote: > 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 warning. > > Signed-off-by: Bitan Biswas > --- > drivers/i2c/busses/i2c-tegra.c | 47 > +++--- > 1 file changed, 39 insertions(+), 8 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH V9] i2c: tegra: remove BUG() macro
On Tuesday 18 June 2019 04:39 PM, Bitan Biswas wrote: 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 warning. Signed-off-by: Bitan Biswas Acked By: Laxman Dewangan Thanks, Laxman
Re: [PATCH V9] i2c: tegra: remove BUG() macro
On 6/18/19 4:09 AM, Bitan Biswas wrote: 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 warning. Signed-off-by: Bitan Biswas --- drivers/i2c/busses/i2c-tegra.c | 47 +++--- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 4dfb4c1..e9ff96d 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -73,6 +73,7 @@ #define I2C_ERR_NO_ACKBIT(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_SHIFT16 @@ -489,6 +490,13 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) size_t buf_remaining = i2c_dev->msg_buf_remaining; int words_to_transfer; + /* +* Catch overflow due to message fully sent +* before the check for RX FIFO availability. +*/ + if (WARN_ON_ONCE(!(i2c_dev->msg_buf_remaining))) + return -EINVAL; + if (i2c_dev->hw->has_mst_fifo) { val = i2c_readl(i2c_dev, I2C_MST_FIFO_STATUS); rx_fifo_avail = (val & I2C_MST_FIFO_STATUS_RX_MASK) >> @@ -515,7 +523,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, , buf_remaining); @@ -523,7 +535,10 @@ 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); + /* RX FIFO must be drained, otherwise it's an Overflow case. */ + if (WARN_ON_ONCE(rx_fifo_avail)) + return -EINVAL; + i2c_dev->msg_buf_remaining = buf_remaining; i2c_dev->msg_buf = buf; @@ -581,7 +596,11 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev) * boundary and fault. */ if (tx_fifo_avail > 0 && buf_remaining > 0) { - BUG_ON(buf_remaining > 3); + /* +* buf_remaining > 3 check not needed as tx_fifo_avail == 0 +* when (words_to_transfer was > tx_fifo_avail) earlier +* in this function for non-zero words_to_transfer. +*/ memcpy(, buf, buf_remaining); val = le32_to_cpu(val); @@ -847,10 +866,15 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id) if (!i2c_dev->is_curr_dma_xfer) { if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) { - if (i2c_dev->msg_buf_remaining) - tegra_i2c_empty_rx_fifo(i2c_dev); - else - BUG(); + if (tegra_i2c_empty_rx_fifo(i2c_dev)) { + /* +* Overflow error condition: message fully sent, +* with no XFER_COMPLETE interrupt but hardware +* asks to transfer more. +*/ + i2c_dev->msg_err |= I2C_ERR_RX_BUFFER_OVERFLOW; + goto err; + } } if (!i2c_dev->msg_read && (status & I2C_INT_TX_FIFO_DATA_REQ)) { @@ -876,7 +900,14 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id) if (status & I2C_INT_PACKET_XFER_COMPLETE) { if (i2c_dev->is_curr_dma_xfer) i2c_dev->msg_buf_remaining = 0; - BUG_ON(i2c_dev->msg_buf_remaining); + /* +* Underflow error condition: XFER_COMPLETE before message +* fully sent. +*/ + if (WARN_ON_ONCE(i2c_dev->msg_buf_remaining)) { + i2c_dev->msg_err |= I2C_ERR_UNKNOWN_INTERRUPT; + goto err; + } complete(_dev->msg_complete); } goto done; Please get back if there are any further comments regarding this patch. -regards, Bitan
Re: [PATCH V9] i2c: tegra: remove BUG() macro
On 6/18/19 4:29 AM, Dmitry Osipenko wrote: 18.06.2019 14:09, Bitan Biswas пишет: 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 warning. Signed-off-by: Bitan Biswas --- drivers/i2c/busses/i2c-tegra.c | 47 +++--- 1 file changed, 39 insertions(+), 8 deletions(-) Thank you very much! Please keep applying all the advises that were given to you during the reviews in the future patches. Thank you Dmitry for the review inputs and patience. I shall try to keep the advices in mind for future reviews. I made a quick test and no problems spotted, all warning spots are silent. Reviewed-by: Dmitry Osipenko Tested-by: Dmitry Osipenko Also, Thank you for help in testing the patch at your end. -regards, Bitan
Re: [PATCH V9] i2c: tegra: remove BUG() macro
18.06.2019 14:09, Bitan Biswas пишет: > 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 warning. > > Signed-off-by: Bitan Biswas > --- > drivers/i2c/busses/i2c-tegra.c | 47 > +++--- > 1 file changed, 39 insertions(+), 8 deletions(-) Thank you very much! Please keep applying all the advises that were given to you during the reviews in the future patches. I made a quick test and no problems spotted, all warning spots are silent. Reviewed-by: Dmitry Osipenko Tested-by: Dmitry Osipenko
[PATCH V9] i2c: tegra: remove BUG() macro
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 warning. Signed-off-by: Bitan Biswas --- drivers/i2c/busses/i2c-tegra.c | 47 +++--- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 4dfb4c1..e9ff96d 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 @@ -489,6 +490,13 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) size_t buf_remaining = i2c_dev->msg_buf_remaining; int words_to_transfer; + /* +* Catch overflow due to message fully sent +* before the check for RX FIFO availability. +*/ + if (WARN_ON_ONCE(!(i2c_dev->msg_buf_remaining))) + return -EINVAL; + if (i2c_dev->hw->has_mst_fifo) { val = i2c_readl(i2c_dev, I2C_MST_FIFO_STATUS); rx_fifo_avail = (val & I2C_MST_FIFO_STATUS_RX_MASK) >> @@ -515,7 +523,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, , buf_remaining); @@ -523,7 +535,10 @@ 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); + /* RX FIFO must be drained, otherwise it's an Overflow case. */ + if (WARN_ON_ONCE(rx_fifo_avail)) + return -EINVAL; + i2c_dev->msg_buf_remaining = buf_remaining; i2c_dev->msg_buf = buf; @@ -581,7 +596,11 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev) * boundary and fault. */ if (tx_fifo_avail > 0 && buf_remaining > 0) { - BUG_ON(buf_remaining > 3); + /* +* buf_remaining > 3 check not needed as tx_fifo_avail == 0 +* when (words_to_transfer was > tx_fifo_avail) earlier +* in this function for non-zero words_to_transfer. +*/ memcpy(, buf, buf_remaining); val = le32_to_cpu(val); @@ -847,10 +866,15 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id) if (!i2c_dev->is_curr_dma_xfer) { if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) { - if (i2c_dev->msg_buf_remaining) - tegra_i2c_empty_rx_fifo(i2c_dev); - else - BUG(); + if (tegra_i2c_empty_rx_fifo(i2c_dev)) { + /* +* Overflow error condition: message fully sent, +* with no XFER_COMPLETE interrupt but hardware +* asks to transfer more. +*/ + i2c_dev->msg_err |= I2C_ERR_RX_BUFFER_OVERFLOW; + goto err; + } } if (!i2c_dev->msg_read && (status & I2C_INT_TX_FIFO_DATA_REQ)) { @@ -876,7 +900,14 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id) if (status & I2C_INT_PACKET_XFER_COMPLETE) { if (i2c_dev->is_curr_dma_xfer) i2c_dev->msg_buf_remaining = 0; - BUG_ON(i2c_dev->msg_buf_remaining); + /* +* Underflow error condition: XFER_COMPLETE before message +* fully sent. +*/ + if (WARN_ON_ONCE(i2c_dev->msg_buf_remaining)) { + i2c_dev->msg_err |= I2C_ERR_UNKNOWN_INTERRUPT; + goto err; + } complete(_dev->msg_complete); } goto done; -- 2.7.4