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