linux-kernel - Re: [PATCH wireless-next 18/20] ti: Use pr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1339749374.4502.80.camel@cumari.coelho.fi>
Date:	Fri, 15 Jun 2012 11:36:14 +0300
From:	Luciano Coelho <coelho@...com>
To:	Joe Perches <joe@...ches.com>
Cc:	linux-kernel@...r.kernel.org,
	"John W. Linville" <linville@...driver.com>,
	linux-wireless@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH wireless-next 18/20] ti: Use pr_<level>

Hi Joe,

On Thu, 2012-06-14 at 22:53 -0700, Joe Perches wrote:
> Use a more current logging style.
> 
> Convert printks to pr_<level>.
> Coalesce formats, align arguments.
> 
> Add newlines to calls, remove from macros.
> Remove unnecessary duplicate newlines from calls.
> Remove spaces before newlines.
> 
> Signed-off-by: Joe Perches <joe@...ches.com>
> ---

As Johannes already mentioned, I have to also ask why do you need to do
this? I see the point, slightly, with the wl1251 code, because it is
still using printks, but with wlcore and friends, there's no functional
change.

[...]

> diff --git a/drivers/net/wireless/ti/wl1251/wl1251.h b/drivers/net/wireless/ti/wl1251/wl1251.h
> index 9d8f581..e4d0063 100644
> --- a/drivers/net/wireless/ti/wl1251/wl1251.h
> +++ b/drivers/net/wireless/ti/wl1251/wl1251.h
> @@ -54,22 +54,22 @@ enum {
>  
>  #define DEBUG_DUMP_LIMIT 1024
>  
> -#define wl1251_error(fmt, arg...) \
> -	printk(KERN_ERR DRIVER_PREFIX "ERROR " fmt "\n", ##arg)
> +#define wl1251_err(fmt, arg...)				\
> +	pr_err(DRIVER_PREFIX "ERROR " fmt, ##arg)
>  
> -#define wl1251_warning(fmt, arg...) \
> -	printk(KERN_WARNING DRIVER_PREFIX "WARNING " fmt "\n", ##arg)
> +#define wl1251_warn(fmt, arg...)				\
> +	pr_warn(DRIVER_PREFIX "WARNING " fmt, ##arg)
>  
> -#define wl1251_notice(fmt, arg...) \
> -	printk(KERN_INFO DRIVER_PREFIX fmt "\n", ##arg)
> +#define wl1251_notice(fmt, arg...)		\
> +	pr_info(DRIVER_PREFIX fmt, ##arg)
>  
> -#define wl1251_info(fmt, arg...) \
> -	printk(KERN_DEBUG DRIVER_PREFIX fmt "\n", ##arg)
> +#define wl1251_info(fmt, arg...)				\
> +	printk(KERN_DEBUG DRIVER_PREFIX fmt, ##arg)
>  
>  #define wl1251_debug(level, fmt, arg...) \
> -	do { \
> +	do {				 \
>  		if (level & DEBUG_LEVEL) \
> -			printk(KERN_DEBUG DRIVER_PREFIX fmt "\n", ##arg); \
> +			printk(KERN_DEBUG DRIVER_PREFIX fmt, ##arg); \
>  	} while (0)

As mentioned above, this change here is acceptable, but removing the \n
from the macro and adding it at the end of each and every call is
useless and will just skew git-blame and make it harder to do rebase or
apply patches because of conflicts.

[...]

> diff --git a/drivers/net/wireless/ti/wlcore/debug.h b/drivers/net/wireless/ti/wlcore/debug.h
> index 6b800b3..83b72c2 100644
> --- a/drivers/net/wireless/ti/wlcore/debug.h
> +++ b/drivers/net/wireless/ti/wlcore/debug.h
> @@ -61,22 +61,22 @@ extern u32 wl12xx_debug_level;
>  
>  #define DEBUG_DUMP_LIMIT 1024
>  
> -#define wl1271_error(fmt, arg...) \
> -	pr_err(DRIVER_PREFIX "ERROR " fmt "\n", ##arg)
> +#define wl1271_err(fmt, arg...)				\
> +	pr_err(DRIVER_PREFIX "ERROR " fmt, ##arg)
>  
> -#define wl1271_warning(fmt, arg...) \
> -	pr_warning(DRIVER_PREFIX "WARNING " fmt "\n", ##arg)
> +#define wl1271_warn(fmt, arg...)				\
> +	pr_warn(DRIVER_PREFIX "WARNING " fmt, ##arg)

Using pr_warn instead of pr_warning is pointless.  pr_warning is the
main definition, actually, and pr_warn is just a "mirror".


> -#define wl1271_notice(fmt, arg...) \
> -	pr_info(DRIVER_PREFIX fmt "\n", ##arg)
> +#define wl1271_notice(fmt, arg...)		\
> +	pr_info(DRIVER_PREFIX fmt, ##arg)
>  
> -#define wl1271_info(fmt, arg...) \
> -	pr_info(DRIVER_PREFIX fmt "\n", ##arg)
> +#define wl1271_info(fmt, arg...)		\
> +	pr_info(DRIVER_PREFIX fmt, ##arg)
>  
> -#define wl1271_debug(level, fmt, arg...) \
> -	do { \
> +#define wl1271_debug(level, fmt, arg...)	\
> +	do {					\
>  		if (level & wl12xx_debug_level) \
> -			pr_debug(DRIVER_PREFIX fmt "\n", ##arg); \
> +			pr_debug(DRIVER_PREFIX fmt, ##arg); \
>  	} while (0)

Again, moving the \n from the macro definition and changing all the
callers is an unnecessary, intrusive change.

We have wanted to change "wl1271" to "wl12xx" for a long time, but we
don't do it exactly because this means we need to change the calls
everywhere.

So, this is a NACK from me, unless you have a really good reason why
this change is needed.

--
Luca.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/