On Thu, 2022-09-22 at 19:05 -0700, John Hubbard wrote: > On 9/20/22 05:23, David Hildenbrand wrote: > > checkpatch does not point out that VM_BUG_ON() and friends should be > > avoided, however, Linus notes: > > > > VM_BUG_ON() has the exact same semantics as BUG_ON. It is literally > > no different, the only difference is "we can make the code smaller > > because these are less important". [1] > > > > So let's warn on VM_BUG_ON() and other BUG variants as well. While at it, > > make it clearer that the kernel really shouldn't be crashed. > > > > As there are some subsystem BUG macros that actually don't end up crashing > > the kernel -- for example, KVM_BUG_ON() -- exclude these manually. > > > > [1] https://lore.kernel.org/r/CAHk-=wg40EAZofO16Eviaj7mfqDhZ2gVEbvfsMf6gYzspRjYvw@xxxxxxxxxxxxxx [] > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > > @@ -4695,12 +4695,12 @@ sub process { > > } > > } > > > > -# avoid BUG() or BUG_ON() > > - if ($line =~ /\b(?:BUG|BUG_ON)\b/) { > > +# do not use BUG() or variants > > + if ($line =~ /\b(?!AA_|BUILD_|DCCP_|IDA_|KVM_|RWLOCK_|snd_|SPIN_)(?:[a-zA-Z_]*_)?BUG(?:_ON)?(?:_[A-Z_]+)?\s*\(/) { > > Should this be a separate patch? Adding a bunch of exceptions to the BUG() rules is > a separate and distinct thing from adding VM_BUG_ON() and other *BUG*() variants to > the mix. Not in my opinion. > > my $msg_level = \&WARN; > > $msg_level = \&CHK if ($file); > > &{$msg_level}("AVOID_BUG", > > - "Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()\n" . $herecurr); > > + "Do not crash the kernel unless it is unavoidable - use WARN_ON_ONCE & recovery code (if reasonable) rather than BUG() or variants.\n" . $herecurr); > > Here's a requested tweak, to clean up the output and fix punctuation: > > "Avoid crashing the kernel--use WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or variants.\n" . $herecurr); Fixing punctuation here would be removing the trailing period as checkpatch only has periods for multi-sentence output messages. And I think that "Do not crash" is a stronger statement than "Avoid crashing" so I prefer the original suggestion but it's not a big deal either way.
