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

[Dan Carpenter <dan.carpenter@oracle.com>]

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