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: <20220125090233.GR1978@kadam>
Date:   Tue, 25 Jan 2022 12:02:33 +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 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