staging: olpc_dcon: Replace BUG/BUG_ON with WARN_ON

PATCH In review Anchal Jain 3 participants 6 messages Sep 15, 2016 Staging

lore

Share & tools

lore

Anchal Jain

author

2016-09-15 14:51 UTC #0 lore raw

PATCH 1/1 staging: olpc_dcon: Replace BUG/BUG_ON with WARN_ON

This patch fixed the warning detect by checkpatch.pl

Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON().

Signed-off-by Anchal Jain <[email protected]>

---
 drivers/staging/olpc_dcon/olpc_dcon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/staging/olpc_dcon/olpc_dcon.c +2 -2

index f45b2ef..62e70ee 100644
@@ -141,7 +141,7 @@ power_up:
 	}
 	if (x < 0) {
 		pr_err("unable to stabilize dcon's smbus, reasserting power and praying.\n");
-		BUG_ON(olpc_board_at_least(olpc_board(0xc2)));
+		WAARN_ON(olpc_board_at_least(olpc_board(0xc2)));
 		pm = 0;
 		olpc_ec_cmd(EC_DCON_POWER_MODE, &pm, 1, NULL, 0);
 		msleep(100);

@@ -369,7 +369,7 @@ static void dcon_source_switch(struct work_struct *work)
 		break;
 	}
 	default:
-		BUG();
+		WARN_ON();
 	}
 
 	dcon->curr_src = source;

Signature · 1 line

1.9.1

Julia Lawall

2016-09-15 14:56 UTC #1 lore raw

Quoted 4 lines · Anchal Jain

This patch fixed the warning detect by checkpatch.pl
Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON().

I think that this is not a good idea. It may be essential to crash the kernel in these cases. You would have to know why the kernel is able to continue.

julia

Quoted 38 lines

Signed-off-by Anchal Jain <[email protected]>
---
 drivers/staging/olpc_dcon/olpc_dcon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/olpc_dcon/olpc_dcon.c +2 -3
index f45b2ef..62e70ee 100644
@@ -141,7 +141,7 @@ power_up:
 	}
 	if (x < 0) {
 		pr_err("unable to stabilize dcon's smbus, reasserting power and praying.\n");
-		BUG_ON(olpc_board_at_least(olpc_board(0xc2)));
+		WAARN_ON(olpc_board_at_least(olpc_board(0xc2)));
 		pm = 0;
 		olpc_ec_cmd(EC_DCON_POWER_MODE, &pm, 1, NULL, 0);
 		msleep(100);
@@ -369,7 +369,7 @@ static void dcon_source_switch(struct work_struct *work)
 		break;
 	}
 	default:
-		BUG();
+		WARN_ON();
 	}
 
 	dcon->curr_src = source;
--
1.9.1
--
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/20160915145101.GA6064%40life-desktop.
For more options, visit https://groups.google.com/d/optout.

Anchal Jain

author

2016-09-15 15:08 UTC #2 lore raw

Quoted 3 lines

I think that this is not a good idea. It may be essential to crash the kernel in these cases. You would have to know why the kernel is able to continue

Because when we use BUG_ON and WARN_ON macro which will abort execution and produce diagnostic messages, a stack trace and a list of modules.

but we prefer to use a WARN_ON because it is a less lethal mechanism,
which will produce a backtrace without crashing the Kernel.

Quoted 61 lines · Julia Lawall

Quoted 4 lines · Anchal Jain
This patch fixed the warning detect by checkpatch.pl
Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON().
I think that this is not a good idea. It may be essential to crash the kernel in these cases. You would have to know why the kernel is able to continue.
julia
Review
Quoted 7 lines
Signed-off-by Anchal Jain <[email protected]>
---
 drivers/staging/olpc_dcon/olpc_dcon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/staging/olpc_dcon/olpc_dcon.c
