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