linux-kernel - Re: [PATCH] staging: rtl8723bs: cleanup macros within include/rtw

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]

Date:   Wed, 10 Feb 2021 21:40:27 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Phillip Potter <phil@...lpotter.co.uk>
Cc:     gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
        luk@...cz.pl, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8723bs: cleanup macros within
 include/rtw_debug.h

On Wed, Feb 10, 2021 at 05:00:03PM +0000, Phillip Potter wrote:
> Remove do/while loops from DBG_871X, MSG_8192C and DBG_8192C.

I'm pretty hip to checkpatch.pl warnings, but I had forgotten what the
warning was for this:

WARNING: Single statement macros should not use a do {} while (0) loop

Please, include it for people who are forgetful like I am.

> Also
> fix opening brace placements and trailing single statement layout within
> RT_PRINT_DATA, as well as making newline character placement more
> consistent and removing camel case where possible. Finally, add
> parentheses for DBG_COUNTER definition.
> 
> This fixes 3 checkpatch warnings, 5 checkpatch errors and 3 checkpatch
> checks.

This patch would be easier to review if it were split into multiple
patches.

> 
> Signed-off-by: Phillip Potter <phil@...lpotter.co.uk>
> ---
>  drivers/staging/rtl8723bs/include/rtw_debug.h | 40 +++++++++----------
>  1 file changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/include/rtw_debug.h b/drivers/staging/rtl8723bs/include/rtw_debug.h
> index c90adfb87261..d06ac9540cf7 100644
> --- a/drivers/staging/rtl8723bs/include/rtw_debug.h
> +++ b/drivers/staging/rtl8723bs/include/rtw_debug.h
> @@ -201,19 +201,16 @@
>  #ifdef DEBUG
>  #if	defined(_dbgdump)
>  	#undef DBG_871X
> -	#define DBG_871X(...)     do {\
> -		_dbgdump(DRIVER_PREFIX __VA_ARGS__);\
> -	} while (0)
> +	#define DBG_871X(...)\
> +		_dbgdump(DRIVER_PREFIX __VA_ARGS__)

This can fit on one line:

	#define DBG_871X(...) _dbgdump(DRIVER_PREFIX __VA_ARGS__)

It's tough with staging code to know how much to change at one time
because even after you change the code then it still looks rubbish.
This define shouldn't be indented.  The _dbgdump() macro is just

#define _dbgdump printk

so you know, no printk level.  Wow.  etc.  This code is crap.

>  
>  	#undef MSG_8192C
> -	#define MSG_8192C(...)     do {\
> -		_dbgdump(DRIVER_PREFIX __VA_ARGS__);\
> -	} while (0)
> +	#define MSG_8192C(...)\
> +		_dbgdump(DRIVER_PREFIX __VA_ARGS__)
>  
>  	#undef DBG_8192C
> -	#define DBG_8192C(...)     do {\
> -		_dbgdump(DRIVER_PREFIX __VA_ARGS__);\
> -	} while (0)
> +	#define DBG_8192C(...)\
> +		_dbgdump(DRIVER_PREFIX __VA_ARGS__)
>  #endif /* defined(_dbgdump) */
>  #endif /* DEBUG */
>  

Yeah.  Do all the above as one patch.

> @@ -235,25 +232,26 @@
>  
>  #if	defined(_dbgdump)
>  	#undef RT_PRINT_DATA
> -	#define RT_PRINT_DATA(_Comp, _Level, _TitleString, _HexData, _HexDataLen)			\
> -		if (((_Comp) & GlobalDebugComponents) && (_Level <= GlobalDebugLevel))	\
> -		{									\
> +	#define RT_PRINT_DATA(_comp, _level, _title_string, _hex_data, _hex_data_len)		\
> +	do {											\
> +		if (((_comp) & GlobalDebugComponents) && ((_level) <= GlobalDebugLevel)) {	\
>  			int __i;								\
> -			u8 *ptr = (u8 *)_HexData;				\
> +			u8 *ptr = (u8 *)_hex_data;						\
>  			_dbgdump("%s", DRIVER_PREFIX);						\
> -			_dbgdump(_TitleString);						\
> -			for (__i = 0; __i < (int)_HexDataLen; __i++)				\
> -			{								\
> +			_dbgdump(_title_string);						\
> +			for (__i = 0; __i < (int)_hex_data_len; __i++) {			\
>  				_dbgdump("%02X%s", ptr[__i], (((__i + 1) % 4) == 0)?"  ":" ");	\
> -				if (((__i + 1) % 16) == 0)	_dbgdump("\n");			\
> -			}								\
> -			_dbgdump("\n");							\
> -		}
> +				if (((__i + 1) % 16) == 0)					\
> +					_dbgdump("\n");						\
> +			}									\
> +			_dbgdump("\n");								\
> +		}										\
> +	} while (0)

This is okay, I suppose but we have functions to dump hex data.  I can't
remember what they are...  One patch for this.

>  #endif /* defined(_dbgdump) */
>  #endif /* DEBUG_RTL871X */
>  
>  #ifdef CONFIG_DBG_COUNTER
> -#define DBG_COUNTER(counter) counter++
> +#define DBG_COUNTER(counter) ((counter)++)

Heh...  I think these counters are write only variables.  Double check
and then just delete everything to do with CONFIG_DBG_COUNTER.
(In a separate patch).

regards,
dan carpenter