Re: [PATCH v2 09/20] staging: rtl8723bs: put parentheses on macros with complex values in include/rtw_debug.h

2021-03-28 Thread Greg KH 
On Sun, Mar 28, 2021 at 04:43:24PM +0200, Fabio Aiuto wrote:
> On Sun, Mar 28, 2021 at 02:08:41PM +0200, Greg KH wrote:
> > On Sat, Mar 27, 2021 at 03:24:08PM +0100, Fabio Aiuto wrote:
> > > fix the following checkpatch warning:
> > > 
> > > ERROR: Macros starting with if should be enclosed by a
> > > do - while loop to avoid possible if/else logic defects
> > > + #define RT_PRINT_DATA(_Comp, _Level,
> > >   _TitleString, _HexData, _HexDataLen)\
> > > 
> > > Signed-off-by: Fabio Aiuto 
> > > ---
> > >  drivers/staging/rtl8723bs/include/rtw_debug.h | 28 ++-
> > >  1 file changed, 15 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/staging/rtl8723bs/include/rtw_debug.h 
> > > b/drivers/staging/rtl8723bs/include/rtw_debug.h
> > > index d1c557818305..b00f8a6c4312 100644
> > > --- a/drivers/staging/rtl8723bs/include/rtw_debug.h
> > > +++ b/drivers/staging/rtl8723bs/include/rtw_debug.h
> > > @@ -236,19 +236,21 @@
> > >  #if  defined(_dbgdump)
> > >   #undef RT_PRINT_DATA
> > >   #define RT_PRINT_DATA(_Comp, _Level, _TitleString, _HexData, 
> > > _HexDataLen)   \
> > > - if (((_Comp) & GlobalDebugComponents) && (_Level <= 
> > > GlobalDebugLevel))  \
> > > - {   
> > > \
> > > - int __i;
> > > \
> > > - u8 *ptr = (u8 *)_HexData;   
> > > \
> > > - _dbgdump("%s", DRIVER_PREFIX);  
> > > \
> > > - _dbgdump(_TitleString); 
> > > \
> > > - for (__i = 0; __i < (int)_HexDataLen; __i++)
> > > \
> > > - {   
> > > \
> > > - _dbgdump("%02X%s", ptr[__i], (((__i + 1) % 4) 
> > > == 0)?"  ":" ");  \
> > > - if (((__i + 1) % 16) == 0)  _dbgdump("\n"); 
> > > \
> > > - }   
> > > \
> > > - _dbgdump("\n"); 
> > > \
> > > - }
> > 
> > This whole nightmare needs to just be deleted and any callers of it need
> > to use the in-kernel functions for this instead.
> > 
> > thanks,
> > 
> > greg k-h
> 
> ok, will we get rid of the private debug level and components tracking as 
> well?

Please do.

> Maybe encapsulating the built-in proposed by Joe, keeping the macro alive?

No, do not keep it alive.  Individual debugging/trace/info macros for
drivers do not make sense at all.  We have kernel-wide infrastructure
for this, it is bold for an individual driver to somehow think that they
need a "custom" solution for this instead.

Just use the normal apis, that is what they are there for.

thanks,

greg k-h

Re: [PATCH v2 09/20] staging: rtl8723bs: put parentheses on macros with complex values in include/rtw_debug.h

2021-03-28 Thread Fabio Aiuto 
On Sun, Mar 28, 2021 at 02:08:41PM +0200, Greg KH wrote:
> On Sat, Mar 27, 2021 at 03:24:08PM +0100, Fabio Aiuto wrote:
> > fix the following checkpatch warning:
> > 
> > ERROR: Macros starting with if should be enclosed by a
> > do - while loop to avoid possible if/else logic defects
> > +   #define RT_PRINT_DATA(_Comp, _Level,
> > _TitleString, _HexData, _HexDataLen)\
> > 
> > Signed-off-by: Fabio Aiuto 
> > ---
> >  drivers/staging/rtl8723bs/include/rtw_debug.h | 28 ++-
> >  1 file changed, 15 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8723bs/include/rtw_debug.h 
> > b/drivers/staging/rtl8723bs/include/rtw_debug.h
> > index d1c557818305..b00f8a6c4312 100644
> > --- a/drivers/staging/rtl8723bs/include/rtw_debug.h
> > +++ b/drivers/staging/rtl8723bs/include/rtw_debug.h
> > @@ -236,19 +236,21 @@
> >  #ifdefined(_dbgdump)
> > #undef RT_PRINT_DATA
> > #define RT_PRINT_DATA(_Comp, _Level, _TitleString, _HexData, 
> > _HexDataLen)   \
> > -   if (((_Comp) & GlobalDebugComponents) && (_Level <= 
> > GlobalDebugLevel))  \
> > -   {   
> > \
> > -   int __i;
> > \
> > -   u8 *ptr = (u8 *)_HexData;   
> > \
> > -   _dbgdump("%s", DRIVER_PREFIX);  
> > \
> > -   _dbgdump(_TitleString); 
> > \
> > -   for (__i = 0; __i < (int)_HexDataLen; __i++)
> > \
> > -   {   
> > \
> > -   _dbgdump("%02X%s", ptr[__i], (((__i + 1) % 4) 
> > == 0)?"  ":" ");  \
> > -   if (((__i + 1) % 16) == 0)  _dbgdump("\n"); 
> > \
> > -   }   
> > \
> > -   _dbgdump("\n"); 
> > \
> > -   }
> 
> This whole nightmare needs to just be deleted and any callers of it need
> to use the in-kernel functions for this instead.
> 
> thanks,
> 
> greg k-h

