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

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

[Phillip Potter]

Remove do/while loops from DBG_871X, MSG_8192C and DBG_8192C. 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.

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__)
 
 	#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 */
 
@@ -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)
 #endif /* defined(_dbgdump) */
 #endif /* DEBUG_RTL871X */
 
 #ifdef CONFIG_DBG_COUNTER
-#define DBG_COUNTER(counter) counter++
+#define DBG_COUNTER(counter) ((counter)++)
 #else
 #define DBG_COUNTER(counter) do {} while (0)
 #endif
-- 
2.29.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Greg KH]

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. 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.
> 
> 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__)
>  
>  	#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__)

Odd, the do/while is correct here, why is checkpatch complaining about
it?

>  #endif /* defined(_dbgdump) */
>  #endif /* DEBUG */
>  
> @@ -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;								\

This is not the same as the above stuff, when you find yourself writing
"also" in a changelog text, that's a huge hint you should break the
patch up into a patch series.

Please do that here, this is too much for one patch.

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Phillip Potter]

On Wed, Feb 10, 2021 at 06:12:54PM +0100, Greg KH 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. 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.
> > 
> > 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__)
> >  
> >  	#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__)
> 
> Odd, the do/while is correct here, why is checkpatch complaining about
> it?

The warning it gives me for these is:
WARNING: Single statement macros should not use a do {} while (0) loop

> 
> >  #endif /* defined(_dbgdump) */
> >  #endif /* DEBUG */
> >  
> > @@ -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;								\
> 
> This is not the same as the above stuff, when you find yourself writing
> "also" in a changelog text, that's a huge hint you should break the
> patch up into a patch series.
> 
> Please do that here, this is too much for one patch.
> 
> thanks,
> 
> greg k-h

Thank you for the feedback, I'll do this - shall I leave out the
do/while stuff if you're saying checkpatch is wrong?

Regards,
Phil
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Greg KH]

On Wed, Feb 10, 2021 at 05:34:38PM +0000, Phillip Potter wrote:
> On Wed, Feb 10, 2021 at 06:12:54PM +0100, Greg KH 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. 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.
> > > 
> > > 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__)
> > >  
> > >  	#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__)
> > 
> > Odd, the do/while is correct here, why is checkpatch complaining about
> > it?
> 
> The warning it gives me for these is:
> WARNING: Single statement macros should not use a do {} while (0) loop

Ah.

What a mess.

I would recommend starting to unwind the "debugging" macro mess here,
all that a driver should be using is the netdev_dbg() and friends
functions, not this mess of "printk or no printk" that it currently is.

If you replace _dbgdump with what it is defined with, then go from there
and replace the DBG_8192C and friends with what they should be, and so
on.  That's a much better overall solution than to just paper over this
with a checkpatch cleanup.

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Dan Carpenter]

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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Phillip Potter]

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

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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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@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.
> 
> 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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Dan Carpenter]

On Wed, Feb 10, 2021 at 06:55:44PM +0000, Phillip Potter wrote:
> > > 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?

Yes.  Ideally in drivers everything would use dev_dbg() and dev_err() or
whatever.  But it's perhaps tricky to convert everything in a single
patch so changing _dbgdump() to "#define pr_debug" as an intermediate
step is probably fine.

Look at how people do pr_fmt():
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

You could do a patch that does a mass replacement of DBG_871X with
pr_debug().  Again, I haven't really looked at this code so you'll have
to double check and consider what is the best way to break up the
patches.

> 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?

It's tricky to know how to break up patches.  Probably the simplest
advice is to only clean up a single type of checkpatch warning at a
time.  But fix all the instances of that warning in a file.  Don't
change anything else even if it is tempting.  Do that in the next patch.

The actuall rules are slightly more complicated and nuanced than that,
but if you just fix one type at a time then that's okay.

One thing is that your patches should not introduce new checkpatch
warnings.  So if you have two statements in an if statement and you
delete one, then that means you have to delete he curly braces as well.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Phillip Potter]

> > 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?
> 
> Yes.  Ideally in drivers everything would use dev_dbg() and dev_err() or
> whatever.  But it's perhaps tricky to convert everything in a single
> patch so changing _dbgdump() to "#define pr_debug" as an intermediate
> step is probably fine.
> 
> Look at how people do pr_fmt():
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
> You could do a patch that does a mass replacement of DBG_871X with
> pr_debug().  Again, I haven't really looked at this code so you'll have
> to double check and consider what is the best way to break up the
> patches.
> 

That sounds great, I'll take a look, thanks.

> > 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?
> 
> It's tricky to know how to break up patches.  Probably the simplest
> advice is to only clean up a single type of checkpatch warning at a
> time.  But fix all the instances of that warning in a file.  Don't
> change anything else even if it is tempting.  Do that in the next patch.
> 
> The actuall rules are slightly more complicated and nuanced than that,
> but if you just fix one type at a time then that's okay.
> 
> One thing is that your patches should not introduce new checkpatch
> warnings.  So if you have two statements in an if statement and you
> delete one, then that means you have to delete he curly braces as well.
> 
> regards,
> dan carpenter
> 

Thanks again for the feedback. I will work on something over the next
few days.

Regards,
Phil
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel