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: <YfCbw3ENI/1tEdwi@equinox>
Date:   Wed, 26 Jan 2022 00:54:27 +0000
From:   Phillip Potter <phil@...lpotter.co.uk>
To:     Dan Carpenter <dan.carpenter@...cle.com>
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 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?

I had quite a few unused variable errors along the way, but evidently
with the way my build system is setup it doesn't currently show set but
unused variables. I will fix this and deal with the kernel test robot
warnings. Many thanks.

Regards,
Phil