ok, will we get rid of the private debug level and components tracking as well?

Maybe encapsulating the built-in proposed by Joe, keeping the macro alive?

Thank you,

Fabio

Re: [PATCH v2 09/20] staging: rtl8723bs: put parentheses on macros with complex values in include/rtw_debug.h

2021-03-28 Thread Greg KH 
On Sat, Mar 27, 2021 at 03:24:08PM +0100, Fabio Aiuto wrote:
> fix the following checkpatch warning:
> 
> ERROR: Macros starting with if should be enclosed by a
> do - while loop to avoid possible if/else logic defects
> + #define RT_PRINT_DATA(_Comp, _Level,
>   _TitleString, _HexData, _HexDataLen)\
> 
> Signed-off-by: Fabio Aiuto 
> ---
>  drivers/staging/rtl8723bs/include/rtw_debug.h | 28 ++-
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/include/rtw_debug.h 
> b/drivers/staging/rtl8723bs/include/rtw_debug.h
> index d1c557818305..b00f8a6c4312 100644
> --- a/drivers/staging/rtl8723bs/include/rtw_debug.h
> +++ b/drivers/staging/rtl8723bs/include/rtw_debug.h
> @@ -236,19 +236,21 @@
>  #if  defined(_dbgdump)
>   #undef RT_PRINT_DATA
>   #define RT_PRINT_DATA(_Comp, _Level, _TitleString, _HexData, 
> _HexDataLen)   \
> - if (((_Comp) & GlobalDebugComponents) && (_Level <= 
> GlobalDebugLevel))  \
> - {   
> \
> - int __i;
> \
> - u8 *ptr = (u8 *)_HexData;   
> \
> - _dbgdump("%s", DRIVER_PREFIX);  
> \
> - _dbgdump(_TitleString); 
> \
> - for (__i = 0; __i < (int)_HexDataLen; __i++)
> \
> - {   
> \
> - _dbgdump("%02X%s", ptr[__i], (((__i + 1) % 4) 
> == 0)?"  ":" ");  \
> - if (((__i + 1) % 16) == 0)  _dbgdump("\n"); 
> \
> - }   
> \
> - _dbgdump("\n"); 
> \
> - }

This whole nightmare needs to just be deleted and any callers of it need
to use the in-kernel functions for this instead.

thanks,

greg k-h

Re: [PATCH v2 09/20] staging: rtl8723bs: put parentheses on macros with complex values in include/rtw_debug.h

2021-03-28 Thread Joe Perches 
On Sat, 2021-03-27 at 15:24 +0100, Fabio Aiuto wrote:
> fix the following checkpatch warning:
> 
> ERROR: Macros starting with if should be enclosed by a
> do - while loop to avoid possible if/else logic defects
> + #define RT_PRINT_DATA(_Comp, _Level,
>   _TitleString, _HexData, _HexDataLen)\
> 
> Signed-off-by: Fabio Aiuto 

It's good to use checkpatch as a guide to improve code, but this
particular code is just a mess to begin with and it makes a
complete mess of the the dmesg log if it's actually enabled.

Try substituting print_hex_dump_debug for this instead.