drivers/staging/olpc_dcon/olpc_dcon.c power_up: · line 143
Quoted 7 lines
index f45b2ef..62e70ee 100644
drivers/staging/olpc_dcon/olpc_dcon.c +0 -0
@@ -141,7 +141,7 @@ power_up:
      }
      if (x < 0) {
              pr_err("unable to stabilize dcon's smbus, reasserting
power and praying.\n");
drivers/staging/olpc_dcon/olpc_dcon.c static void dcon_source_switch(struct work_struct
Quoted 6 lines
drivers/staging/olpc_dcon/olpc_dcon.c +1 -1
-             BUG_ON(olpc_board_at_least(olpc_board(0xc2)));
+             WAARN_ON(olpc_board_at_least(olpc_board(0xc2)));
              pm = 0;
              olpc_ec_cmd(EC_DCON_POWER_MODE, &pm, 1, NULL, 0);
              msleep(100);
@@ -369,7 +369,7 @@ static void dcon_source_switch(struct work_struct
*work)
Quoted 11 lines
drivers/staging/olpc_dcon/olpc_dcon.c +1 -2
              break;
      }
      default:
-             BUG();
+             WARN_ON();
      }
 
      dcon->curr_src = source;
--
1.9.1
Quoted 2 lines
--
You received this message because you are subscribed to the Google
Groups "outreachy-kernel" group.
Quoted 1 line
To unsubscribe from this group and stop receiving emails from it, send
an email to [email protected].
Quoted 2 lines
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/20160915145101.GA6064%40life-desktop.
Quoted 2 lines
For more options, visit https://groups.google.com/d/optout.

Julia Lawall

2016-09-15 19:15 UTC #3 lore raw

Quoted 10 lines · Anchal Jain

Quoted 4 lines
I think that this is not a good idea. It may be essential to crash the kernel in these cases. You would have to know why the kernel is able to continue
Because when we use BUG_ON and WARN_ON macro which will abort execution and produce diagnostic messages, a stack trace and a list of modules.
but we prefer to use a WARN_ON because it is a less lethal mechanism, which will produce a backtrace without crashing the Kernel.

Yes, but maybe the next instruction will do something that should not be allowed to happen. I understand what is the difference between BUG_ON and WARN_ON, but not why WARN_ON is safe in this particular context.

Think of BUG_ON as hitting the brakes in a car. You might not normally want to do that when driving down the middle of a highway. But it could be a good thing to do if you are about to hit a brick wall.

julia

Quoted 80 lines · Julia Lawall

      On Thu, 15 Sep 2016, Anchal Jain wrote:

      > This patch fixed the warning detect by checkpatch.pl
      >
      > Avoid crashing the kernel - try using WARN_ON & recovery code
      rather than
      > BUG() or BUG_ON().

      I think that this is not a good idea.  It may be essential to
      crash the
      kernel in these cases.  You would have to know why the kernel is
      able to
      continue.

      julia


      >
      > Signed-off-by: Anchal Jain <[email protected]>
      > ---
      >  drivers/staging/olpc_dcon/olpc_dcon.c | 4 ++--
      >  1 file changed, 2 insertions(+), 2 deletions(-)
      >
      > diff --git a/drivers/staging/olpc_dcon/olpc_dcon.c
      b/drivers/staging/olpc_dcon/olpc_dcon.c
      > index f45b2ef..62e70ee 100644
      > --- a/drivers/staging/olpc_dcon/olpc_dcon.c
      > +++ b/drivers/staging/olpc_dcon/olpc_dcon.c
      > @@ -141,7 +141,7 @@ power_up:
      >       }
      >       if (x < 0) {
      >               pr_err("unable to stabilize dcon's smbus,
      reasserting power and praying.\n");
      > -             BUG_ON(olpc_board_at_least(olpc_board(0xc2)));
      > +             WAARN_ON(olpc_board_at_least(olpc_board(0xc2)));
      >               pm = 0;
      >               olpc_ec_cmd(EC_DCON_POWER_MODE, &pm, 1, NULL,
      0);
      >               msleep(100);
      > @@ -369,7 +369,7 @@ static void dcon_source_switch(struct
      work_struct *work)
      >               break;
      >       }
      >       default:
      > -             BUG();
      > +             WARN_ON();
      >       }
      >
      >       dcon->curr_src = source;
      > --
      > 1.9.1
      >
Quoted 2 lines
--
You received this message because you are subscribed to the Google
Groups "outreachy-kernel" group.
Quoted 1 line
To unsubscribe from this group and stop receiving emails from it,
send an email to [email protected].
Quoted 1 line
To post to this group, send email to
[email protected].
Quoted 1 line
To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/20160915145101.GA6064%40
life-desktop.
Quoted 2 lines
For more options, visit https://groups.google.com/d/optout.
--
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 visithttps://groups.google.com/d/msgid/outreachy-kernel/CAAbeOSt2NVAJ51RBMhoe9d2 m%2B2rfQ_pA3AxoU8-ObBNf10LSCg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Greg KH

maintainer

2016-09-15 15:39 UTC #4 lore raw

In reply to Anchal Jain

Quoted 15 lines · Anchal Jain

This patch fixed the warning detect by checkpatch.pl
Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON().
Signed-off-by Anchal Jain <[email protected]>
---
 drivers/staging/olpc_dcon/olpc_dcon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/olpc_dcon/olpc_dcon.c +0 -0
index f45b2ef..62e70ee 100644

drivers/staging/olpc_dcon/olpc_dcon.c power_up: · line 144

Quoted 5 lines

drivers/staging/olpc_dcon/olpc_dcon.c +1 -1

@@ -141,7 +141,7 @@ power_up:
 	}
 	if (x < 0) {
 		pr_err("unable to stabilize dcon's smbus, reasserting power and praying.\n");
-		BUG_ON(olpc_board_at_least(olpc_board(0xc2)));
+		WAARN_ON(olpc_board_at_least(olpc_board(0xc2)));

Did you build this?

Also, the original code is correct here, the line above should have given you a hint about this.

Remember, checkpatch is just a guideline, not hard and fast rules. Here it is incorrect.

And again, you didn't even build the code, which is a bit rude :(

thanks,

greg k-h

Anchal Jain

author

2016-09-15 15:42 UTC #5 lore raw

Sorry for doing this kind of mistake, Next Time this is not repeat

Quoted 38 lines · Greg KH

Review
Quoted 11 lines · Anchal Jain
This patch fixed the warning detect by checkpatch.pl
Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON().
Signed-off-by Anchal Jain <[email protected]>
---
 drivers/staging/olpc_dcon/olpc_dcon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/staging/olpc_dcon/olpc_dcon.c
drivers/staging/olpc_dcon/olpc_dcon.c power_up: · line 143
Quoted 7 lines
index f45b2ef..62e70ee 100644
drivers/staging/olpc_dcon/olpc_dcon.c +0 -0
@@ -141,7 +141,7 @@ power_up:
      }
      if (x < 0) {
              pr_err("unable to stabilize dcon's smbus, reasserting
power and praying.\n");
drivers/staging/olpc_dcon/olpc_dcon.c power_up: · line 144
Quoted 2 lines
drivers/staging/olpc_dcon/olpc_dcon.c +1 -1
-             BUG_ON(olpc_board_at_least(olpc_board(0xc2)));
+             WAARN_ON(olpc_board_at_least(olpc_board(0xc2)));
Did you build this?
Also, the original code is correct here, the line above should have given you a hint about this.
Remember, checkpatch is just a guideline, not hard and fast rules. Here it is incorrect.
And again, you didn't even build the code, which is a bit rude :(
thanks,
greg k-h