linux-kernel - Re: [PATCH 02/10] staging: r8188eu: remove smaller sets of DBG

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220126060017.GU1978@kadam>
Date:   Wed, 26 Jan 2022 09:00:17 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Phillip Potter <phil@...lpotter.co.uk>
Cc:     gregkh@...uxfoundation.org, Larry.Finger@...inger.net,
        straube.linux@...il.com, martin@...ser.cx,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        paskripkin@...il.com
Subject: Re: [PATCH 02/10] staging: r8188eu: remove smaller sets of DBG_88E
 calls from core dir

On Wed, Jan 26, 2022 at 12:54:27AM +0000, Phillip Potter wrote:
> On Tue, Jan 25, 2022 at 12:02:33PM +0300, Dan Carpenter wrote:
> > On Mon, Jan 24, 2022 at 10:44:07PM +0000, Phillip Potter wrote:
> > >  void rtw_sctx_done_err(struct submit_ctx **sctx, int status)
> > >  {
> > >  	if (*sctx) {
> > > -		if (rtw_sctx_chk_waring_status(status))
> > > -			DBG_88E("%s status:%d\n", __func__, status);
> > > +		rtw_sctx_chk_waring_status(status);
> > 
> > In a follow on patch, can you just remove the rtw_sctx_chk_waring_status()
> > function?
> > 
> > Looking through these warnings, a mass delete is the right thing.  The
> > debugging messages are garbage.  Use ftrace.  Some of the errors are
> > debug level so they will never be seen.  If you enable printing error
> > messages, it print a debug information but at error level so that's
> > useless.  Forget about it.  Just delete it.
> > 
> > You're going to need to redo these again because kbuild found some
> > build warnings...
> > 
> > Normally we would say if you're deleting a printk() and that makes the
> > curly braces unnecessary, then you have to delete the curly braces in
> > the same patch.  But in this case, you're changing a ton of code and
> > deleting the curly braces makes it hard to review.  I would prefer it
> > if you just deleted the printks and fixed up the curly braces in a
> > follow up patch.  And please don't fix long line warnings either.  Do
> > that in another even later patch if you want.
> > 
> > You do need to fix any build warnings about unused variables etc, but
> > don't worry about checkpatch until the end.
> > 
> > Basically as much as possible I'd prefer to only see deleted lines in
> > this patch.  No insertions.
> > 
> >  12 files changed, 66 insertions(+), 472 deletions(-)
> > 
> > There was one function where you removed the padatper argument because
> > it wasn't necessary.  That's great but do it in a follow on patch.
> > Keep a notepad app next to your vim and write down notes as you go:
> > 
> > 0) Re-run checkpatch and delete unnecessary curly braces
> > 1) Delete rtw_sctx_chk_waring_status()
> > 2) padatper not needed for frob_whatever()
> > 3) Fix long lines
> > 
> > regards,
> > dan carpenter
> 
> Hi Dan,
> 
> Thank you for the feedback - reason I removed the padapter arg is
> because I was the one that added it. To be clear, you're happy with this
> and the brace changes etc. as long as I take them out within a separate
> patch at the end of the series?
> 

Yes, please.  It's not normally what we tell people to do but with this
kind of mass delete patch it makes things easier to review if that step
is done separately.

regards,
dan carpenter