i2c: tegra: remove BUG() macro

PATCH Applied Bitan Biswas 5 participants 7 messages Jun 18, 2019 – Jun 21, 2019

Subsystems (3)

Tegra architecture support I2C subsystem host drivers Tegra I2C driver

lore

Share & tools

lore

Bitan Biswas

author

2019-06-18 11:09 UTC #0 lore raw

PATCH v9 1/1 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 <[email protected]>

---
 drivers/i2c/busses/i2c-tegra.c | 47 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 8 deletions(-)

drivers/i2c/busses/i2c-tegra.c +39 -8

index 4dfb4c1..e9ff96d 100644
@@ -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, &val, 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(&val, 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(&i2c_dev->msg_complete);
 	}
 	goto done;

Signature · 1 line

2.7.4

Dmitry Osipenko

reviewer Reviewed-by Tested-by on v9 1/1

2019-06-18 11:29 UTC #1 lore raw

18.06.2019 14:09, Bitan Biswas пишет:

Quoted 9 lines

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 <[email protected]>
---
 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 <[email protected]>

Tested-by Dmitry Osipenko <[email protected]>

Bitan Biswas

author

2019-06-18 11:52 UTC #2 lore raw

Quoted 6 lines · Dmitry Osipenko

18.06.2019 14:09, Bitan Biswas пишет:
Quoted 5 lines
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.

Quoted 7 lines

Quoted 4 lines
Signed-off-by Bitan Biswas <[email protected]>
---
  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.

Quoted 6 lines

I made a quick test and no problems spotted, all warning spots are silent.
Reviewed-by Dmitry Osipenko <[email protected]>
Tested-by Dmitry Osipenko <[email protected]>

Also, Thank you for help in testing the patch at your end.

-regards,
Bitan

Bitan Biswas

author

2019-06-20 05:00 UTC #3 lore raw

In reply to Bitan Biswas

Quoted 101 lines · Bitan Biswas

Signed-off-by Bitan Biswas <[email protected]>

---
  drivers/i2c/busses/i2c-tegra.c | 47 +++++++++++++++++++++++++++++++++++-------
  1 file changed, 39 insertions(+), 8 deletions(-)

drivers/i2c/busses/i2c-tegra.c +32 -8

index 4dfb4c1..e9ff96d 100644
@@ -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, &val, 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(&val, 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);
+		/*

drivers/i2c/busses/i2c-tegra.c static irqreturn_t tegra_i2c_isr(int irq, void *dev_id) · line 913

Quoted 10 lines

drivers/i2c/busses/i2c-tegra.c +7 -0

+		 * 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(&i2c_dev->msg_complete);
  	}
  	goto done;

Please get back if there are any further comments regarding this patch.

-regards,
Bitan

Laxman Dewangan

maintainer

2019-06-20 09:23 UTC #4 lore raw

In reply to Bitan Biswas

Quoted 7 lines · 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 <[email protected]>

Acked By: Laxman Dewangan <[email protected]>

Thanks,
Laxman

Thierry Reding

Acked-by on v9 1/1

2019-06-20 10:30 UTC #5 lore raw

In reply to Bitan Biswas

Quoted 9 lines · 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 <[email protected]>
---
 drivers/i2c/busses/i2c-tegra.c | 47 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 8 deletions(-)

Acked-by Thierry Reding <[email protected]>

Wolfram Sang

Applied

2019-06-21 21:26 UTC #6 lore raw

In reply to Bitan Biswas

Quoted 6 lines · 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 <[email protected]>

Applied to for-next, thanks!