> ---
>  drivers/staging/rtl8723bs/include/rtw_debug.h | 28 ++-
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/include/rtw_debug.h 
> b/drivers/staging/rtl8723bs/include/rtw_debug.h
> index d1c557818305..b00f8a6c4312 100644
> --- a/drivers/staging/rtl8723bs/include/rtw_debug.h
> +++ b/drivers/staging/rtl8723bs/include/rtw_debug.h
> @@ -236,19 +236,21 @@
>  #if  defined(_dbgdump)
>   #undef RT_PRINT_DATA
>   #define RT_PRINT_DATA(_Comp, _Level, _TitleString, _HexData, 
> _HexDataLen)   \
> - if (((_Comp) & GlobalDebugComponents) && (_Level <= 
> GlobalDebugLevel))  \
> - {   
> \
> - int __i;
> \
> - u8 *ptr = (u8 *)_HexData;   
> \
> - _dbgdump("%s", DRIVER_PREFIX);  
> \
> - _dbgdump(_TitleString); 
> \
> - for (__i = 0; __i < (int)_HexDataLen; __i++)
> \
> - {   
> \
> - _dbgdump("%02X%s", ptr[__i], (((__i + 1) % 4) 
> == 0)?"  ":" ");  \
> - if (((__i + 1) % 16) == 0)  _dbgdump("\n"); 
> \
> - }   
> \
> - _dbgdump("\n"); 
> \
> - }
> + do { \
> + if (((_Comp) & GlobalDebugComponents) && (_Level <= 
> GlobalDebugLevel))  \
> + {   
> \
> + int __i;
> \
> + u8 *ptr = (u8 *)_HexData;   
> \
> + _dbgdump("%s", DRIVER_PREFIX);  
> \
> + _dbgdump(_TitleString); 
> \
> + for (__i = 0; __i < (int)_HexDataLen; __i++)
> \
> + {   
> \
> + _dbgdump("%02X%s", ptr[__i], (((__i + 
> 1) % 4) == 0)?"  ":" ");  \
> + if (((__i + 1) % 16) == 0)  
> _dbgdump("\n"); \
> + }   
> \
> + _dbgdump("\n"); 
> \
> + } \
> + } while (0)
>  #endif /* defined(_dbgdump) */
>  #endif /* DEBUG_RTL871X */
>  
>

[PATCH v2 09/20] staging: rtl8723bs: put parentheses on macros with complex values in include/rtw_debug.h

2021-03-27 Thread Fabio Aiuto 
fix the following checkpatch warning:

ERROR: Macros starting with if should be enclosed by a
do - while loop to avoid possible if/else logic defects
+   #define RT_PRINT_DATA(_Comp, _Level,
_TitleString, _HexData, _HexDataLen)\

Signed-off-by: Fabio Aiuto 
---
 drivers/staging/rtl8723bs/include/rtw_debug.h | 28 ++-
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/rtl8723bs/include/rtw_debug.h 
b/drivers/staging/rtl8723bs/include/rtw_debug.h
index d1c557818305..b00f8a6c4312 100644
--- a/drivers/staging/rtl8723bs/include/rtw_debug.h
+++ b/drivers/staging/rtl8723bs/include/rtw_debug.h
@@ -236,19 +236,21 @@
 #ifdefined(_dbgdump)
#undef RT_PRINT_DATA
#define RT_PRINT_DATA(_Comp, _Level, _TitleString, _HexData, 
_HexDataLen)   \
-   if (((_Comp) & GlobalDebugComponents) && (_Level <= 
GlobalDebugLevel))  \
-   {   
\
-   int __i;
\
-   u8 *ptr = (u8 *)_HexData;   
\
-   _dbgdump("%s", DRIVER_PREFIX);  
\
-   _dbgdump(_TitleString); 
\
-   for (__i = 0; __i < (int)_HexDataLen; __i++)
\
-   {   
\
-   _dbgdump("%02X%s", ptr[__i], (((__i + 1) % 4) 
== 0)?"  ":" ");  \
-   if (((__i + 1) % 16) == 0)  _dbgdump("\n"); 
\
-   }   
\
-   _dbgdump("\n"); 
\
-   }
+   do { \
+   if (((_Comp) & GlobalDebugComponents) && (_Level <= 
GlobalDebugLevel))  \
+   {   
\
+   int __i;
\
+   u8 *ptr = (u8 *)_HexData;   
\
+   _dbgdump("%s", DRIVER_PREFIX);  
\
+   _dbgdump(_TitleString); 
\
+   for (__i = 0; __i < (int)_HexDataLen; __i++)
\
+   {   
\
+   _dbgdump("%02X%s", ptr[__i], (((__i + 
1) % 4) == 0)?"  ":" ");  \
+   if (((__i + 1) % 16) == 0)  
_dbgdump("\n"); \
+   }   
\
+   _dbgdump("\n"); 
\
+   } \
+   } while (0)
 #endif /* defined(_dbgdump) */
 #endif /* DEBUG_RTL871X */
 
-- 
2.20.1