* False positive for 'Possible unnecessary KERN_ERR' warning in checkpatch
@ 2021-09-14 16:05 Niklas Söderlund
2021-09-16 3:46 ` Joe Perches
0 siblings, 1 reply; 3+ messages in thread
From: Niklas Söderlund @ 2021-09-14 16:05 UTC (permalink / raw)
To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel
Hi Joe,
Maybe you are already aware of this, but in case you are not.
The issue is the checkpatch check for unnecessary KERN_<LEVEL> for log
functions. If a single line contains a statement that match a log
function name from $logFunctions, such as foo_err() and the same line
contains a KERN_<LEVEL> statement the 'WARNING: Possible unnecessary
KERN_ERR' is triggered. This is true even if the KERN_<LEVEL> statement
is not part of the arguments to the foo_err() definition that triggers
the first part of the check.
This can be demonstrated by,
./scripts/checkpatch.pl --mailback --git c821e617896e99b8
Where we get (among others) the warning,
WARNING: Possible unnecessary KERN_ERR
#38: FILE: drivers/net/ethernet/netronome/nfp/nfp_net.h:63:
+#define nn_err(nn, fmt, args...) nn_pr(nn, KERN_ERR, fmt, ## args)
Looking at the code in checkpatch.pl we have,
our $logFunctions = qr{(?x:
printk(?:_ratelimited|_once|_deferred_once|_deferred|)|
(?:[a-z0-9]+_){1,2}(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|cont|WARN)(?:_ratelimited|_once|)|
TP_printk|
WARN(?:_RATELIMIT|_ONCE|)|
panic|
MODULE_[A-Z_]+|
seq_vprintf|seq_printf|seq_puts
)};
...
# check for logging functions with KERN_<LEVEL>
if ($line !~ /printk(?:_ratelimited|_once)?\s*\(/ &&
$line =~ /\b$logFunctions\s*\(.*\b(KERN_[A-Z]+)\b/) {
my $level = $1;
if (WARN("UNNECESSARY_KERN_LEVEL",
"Possible unnecessary $level\n" . $herecurr) &&
$fix) {
$fixed[$fixlinenr] =~ s/\s*$level\s*//;
}
}
Looking at the line from above that triggers the warning,
#define nn_err(nn, fmt, args...) nn_pr(nn, KERN_ERR, fmt, ## args)
We see that the warning is triggers by the regexp but that it matches
the first part on nn_err( and then the second part of on the second
argument to nn_pr, KERN_ERR. I believe this to be a false positive.
Unfortunately my Perl skills are not good enough to fix the check to only
look for KERN_[A-Z]+ inside the argument list to the log function name
that matches the first part of the regexp.
--
Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: False positive for 'Possible unnecessary KERN_ERR' warning in checkpatch 2021-09-14 16:05 False positive for 'Possible unnecessary KERN_ERR' warning in checkpatch Niklas Söderlund @ 2021-09-16 3:46 ` Joe Perches 2021-09-20 9:57 ` Niklas Söderlund 0 siblings, 1 reply; 3+ messages in thread From: Joe Perches @ 2021-09-16 3:46 UTC (permalink / raw) To: Niklas Söderlund; +Cc: Andy Whitcroft, linux-kernel On Tue, 2021-09-14 at 18:05 +0200, Niklas Söderlund wrote: > Hi Joe, > > Maybe you are already aware of this, but in case you are not. > > The issue is the checkpatch check for unnecessary KERN_<LEVEL> for log > functions. If a single line contains a statement that match a log > function name from $logFunctions, such as foo_err() and the same line > contains a KERN_<LEVEL> statement the 'WARNING: Possible unnecessary > KERN_ERR' is triggered. This is true even if the KERN_<LEVEL> statement > is not part of the arguments to the foo_err() definition that triggers > the first part of the check. > > This can be demonstrated by, > > ./scripts/checkpatch.pl --mailback --git c821e617896e99b8 > > Where we get (among others) the warning, > > WARNING: Possible unnecessary KERN_ERR > #38: FILE: drivers/net/ethernet/netronome/nfp/nfp_net.h:63: > +#define nn_err(nn, fmt, args...) nn_pr(nn, KERN_ERR, fmt, ## args) > > Looking at the code in checkpatch.pl we have, > > our $logFunctions = qr{(?x: > printk(?:_ratelimited|_once|_deferred_once|_deferred|)| > (?:[a-z0-9]+_){1,2}(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|cont|WARN)(?:_ratelimited|_once|)| > TP_printk| > WARN(?:_RATELIMIT|_ONCE|)| > panic| > MODULE_[A-Z_]+| > seq_vprintf|seq_printf|seq_puts > )}; > > ... > > # check for logging functions with KERN_<LEVEL> > if ($line !~ /printk(?:_ratelimited|_once)?\s*\(/ && > $line =~ /\b$logFunctions\s*\(.*\b(KERN_[A-Z]+)\b/) { > my $level = $1; > if (WARN("UNNECESSARY_KERN_LEVEL", > "Possible unnecessary $level\n" . $herecurr) && > $fix) { > $fixed[$fixlinenr] =~ s/\s*$level\s*//; > } > } > > Looking at the line from above that triggers the warning, > > #define nn_err(nn, fmt, args...) nn_pr(nn, KERN_ERR, fmt, ## args) > > We see that the warning is triggers by the regexp but that it matches > the first part on nn_err( and then the second part of on the second > argument to nn_pr, KERN_ERR. I believe this to be a false positive. > > Unfortunately my Perl skills are not good enough to fix the check to only > look for KERN_[A-Z]+ inside the argument list to the log function name > that matches the first part of the regexp. > I would have avoided it and gotten dynamic debug support at the same time with: --- drivers/net/ethernet/netronome/nfp/nfp_net.h | 48 ++++++++++++++-------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net.h b/drivers/net/ethernet/netronome/nfp/nfp_net.h index df203738511bf..46178a7244ad8 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_net.h +++ b/drivers/net/ethernet/netronome/nfp/nfp_net.h @@ -25,32 +25,32 @@ #include "nfp_net_ctrl.h" -#define nn_pr(nn, lvl, fmt, args...) \ - ({ \ - struct nfp_net *__nn = (nn); \ +#define nn_pr(nn, lvl, fmt, ...) \ +({ \ + struct nfp_net *__nn = (nn); \ \ - if (__nn->dp.netdev) \ - netdev_printk(lvl, __nn->dp.netdev, fmt, ## args); \ - else \ - dev_printk(lvl, __nn->dp.dev, "ctrl: " fmt, ## args); \ - }) - -#define nn_err(nn, fmt, args...) nn_pr(nn, KERN_ERR, fmt, ## args) -#define nn_warn(nn, fmt, args...) nn_pr(nn, KERN_WARNING, fmt, ## args) -#define nn_info(nn, fmt, args...) nn_pr(nn, KERN_INFO, fmt, ## args) -#define nn_dbg(nn, fmt, args...) nn_pr(nn, KERN_DEBUG, fmt, ## args) - -#define nn_dp_warn(dp, fmt, args...) \ - ({ \ - struct nfp_net_dp *__dp = (dp); \ + if (__nn->dp.netdev) \ + netdev_##lvl(__nn->dp.netdev, fmt, ##__VA_ARGS__); \ + else \ + dev_##lvl(__nn->dp.dev, "ctrl: " fmt, ##__VA_ARGS__); \ +}) + +#define nn_err(nn, fmt, ...) nn_pr(nn, err, fmt, ##__VA_ARGS__) +#define nn_warn(nn, fmt, ...) nn_pr(nn, warn, fmt, ##__VA_ARGS__) +#define nn_info(nn, fmt, ...) nn_pr(nn, info, fmt, ##__VA_ARGS__) +#define nn_dbg(nn, fmt, ...) nn_pr(nn, dbg, fmt, ##__VA_ARGS__) + +#define nn_dp_warn(dp, fmt, ...) \ +({ \ + struct nfp_net_dp *__dp = (dp); \ \ - if (unlikely(net_ratelimit())) { \ - if (__dp->netdev) \ - netdev_warn(__dp->netdev, fmt, ## args); \ - else \ - dev_warn(__dp->dev, fmt, ## args); \ - } \ - }) + if (unlikely(net_ratelimit())) { \ + if (__dp->netdev) \ + netdev_warn(__dp->netdev, fmt, ##__VA_ARGS__); \ + else \ + dev_warn(__dp->dev, fmt, ##__VA_ARGS__); \ + } \ +}) /* Max time to wait for NFP to respond on updates (in seconds) */ #define NFP_NET_POLL_TIMEOUT 5 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: False positive for 'Possible unnecessary KERN_ERR' warning in checkpatch 2021-09-16 3:46 ` Joe Perches @ 2021-09-20 9:57 ` Niklas Söderlund 0 siblings, 0 replies; 3+ messages in thread From: Niklas Söderlund @ 2021-09-20 9:57 UTC (permalink / raw) To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel Hi Joe, Thanks for your help. I like your solution. Would you like to send a patch for this or would you like me to do that with you in a Suggested-by ? On 2021-09-15 20:46:22 -0700, Joe Perches wrote: > On Tue, 2021-09-14 at 18:05 +0200, Niklas Söderlund wrote: > > Hi Joe, > > > > Maybe you are already aware of this, but in case you are not. > > > > The issue is the checkpatch check for unnecessary KERN_<LEVEL> for log > > functions. If a single line contains a statement that match a log > > function name from $logFunctions, such as foo_err() and the same line > > contains a KERN_<LEVEL> statement the 'WARNING: Possible unnecessary > > KERN_ERR' is triggered. This is true even if the KERN_<LEVEL> statement > > is not part of the arguments to the foo_err() definition that triggers > > the first part of the check. > > > > This can be demonstrated by, > > > > ./scripts/checkpatch.pl --mailback --git c821e617896e99b8 > > > > Where we get (among others) the warning, > > > > WARNING: Possible unnecessary KERN_ERR > > #38: FILE: drivers/net/ethernet/netronome/nfp/nfp_net.h:63: > > +#define nn_err(nn, fmt, args...) nn_pr(nn, KERN_ERR, fmt, ## args) > > > > Looking at the code in checkpatch.pl we have, > > > > our $logFunctions = qr{(?x: > > printk(?:_ratelimited|_once|_deferred_once|_deferred|)| > > (?:[a-z0-9]+_){1,2}(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|cont|WARN)(?:_ratelimited|_once|)| > > TP_printk| > > WARN(?:_RATELIMIT|_ONCE|)| > > panic| > > MODULE_[A-Z_]+| > > seq_vprintf|seq_printf|seq_puts > > )}; > > > > ... > > > > # check for logging functions with KERN_<LEVEL> > > if ($line !~ /printk(?:_ratelimited|_once)?\s*\(/ && > > $line =~ /\b$logFunctions\s*\(.*\b(KERN_[A-Z]+)\b/) { > > my $level = $1; > > if (WARN("UNNECESSARY_KERN_LEVEL", > > "Possible unnecessary $level\n" . $herecurr) && > > $fix) { > > $fixed[$fixlinenr] =~ s/\s*$level\s*//; > > } > > } > > > > Looking at the line from above that triggers the warning, > > > > #define nn_err(nn, fmt, args...) nn_pr(nn, KERN_ERR, fmt, ## args) > > > > We see that the warning is triggers by the regexp but that it matches > > the first part on nn_err( and then the second part of on the second > > argument to nn_pr, KERN_ERR. I believe this to be a false positive. > > > > Unfortunately my Perl skills are not good enough to fix the check to only > > look for KERN_[A-Z]+ inside the argument list to the log function name > > that matches the first part of the regexp. > > > > I would have avoided it and gotten dynamic debug support at the > same time with: > --- > drivers/net/ethernet/netronome/nfp/nfp_net.h | 48 ++++++++++++++-------------- > 1 file changed, 24 insertions(+), 24 deletions(-) > > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net.h b/drivers/net/ethernet/netronome/nfp/nfp_net.h > index df203738511bf..46178a7244ad8 100644 > --- a/drivers/net/ethernet/netronome/nfp/nfp_net.h > +++ b/drivers/net/ethernet/netronome/nfp/nfp_net.h > @@ -25,32 +25,32 @@ > > #include "nfp_net_ctrl.h" > > -#define nn_pr(nn, lvl, fmt, args...) \ > - ({ \ > - struct nfp_net *__nn = (nn); \ > +#define nn_pr(nn, lvl, fmt, ...) \ > +({ \ > + struct nfp_net *__nn = (nn); \ > \ > - if (__nn->dp.netdev) \ > - netdev_printk(lvl, __nn->dp.netdev, fmt, ## args); \ > - else \ > - dev_printk(lvl, __nn->dp.dev, "ctrl: " fmt, ## args); \ > - }) > - > -#define nn_err(nn, fmt, args...) nn_pr(nn, KERN_ERR, fmt, ## args) > -#define nn_warn(nn, fmt, args...) nn_pr(nn, KERN_WARNING, fmt, ## args) > -#define nn_info(nn, fmt, args...) nn_pr(nn, KERN_INFO, fmt, ## args) > -#define nn_dbg(nn, fmt, args...) nn_pr(nn, KERN_DEBUG, fmt, ## args) > - > -#define nn_dp_warn(dp, fmt, args...) \ > - ({ \ > - struct nfp_net_dp *__dp = (dp); \ > + if (__nn->dp.netdev) \ > + netdev_##lvl(__nn->dp.netdev, fmt, ##__VA_ARGS__); \ > + else \ > + dev_##lvl(__nn->dp.dev, "ctrl: " fmt, ##__VA_ARGS__); \ > +}) > + > +#define nn_err(nn, fmt, ...) nn_pr(nn, err, fmt, ##__VA_ARGS__) > +#define nn_warn(nn, fmt, ...) nn_pr(nn, warn, fmt, ##__VA_ARGS__) > +#define nn_info(nn, fmt, ...) nn_pr(nn, info, fmt, ##__VA_ARGS__) > +#define nn_dbg(nn, fmt, ...) nn_pr(nn, dbg, fmt, ##__VA_ARGS__) > + > +#define nn_dp_warn(dp, fmt, ...) \ > +({ \ > + struct nfp_net_dp *__dp = (dp); \ > \ > - if (unlikely(net_ratelimit())) { \ > - if (__dp->netdev) \ > - netdev_warn(__dp->netdev, fmt, ## args); \ > - else \ > - dev_warn(__dp->dev, fmt, ## args); \ > - } \ > - }) > + if (unlikely(net_ratelimit())) { \ > + if (__dp->netdev) \ > + netdev_warn(__dp->netdev, fmt, ##__VA_ARGS__); \ > + else \ > + dev_warn(__dp->dev, fmt, ##__VA_ARGS__); \ > + } \ > +}) > > /* Max time to wait for NFP to respond on updates (in seconds) */ > #define NFP_NET_POLL_TIMEOUT 5 > > -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-09-20 9:57 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-09-14 16:05 False positive for 'Possible unnecessary KERN_ERR' warning in checkpatch Niklas Söderlund 2021-09-16 3:46 ` Joe Perches 2021-09-20 9:57 ` Niklas Söderlund
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.