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

[Greg KH]

On Wed, Feb 10, 2021 at 06:55:44PM +0000, Phillip Potter wrote:
> On Wed, Feb 10, 2021 at 09:40:27PM +0300, Dan Carpenter wrote:
> > 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@xxxxxxxxxxxxxxxx>
> > > ---
> > >  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.
> 
> So I'm in the process of stripping out _dbgdump entirely as per Greg
> K-H's suggestion - am I to understand raw printk is frowned upon though,
> even with the correct KERN_x level specified?
> 
> > 
> > >  
> > >  	#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
> > 
> 
> Thank you for your feedback (and thank you Greg for yours also). I
> hugely appreciate it as a novice/newb.
> 
> One query I have is that individual patches I'm working on for this file are
> generating an awful lot of checkpatch warnings themselves due to the
> nature of the existing violations on the relevant lines. Is it
> considered acceptable for me to still submit these, providing I do so in
> a series which cleans up the other violations in separate patches?

Yes, that is fine, and expected in many of these files :(

thanks,

greg k-h