Skip to contentSkip to conversation

staging: nvec: Avoid the use of BUG_ON

PATCH Superseded Laura Garcia Liebana 2 participants 4 messages Feb 21, 2016 – Feb 22, 2016

Subsystems (2)

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 staging: nvec: Avoid the use of BUG_ON

Prevent a kernel panic by avoiding the use of the BUG_ON macro and using the WARN_ON macro instead. Checkpatch detected this issue.

---
 drivers/staging/nvec/nvec.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
drivers/staging/nvec/nvec.c +12 -2
index c335ae2..3b5b94f 100644
@@ -641,11 +641,21 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
 			nvec_msg_free(nvec, nvec->rx);
 			nvec->state = 3;
 			nvec_tx_set(nvec);
-			BUG_ON(nvec->tx->size < 1);
+			if (WARN_ON(nvec->tx->size < 1)) {
+				dev_err(nvec->dev,
+					"Invalid TX size\n");
+				nvec->state = 0;
+				break;
+			}
 			to_send = nvec->tx->data[0];
 			nvec->tx->pos = 1;
 		} else if (status == (I2C_SL_IRQ)) {
-			BUG_ON(nvec->rx == NULL);
+			if (WARN_ON(!nvec->rx)) {
+				dev_err(nvec->dev,
+					"Invalid RX\n");
+				nvec->state = 0;
+				break;
+			}
 			nvec->rx->data[1] = received;
 			nvec->rx->pos = 2;
 			nvec->state = 4;
Signature · 1 line
2.7.0
#1 lore raw
Quoted 2 lines · Laura Garcia Liebana

Prevent a kernel panic by avoiding the use of the BUG_ON macro and using the WARN_ON macro instead. Checkpatch detected this issue.

Why is nvec->state = 0; the right thing to do? What will happen when the break is taken?

julia

Quoted 43 lines
---
 drivers/staging/nvec/nvec.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
drivers/staging/nvec/nvec.c +12 -3
index c335ae2..3b5b94f 100644
@@ -641,11 +641,21 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
 			nvec_msg_free(nvec, nvec->rx);
 			nvec->state = 3;
 			nvec_tx_set(nvec);
-			BUG_ON(nvec->tx->size < 1);
+			if (WARN_ON(nvec->tx->size < 1)) {
+				dev_err(nvec->dev,
+					"Invalid TX size\n");
+				nvec->state = 0;
+				break;
+			}
 			to_send = nvec->tx->data[0];
 			nvec->tx->pos = 1;
 		} else if (status == (I2C_SL_IRQ)) {
-			BUG_ON(nvec->rx == NULL);
+			if (WARN_ON(!nvec->rx)) {
+				dev_err(nvec->dev,
+					"Invalid RX\n");
+				nvec->state = 0;
+				break;
+			}
 			nvec->rx->data[1] = received;
 			nvec->rx->pos = 2;
 			nvec->state = 4;
--

2.7.0

--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160221081553.GA4327%40sonyv.
For more options, visit https://groups.google.com/d/optout.

#2 lore raw
Quoted 46 lines · Julia Lawall
Quoted 2 lines · Laura Garcia Liebana

Prevent a kernel panic by avoiding the use of the BUG_ON macro and using the WARN_ON macro instead. Checkpatch detected this issue.

Why is nvec->state = 0; the right thing to do? What will happen when the break is taken?

julia

Quoted 36 lines
---
 drivers/staging/nvec/nvec.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
drivers/staging/nvec/nvec.c +12 -3
index c335ae2..3b5b94f 100644
@@ -641,11 +641,21 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
 			nvec_msg_free(nvec, nvec->rx);
 			nvec->state = 3;
 			nvec_tx_set(nvec);
-			BUG_ON(nvec->tx->size < 1);
+			if (WARN_ON(nvec->tx->size < 1)) {
+				dev_err(nvec->dev,
+					"Invalid TX size\n");
+				nvec->state = 0;
+				break;
+			}
 			to_send = nvec->tx->data[0];
 			nvec->tx->pos = 1;
 		} else if (status == (I2C_SL_IRQ)) {
-			BUG_ON(nvec->rx == NULL);
+			if (WARN_ON(!nvec->rx)) {
+				dev_err(nvec->dev,
+					"Invalid RX\n");
+				nvec->state = 0;
+				break;
+			}
 			nvec->rx->data[1] = received;
 			nvec->rx->pos = 2;
 			nvec->state = 4;
--

2.7.0

Quoted 7 lines
Quoted 7 lines

--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160221081553.GA4327%40sonyv.
For more options, visit https://groups.google.com/d/optout.

I got inspired by an earlier case, for example in case 1:

nvec->rx = nvec_msg_alloc(nvec, NVEC_MSG_RX);
/* Should not happen in a normal world */

if (unlikely(!nvec->rx)) {
        nvec->state = 0;
        break;
}

A similar case appears in the case 2 (same function):

if (nvec->rx->data[0] != 0x01) {
        dev_err(nvec->dev,
                "Read without prior read command\n");
        nvec->state = 0;
        break;
}

Please let me know your thoughts about it.
Kind Regards.

#3 lore raw
Quoted 65 lines · Laura Garcia
Quoted 46 lines · Julia Lawall
Quoted 2 lines · Laura Garcia Liebana

Prevent a kernel panic by avoiding the use of the BUG_ON macro and using the WARN_ON macro instead. Checkpatch detected this issue.

Why is nvec->state = 0; the right thing to do? What will happen when the break is taken?

julia

Quoted 36 lines
---
 drivers/staging/nvec/nvec.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
drivers/staging/nvec/nvec.c +12 -3
index c335ae2..3b5b94f 100644
@@ -641,11 +641,21 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
 			nvec_msg_free(nvec, nvec->rx);
 			nvec->state = 3;
 			nvec_tx_set(nvec);
-			BUG_ON(nvec->tx->size < 1);
+			if (WARN_ON(nvec->tx->size < 1)) {
+				dev_err(nvec->dev,
+					"Invalid TX size\n");
+				nvec->state = 0;
+				break;
+			}
 			to_send = nvec->tx->data[0];
 			nvec->tx->pos = 1;
 		} else if (status == (I2C_SL_IRQ)) {
-			BUG_ON(nvec->rx == NULL);
+			if (WARN_ON(!nvec->rx)) {
+				dev_err(nvec->dev,
+					"Invalid RX\n");
+				nvec->state = 0;
+				break;
+			}
 			nvec->rx->data[1] = received;
 			nvec->rx->pos = 2;
 			nvec->state = 4;
--

2.7.0

Quoted 7 lines
Quoted 7 lines

--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160221081553.GA4327%40sonyv.
For more options, visit https://groups.google.com/d/optout.

I got inspired by an earlier case, for example in case 1:

nvec->rx = nvec_msg_alloc(nvec, NVEC_MSG_RX);
/* Should not happen in a normal world */

if (unlikely(!nvec->rx)) {
        nvec->state = 0;
        break;
}

A similar case appears in the case 2 (same function):

Quoted 8 lines
if (nvec->rx->data[0] != 0x01) {
        dev_err(nvec->dev,
                "Read without prior read command\n");
        nvec->state = 0;
        break;
}

Please let me know your thoughts about it.

I don't know. It seems risky. I think that one would have to consult with the developers. I don't know if these particular problems can be survived.

julia