Re: False positive for 'Possible unnecessary KERN_ERR' warning in checkpatch

[Niklas Söderlund]

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