linux-kernel - Re: [PATCH] staging: rtl8723bs: cleanup macros within include/rtw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210210210034.GA1919@kernelvm>
Date:   Wed, 10 Feb 2021 21:00:34 +0000
From:   Phillip Potter <phil@...lpotter.co.uk>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     devel@...verdev.osuosl.org, gregkh@...uxfoundation.org,
        luk@...cz.pl, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8723bs: cleanup macros within
 include/rtw_debug.h

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