i2c: tegra: remove BUG, BUG_ON

PATCH Superseded Bitan Biswas 2 participants 5 messages Jun 17, 2019 – Jun 18, 2019

Subsystems (3)

Tegra architecture support I2C subsystem host drivers Tegra I2C driver

lore

Share & tools

lore

Bitan Biswas

author

2019-06-17 05:09 UTC #0 lore raw

PATCH v7 1/1 i2c: tegra: remove BUG, BUG_ON

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.

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

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

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

index 4dfb4c1..b155b61 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

@@ -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)) &&
+	    WARN_ON_ONCE(rx_fifo_avail))
+		return -EINVAL;
+
+	/*
+	 * buf_remaining > 0 at this point can only have rx_fifo_avail == 0
+	 * 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;

@@ -581,7 +594,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 +864,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 +898,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

2019-06-17 12:13 UTC #1 lore raw

17.06.2019 8:09, Bitan Biswas пишет:

Quoted 7 lines

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.

Quoted 31 lines

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

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

drivers/i2c/busses/i2c-tegra.c +6 -1

index 4dfb4c1..b155b61 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

@@ -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);

drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) · line 531

Quoted 5 lines

drivers/i2c/busses/i2c-tegra.c +1 -1

@@ -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?

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.

drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) · line 536

Quoted 5 lines

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

+	    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.

Quoted 5 lines

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

+	 * 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]

Bitan Biswas

author

2019-06-17 18:41 UTC #2 lore raw

Quoted 13 lines · Dmitry Osipenko

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

Quoted 6 lines

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.

Quoted 35 lines

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

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

drivers/i2c/busses/i2c-tegra.c +6 -1

index 4dfb4c1..b155b61 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

@@ -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--;
  	}

Quoted 6 lines

drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) · line 531
Quoted 2 lines
drivers/i2c/busses/i2c-tegra.c +1 -1
-	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:

Quoted 5 lines

drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) · line 533
Quoted 2 lines
drivers/i2c/busses/i2c-tegra.c +2 -0
+			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?

Quoted 4 lines

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.

Quoted 10 lines

drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) · line 536
Quoted 5 lines
drivers/i2c/busses/i2c-tegra.c +5 -0
+	    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.

Quoted 8 lines

Quoted 5 lines

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

+	 * 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]

Dmitry Osipenko

reviewer

2019-06-17 19:28 UTC #3 lore raw

17.06.2019 21:41, Bitan Biswas пишет:

Quoted 65 lines · Dmitry Osipenko

Quoted 13 lines
17.06.2019 8:09, Bitan Biswas пишет:
Quoted 7 lines
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.
Quoted 6 lines
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.
Quoted 38 lines
Quoted 31 lines
Signed-off-by Bitan Biswas <[email protected]>
---
drivers/i2c/busses/i2c-tegra.c | 45 ++++++++++++++++++++++++++++++++++--------
  1 file changed, 37 insertions(+), 8 deletions(-)
drivers/i2c/busses/i2c-tegra.c +6 -1
index 4dfb4c1..b155b61 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
@@ -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);
drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) · line 531
Quoted 4 lines
drivers/i2c/busses/i2c-tegra.c +1 -0
@@ -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.
Quoted 1 line
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:

Quoted 7 lines

Quoted 5 lines
drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) · line 533
Quoted 2 lines
drivers/i2c/busses/i2c-tegra.c +2 -0
+            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?

Let's put it at the very beginning. This may give a bit more information about the problem by knowing if the offending overflow happens after or during of the buffer's fill up.

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;

	if (WARN_ON(!i2c_dev->msg_buf_remaining))
		return -EINVAL;

...

In general, the original logic should be preserved during of refactoring. In this case we are keeping the original check and then also making it a bit more informative.

Quoted 6 lines

Quoted 5 lines
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.

Yes, please clean up all the occurrences in the code if there are any. And please do it in a separate patch.

Bitan Biswas

author

2019-06-18 04:29 UTC #4 lore raw

Quoted 88 lines · Dmitry Osipenko

17.06.2019 21:41, Bitan Biswas пишет:
Quoted 65 lines · Dmitry Osipenko
Quoted 13 lines
17.06.2019 8:09, Bitan Biswas пишет:
Quoted 7 lines
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.
Quoted 6 lines
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.
Quoted 38 lines
Quoted 31 lines
Signed-off-by Bitan Biswas <[email protected]>
---
   drivers/i2c/busses/i2c-tegra.c | 45 ++++++++++++++++++++++++++++++++++--------
   1 file changed, 37 insertions(+), 8 deletions(-)
drivers/i2c/busses/i2c-tegra.c +6 -1
index 4dfb4c1..b155b61 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
@@ -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);
drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) · line 531
Quoted 4 lines
drivers/i2c/busses/i2c-tegra.c +1 -0
@@ -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.
Quoted 1 line
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:
Quoted 7 lines
Quoted 5 lines
drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) · line 533
Quoted 2 lines
drivers/i2c/busses/i2c-tegra.c +2 -0
+            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?
Let's put it at the very beginning. This may give a bit more information about the problem by knowing if the offending overflow happens after or during of the buffer's fill up.
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;

	if (WARN_ON(!i2c_dev->msg_buf_remaining))
		return -EINVAL;

Quoted 2 lines

...

Quoted 3 lines

In general, the original logic should be preserved during of refactoring. In this case we are keeping the original check and then also making it a bit more informative.

I feel the msg_buf_remaining check was not there in original code. The corresponding line was probably the following checking for error when (buf_remaining > 0) after all work in the function tegra_i2c_empty_rx_fifo()

https://elixir.bootlin.com/linux/v5.2-rc5/source/drivers/i2c/busses/i2c-tegra.c#L536

Quoted 10 lines

Quoted 6 lines
Quoted 5 lines
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.
Yes, please clean up all the occurrences in the code if there are any. And please do it in a separate patch.

I reviewed the source for unnecessary parentheses and do not find any. Hence not planning to push any patch. Let me know if I missed a case.

-Thanks,
Bitan