Re: [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h

[Dan Carpenter <dan.carpenter@oracle.com>]

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@philpotter.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