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]
Message-ID: <20210210185544.GA1786@kernelvm>
Date:   Wed, 10 Feb 2021 18:55:44 +0000
From:   Phillip Potter <phil@...lpotter.co.uk>
To:     Dan Carpenter <dan.carpenter@...cle.com>
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 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@...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.

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?

Regards,
Phil