* [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h @ 2021-02-10 17:00 Phillip Potter 2021-02-10 17:12 ` Greg KH 2021-02-10 18:40 ` Dan Carpenter 0 siblings, 2 replies; 9+ messages in thread From: Phillip Potter @ 2021-02-10 17:00 UTC (permalink / raw) To: gregkh; +Cc: devel, luk, linux-kernel 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h 2021-02-10 17:00 [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h Phillip Potter @ 2021-02-10 17:12 ` Greg KH 2021-02-10 17:34 ` Phillip Potter 2021-02-10 18:40 ` Dan Carpenter 1 sibling, 1 reply; 9+ messages in thread From: Greg KH @ 2021-02-10 17:12 UTC (permalink / raw) To: Phillip Potter; +Cc: devel, luk, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h 2021-02-10 17:12 ` Greg KH @ 2021-02-10 17:34 ` Phillip Potter 2021-02-10 17:48 ` Greg KH 0 siblings, 1 reply; 9+ messages in thread From: Phillip Potter @ 2021-02-10 17:34 UTC (permalink / raw) To: Greg KH; +Cc: devel, luk, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h 2021-02-10 17:34 ` Phillip Potter @ 2021-02-10 17:48 ` Greg KH 0 siblings, 0 replies; 9+ messages in thread From: Greg KH @ 2021-02-10 17:48 UTC (permalink / raw) To: Phillip Potter; +Cc: devel, luk, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h 2021-02-10 17:00 [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h Phillip Potter 2021-02-10 17:12 ` Greg KH @ 2021-02-10 18:40 ` Dan Carpenter 2021-02-10 18:55 ` Phillip Potter 1 sibling, 1 reply; 9+ messages in thread From: Dan Carpenter @ 2021-02-10 18:40 UTC (permalink / raw) To: Phillip Potter; +Cc: devel, gregkh, luk, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h 2021-02-10 18:40 ` Dan Carpenter @ 2021-02-10 18:55 ` Phillip Potter 2021-02-10 19:36 ` Greg KH 2021-02-10 20:18 ` Dan Carpenter 0 siblings, 2 replies; 9+ messages in thread From: Phillip Potter @ 2021-02-10 18:55 UTC (permalink / raw) To: Dan Carpenter; +Cc: devel, gregkh, luk, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h 2021-02-10 18:55 ` Phillip Potter @ 2021-02-10 19:36 ` Greg KH 2021-02-10 20:18 ` Dan Carpenter 1 sibling, 0 replies; 9+ messages in thread From: Greg KH @ 2021-02-10 19:36 UTC (permalink / raw) To: Phillip Potter; +Cc: devel, luk, Dan Carpenter, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h 2021-02-10 18:55 ` Phillip Potter 2021-02-10 19:36 ` Greg KH @ 2021-02-10 20:18 ` Dan Carpenter 2021-02-10 21:00 ` Phillip Potter 1 sibling, 1 reply; 9+ messages in thread From: Dan Carpenter @ 2021-02-10 20:18 UTC (permalink / raw) To: Phillip Potter; +Cc: devel, gregkh, luk, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h 2021-02-10 20:18 ` Dan Carpenter @ 2021-02-10 21:00 ` Phillip Potter 0 siblings, 0 replies; 9+ messages in thread From: Phillip Potter @ 2021-02-10 21:00 UTC (permalink / raw) To: Dan Carpenter; +Cc: devel, gregkh, luk, linux-kernel > > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-02-10 21:00 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-10 17:00 [PATCH] staging: rtl8723bs: cleanup macros within include/rtw_debug.h Phillip Potter 2021-02-10 17:12 ` Greg KH 2021-02-10 17:34 ` Phillip Potter 2021-02-10 17:48 ` Greg KH 2021-02-10 18:40 ` Dan Carpenter 2021-02-10 18:55 ` Phillip Potter 2021-02-10 19:36 ` Greg KH 2021-02-10 20:18 ` Dan Carpenter 2021-02-10 21:00 ` Phillip Potter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).